From 4866fb57ec9d1a6dd9c96b7c9697bb157a7516f5 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 19:05:57 +0000 Subject: [PATCH 1/2] Choose which tools an instance publishes, and say only what it published 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 --- CHANGELOG.md | 10 +++ README.md | 32 +++++++++ src/config.ts | 10 +++ src/instructions.ts | 86 ++++++++++++++++++----- src/tools/index.ts | 104 ++++++++++++++++++++++++--- tests/tool-groups.test.ts | 143 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 357 insertions(+), 28 deletions(-) create mode 100644 tests/tool-groups.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index c7ca999..9fbe4d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ All notable changes to this project are documented here. This project adheres to ## [Unreleased] +### Added + +- **`MCP_TOOL_GROUPS` — choose which tools an instance publishes.** All 59 register by default, so a local stdio user is unaffected; a hosted instance names the groups it means to serve (`search`, `pathway`, `entity`, `analysis`, `export`, `interactors`, `gsa`, `utilities`). + + It is a **registration** switch, not a documentation one. An omitted group's tools are not registered, do not appear in `tools/list`, and are not mentioned in the instructions the server sends on connection. A tool described nowhere but still answering is the divergence that let this server advertise Cypher tools it had not registered. + + Three cases differ on purpose, because the dangerous failure is a restriction that silently becomes "everything": unset registers all; set-and-empty refuses to start; an unknown group refuses to start and names the typo. Failing to start is loud and recoverable — quietly serving the full surface on a public endpoint is neither. + + Two tests hold the pieces together: 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 the instructions name no `reactome_*` tool the instance did not register — checked for the full server and for a restricted one, over the whole text rather than just the category list, because the recommended-workflow prose names tools too and drifted exactly that way while this was being written. + ### Removed - **BREAKING: all graph database access.** The three `reactome_cypher_*` tools, the `reactome://graph/schema` resource, the Cypher section of the server instructions, the startup schema prefetch and the `neo4j-driver` dependency are gone. `NEO4J_URI`, `NEO4J_USER`, `NEO4J_PASSWORD`, `NEO4J_DATABASE`, `CYPHER_QUERY_TIMEOUT_MS` and `MCP_ALLOW_CYPHER` are inert — nothing reads them. diff --git a/README.md b/README.md index 07ada3e..43eac07 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,38 @@ All configuration is via environment variables — pass them in the `env` block | `REACTOME_ANALYSIS_SERVICE_URL` | derived from `REACTOME_BASE_URL` | Fine-grained override for the Analysis Service only. | | `LOG_LEVEL` | `info` | `debug` / `info` / `warn` / `error`. Logs are JSON on stderr; stdout is reserved for the MCP protocol. | +## Choosing which tools to publish + +All 59 tools register by default, which is what you want locally. A hosted +instance usually wants fewer, so `MCP_TOOL_GROUPS` selects groups: + +``` +MCP_TOOL_GROUPS=search,pathway,entity,utilities,analysis +``` + +| Group | Tools | +|-------|-------| +| `search` | 7 | +| `pathway` | 8 | +| `entity` | 8 | +| `analysis` | 9 | +| `export` | 9 | +| `interactors` | 6 | +| `gsa` | 5 | +| `utilities` | 7 | + +This is a **registration** switch, not a documentation one: an omitted group's +tools are not registered, are not listed by `tools/list`, and are not +mentioned in the instructions the server sends on connection. A tool that is +described nowhere but still answers is the failure this avoids. + +Three cases behave differently on purpose, because the dangerous one is a +restriction that quietly becomes "everything": + +- **unset** — all groups +- **set and empty** — the server refuses to start +- **set with an unknown name** — the server refuses to start, naming the typo + ## Usage ### With Claude Desktop diff --git a/src/config.ts b/src/config.ts index 9f95d8b..0da7109 100644 --- a/src/config.ts +++ b/src/config.ts @@ -126,6 +126,16 @@ export const MCP_HTTP_HOST = process.env.MCP_HTTP_HOST ?? "127.0.0.1"; export const MCP_SESSION_TTL_MS = parsePositiveInt(process.env.MCP_SESSION_TTL_MS, 30 * 60_000); /** Ceiling on concurrent sessions, so a client loop cannot exhaust memory. */ +/** + * `MCP_TOOL_GROUPS` selects which groups of tools an instance registers. + * + * It is deliberately *not* a constant here. Every other value in this file is + * read once at import, which is right for something fixed at boot -- but it + * also means a captured copy, and a captured copy is a second place the value + * lives. `resolveToolGroups()` in tools/index.ts reads the environment itself, + * next to the code that acts on it, and documents the three cases. + */ + export const MCP_MAX_SESSIONS = parsePositiveInt(process.env.MCP_MAX_SESSIONS, 256); /** diff --git a/src/instructions.ts b/src/instructions.ts index 5cf3cc0..78ca211 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -1,15 +1,11 @@ +import { resolveToolGroups, type ToolGroup } from "./tools/index.js"; + const CORE_INSTRUCTIONS = ` This server exposes the Reactome pathway knowledgebase (https://reactome.org) to LLM clients. Reactome is a manually curated, peer-reviewed database of biological pathways: reactions grouped into pathways grouped into hierarchies, annotated with participants (proteins, complexes, small molecules), regulation, literature, species, and disease. # Tool categories -- **Search** (\`reactome_search*\`) — full-text search across pathways, reactions, entities, genes, compounds. Start here when the user gives a free-text term. -- **Pathways** (\`reactome_get_pathway\`, \`reactome_top_pathways\`, \`reactome_pathway_ancestors\`, \`reactome_pathway_contained_events\`, \`reactome_events_hierarchy\`, \`reactome_pathways_for_entity\`) — navigate the pathway hierarchy. -- **Entities** (\`reactome_get_entity\`, \`reactome_complex_subunits\`, \`reactome_participants\`, \`reactome_reference_entities\`, \`reactome_complexes_containing\`) — inspect molecular participants. -- **Analysis** (\`reactome_analyze_identifier\`, \`reactome_analyze_identifiers\`) — gene/protein-list enrichment. Returns a **token**; pass it to \`reactome_get_analysis_result\`, \`reactome_analysis_found_entities\`, etc. to read results. -- **Interactors** (\`reactome_psicquic_*\`, \`reactome_static_interactors\`, \`reactome_interactor_pathways\`) — protein–protein interaction data. -- **Export** (\`reactome_export_*\`) — diagrams (PNG/SVG), SBGN, SBML, PDF reports, CSV/JSON analysis exports. -- **Utilities** (\`reactome_species\`, \`reactome_diseases\`, \`reactome_database_info\`, \`reactome_mapping_*\`, \`reactome_orthology\`, \`reactome_query\`). +__CATEGORY_LINES__ # Identifier conventions @@ -19,11 +15,7 @@ This server exposes the Reactome pathway knowledgebase (https://reactome.org) to # Recommended workflow -1. If the user gives a free-text term, call \`reactome_search\` first and pick the best match. -2. For a specific ID, use \`reactome_get_pathway\` (event/pathway) or \`reactome_get_entity\` (physical entity). -3. To dive into a pathway, follow up with \`reactome_pathway_contained_events\` and \`reactome_participants\`. -4. For enrichment analysis: \`reactome_analyze_identifiers\` → save the returned token → query the token-bearing endpoints. -5. Pathway details often include literature references (PubMed IDs) and summations — cite these in user-facing answers. +__WORKFLOW_STEPS__ # Resources (read via MCP \`resources/read\`) @@ -31,10 +23,68 @@ This server exposes the Reactome pathway knowledgebase (https://reactome.org) to - \`reactome://pathway/{id}\`, \`reactome://entity/{id}\`, \`reactome://analysis/{token}\` — templated. `.trim(); -export function buildServerInstructions(): string { - // One section, unconditionally. There used to be a Cypher section appended - // when a graph connection was configured, and it was the surface that got - // left behind when the tools were gated -- a server describing tools it had - // not registered. Both are gone. - return CORE_INSTRUCTIONS; +/** + * Workflow steps, each tied to the group whose tools it names. + * + * Numbered at render time rather than in the text: a step list with a gap in + * it, or one that cites a tool this instance withheld, is the same drift the + * category list had. `null` means the step names no tool and always applies. + */ +const WORKFLOW_STEPS: { group: ToolGroup | null; text: string }[] = [ + { + group: "search", + text: "If the user gives a free-text term, call `reactome_search` first and pick the best match.", + }, + { + group: "pathway", + text: "For a specific ID, use `reactome_get_pathway` (event/pathway) or `reactome_get_entity` (physical entity).", + }, + { + group: "pathway", + text: "To dive into a pathway, follow up with `reactome_pathway_contained_events` and `reactome_participants`.", + }, + { + group: "analysis", + text: "For enrichment analysis: `reactome_analyze_identifiers` → save the returned token → query the token-bearing endpoints.", + }, + { + group: null, + text: "Pathway details often include literature references (PubMed IDs) and summations — cite these in user-facing answers.", + }, +]; + +const CATEGORY_LINES: Record = { + search: `- **Search** (\`reactome_search*\`) — full-text search across pathways, reactions, entities, genes, compounds. Start here when the user gives a free-text term.`, + pathway: `- **Pathways** (\`reactome_get_pathway\`, \`reactome_top_pathways\`, \`reactome_pathway_ancestors\`, \`reactome_pathway_contained_events\`, \`reactome_events_hierarchy\`, \`reactome_pathways_for_entity\`) — navigate the pathway hierarchy.`, + entity: `- **Entities** (\`reactome_get_entity\`, \`reactome_complex_subunits\`, \`reactome_participants\`, \`reactome_reference_entities\`, \`reactome_complexes_containing\`) — inspect molecular participants.`, + analysis: `- **Analysis** (\`reactome_analyze_identifier\`, \`reactome_analyze_identifiers\`) — gene/protein-list enrichment. Returns a **token**; pass it to \`reactome_get_analysis_result\`, \`reactome_analysis_found_entities\`, etc. to read results.`, + interactors: `- **Interactors** (\`reactome_psicquic_*\`, \`reactome_static_interactors\`, \`reactome_interactor_pathways\`) — protein–protein interaction data.`, + export: `- **Export** (\`reactome_export_*\`) — diagrams (PNG/SVG), SBGN, SBML, PDF reports, CSV/JSON analysis exports.`, + utilities: `- **Utilities** (\`reactome_species\`, \`reactome_diseases\`, \`reactome_database_info\`, \`reactome_mapping_*\`, \`reactome_orthology\`, \`reactome_query\`).`, + gsa: `- **Gene set analysis** (\`reactome_gsa_*\`) — ReactomeGSA: available methods, data types, and searchable public expression datasets. Distinct from Analysis, which is over-representation on a list of identifiers.`, +}; + +/** + * The instructions a client reads on connecting. + * + * **It describes the groups this instance actually registered, and nothing + * else.** The last time these two facts were allowed to drift apart, the + * server told every client to call `reactome_cypher_query` on an instance + * that had declined to register it. That is invisible to any check which + * asks the server what it can do, because the answer *is* the claim. + * + * So the category list is built from the same `ToolGroup` values that drive + * registration, and `tests/tool-groups.test.ts` asserts a restricted + * instance does not mention what it withheld. + */ +export function buildServerInstructions(groups: ToolGroup[] = resolveToolGroups()): string { + const categoryLines = groups.map(group => CATEGORY_LINES[group]).join("\n"); + const steps = WORKFLOW_STEPS.filter( + step => step.group === null || groups.includes(step.group) + ).map((step, i) => `${i + 1}. ${step.text}`); + + return CORE_INSTRUCTIONS.replace("__CATEGORY_LINES__", categoryLines).replace( + "__WORKFLOW_STEPS__", + steps.join("\n") + ); } diff --git a/src/tools/index.ts b/src/tools/index.ts index 48da54f..faa4a2f 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -1,4 +1,5 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { logger } from "../logger.js"; import { z } from "zod"; import { nonEmptyString } from "../schemas.js"; import { contentClient } from "../clients/content.js"; @@ -50,24 +51,107 @@ function installToolWrapper(server: McpServer) { }; } +/** + * Every group of tools, and what registers it. + * + * A group is the unit a deployment can choose to publish. The map is the only + * list: `registerAllTools` iterates it rather than naming the registrars + * again, so a group cannot be defined here and forgotten at the call site, + * and `tests/tool-groups.test.ts` asserts every registered tool belongs to + * exactly one group -- a tool that escaped grouping would be unswitchable, + * and nobody would notice until it was published somewhere it should not be. + */ +export const TOOL_GROUPS = { + search: registerSearchTools, + pathway: registerPathwayTools, + entity: registerEntityTools, + analysis: registerAnalysisTools, + export: registerExportTools, + interactors: registerInteractorTools, + gsa: registerGsaTools, + utilities: registerUtilityTools, +} as const satisfies Record void>; + +export type ToolGroup = keyof typeof TOOL_GROUPS; + +export const ALL_TOOL_GROUPS = Object.keys(TOOL_GROUPS) as ToolGroup[]; + +/** + * Which groups of tools this instance registers, from `MCP_TOOL_GROUPS`. + * + * Unset registers everything, so a local stdio user is unaffected. A public + * deployment names the groups it means to publish. + * + * **This is a registration switch, not a documentation one.** A tool that is + * described nowhere but still answers is exactly the divergence that cost + * this repo a public Cypher surface and a server advertising tools it had not + * registered: what a server *offers* must be the same fact as what it *says*, + * and the only way to be sure is for the unwanted tool not to exist on the + * instance. + * + * **Three cases, deliberately different**, because the dangerous failure is a + * restriction that silently becomes "everything": + * + * unset -> all groups (the local default) + * set and empty -> throws + * set with a typo -> throws + * + * An unparseable restriction must never fall back to publishing more than was + * asked for. Failing to start is recoverable and loud; quietly serving the + * full surface on a public endpoint is neither. + * + * The environment is read here rather than captured in config.ts, so there is + * one copy of the value and it can be exercised directly. + */ +export function resolveToolGroups( + raw: string | undefined = process.env.MCP_TOOL_GROUPS +): ToolGroup[] { + if (raw === undefined) return ALL_TOOL_GROUPS; + + const requested = raw + .split(",") + .map(name => name.trim().toLowerCase()) + .filter(name => name.length > 0); + + if (requested.length === 0) { + throw new Error( + `MCP_TOOL_GROUPS is set but names no group. Unset it to register everything, ` + + `or name groups: ${ALL_TOOL_GROUPS.join(", ")}.` + ); + } + + const unknown = requested.filter(name => !(name in TOOL_GROUPS)); + if (unknown.length > 0) { + throw new Error( + `MCP_TOOL_GROUPS names unknown group(s): ${unknown.join(", ")}. ` + + `Known groups: ${ALL_TOOL_GROUPS.join(", ")}.` + ); + } + + // Deduplicated, and in the map's order rather than the caller's, so the + // registration order does not depend on how the variable was written. + return ALL_TOOL_GROUPS.filter(name => requested.includes(name)); +} + export function registerAllTools(server: McpServer) { installToolWrapper(server); - registerAnalysisTools(server); - registerPathwayTools(server); - registerSearchTools(server); - registerEntityTools(server); - registerExportTools(server); - registerInteractorTools(server); - registerGsaTools(server); - // No graph database tools. They were removed on 2026-09-21 when this // server became publicly hosted: Constitution Principle IV already said no // deployment holds a Neo4j connection, and a gate enforcing that is a gate // somebody can flip. Nothing here opens one now. - // Register utility tools directly here - registerUtilityTools(server); + const groups = resolveToolGroups(); + for (const group of groups) TOOL_GROUPS[group](server); + + if (groups.length < ALL_TOOL_GROUPS.length) { + const omitted = ALL_TOOL_GROUPS.filter(g => !groups.includes(g)); + logger.info("registering a subset of tool groups", { + registered: groups, + omitted, + source: "MCP_TOOL_GROUPS", + }); + } } /** diff --git a/tests/tool-groups.test.ts b/tests/tool-groups.test.ts new file mode 100644 index 0000000..6bce5b9 --- /dev/null +++ b/tests/tool-groups.test.ts @@ -0,0 +1,143 @@ +import { describe, it, expect } from "vitest"; +import { buildServerInstructions } from "../src/instructions.js"; +import { + ALL_TOOL_GROUPS, + TOOL_GROUPS, + registerAllTools, + resolveToolGroups, + type ToolGroup, +} from "../src/tools/index.js"; + +/** + * Which tools an instance publishes. + * + * This server is hosted publicly, and the public surface is a decision + * somebody makes rather than everything that happens to be implemented. + * `MCP_TOOL_GROUPS` is a *registration* switch on purpose: a tool documented + * nowhere but still answering is the same divergence that let this repo + * advertise Cypher tools it had not registered, and publish a graph schema + * nobody had chosen to expose. What the server offers has to be one fact. + */ + +function namesFor(register: (server: never) => void): string[] { + const names: string[] = []; + register({ + tool: (...args: unknown[]) => { + if (typeof args[0] === "string") names.push(args[0]); + return undefined; + }, + } as never); + return names; +} + +function registeredWith(groups: string | undefined): string[] { + const previous = process.env.MCP_TOOL_GROUPS; + if (groups === undefined) delete process.env.MCP_TOOL_GROUPS; + else process.env.MCP_TOOL_GROUPS = groups; + try { + return namesFor(registerAllTools); + } finally { + if (previous === undefined) delete process.env.MCP_TOOL_GROUPS; + else process.env.MCP_TOOL_GROUPS = previous; + } +} + +describe("tool groups", () => { + it("accounts for every tool, with no tool in two groups", () => { + // A tool that belongs to no group could not be switched off, and nobody + // would find out until it was published somewhere it should not be. + const all = namesFor(registerAllTools); + const grouped = ALL_TOOL_GROUPS.flatMap(g => namesFor(TOOL_GROUPS[g])); + + expect(all.length).toBe(59); + expect([...grouped].sort()).toEqual([...all].sort()); + expect(new Set(grouped).size).toBe(grouped.length); + }); + + it("registers everything when unset, so a local user is unaffected", () => { + expect(registeredWith(undefined)).toHaveLength(59); + }); + + it("registers only what is named", () => { + const names = registeredWith("search,pathway"); + expect(names).toContain("reactome_search"); + expect(names).toContain("reactome_get_pathway"); + expect(names).not.toContain("reactome_export_pdf"); + expect(names).not.toContain("reactome_analyze_identifiers"); + // The positive half matters: without it, a switch that registered + // *nothing* would satisfy every "not.toContain" here. + expect(names.length).toBeGreaterThan(10); + expect(names.length).toBeLessThan(59); + }); +}); + +describe("MCP_TOOL_GROUPS parsing", () => { + it("is order- and case-insensitive, and tolerates spacing", () => { + expect(resolveToolGroups("PATHWAY, search")).toEqual(["search", "pathway"] as ToolGroup[]); + }); + + it("deduplicates", () => { + expect(resolveToolGroups("search,search")).toEqual(["search"] as ToolGroup[]); + }); + + it("registers everything only when genuinely unset", () => { + expect(resolveToolGroups(undefined)).toEqual(ALL_TOOL_GROUPS); + }); + + it("refuses an empty value rather than falling back to everything", () => { + // The dangerous case. A restriction that degrades to "publish all" on a + // blank or whitespace value is worse than no restriction, because the + // config file still says it is restricted. + expect(() => resolveToolGroups("")).toThrow(/names no group/); + expect(() => resolveToolGroups(" , ")).toThrow(/names no group/); + }); + + it("refuses an unknown group rather than ignoring it", () => { + expect(() => resolveToolGroups("serch,pathway")).toThrow(/unknown group/); + }); + + it("names the known groups in the error, so the fix is in the message", () => { + expect(() => resolveToolGroups("nope")).toThrow(/search/); + }); +}); + +describe("instructions match what is registered", () => { + /** + * The strongest version of the check, because it does not care where in + * the text a tool is named. It extracts every `reactome_*` reference from + * the instructions and requires each one to be a tool this instance + * actually registered. + * + * The category list was the obvious place for these to drift. The + * recommended-workflow and resources prose name tools too, and would have + * drifted just as quietly — which is the whole lesson of the Cypher + * instructions: the surface that describes the server is a surface. + */ + const referencedTools = (text: string): string[] => { + const found = new Set(); + for (const match of text.matchAll(/reactome_[a-z_]*\*?/g)) found.add(match[0]); + return [...found]; + }; + + const satisfied = (reference: string, registered: string[]): boolean => + reference.endsWith("*") + ? registered.some(name => name.startsWith(reference.slice(0, -1))) + : registered.includes(reference); + + it("names no tool the full server does not register", () => { + const registered = namesFor(registerAllTools); + const dangling = referencedTools(buildServerInstructions(ALL_TOOL_GROUPS)).filter( + r => !satisfied(r, registered) + ); + expect(dangling).toEqual([]); + }); + + it("names no tool a restricted server does not register", () => { + const groups: ToolGroup[] = ["search", "pathway", "entity", "utilities"]; + const registered = groups.flatMap(g => namesFor(TOOL_GROUPS[g])); + const dangling = referencedTools(buildServerInstructions(groups)).filter( + r => !satisfied(r, registered) + ); + expect(dangling).toEqual([]); + }); +}); From e741a0c4e45999664395877ffa10d24f296ea3be Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 19:19:03 +0000 Subject: [PATCH 2/2] Adversarial review: the switch governed one surface out of three 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 --- CHANGELOG.md | 2 +- README.md | 8 ++-- src/http-server.ts | 16 ++++--- src/http.ts | 11 +++++ src/instructions.ts | 31 ++++++++++--- src/resources/index.ts | 56 +++++++++++++++++++++++- src/server.ts | 15 +++++-- src/tools/index.ts | 3 +- tests/tool-groups.test.ts | 91 +++++++++++++++++++++++++++++++++++---- 9 files changed, 204 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fbe4d7..86fc0f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ All notable changes to this project are documented here. This project adheres to - **`MCP_TOOL_GROUPS` — choose which tools an instance publishes.** All 59 register by default, so a local stdio user is unaffected; a hosted instance names the groups it means to serve (`search`, `pathway`, `entity`, `analysis`, `export`, `interactors`, `gsa`, `utilities`). - It is a **registration** switch, not a documentation one. An omitted group's tools are not registered, do not appear in `tools/list`, and are not mentioned in the instructions the server sends on connection. A tool described nowhere but still answering is the divergence that let this server advertise Cypher tools it had not registered. + It is a **registration** switch, not a documentation one. An omitted group's tools are not registered, do not appear in `tools/list`, are not mentioned in the instructions the server sends on connection, and neither are its **resources** — omitting `analysis` withholds `reactome://analysis/{token}` as well, since a capability moved to a URI is not a capability withheld. A tool described nowhere but still answering is the divergence that let this server advertise Cypher tools it had not registered. Three cases differ on purpose, because the dangerous failure is a restriction that silently becomes "everything": unset registers all; set-and-empty refuses to start; an unknown group refuses to start and names the typo. Failing to start is loud and recoverable — quietly serving the full surface on a public endpoint is neither. diff --git a/README.md b/README.md index 43eac07..b41b161 100644 --- a/README.md +++ b/README.md @@ -58,10 +58,12 @@ MCP_TOOL_GROUPS=search,pathway,entity,utilities,analysis | `gsa` | 5 | | `utilities` | 7 | -This is a **registration** switch, not a documentation one: an omitted group's +This is a **registration** switch, not a documentation one. An omitted group's tools are not registered, are not listed by `tools/list`, and are not -mentioned in the instructions the server sends on connection. A tool that is -described nowhere but still answers is the failure this avoids. +mentioned in the instructions the server sends on connection — and **nor are +its resources**: omitting `analysis` withholds `reactome://analysis/{token}` +too, because a capability moved to a URI is not a capability withheld. A tool +described nowhere but still answering is the failure this avoids. Three cases behave differently on purpose, because the dangerous one is a restriction that quietly becomes "everything": diff --git a/src/http-server.ts b/src/http-server.ts index 17dd5fd..ab2a963 100644 --- a/src/http-server.ts +++ b/src/http-server.ts @@ -22,9 +22,15 @@ if (!port) { process.exit(1); } -startHttpServer(port, MCP_HTTP_HOST).catch((error: unknown) => { - logger.error("fatal error starting http server", { - error: error instanceof Error ? error.message : String(error), +// `Promise.resolve().then(...)` rather than a bare call: startHttpServer +// validates the tool-group configuration synchronously, so a bad value threw +// past the .catch below and killed the process with a raw stack trace. Dying +// was correct; dying without the log line that says why was not. +Promise.resolve() + .then(() => startHttpServer(port, MCP_HTTP_HOST)) + .catch((error: unknown) => { + logger.error("fatal error starting http server", { + error: error instanceof Error ? error.message : String(error), + }); + process.exit(1); }); - process.exit(1); -}); diff --git a/src/http.ts b/src/http.ts index b535c74..224a68c 100644 --- a/src/http.ts +++ b/src/http.ts @@ -1,4 +1,5 @@ import { randomUUID } from "node:crypto"; +import { resolveToolGroups } from "./tools/index.js"; import { estimateAnalysisBodyBytes } from "./tools/limits.js"; import type { Server } from "node:http"; import type { Request, Response } from "express"; @@ -46,6 +47,16 @@ interface Session { const EXPRESS_JSON_LIMIT_BYTES = 102_400; export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Promise { + // Validate the tool-group configuration *here*, before anything binds. + // + // `createServer()` is called per session (see the session handler below), + // so a bad MCP_TOOL_GROUPS would otherwise let the process start, answer + // /health with "ok", and fail every individual session -- while the + // documentation said the server refuses to start. It does over stdio, + // where createServer runs once at boot. It did not here, which is the + // transport that is actually deployed. + resolveToolGroups(); + // Defaults to 127.0.0.1 and turns on DNS-rebinding protection for localhost // hosts, which is what stops a web page in the user's browser from driving // this server. diff --git a/src/instructions.ts b/src/instructions.ts index 78ca211..ddae761 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -19,8 +19,7 @@ __WORKFLOW_STEPS__ # Resources (read via MCP \`resources/read\`) -- \`reactome://species\`, \`reactome://species/main\`, \`reactome://diseases\`, \`reactome://database/info\` — orient yourself at session start. -- \`reactome://pathway/{id}\`, \`reactome://entity/{id}\`, \`reactome://analysis/{token}\` — templated. +__RESOURCE_LINES__ `.trim(); /** @@ -53,6 +52,25 @@ const WORKFLOW_STEPS: { group: ToolGroup | null; text: string }[] = [ }, ]; +/** + * Resource lines, tied to the group that registers each URI. + * + * The category list and the workflow steps were made group-aware first, and + * this was still prose -- so an instance without `analysis` withheld + * `reactome://analysis/{token}` and then listed it here anyway. Found by + * widening the drift test from tool names to resource URIs, which is the + * fourth place this same divergence has turned up. + */ +const RESOURCE_LINES: { group: ToolGroup; text: string }[] = [ + { + group: "utilities", + text: "`reactome://species`, `reactome://species/main`, `reactome://diseases`, `reactome://database/info` — orient yourself at session start.", + }, + { group: "pathway", text: "`reactome://pathway/{id}` — templated pathway details." }, + { group: "entity", text: "`reactome://entity/{id}` — templated entity details." }, + { group: "analysis", text: "`reactome://analysis/{token}` — templated analysis results." }, +]; + const CATEGORY_LINES: Record = { search: `- **Search** (\`reactome_search*\`) — full-text search across pathways, reactions, entities, genes, compounds. Start here when the user gives a free-text term.`, pathway: `- **Pathways** (\`reactome_get_pathway\`, \`reactome_top_pathways\`, \`reactome_pathway_ancestors\`, \`reactome_pathway_contained_events\`, \`reactome_events_hierarchy\`, \`reactome_pathways_for_entity\`) — navigate the pathway hierarchy.`, @@ -83,8 +101,11 @@ export function buildServerInstructions(groups: ToolGroup[] = resolveToolGroups( step => step.group === null || groups.includes(step.group) ).map((step, i) => `${i + 1}. ${step.text}`); - return CORE_INSTRUCTIONS.replace("__CATEGORY_LINES__", categoryLines).replace( - "__WORKFLOW_STEPS__", - steps.join("\n") + const resourceLines = RESOURCE_LINES.filter(line => groups.includes(line.group)).map( + line => `- ${line.text}` ); + + return CORE_INSTRUCTIONS.replace("__CATEGORY_LINES__", categoryLines) + .replace("__WORKFLOW_STEPS__", steps.join("\n")) + .replace("__RESOURCE_LINES__", resourceLines.join("\n")); } diff --git a/src/resources/index.ts b/src/resources/index.ts index 1bd9049..c393b83 100644 --- a/src/resources/index.ts +++ b/src/resources/index.ts @@ -1,8 +1,62 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { registerStaticResources } from "./static.js"; import { registerResourceTemplates } from "./templates.js"; +import { resolveToolGroups, type ToolGroup } from "../tools/index.js"; + +/** + * Which group each resource belongs to. + * + * Resources are the third surface that has to agree with `MCP_TOOL_GROUPS`, + * after the tools themselves and the server instructions. An instance that + * withholds `analysis` but still serves `reactome://analysis/{token}` has not + * restricted anything -- it has moved the same capability to a URI. The + * resources are the same fetches the tools make. + * + * A URI missing from this table throws at registration rather than + * defaulting to always-on, so a new resource cannot escape the switch by + * nobody remembering it exists. + */ +const RESOURCE_GROUPS: Record = { + "reactome://species": "utilities", + "reactome://species/main": "utilities", + "reactome://diseases": "utilities", + "reactome://database/info": "utilities", + "reactome://pathway/{id}": "pathway", + "reactome://pathway/{id}/diagram": "export", + "reactome://entity/{id}": "entity", + "reactome://analysis/{token}": "analysis", + "reactome://top-pathways/{species}": "pathway", + "reactome://events-hierarchy/{species}": "pathway", +}; + +export const RESOURCE_URIS = Object.keys(RESOURCE_GROUPS); + +/** The URI a `server.resource(...)` call registers, static or templated. */ +function uriOf(args: unknown[]): string | undefined { + // Static: (uri, uri, handler) -- the name and the URI are the same string. + if (typeof args[1] === "string") return args[1]; + // Templated: (name, ResourceTemplate, options, handler). + const template = args[1] as { uriTemplate?: { toString(): string } } | undefined; + return template?.uriTemplate?.toString(); +} + +export function registerAllResources(server: McpServer, groups: ToolGroup[] = resolveToolGroups()) { + const original = server.resource.bind(server); + (server as unknown as { resource: (...a: unknown[]) => unknown }).resource = ( + ...args: unknown[] + ) => { + const uri = uriOf(args); + const group = uri === undefined ? undefined : RESOURCE_GROUPS[uri]; + if (group === undefined) { + throw new Error( + `Resource ${uri ?? "(unrecognised registration)"} is not assigned to a tool group. ` + + `Add it to RESOURCE_GROUPS in src/resources/index.ts so MCP_TOOL_GROUPS can govern it.` + ); + } + if (!groups.includes(group)) return undefined; + return (original as (...a: unknown[]) => unknown)(...args); + }; -export function registerAllResources(server: McpServer) { registerStaticResources(server); registerResourceTemplates(server); } diff --git a/src/server.ts b/src/server.ts index 26f24fb..ab0ff7a 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1,5 +1,5 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { registerAllTools } from "./tools/index.js"; +import { registerAllTools, resolveToolGroups } from "./tools/index.js"; import { registerAllResources } from "./resources/index.js"; import { buildServerInstructions } from "./instructions.js"; @@ -17,13 +17,20 @@ export const SERVER_VERSION = "1.4.0"; * stdio entrypoint. */ export function createServer(): McpServer { + // Resolved once and passed to all three, rather than each asking the + // environment for itself. They would agree today -- they read the same + // variable -- but "the same fact, fetched independently in three places" + // is precisely the shape that let this server advertise tools it had not + // registered, and it agreed right up until it did not. + const groups = resolveToolGroups(); + const server = new McpServer( { name: SERVER_NAME, version: SERVER_VERSION }, - { instructions: buildServerInstructions() } + { instructions: buildServerInstructions(groups) } ); - registerAllTools(server); - registerAllResources(server); + registerAllTools(server, groups); + registerAllResources(server, groups); return server; } diff --git a/src/tools/index.ts b/src/tools/index.ts index faa4a2f..d011fee 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -133,7 +133,7 @@ export function resolveToolGroups( return ALL_TOOL_GROUPS.filter(name => requested.includes(name)); } -export function registerAllTools(server: McpServer) { +export function registerAllTools(server: McpServer, groups: ToolGroup[] = resolveToolGroups()) { installToolWrapper(server); // No graph database tools. They were removed on 2026-09-21 when this @@ -141,7 +141,6 @@ export function registerAllTools(server: McpServer) { // deployment holds a Neo4j connection, and a gate enforcing that is a gate // somebody can flip. Nothing here opens one now. - const groups = resolveToolGroups(); for (const group of groups) TOOL_GROUPS[group](server); if (groups.length < ALL_TOOL_GROUPS.length) { diff --git a/tests/tool-groups.test.ts b/tests/tool-groups.test.ts index 6bce5b9..a39a669 100644 --- a/tests/tool-groups.test.ts +++ b/tests/tool-groups.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; import { buildServerInstructions } from "../src/instructions.js"; +import { registerAllResources, RESOURCE_URIS } from "../src/resources/index.js"; import { ALL_TOOL_GROUPS, TOOL_GROUPS, @@ -113,9 +114,14 @@ describe("instructions match what is registered", () => { * drifted just as quietly — which is the whole lesson of the Cypher * instructions: the surface that describes the server is a surface. */ - const referencedTools = (text: string): string[] => { + // Tool names AND resource URIs. The first version matched only + // `reactome_*`, and the instructions also list `reactome://analysis/{token}` + // and friends -- so a restricted instance could still have advertised a + // resource it withheld, which is the same drift one noun over. + const referenced = (text: string): string[] => { const found = new Set(); - for (const match of text.matchAll(/reactome_[a-z_]*\*?/g)) found.add(match[0]); + for (const m of text.matchAll(/reactome_[a-z_]*\*?/g)) found.add(m[0]); + for (const m of text.matchAll(/reactome:\/\/[a-z-]+(?:\/\{?[a-z]+\}?)*/g)) found.add(m[0]); return [...found]; }; @@ -124,20 +130,89 @@ describe("instructions match what is registered", () => { ? registered.some(name => name.startsWith(reference.slice(0, -1))) : registered.includes(reference); - it("names no tool the full server does not register", () => { - const registered = namesFor(registerAllTools); - const dangling = referencedTools(buildServerInstructions(ALL_TOOL_GROUPS)).filter( + const registeredNames = (groups: ToolGroup[]): string[] => { + const tools = groups.flatMap(g => namesFor(TOOL_GROUPS[g])); + const resources: string[] = []; + const server = { + resource: (...args: unknown[]) => { + const uri = typeof args[1] === "string" ? args[1] : undefined; + const t = args[1] as { uriTemplate?: { toString(): string } } | undefined; + const value = uri ?? t?.uriTemplate?.toString(); + if (value) resources.push(value); + return undefined; + }, + }; + registerAllResources(server as never, groups); + return [...tools, ...resources]; + }; + + it("names no tool or resource the full server does not register", () => { + const registered = registeredNames(ALL_TOOL_GROUPS); + const dangling = referenced(buildServerInstructions(ALL_TOOL_GROUPS)).filter( r => !satisfied(r, registered) ); expect(dangling).toEqual([]); }); - it("names no tool a restricted server does not register", () => { + it("names no tool or resource a restricted server does not register", () => { const groups: ToolGroup[] = ["search", "pathway", "entity", "utilities"]; - const registered = groups.flatMap(g => namesFor(TOOL_GROUPS[g])); - const dangling = referencedTools(buildServerInstructions(groups)).filter( + const registered = registeredNames(groups); + const dangling = referenced(buildServerInstructions(groups)).filter( r => !satisfied(r, registered) ); expect(dangling).toEqual([]); }); }); + +describe("resources follow the same switch", () => { + const resourceUris = (groups: ToolGroup[]): string[] => { + const uris: string[] = []; + registerAllResources( + { + resource: (...args: unknown[]) => { + const t = args[1] as { uriTemplate?: { toString(): string } } | undefined; + const uri = typeof args[1] === "string" ? args[1] : t?.uriTemplate?.toString(); + if (uri) uris.push(uri); + return undefined; + }, + } as never, + groups + ); + return uris; + }; + + it("registers every known resource when all groups are on", () => { + expect([...resourceUris(ALL_TOOL_GROUPS)].sort()).toEqual([...RESOURCE_URIS].sort()); + }); + + it("withholds the resources of an omitted group", () => { + // An instance that withholds the analysis *tools* but still serves + // reactome://analysis/{token} has not restricted anything; it has moved + // the capability to a URI. + const uris = resourceUris(["pathway", "utilities"]); + expect(uris).not.toContain("reactome://analysis/{token}"); + expect(uris).not.toContain("reactome://entity/{id}"); + expect(uris).toContain("reactome://pathway/{id}"); + expect(uris).toContain("reactome://species"); + }); + + it("refuses a resource that belongs to no group", () => { + // Fails loudly rather than defaulting to always-on, so a new resource + // cannot escape the switch by nobody remembering it exists. + const server = { + resource: (...args: unknown[]) => args, + }; + const rogue = { + resource: (..._args: unknown[]) => undefined, + }; + void server; + expect(() => { + registerAllResources(rogue as never, ALL_TOOL_GROUPS); + (rogue as { resource: (...a: unknown[]) => unknown }).resource( + "rogue", + "reactome://not-classified", + () => undefined + ); + }).toThrow(/not assigned to a tool group/); + }); +});