Skip to content

Scope MCP tool availability to referenced servers only - #334657

Open
luotianyiismywife wants to merge 2 commits into
microsoft:mainfrom
luotianyiismywife:fix/334569-ask-mode-mcp-toolset-scoped
Open

Scope MCP tool availability to referenced servers only#334657
luotianyiismywife wants to merge 2 commits into
microsoft:mainfrom
luotianyiismywife:fix/334569-ask-mode-mcp-toolset-scoped

Conversation

@luotianyiismywife

Copy link
Copy Markdown

Description

Fixes #334569 — MCP tool calls intermittently rejected with "disabled by the user" while the cache shows enabled and the server log shows execution.

This is the conservative alternative to #334641. Both PRs fix the same root cause; this one scopes the fix to the referenced MCP server only, preserving the authorization boundary. Maintainers can choose either approach (see the issue for the comparison).

Root Cause

In ask mode, MCP tools can only enter availableTools via request.toolReferences (the askAgentToolFilter in askAgentIntent.ts). When the user references an MCP server (via the @ picker), the toolset is expanded into a static snapshot at request-parse time (chatRequestParser.ts):

const value = Array.from(toolset.getTools()).map(t => ...);

MCP servers connect and discover their tools asynchronously. If the request is parsed while discovery is still in progress, the snapshot only contains the tools registered up to that point. Tools discovered later are registered in the tools service but never enter availableTools — the model's calls to them are rejected with Tool ... is currently disabled by the user.

Fix (scoped to the referenced server)

In askAgentIntent.ts:

  1. Compute the set of referenced MCP servers from request.toolReferences — for each referenced tool that is currently registered with an LanguageModelToolMCPSource, add that server's identity (label + name) to the set.
  2. In the filter, a currently-registered MCP tool is enabled only if it belongs to one of the referenced servers (matching via LanguageModelToolMCPSource). Tools from other MCP servers the user never referenced stay unavailable — the authorization boundary is preserved.
const referencedMcpServerKeys = new Set<string>();
for (const ref of request.toolReferences) {
    const referencedTool = toolsService.getTool(ref.name);
    const source = referencedTool?.source;
    if (source instanceof LanguageModelToolMCPSource) {
        referencedMcpServerKeys.add(mcpServerKey(source.label, source.name));
    }
}

Why not just enable all MCP tools (see #334641)

The request snapshot is both a timing mechanism and an authorization boundary: it limits the model to the servers the user explicitly referenced. Enabling every mcp-tagged tool whenever the user references any MCP server would expose the tools of all configured MCP servers (e.g. in a multi-server setup, referencing Firefox would also expose an unrelated IDA/GitKraken server's tools). This PR fixes the timing issue without widening that boundary.

Validation

  • askAgentIntent.spec.ts — 8 unit tests covering:
    • codesearch-tagged and directly referenced tools still included
    • MCP tools of a referenced server included (snapshot case)
    • MCP tools of a non-referenced server excluded
    • MCP tools without a resolvable source excluded (conservative fallback)
    • no references / non-MCP references → MCP tools excluded
  • All 8 tests pass (vitest run src/extension/intents/test/node/askAgentIntent.spec.ts).

Related

When the user references an MCP server in ask mode, the toolReferences
in the request are a snapshot taken when the request was parsed. If the
MCP server is still discovering its tools at that point, the snapshot
contains only the tools registered so far, and tools discovered later
are reported as "disabled by the user" when the model tries to call
them.

Include all currently registered tools of the *referenced* MCP server
(identified via LanguageModelToolMCPSource) rather than all MCP tools,
so availability reflects the live registration state without widening
the scope to MCP servers the user never referenced.

Fixes microsoft#334569

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The server key is not uniquely scoped and the refactor introduces a typecheck failure.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Scopes Ask mode MCP tool availability to explicitly referenced servers.

Changes:

  • Resolves referenced MCP servers and includes their newly discovered tools.
  • Adds unit coverage for inclusion and exclusion scenarios.
File summaries
File Description
askAgentIntent.ts Adds server-scoped MCP filtering.
askAgentIntent.spec.ts Tests MCP filtering behavior.
Review details

Suppressed comments (1)

extensions/copilot/src/extension/intents/node/askAgentIntent.ts:75

  • label and the server-announced name are not a unique MCP server identity. Two separately configured instances can share both values, while VS Code distinguishes MCP sources with collectionId plus definitionId (src/vs/workbench/contrib/chat/common/tools/languageModelToolsService.ts:139-143). Referencing one such instance will therefore add the same key for both and expose the other instance's tools, breaking the authorization boundary this change is intended to preserve. Please carry a stable source/toolset identity through the API/request and match on that instead of display metadata.
/** Stable identity of an MCP server, for matching tools to the server that published them. */
function mcpServerKey(label: string, name: string): string {
	return `${label}\u0000${name}`;
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread extensions/copilot/src/extension/intents/node/askAgentIntent.ts
Copilot review feedback:

1. label + name are not a unique MCP server identity: two separately
   configured servers can share both, so referencing one would have
   exposed the other instances tools. Match on collectionId +
   definitionId instead, which the workbench already uses as the
   stable ToolDataSource key for MCP tools.

2. Remove the stale lookForTags declaration in getTools, which is now
   unused and would fail the extension typecheck (noUnusedLocals).

This adds collectionId and definitionId to the proposed
LanguageModelToolMCPSource API so extensions can disambiguate MCP
servers, and threads the values through the ext host converters and
the test shim.
@luotianyiismywife

Copy link
Copy Markdown
Author

Copilot review feedback addressed (commit 9a70720)

  1. Stale lookForTags declaration removed from getTools (would have failed the extension typecheck via noUnusedLocals).

  2. MCP server identity hardened: LanguageModelToolMCPSource now carries collectionId + definitionId (the stable identity the workbench already uses for ToolDataSource keys), and askAgentToolFilter matches on that instead of label + name - so two separately configured servers sharing a label/name can no longer leak into each other's scope. Added a dedicated test for this case (9 tests passing).

copilot-pull-request-reviewer please re-review the updated changes.

@luotianyiismywife

Copy link
Copy Markdown
Author

copilot-pull-request-reviewer please re-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP tool calls intermittently rejected "disabled by the user" while cache shows enabled and server log shows execution

3 participants