Remove graph database access rather than gating it - #39
Merged
Merged
Conversation
Adam, on hosting this publicly: the MCP must not provide access to Neo4j through Cypher or any other direct query. So the capability is gone rather than switched off. Removed: the three `reactome_cypher_*` tools, the `reactome://graph/schema` resource, the Cypher section of the server instructions, the startup schema prefetch, `src/clients/neo4j.ts`, `src/graph/`, and the `neo4j-driver` dependency. `NEO4J_URI` and `MCP_ALLOW_CYPHER` are inert -- nothing reads them. **Why removed rather than gated, in one fact.** Constitution Principle IV already said no deployment holds a Neo4j connection, and enforced it with a gate. That made the property true of a *configuration* rather than of the code, and a configuration has as many copies as there are call sites. #36 found three and corrected them. There were four. `src/http-server.ts` -- the entrypoint that actually runs in the hosted deployment -- still opened a connection on `NEO4J_URI` alone, and I did not notice while writing a commit message claiming all three surfaces now shared one predicate. Neither consumer needed it. The chatbot queries the graph directly with its own driver, in `data_generation`, when it builds embeddings; the serving path never imports it. The website needs the REST-backed tools only. **The public surface is unchanged at 59 tools.** The Cypher tools were only ever the 60th to 62nd, and only with both switches on. `tests/no-graph-access.test.ts` asserts the absence with the old switches turned **on**, because asserting it with them off would confirm the gate still works rather than that the tools are gone. Five checks: no Cypher tool, no schema resource, no mention in the instructions, no driver dependency, no source file importing a driver. Verified by sabotage. Re-registering `reactome_cypher_query` fails three tests across three files. The source-walk was checked separately by pointing it at a package that *is* imported, since an import of a removed dependency cannot compile and "no tests" proves nothing -- it named seven real files, so the walker reads what it claims to. Docs corrected rather than deleted: SECURITY.md said the server was designed for a trusted local curator and not hardened for internet-facing use, which is now half wrong and half load-bearing -- it still has no auth, TLS or rate limiting of its own, and that belongs at the proxy. The constitution and spec 002 record why the gate was replaced, not just that it was. **While in the README: it claimed to list every tool and did not.** "Over 40 tools", no GSA section at all, and Pathways undercounted by one -- six registered tools documented nowhere, on the file that is about to be the public description of a public server. Now 59, with the missing section, and `tests/readme-tools.test.ts` fails if a tool goes undocumented or the count drifts. A prose count goes stale the first time someone adds a tool. 104 tests, lint/format/typecheck/build clean under node:22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamjohnwright
added a commit
that referenced
this pull request
Sep 21, 2026
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.
Adam, on hosting this publicly: "we don't want the mcp to be providing access to neo4j through cypher or any other direct query." So the capability is gone rather than switched off.
Removed: the three
reactome_cypher_*tools, thereactome://graph/schemaresource, the Cypher section of the server instructions, the startup schema prefetch,src/clients/neo4j.ts,src/graph/, and theneo4j-driverdependency.NEO4J_URIandMCP_ALLOW_CYPHERare inert — nothing reads them.Why removed rather than gated, in one fact
Constitution Principle IV already said no deployment holds a Neo4j connection, and enforced it with a gate. That makes the property true of a configuration rather than of the code — and a configuration has as many copies as it has call sites.
#36 found three copies and corrected them. There were four.
src/http-server.ts— the entrypoint that actually runs in the hosted deployment — still opened a connection onNEO4J_URIalone:I missed it while writing a commit message claiming all three surfaces now shared one predicate.
Neither consumer needed graph access anyway. The chatbot queries Neo4j directly, with its own driver in
data_generation, when it builds embeddings — the serving path never imports it. The website needs the REST-backed tools only.The public surface is unchanged at 59 tools. The Cypher tools were only ever the 60th–62nd, and only with both switches on.
The test
tests/no-graph-access.test.tsasserts the absence with the old switches turned on. Asserting it with them off would confirm the gate still works, not that the tools are gone. Five checks: no Cypher tool, no schema resource, no mention in the instructions, no driver dependency, no source file importing a driver.Verified by sabotage:
reactome_cypher_queryfails three tests across three filesDocs corrected, not deleted
SECURITY.mdsaid the server was designed for a trusted local curator and not hardened for internet-facing use. That is now half wrong and half load-bearing: it still has no auth, TLS or rate limiting of its own, and that belongs at the proxy. What removing graph access changes is the blast radius if the proxy is misconfigured. The constitution and spec 002 record why the gate was replaced, not just that it was.While in the README: it claimed to list every tool and did not
"Over 40 tools", no GSA section at all, and Pathways undercounted by one — six registered tools documented nowhere, on the file that is about to be the public description of a public server. Now 59, with the missing section, and
tests/readme-tools.test.tsfails if a tool goes undocumented or the count drifts. A prose count goes stale the first time someone adds a tool.Breaking
A curator running this locally over stdio against
reactome_neo4j_envloses those three tools, with no replacement in this server. That audience is who they were built for; the decision is deliberate.104 tests, lint/format/typecheck/build clean under
node:22.🤖 Generated with Claude Code