feat(core): add first-class MCP support - #21
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR adds a Core-owned MCP subsystem with versioned configuration, OAuth credentials and callbacks, process-wide registry lifecycle, unified deferred tool search, management callables, runtime and agent integration, Claude Code ToolSearch support, selected-tool persistence, and Bash/filesystem credential protections. ChangesFirst-Class MCP Subsystem
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc59893605
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/core/src/tools/bash-safety/format.ts (1)
45-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact compound sensitive query parameter names.
?access_token=...previously matched the genericTOKENrule, but Line 45 now excludes query parameters and Line 54 only matches exacttoken,key, andsecret. This leaks common OAuth/API credentials into logs and blocked-command messages.Proposed fix
- result = result.replace(/([?&](?:code|state|token|key|secret)=)([^&#\s"']*)/gi, "$1<redacted>"); + result = result.replace( + /([?&](?:(?:code|state)|(?:[a-z0-9_]*(?:token|secret|password|pass|key|credentials)[a-z0-9_]*))=)([^&#\s"']*)/gi, + "$1<redacted>", + );Add cases for
access_token,refresh_token,client_secret, andapi_key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/tools/bash-safety/format.ts` around lines 45 - 54, Add the compound sensitive query parameter names access_token, refresh_token, client_secret, and api_key to the query-parameter redaction pattern in the result formatting flow, while preserving existing redaction for code, state, token, key, and secret. Update the relevant replace expression in the formatter so each value is replaced with <redacted>.
🧹 Nitpick comments (12)
apps/core/src/mcp/registry.ts (1)
584-603: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a hard page/tool cap in addition to the repeated-cursor guard.
A server returning an unbounded stream of unique cursors is only bounded by the init deadline; until then
toolsandseenCursorsgrow without limit. A simple max-page (or max-tool) ceiling makes the failure mode explicit and memory-bounded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/mcp/registry.ts` around lines 584 - 603, Add a finite page or tool-count ceiling to the tool-pagination loop around client.listTools, alongside the existing seenCursors guard. Track the accumulated pages or tools and throw a clear error when the configured hard cap is reached, while preserving normal pagination and repeated-cursor detection below the limit.apps/core/tests/tools/bash.test.ts (1)
74-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name promises redaction verification, but nothing asserts it.
This only checks the command executes with the URL intact; the "redacting only displayed command text" half is unverified. Capture the logger output (as done in
apps/core/tests/tool-server-create-tool-server.test.ts) and assert the logged command has no?code=/statevalues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/tests/tools/bash.test.ts` around lines 74 - 83, Update the test around executeBash to capture logger output using the existing pattern from the tool-server tests, then assert the logged command redacts the callback URL’s code and state values while retaining the execution assertion and original callback behavior.packages/utils/prompt-templates/TOOLS.md (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: use the em dash separator for consistency.
Every other bullet in this list uses
—; this one uses-.✏️ Proposed change
-- `mcp.*` - Manage configured MCP servers. Load the `mcp-management` skill before adding, removing, authenticating, or reloading one. +- `mcp.*` — Manage configured MCP servers. Load the `mcp-management` skill before adding, removing, authenticating, or reloading one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/utils/prompt-templates/TOOLS.md` at line 73, Update the mcp.* bullet in the tools list to use the same em dash separator as the other bullets, preserving the existing wording.apps/core/src/tool-server/tools/mcp.ts (1)
285-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the config-read failure in the
mcp.reloadresult instead of discarding it.The
catch {}at Line 290 drops the error entirely, so a malformed/unreadable config file yields a payload indistinguishable from a clean reload while provider reconciliation was silently skipped. Include the reason in the response.As per coding guidelines, "Safely convert unknown caught errors with
e instanceof Error ? e.message : String(e)and do not silently swallow errors."♻️ Proposed change
let snapshot: McpConfigFileSnapshot; try { snapshot = await readMcpConfigFile(this.params.configPath); - } catch { - return { reload: safeReloadOutcomes(await this.params.registry.reload(serverId)) }; + } catch (e) { + return { + configError: e instanceof Error ? e.message : String(e), + reload: safeReloadOutcomes(await this.params.registry.reload(serverId)), + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/tool-server/tools/mcp.ts` around lines 285 - 295, Update the mcp.reload operation around readMcpConfigFile to capture the caught error and include its message in the returned result when config loading fails, while preserving the existing registry reload behavior. Convert unknown errors safely with e instanceof Error ? e.message : String(e), and ensure the failure is no longer silently discarded or confused with a clean reload.Source: Coding guidelines
apps/core/tests/mcp/management.test.ts (1)
496-511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
"one-time-state"insecretsis vacuous here.The only output that can contain it is the
mcp.authresponse (Line 506), which is intentionally excluded fromserializedSafeOutputs. Consider dropping it from the list or asserting explicitly thatmcp.status/mcp.listnever echo prior auth state, so the intent is unambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/tests/mcp/management.test.ts` around lines 496 - 511, Remove "one-time-state" from the shared secrets list, or explicitly include the mcp.auth result in a dedicated assertion that mcp.status and mcp.list do not expose prior OAuth callback state. Keep the existing credential checks for added, list, and status outputs unchanged.apps/core/src/tool-server/tools/onboarding.ts (1)
1128-1210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the bundled-skill install step; four identical blocks now differ only by name.
Each of the four steps repeats the same
path.join(findWorkspaceRoot(), "packages", "utils", "skill-templates", …)+copyFileIfNeeded+ result shape, and each re-walks the filesystem to find the workspace root. A small helper removes the duplication and the repeated lookup.♻️ Proposed refactor
+ const installBundledSkill = (name: string) => + runStep(`skills.${name}`, async () => { + const src = path.join( + findWorkspaceRoot(), + "packages", + "utils", + "skill-templates", + name, + "SKILL.md", + ); + const dst = path.join(paths.lilacSkillsDir, name, "SKILL.md"); + const { copied, overwritten } = await copyFileIfNeeded({ + from: src, + to: dst, + overwrite: input.overwriteSkills, + }); + return { + status: copied ? "installed" : "already_present", + details: { src, dst, overwritten }, + }; + }); + + for (const name of ["coding-agent", "mcp-management", "mcporter", "gog"]) { + await installBundledSkill(name); + }Note: ordering must stay
coding-agent,mcp-management,mcporter,gogto matchapps/core/tests/tools/onboarding-default-skills.test.tsLines 65-70.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/tool-server/tools/onboarding.ts` around lines 1128 - 1210, Extract the repeated bundled-skill installation logic into a local helper near the onboarding flow that accepts the skill name, resolves findWorkspaceRoot() once for the shared template directory, calls copyFileIfNeeded with the existing overwriteSkills option, and returns the same status/details shape. Replace the four inline runStep callbacks with helper calls while preserving the order coding-agent, mcp-management, mcporter, then gog.apps/core/tests/mcp/oauth.test.ts (1)
26-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly the
dynamicclient path is exercised.Every test uses
client: { type: "dynamic" }, so the static-client branch inclientInformation()(value-source resolution forclientId/clientSecret, including thecredentialserror path and theclient_secret_postmetadata switch) has no coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/tests/mcp/oauth.test.ts` around lines 26 - 46, Add test coverage for the static-client branch of clientInformation() in the OAuth test setup, using oauthConfig() to configure static clientId/clientSecret value sources. Exercise both successful resolution—including client_secret_post metadata handling—and the credentials error path, while preserving the existing dynamic-client tests.apps/core/tests/mcp/credential-file.test.ts (1)
29-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the serialized update queue.
The
updateQueueschaining incredential-file.ts(lines 139-164), including the failure path where a rejected update must not stall subsequent updates, is untested. A test issuing twoupdateMcpOAuthCredentialFilecalls concurrently (one throwing) and asserting both the final merged content and that the map entry is released would lock in that behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/tests/mcp/credential-file.test.ts` around lines 29 - 127, Add a test around updateMcpOAuthCredentialFile that starts two concurrent updates for the same server, makes the first update reject, and verifies the second still completes with the expected merged credential content. Also assert the serialized updateQueues entry is removed after processing, using the module’s existing observable access or test hook.apps/core/src/mcp/credential-file.ts (2)
139-164: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffSerialization is per-process only.
updateQueuesis a module-level map, so concurrent updates from a second Core process (or a second module instance under different resolution) can lose writes — the rename keeps the file uncorrupted, but a read-modify-write can clobber a token refresh. If multi-process use is possible, anO_EXCLlockfile around the read-write section would close the TOCTOU window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/mcp/credential-file.ts` around lines 139 - 164, Add cross-process serialization to the read-modify-write section of the credential update flow, centered on the queue callback that calls readMcpOAuthCredentialFile and writeMcpOAuthCredentialFileAtomic. Acquire an O_EXCL lockfile before reading, hold it through the atomic write, and reliably release it in cleanup/error paths; retain the existing updateQueues behavior for in-process serialization.
55-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve the underlying cause on
McpOAuthCredentialError.Both catch sites discard the original error, so parse/IO failures are undiagnosable. The redacted message can stay while still passing
cause(JSON parse errors can embed content, so keep it out ofmessageonly).♻️ Suggested change
export class McpOAuthCredentialError extends Error { constructor( readonly credentialPath: string, operation: "read" | "write", + options?: { cause?: unknown }, ) { - super(`Failed to ${operation} MCP OAuth credentials at ${credentialPath}`); + super(`Failed to ${operation} MCP OAuth credentials at ${credentialPath}`, options); this.name = "McpOAuthCredentialError"; } }As per coding guidelines, "do not silently swallow errors".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/mcp/credential-file.ts` around lines 55 - 93, Update both catch blocks in readMcpOAuthCredentialFile to pass the caught error as the cause when constructing McpOAuthCredentialError, while preserving its existing redacted message. Extend McpOAuthCredentialError to accept and assign the underlying cause without exposing it in the message, including for file I/O and JSON/schema parsing failures.Source: Coding guidelines
apps/core/src/mcp/oauth-callback.ts (1)
106-131: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider a Host-header check on the loopback callback.
The listener binds to
localhost, but a browser on the machine will send whatever hostname resolves there; a DNS-rebinding page can reachhttp://attacker.test:1456/mcp/oauth/callbackand burn/observe pending states. Rejecting requests whoseHostis notlocalhost/127.0.0.1[:port]is a cheap hardening step. The 404 branch on Line 115 also omits theContent-Typeheader the other responses set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/mcp/oauth-callback.ts` around lines 106 - 131, Update handleRequest to validate the request Host header before processing the callback, accepting only localhost or 127.0.0.1 with the expected optional port and returning the existing invalid/not-found response for other hosts. Also update the pathname-mismatch 404 response to include the same text/plain Content-Type header used by the other responses.apps/core/src/transcript/transcript-store.ts (1)
166-222: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo retention/pruning for
session_loaded_tools.Unlike
request_transcriptsandsurface_message_to_request, which are bounded bypruneRetention()(TTL + max-row clamp),session_loaded_toolshas no cleanup path. Selections only ever accumulate (upsert never deletes), so long-lived sessions touching many catalog entries over time will grow this table unbounded, with no tie-in to the existing retention logic.Consider adding a TTL/row-cap sweep for
session_loaded_toolssimilar topruneRetention(), or pruning per-session ids on session teardown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/transcript/transcript-store.ts` around lines 166 - 222, Extend the existing pruneRetention() cleanup flow to prune session_loaded_tools using the same TTL and maximum-row retention policy as request_transcripts and surface_message_to_request. Remove expired entries and enforce the row cap, while preserving selectSessionToolIds() and listSessionToolIds() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/core/src/mcp/config.ts`:
- Around line 122-129: Update sortRecord to construct the normalized result with
Object.fromEntries over the sorted, filtered record entries, rather than
assigning keys into a plain object. Preserve omission of undefined values while
ensuring allowed keys such as __proto__ are stored as own properties.
In `@apps/core/src/mcp/credential-file.ts`:
- Around line 16-40: Update the persistence validation used by saveTokens() and
the server-information save path so unknown provider fields are ignored before
writeMcpOAuthCredentialFileAtomic() persists credentials. Match
oauthClientInformationSchema’s non-strict behavior for the OAuth token and
authorization-server schemas, or explicitly strip unknown keys while preserving
all declared fields.
In `@apps/core/src/mcp/oauth-provider.ts`:
- Around line 226-252: Preserve the redacted error messages while retaining
underlying causes across both MCP OAuth error paths: in
apps/core/src/mcp/oauth-provider.ts lines 226-252, update the startAuthorization
and completeAuthorization catch blocks to capture unknown errors, safely convert
them with the prescribed Error-message/String fallback, and pass them as cause
to McpOAuthProviderError; in apps/core/src/mcp/credential-file.ts lines 55-93,
add optional cause support to McpOAuthCredentialError and forward safely
converted causes from the read/write catch blocks.
- Around line 255-275: Update createPendingAuthorization to reclaim stale
entries in starting or completing states, not only pending entries, by tracking
each authorization’s creation time and evicting entries older than a defined
timeout before enforcing MAX_PENDING_AUTHORIZATIONS. Preserve implicitPending
cleanup when evicting its associated entry and retain the existing error when no
capacity can be reclaimed.
- Around line 111-200: Remove the shared implicitPending-based flow from the
instance methods state, saveState, saveCodeVerifier, and codeVerifier by routing
client-initiated authorization through the same per-attempt provider created by
providerForAttempt(pending). Ensure each concurrent authorization retains its
own pending state and code verifier, and update redirectToAuthorization-related
handling consistently without relying on a mutable provider-wide slot.
- Around line 134-144: Update clientInformation() so the expiration check treats
client_secret_expires_at equal to 0 as non-expiring. Only return undefined when
the value is defined, nonzero, and earlier than or equal to the current
timestamp; preserve the existing behavior for unexpired and missing values.
In `@apps/core/src/plugins/builtin/local-tools.ts`:
- Around line 78-107: Update canonicalizeAsFarAsExists to track symlink hops and
enforce a maximum of 40 while resolving missing segments. Increment the counter
before following each symbolic link, and fail closed by throwing once the limit
is exceeded; preserve the existing resolution behavior for paths within the
limit.
In `@apps/core/src/tool-server/create-tool-server.ts`:
- Around line 111-149: Update collectMcpAddSensitiveValues to collect string
values from stdio command and args fields, including nested values, in addition
to the existing headers, env, and clientsecret traversal. Also cover top-level
auth.client.clientSecret sources so OAuth secrets passed through command-line
arguments are included in the returned sensitive-value set.
In `@packages/bash-safety/src/analyze/segment.ts`:
- Around line 795-836: Update analyzeProtectedPathTokens to inspect attached
path values in short options instead of skipping every token that starts with
“-”; specifically handle forms such as “-C/data/secret” and “-C=data/secret”
while preserving normal flag handling. Add regression coverage for
protected-path access through tar “-C” and git “-C” attached paths.
In `@PROJECT.md`:
- Around line 320-322: Correct the inconsistent indentation of the sibling list
items in PROJECT.md so the entries beginning “Capability-bound plugins” and
“Configured MCP servers” align with the preceding “apps/tool-bridge/client.ts”
bullet. Preserve the existing bullet text and ordering.
---
Outside diff comments:
In `@apps/core/src/tools/bash-safety/format.ts`:
- Around line 45-54: Add the compound sensitive query parameter names
access_token, refresh_token, client_secret, and api_key to the query-parameter
redaction pattern in the result formatting flow, while preserving existing
redaction for code, state, token, key, and secret. Update the relevant replace
expression in the formatter so each value is replaced with <redacted>.
---
Nitpick comments:
In `@apps/core/src/mcp/credential-file.ts`:
- Around line 139-164: Add cross-process serialization to the read-modify-write
section of the credential update flow, centered on the queue callback that calls
readMcpOAuthCredentialFile and writeMcpOAuthCredentialFileAtomic. Acquire an
O_EXCL lockfile before reading, hold it through the atomic write, and reliably
release it in cleanup/error paths; retain the existing updateQueues behavior for
in-process serialization.
- Around line 55-93: Update both catch blocks in readMcpOAuthCredentialFile to
pass the caught error as the cause when constructing McpOAuthCredentialError,
while preserving its existing redacted message. Extend McpOAuthCredentialError
to accept and assign the underlying cause without exposing it in the message,
including for file I/O and JSON/schema parsing failures.
In `@apps/core/src/mcp/oauth-callback.ts`:
- Around line 106-131: Update handleRequest to validate the request Host header
before processing the callback, accepting only localhost or 127.0.0.1 with the
expected optional port and returning the existing invalid/not-found response for
other hosts. Also update the pathname-mismatch 404 response to include the same
text/plain Content-Type header used by the other responses.
In `@apps/core/src/mcp/registry.ts`:
- Around line 584-603: Add a finite page or tool-count ceiling to the
tool-pagination loop around client.listTools, alongside the existing seenCursors
guard. Track the accumulated pages or tools and throw a clear error when the
configured hard cap is reached, while preserving normal pagination and
repeated-cursor detection below the limit.
In `@apps/core/src/tool-server/tools/mcp.ts`:
- Around line 285-295: Update the mcp.reload operation around readMcpConfigFile
to capture the caught error and include its message in the returned result when
config loading fails, while preserving the existing registry reload behavior.
Convert unknown errors safely with e instanceof Error ? e.message : String(e),
and ensure the failure is no longer silently discarded or confused with a clean
reload.
In `@apps/core/src/tool-server/tools/onboarding.ts`:
- Around line 1128-1210: Extract the repeated bundled-skill installation logic
into a local helper near the onboarding flow that accepts the skill name,
resolves findWorkspaceRoot() once for the shared template directory, calls
copyFileIfNeeded with the existing overwriteSkills option, and returns the same
status/details shape. Replace the four inline runStep callbacks with helper
calls while preserving the order coding-agent, mcp-management, mcporter, then
gog.
In `@apps/core/src/transcript/transcript-store.ts`:
- Around line 166-222: Extend the existing pruneRetention() cleanup flow to
prune session_loaded_tools using the same TTL and maximum-row retention policy
as request_transcripts and surface_message_to_request. Remove expired entries
and enforce the row cap, while preserving selectSessionToolIds() and
listSessionToolIds() behavior.
In `@apps/core/tests/mcp/credential-file.test.ts`:
- Around line 29-127: Add a test around updateMcpOAuthCredentialFile that starts
two concurrent updates for the same server, makes the first update reject, and
verifies the second still completes with the expected merged credential content.
Also assert the serialized updateQueues entry is removed after processing, using
the module’s existing observable access or test hook.
In `@apps/core/tests/mcp/management.test.ts`:
- Around line 496-511: Remove "one-time-state" from the shared secrets list, or
explicitly include the mcp.auth result in a dedicated assertion that mcp.status
and mcp.list do not expose prior OAuth callback state. Keep the existing
credential checks for added, list, and status outputs unchanged.
In `@apps/core/tests/mcp/oauth.test.ts`:
- Around line 26-46: Add test coverage for the static-client branch of
clientInformation() in the OAuth test setup, using oauthConfig() to configure
static clientId/clientSecret value sources. Exercise both successful
resolution—including client_secret_post metadata handling—and the credentials
error path, while preserving the existing dynamic-client tests.
In `@apps/core/tests/tools/bash.test.ts`:
- Around line 74-83: Update the test around executeBash to capture logger output
using the existing pattern from the tool-server tests, then assert the logged
command redacts the callback URL’s code and state values while retaining the
execution assertion and original callback behavior.
In `@packages/utils/prompt-templates/TOOLS.md`:
- Line 73: Update the mcp.* bullet in the tools list to use the same em dash
separator as the other bullets, preserving the existing wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ec8108e-1f02-40b7-b534-249d08d160bc
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
PROJECT.mdapps/core/package.jsonapps/core/src/mcp/catalog-identity.tsapps/core/src/mcp/catalog.tsapps/core/src/mcp/config-file.tsapps/core/src/mcp/config-types.tsapps/core/src/mcp/config.tsapps/core/src/mcp/credential-file.tsapps/core/src/mcp/index.tsapps/core/src/mcp/oauth-callback.tsapps/core/src/mcp/oauth-provider.tsapps/core/src/mcp/registry-types.tsapps/core/src/mcp/registry.tsapps/core/src/mcp/value-source.tsapps/core/src/plugins/builtin/index.tsapps/core/src/plugins/builtin/local-tools.tsapps/core/src/plugins/builtin/server-tools.tsapps/core/src/plugins/manager.tsapps/core/src/plugins/types.tsapps/core/src/runtime/create-core-runtime.tsapps/core/src/surface/bridge/bus-agent-runner.tsapps/core/src/tool-server/create-tool-server.tsapps/core/src/tool-server/tools/index.tsapps/core/src/tool-server/tools/mcp.tsapps/core/src/tool-server/tools/onboarding.tsapps/core/src/tools/bash-impl.tsapps/core/src/tools/bash-safety/format.tsapps/core/src/transcript/transcript-store.tsapps/core/tests/mcp/catalog-identity.test.tsapps/core/tests/mcp/catalog.test.tsapps/core/tests/mcp/config.test.tsapps/core/tests/mcp/credential-file.test.tsapps/core/tests/mcp/fixtures/registry-fixture.tsapps/core/tests/mcp/management.test.tsapps/core/tests/mcp/oauth.test.tsapps/core/tests/mcp/registry.test.tsapps/core/tests/mcp/value-source.test.tsapps/core/tests/plugins/core-tool-plugin-manager.test.tsapps/core/tests/runtime/mcp-startup.test.tsapps/core/tests/surface/bridge/bus-agent-runner.test.tsapps/core/tests/tool-server-create-tool-server.test.tsapps/core/tests/tools/bash-format.test.tsapps/core/tests/tools/bash.test.tsapps/core/tests/tools/fs-mcp-credentials.test.tsapps/core/tests/tools/local-apply-patch-mcp-credentials.test.tsapps/core/tests/tools/onboarding-default-skills.test.tsapps/core/tests/transcript/transcript-store.test.tsapps/tool-bridge/client.test.tsdocs/docker-deployment.mdpackages/agent/ai-sdk-pi-agent.tspackages/agent/tests/active-tools.test.tspackages/bash-safety/src/analyze/ast-walker.tspackages/bash-safety/src/analyze/segment.tspackages/bash-safety/src/types.tspackages/bash-safety/tests/bash-safety.test.tspackages/claude-code-bridge/claude-code-run.tspackages/claude-code-bridge/claude-code-tools.tspackages/claude-code-bridge/tests/claude-code-run.test.tspackages/claude-code-bridge/tests/claude-code-tools.test.tspackages/claude-code-bridge/tests/fixtures/scale-tools-stdio-server.tspackages/plugin-runtime/manager.tspackages/plugin-runtime/tests/manager.test.tspackages/plugin-runtime/types.tspackages/utils/config-templates/mcp-config.example.yamlpackages/utils/prompt-templates/TOOLS.mdpackages/utils/skill-templates/mcp-management/SKILL.mdpackages/utils/skill-templates/mcporter/SKILL.mdpackages/utils/tests/skills.test.tsplan/first-class-mcp-subsystem-attempt-2.md
|
Addressed the confirmed CodeRabbit/Codex blockers in
The Validation: Core 1,299 tests, Bash-safety tests, monorepo harness, all-workspace typecheck, remote-runner build, lint, format, and |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/core/src/tool-server/create-tool-server.ts (1)
779-788: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep
mcp.addvalidation responses non-sensitive.
ToolInputValidationErrorbuildsmessagein the constructor from the originalinput, so the response branch returns that potentially credential-bearing validation summary before the genericmcp.addredaction fallback. Use a generic validation response formcp.add, or add regression coverage proving the generated output cannot include command/arg/env/header/OAuth secrets.Proposed safer response ordering
- e instanceof ToolInputValidationError - ? e.message - : body.callableId === "mcp.add" - ? "mcp.add failed without exposing sensitive configuration" + body.callableId === "mcp.add" + ? e instanceof ToolInputValidationError + ? "mcp.add input validation failed" + : "mcp.add failed without exposing sensitive configuration" + : e instanceof ToolInputValidationError + ? e.message🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/tool-server/create-tool-server.ts` around lines 779 - 788, Update the error-response selection in the tool-server handler so all failures from mcp.add, including ToolInputValidationError, use a generic non-sensitive message before returning any exception message. Preserve the detailed ToolInputValidationError message for other callable IDs, and keep the existing generic handling for non-Error values.apps/core/src/mcp/oauth-provider.ts (1)
290-339: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard
invalidateCredentialswith the same per-attempt active check.
auth()callsinvalidateCredentials()directly on the provider forInvalidClientError,UnauthorizedClientError, andInvalidGrantError; current implementations for credential persistence also delete tokens/client information. A superseded attempt can still invoke this handler after the newer attempt has persisted credentials, bypassing theassertActiveguard used by the other mutating methods.🔒 Proposed fix
- invalidateCredentials: (scope: "all" | "client" | "tokens" | "verifier") => - this.invalidateCredentials(scope), + invalidateCredentials: (scope: "all" | "client" | "tokens" | "verifier") => { + if (!this.isActive(pending)) return Promise.resolve(); + return this.invalidateCredentials(scope); + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/core/src/mcp/oauth-provider.ts` around lines 290 - 339, Update the invalidateCredentials callback in providerForAttempt to call assertActive with the pending attempt and the appropriate "complete" phase before delegating to this.invalidateCredentials(scope). Preserve the existing scope forwarding while preventing superseded attempts from mutating persisted credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/core/src/mcp/oauth-provider.ts`:
- Around line 290-339: Update the invalidateCredentials callback in
providerForAttempt to call assertActive with the pending attempt and the
appropriate "complete" phase before delegating to
this.invalidateCredentials(scope). Preserve the existing scope forwarding while
preventing superseded attempts from mutating persisted credentials.
In `@apps/core/src/tool-server/create-tool-server.ts`:
- Around line 779-788: Update the error-response selection in the tool-server
handler so all failures from mcp.add, including ToolInputValidationError, use a
generic non-sensitive message before returning any exception message. Preserve
the detailed ToolInputValidationError message for other callable IDs, and keep
the existing generic handling for non-Error values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 91c3a637-55c5-42ee-bad8-cf909db31a15
📒 Files selected for processing (16)
PROJECT.mdapps/core/src/mcp/oauth-provider.tsapps/core/src/mcp/registry.tsapps/core/src/plugins/builtin/local-tools.tsapps/core/src/tool-server/create-tool-server.tsapps/core/src/tools/bash-safety/format.tsapps/core/src/transcript/transcript-store.tsapps/core/tests/mcp/oauth.test.tsapps/core/tests/mcp/registry.test.tsapps/core/tests/tool-server-create-tool-server.test.tsapps/core/tests/tools/bash-format.test.tsapps/core/tests/tools/local-apply-patch-mcp-credentials.test.tsapps/core/tests/transcript/transcript-store.test.tspackages/bash-safety/src/analyze/segment.tspackages/bash-safety/src/shell.tspackages/bash-safety/tests/bash-safety.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/core/tests/tools/bash-format.test.ts
- apps/core/src/tools/bash-safety/format.ts
- apps/core/src/plugins/builtin/local-tools.ts
- PROJECT.md
- apps/core/tests/tools/local-apply-patch-mcp-credentials.test.ts
- apps/core/src/transcript/transcript-store.ts
- packages/bash-safety/src/analyze/segment.ts
- apps/core/tests/tool-server-create-tool-server.test.ts
- apps/core/tests/mcp/registry.test.ts
- apps/core/src/mcp/registry.ts
- packages/bash-safety/tests/bash-safety.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84454e00cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Follow-up review findings addressed in
Validation: Core 1,302 tests, monorepo harness, all-workspace typecheck, remote-runner build, lint, format, and |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8e94bb6d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| resolvedCandidate === protectedPath || | ||
| relative(protectedPath, resolvedCandidate).split(sep)[0] !== ".." | ||
| ) { |
There was a problem hiding this comment.
For recursive consumers such as tar, zip, find, or grep -R, this condition only rejects a candidate equal to or below the protected path. A command such as tar -cf /tmp/data.tar /data is therefore approved when /data/secret is protected, because relative('/data/secret', '/data') is .., even though the command traverses and archives the OAuth credentials beneath that directory without dangerouslyAllow. Reject protected-path ancestors when the selected command can recurse into them.
Useful? React with 👍 / 👎.
1 participant
Summary
tools mcp.*management callablestool_search, and Claude Code nativeToolSearchmetadataBehavior Notes
mcp.reloadDATA_DIR/secret/mcp-oauthwhen a server is removedplugin_<plugin>_<tool>model-facing names; no compatibility aliases are includedtool_search; Claude Code uses nativeToolSearchover the same complete catalogValidation
bun run build:remote-runnerinapps/corebun testinapps/core(1,293 tests)bun testbun run typecheckbun run lintbun run fmt:checkgit diff --check main...HEADSummary by CodeRabbit