Skip to content

fix(everything): keep server instructions in step with capability-gated tools - #4835

Open
aton-of-data wants to merge 1 commit into
modelcontextprotocol:mainfrom
aton-of-data:instructions-capability-gated-tools
Open

aton-of-data wants to merge 1 commit into
modelcontextprotocol:mainfrom
aton-of-data:instructions-capability-gated-tools

Conversation

@aton-of-data

Copy link
Copy Markdown

Fixes #4792, and extends it: the mismatch covers six tools, not two, and two of the references are unconditional imperatives rather than caveated ones.

Description

src/everything/docs/instructions.md named capability-gated tools as if every client could call them. This qualifies those references, adds a Capability-Gated Tools section listing the gated set with the capability each one needs, and adds two tests that derive that set from registerConditionalTools so the document cannot drift from the code again.

Server Details

  • Server: everything
  • Changes to: server instructions (docs/instructions.md) and tests

Motivation and Context

Why the drift is structural, not a typo. createServer reads the instructions once (server/index.ts:37, from docs/instructions.md via resources/index.ts:27) and passes them to the McpServer constructor at server/index.ts:75. That happens before oninitialized (server/index.ts:94), which is where registerConditionalTools (tools/index.ts:45-55) runs and where client capabilities first become known. So the instructions are one fixed string for every client, while the tool catalog is not. Nothing held the two together, and they had come apart.

What the instructions said. Four lines in the pre-change file referenced a gated tool:

  • docs/instructions.md:8 — "Use get-roots-list to see client workspace roots before file operations"
  • docs/instructions.md:24 — "Check get-roots-list output to understand the client's workspace context"
  • docs/instructions.md:17,18 — the two the issue names; they state the capability requirement, but read as "the call needs the capability" rather than "the tool is not in your catalog".

Lines 8 and 24 are the sharper half: they are unconditional imperatives, and an agent that follows them on a client declaring no roots capability calls a tool that is absent from tools/list.

The gated set is six. Driving the built server over InMemoryTransport and calling tools/list:

client capabilities tools returned
{} 13
roots, sampling, elicitation.url, tasks.requests.sampling.createMessage, tasks.requests.elicitation.create 19

The six that appear only in the second run are get-roots-list, trigger-sampling-request, trigger-elicitation-request, trigger-url-elicitation, trigger-sampling-request-async and trigger-elicitation-request-async. The new table in instructions.md is exactly that set.

One correction to the issue. #4792 infers the gated set from membership in registerConditionalTools, which gives seven. simulate-research-query is the seventh, and it is not gated: registerSimulateResearchQueryTool (tools/simulate-research-query.ts:236-241) reads clientCapabilities.elicitation only to decide the input_required flow, then calls server.experimental.tasks.registerToolTask unconditionally. It is in registerConditionalTools because it registers after initialization, not because it is conditional — which is why the no-capability count above is 13 and not 12.

How Has This Been Tested?

npm install && npm run build && npm test in src/everything, on Node 22.22.2. tsc clean; vitest goes from 107 passing to 109 passing.

The two new tests fail against the pre-change instructions.md and pass after it, which is the point of adding them:

× documents exactly the tools that client capabilities gate
  AssertionError: instructions.md has no "Capability-Gated Tools" section

× never tells an agent to use a capability-gated tool unconditionally
  AssertionError: mention a gated tool without saying it may be absent
  + [
  +   "- Use `get-roots-list` to see client workspace roots before file operations",
  +   "- Sampling requests (`trigger-sampling-request`) require client sampling capability",
  +   "- Elicitation requests (`trigger-elicitation-request`) require client elicitation capability",
  +   "- Check `get-roots-list` output to understand the client's workspace context",
  + ]

Neither test hard-codes the gated list. Both compute it as the difference between the tools registerConditionalTools registers with every gating capability declared and the tools it registers with {}, so a tool that becomes gated, stops being gated, or is added gated and left undocumented moves the assertion on its own.

The 13 and 19 counts above come from a throwaway script against dist/ using InMemoryTransport and a real Client, not from the tests; it is not included in the diff.

Not tested with an LLM client. I verified the catalog the server returns and the instructions it ships, not how a model behaves on reading them, so I have left that checklist box unticked rather than tick it on a weaker check.

Also unticked: the README box. src/everything/README.md does not enumerate these tools, so there was nothing to bring into line there.

Breaking Changes

None. No tool, schema, capability or configuration changes; the only runtime difference is the text of the instructions field.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly — the README does not list these tools; see above
  • I have tested this with an LLM client — see above
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

prettier --check passes on docs/instructions.md. __tests__/registrations.test.ts was already outside prettier's style before this change (single quotes throughout), so the new block matches the file rather than the formatter; I have not reformatted the file.

Additional context

An alternative worth naming and rejecting: composing the instructions per session, so each client is told about exactly its own tools. That is the better answer in principle and it is not reachable here — instructions is a constructor argument (server/index.ts:75), fixed before capabilities are known. Making it per-session would need an SDK change, so this PR keeps the string static and makes it true for every client instead.

AI disclosure, since the repo works with agents: written with Claude. Scope is the whole diff — the instructions.md edits and both new tests — plus the diagnosis and the reproduction above. I reviewed it, and every figure quoted here was executed rather than inferred.

…ed tools

The instructions string is read once in the server factory and handed to
the McpServer constructor, before oninitialized runs and client
capabilities are known, so every client receives the same text. Six of
the server's tools are registered only when the client declared a
matching capability, and the instructions referred to three of them --
two as unconditional imperatives. A client that declares no capabilities
gets 13 tools in tools/list and none of the six, so an agent following
the instructions calls a tool that is not in its catalog.

Document the gated set and the capability each one needs, qualify the
remaining references, and add two tests that derive the gated set from
registerConditionalTools so the document cannot drift from it again.

This branch has not been deployed

No deployments
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.

everything server: instructions unconditionally reference sampling/elicitation tools that only exist when the client declares those capabilities

1 participant