fix(everything): keep server instructions in step with capability-gated tools - #4835
Open
aton-of-data wants to merge 1 commit into
Open
aton-of-data wants to merge 1 commit into
aton-of-data wants to merge 1 commit into
Conversation
…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
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
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.
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.mdnamed capability-gated tools as if every client could call them. This qualifies those references, adds aCapability-Gated Toolssection listing the gated set with the capability each one needs, and adds two tests that derive that set fromregisterConditionalToolsso the document cannot drift from the code again.Server Details
docs/instructions.md) and testsMotivation and Context
Why the drift is structural, not a typo.
createServerreads the instructions once (server/index.ts:37, fromdocs/instructions.mdviaresources/index.ts:27) and passes them to theMcpServerconstructor atserver/index.ts:75. That happens beforeoninitialized(server/index.ts:94), which is whereregisterConditionalTools(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— "Useget-roots-listto see client workspace roots before file operations"docs/instructions.md:24— "Checkget-roots-listoutput 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
rootscapability calls a tool that is absent fromtools/list.The gated set is six. Driving the built server over
InMemoryTransportand callingtools/list:{}roots,sampling,elicitation.url,tasks.requests.sampling.createMessage,tasks.requests.elicitation.createThe six that appear only in the second run are
get-roots-list,trigger-sampling-request,trigger-elicitation-request,trigger-url-elicitation,trigger-sampling-request-asyncandtrigger-elicitation-request-async. The new table ininstructions.mdis exactly that set.One correction to the issue. #4792 infers the gated set from membership in
registerConditionalTools, which gives seven.simulate-research-queryis the seventh, and it is not gated:registerSimulateResearchQueryTool(tools/simulate-research-query.ts:236-241) readsclientCapabilities.elicitationonly to decide theinput_requiredflow, then callsserver.experimental.tasks.registerToolTaskunconditionally. It is inregisterConditionalToolsbecause 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 testinsrc/everything, on Node 22.22.2.tscclean; vitest goes from 107 passing to 109 passing.The two new tests fail against the pre-change
instructions.mdand pass after it, which is the point of adding them:Neither test hard-codes the gated list. Both compute it as the difference between the tools
registerConditionalToolsregisters 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
13and19counts above come from a throwaway script againstdist/usingInMemoryTransportand a realClient, 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.mddoes 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
instructionsfield.Types of changes
Checklist
prettier --checkpasses ondocs/instructions.md.__tests__/registrations.test.tswas 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 —
instructionsis 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.mdedits and both new tests — plus the diagnosis and the reproduction above. I reviewed it, and every figure quoted here was executed rather than inferred.