fix(mcp): compute operations-profile tools lazily so late-registered ops surface (#1562)#1579
Merged
Conversation
…ops surface The MCP operations profile walked OPERATION_FUNCTION_MAP once at registration time (registerMcpProfile), before components register their operations via server.registerOperation in startOnMainThread. Those late-registered ops (e.g. the built-in agent's agent_prompt) never became MCP tools, so listing them in mcp.operations.allow silently had no effect and tools/call returned -32601. Replace the one-time snapshot with a lazy per-profile tool provider: the operations profile installs a provider that the registry consults on every tools/list / tools/call, walking the live OPERATION_FUNCTION_MAP and applying the allow/deny filter at request time. This removes the boot-ordering dependence and stays consistent as components load. The generic provider mechanism (setProfileToolProvider) merges with statically-registered tools, with static registrations winning on a name collision — so the built-in agent's curated MCP tools (PR #1561) remain a deliberate curated surface and are unaffected. Closes #1562 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-model review (Gemini + Codex both flagged) — the flat name->def registry is a single namespace shared by both MCP profiles, so an unscoped getTool let a static tool in one profile shadow a same-named provider tool in another: the shadowed tool listed but was uncallable (transport rejected the wrong-profile def as Unknown tool). getTool now takes the caller's profile and resolves profile-scoped (static-of-profile wins over its provider, then the provider, then any static by name); dispatchToolsCall passes request.profile. Also drop the module-level toolDefCache — tools/list isn't a hot path, so the provider rebuilds the (cheap, stateless) defs per call rather than carrying cross-test/boot-lived module state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Contributor
|
Reviewed; no blockers found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The MCP operations profile (
components/mcp/tools/operations.ts→registerOperationsTools) walkedOPERATION_FUNCTION_MAPonce at registration time and registered one tool per allowed op. But components register their operations viaserver.registerOperationduringstartOnMainThread, which runs after the profile is built. Those late-registered ops (e.g. the built-in agent'sagent_prompt) never became MCP tools, so listing them inmcp.operations.allowsilently had no effect andtools/callreturned-32601 Unknown tool.Closes #1562.
Fix
Adopts the issue's Option 1 (lazy) — the most robust, order-independent choice.
ProfileToolProviderthe registry consults on everytools/list/tools/call. The operations provider walks the liveOPERATION_FUNCTION_MAPand applies the allow/deny filter at request time, so component-registered ops surface as soon as they exist. No boot-ordering dependence.setProfileToolProvider) merges provider tools with statically-registered tools, deduping by name; static registrations win on collision — so the built-in agent's curated MCP tools (feat(agent): expose the built-in agent over MCP (curated tools) (#626) #1561) remain a deliberate curated surface and are unaffected.tools/listisn't a hot path, so no module-level cache to leak or stale across tests/boots.Cross-model review fix (2nd commit)
Gemini + Codex both flagged that the flat
name→defregistry is a single namespace shared by both profiles, so an unscopedgetToollet a static tool in one profile shadow a same-named provider tool in another — the shadowed tool listed but was uncallable (transport rejected the wrong-profile def).getToolnow takes the caller's profile and resolves profile-scoped;dispatchToolsCallpassesrequest.profile. Also dropped the module-leveltoolDefCache(not a hot path; avoids cross-test/boot module state).Tests
unitTests/components/mcp/toolRegistry.test.js— provider merge, static-wins-on-collision, profile-scopedgetTool, cache invalidation.unitTests/components/mcp/tools/operations.test.js— late-registered op surfaces after provider install; allow/deny applied per request.🤖 Generated with Claude Code