Choose which tools an instance publishes - #40
Merged
Merged
Conversation
The public surface should be a decision somebody makes, not everything that happens to be implemented. `MCP_TOOL_GROUPS` names the groups to register; unset registers all 59, so a local stdio user is unaffected. **A registration switch, not a documentation one.** Listing a subset on a web page while the server still answers all 59 is the same divergence that let this repo advertise Cypher tools it had not registered: what the server offers has to be one fact, and the only way to be sure is for the unwanted tool not to exist on the instance. **Three cases differ on purpose**, because the dangerous failure is a restriction that silently becomes "everything": unset registers all, set and empty throws, an unknown name throws and says which. Failing to start is loud and recoverable; quietly serving the full surface on a public endpoint is neither. **The instructions follow the same list.** This is the part I nearly shipped wrong, and it is the bug I had just spent two PRs removing. A server restricted to search and pathway would still have told every client about Analysis, Export, Interactors and Utilities, because the category list was prose. It is now built from the same `ToolGroup` values that drive registration. Then the general check found one more: the recommended-workflow step 4 named `reactome_analyze_identifiers` and would have survived omitting the analysis group. So the test does not check the category list -- it extracts every `reactome_*` reference from the *whole* instruction text and requires each to be a tool this instance registered, for the full server and a restricted one. The workflow steps are now tied to their groups and numbered at render time, so the list has no gap either. Two structural guards: every registered tool belongs to exactly one group -- an ungrouped tool could not be switched off and nobody would find out until it was published somewhere it should not be -- and `TOOL_GROUPS` is the only list, iterated by `registerAllTools` rather than restating the registrars. `MCP_TOOL_GROUPS` is read where it is used rather than captured in config.ts. Every other value there is read once at import, which is right for something fixed at boot, but a captured copy is a second place the value lives -- and it made the switch untestable, which is how I noticed. Verified by sabotage: making the empty value fall back to all groups fails exactly the test that says it must not, and detaching the enrichment step from its group fails exactly the drift check. 115 tests, lint/format/typecheck/build clean under node:22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A pass over the finished PR rather than the pieces as I wrote them. Three
findings, and the first two are the same mistake I had just spent three PRs
removing, committed inside the feature built to prevent it.
**1. Resources ignored the switch entirely.** `registerAllResources` ran
unconditionally, so an instance that withheld the `analysis` tools still
served `reactome://analysis/{token}` -- and `reactome://pathway/{id}/diagram`
survived omitting `export`, which is the expensive one. A capability moved to
a URI is not a capability withheld. Resources now carry a group each, and a
URI missing from that table throws at registration rather than defaulting to
always-on, so a new resource cannot escape the switch by nobody remembering
it exists.
**2. The drift test only looked at tool names.** It matched `reactome_*` and
not `reactome://*`, so the instructions could still advertise a withheld
resource. Widened -- and it immediately failed, naming
`reactome://analysis/{token}` in the Resources section, which was still
prose after the category list and the workflow steps had been made
group-aware. Fourth instance of one divergence, found only because the check
was widened rather than trusted.
**3. "The server refuses to start" was false on the transport that is
deployed.** `createServer()` is called **per session** over HTTP, so a bad
`MCP_TOOL_GROUPS` let the process bind, answer `/health` with "ok", and fail
every session instead. True over stdio, where createServer runs once at boot
-- and stdio is the one I tested. Exactly the shape of the gate call site I
missed in #39: describing the entrypoint I was not running.
`startHttpServer` now validates before anything binds, and because it throws
synchronously it went past the `.catch` in the entrypoint and killed the
process with a raw stack trace; that is now routed through the logger.
Verified against the built image: unknown group and empty value each log one
line and exit 1, a good value binds.
Also: `createServer` resolves the group list **once** and passes it to the
instructions, the tools and the resources, instead of all three asking the
environment independently. They would agree today. So did the four copies of
the Cypher gate, until one did not.
Verified by sabotage: removing the resource gate fails the withholding test;
the earlier two hold.
118 tests, lint/format/typecheck/build clean under node:22.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The public surface should be a decision somebody makes, not everything that happens to be implemented.
MCP_TOOL_GROUPSnames the groups to register; unset registers all 59, so a local stdio user is unaffected.searchexportpathwayinteractorsentitygsaanalysisutilitiesA registration switch, not a documentation one
Listing a subset on a web page while the server still answers all 59 is the same divergence that let this repo advertise Cypher tools it had not registered. What the server offers has to be one fact, and the only way to be sure is for the unwanted tool not to exist on the instance. An omitted group is absent from
tools/listand from the instructions.Three cases differ on purpose
The dangerous failure is a restriction that silently becomes "everything" — the config file still claims to be restricted while the endpoint serves the lot. Failing to start is loud and recoverable; that is not.
The instructions follow the same list
This is the part I nearly shipped wrong, and it is the bug I had just spent two PRs removing. A server restricted to
search,pathwaywould still have told every client about Analysis, Export, Interactors and Utilities, because the category list was prose. It is now built from the sameToolGroupvalues that drive registration.Then the general check found one more: recommended-workflow step 4 named
reactome_analyze_identifiersand would have survived omitting the analysis group. So the test does not check the category list — it extracts everyreactome_*reference from the whole instruction text and requires each to be a tool this instance registered, for the full server and for a restricted one. Workflow steps are now tied to their groups and numbered at render time, so the list has no gap either.Structural guards
TOOL_GROUPSis the only list —registerAllToolsiterates it rather than restating the registrars, so a group cannot be defined and then forgotten at the call site.MCP_TOOL_GROUPSis read where it is used, not captured inconfig.ts. Every other value there is read once at import, which is right for something fixed at boot — but a captured copy is a second place the value lives, and it made the switch untestable, which is how I noticed.Verification
Sabotage, each against the test written for it:
115 tests, lint/format/typecheck/build clean under
node:22.Suggested deployment value
For the public instance:
search,pathway,entity,utilities,analysis— the cheap cached reads plus enrichment, whose inputs #38 bounded. It leaves outexport(PDF and diagram generation are the expensive calls and nobody has measured what they cost),interactorsandgsa(both fan out to third parties, though Reactome's own public REST API already exposes that, so including them changes nothing about exposure). Easy to widen later — it is one environment variable and a restart.🤖 Generated with Claude Code
Adversarial review of the finished PR
A pass over the whole thing rather than the pieces as I wrote them. Three findings — the first two are the same mistake I had just spent three PRs removing, committed inside the feature built to prevent it.
1. Resources ignored the switch entirely.
registerAllResourcesran unconditionally, so an instance withholding theanalysistools still servedreactome://analysis/{token}, andreactome://pathway/{id}/diagramsurvived omittingexport— the expensive group. A capability moved to a URI is not a capability withheld. Resources now carry a group each, and a URI missing from that table throws at registration rather than defaulting to always-on.2. The drift test only looked at tool names. It matched
reactome_*and notreactome://*. Widened, it immediately failed — namingreactome://analysis/{token}in the instructions' Resources section, still prose after the category list and workflow steps had been made group-aware. Fourth instance of one divergence, found only because the check was widened rather than trusted.3. "The server refuses to start" was false on the deployed transport.
createServer()is called per session over HTTP, so a badMCP_TOOL_GROUPSlet the process bind, answer/healthwith"ok", and fail every session instead. True over stdio, wherecreateServerruns once at boot — and stdio is the one I tested. Same shape as the gate call site I missed in #39: describing the entrypoint I was not running.startHttpServernow validates before anything binds. Because it throws synchronously it went straight past the.catchin the entrypoint and killed the process with a raw stack trace, so that is routed through the logger too. Verified against the built image:Also:
createServerresolves the group list once and passes it to the instructions, the tools and the resources, rather than all three asking the environment independently. They would agree today. So did the four copies of the Cypher gate, until one did not.Sabotage: removing the resource gate fails the withholding test; the two earlier sabotages still hold.
118 tests, lint/format/typecheck/build clean.