fix(mcp): the modern era still answers two methods 2026-07-28 removed - #192
fix(mcp): the modern era still answers two methods 2026-07-28 removed#192hsw wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new CI job references actions/checkout@v7, which is not a valid major version and will break workflow execution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the MCP Worker’s modern (2026-07-28) response shape so modern clients no longer reject tools/list / prompts/list responses, and adds a dedicated, dependency-free node:test suite plus CI coverage for the mcp/ package.
Changes:
- Add an era-boundary normaliser (
asModernResult) to stamp modernresultTypeand method-scoped cache hints (ttlMs,cacheScope) without altering legacy wire responses. - Add a
node:testharness that drives the Worker’s realfetch()in-process, with a TS resolve hook so tests can import the Worker’s TypeScript sources directly. - Add an
mcpjob to CI to build the generated data manifest, type-check, and run tests for themcp/subpackage.
File summaries
| File | Description |
|---|---|
mcp/src/index.ts |
Stamps modern results with resultType + cache hints at the era boundary; rejects removed methods in modern era; keeps legacy responses unchanged. |
mcp/test/result-shape.test.mjs |
Verifies modern required fields and pins legacy byte-identity key shape across methods and edge cases. |
mcp/test/cache-hints.test.mjs |
Ensures cache hints appear only where required in modern era and never leak into legacy. |
mcp/test/era-gate.test.mjs |
Pins modern-era transport rejection rules and supported modern protocol versions. |
mcp/test/harness.test.mjs |
Sanity-checks that the harness exercises the real Worker paths in both eras. |
mcp/test-lib/harness.mjs |
In-process request driver for legacy/modern message shapes and header mirroring. |
mcp/test-lib/cases.mjs |
Central method × era test case table and modern transport rejection matrix. |
mcp/ts-resolve-hook.mjs |
Node registerHooks resolver to append .ts for local extensionless imports in tests. |
mcp/package.json |
Adds pretest + test scripts to run node:test with strip-types + resolver hook. |
mcp/README.md |
Documents how to run tests/typecheck and the node/version requirement rationale. |
CLAUDE.md |
Updates repo guidance to mention the mcp/ test suite and CI job at a high level. |
.github/workflows/ci.yml |
Adds a separate mcp CI job to install, build manifest, type-check, and run tests. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
51bbaa0 gave every result that flows through complete()/publicComplete() the members revision 2026-07-28 requires, which is what jdevalk#186 reported. Two branches of handleRpc do not flow through it: `ping` and `logging/setLevel` still answer a bare `ok(id, {})`. That revision cannot accept either shape. `Result.required` is `["resultType"]` and `EmptyResult` is a `$ref` to `Result`, so the bare `{}` they return is schema-invalid — a conforming client rejects it exactly as it rejected the tools/list result before 51bbaa0. Stamping them instead would be no better: schema.json for that revision defines no `PingRequest` and no `SetLevelRequest`, and neither method appears in its `ClientRequest` union, so a stamped result would report success for a method the era does not have. `logging/setLevel` gave way to the io.modelcontextprotocol/logLevel `_meta` key (SEP-2577). So the modern era answers -32601, paired with HTTP 404 by the mapping already there. The legacy era is untouched: both methods answer exactly as before, and a batch — which can only be legacy, batching was removed in 2025-06-18 — never reaches the gate. `initialize` is deliberately not in the list. It selects the handshake era for its own message (basic/versioning), so it is answered in that shape. The gate reads a SUPPORTED version rather than the mere presence of the `_meta` key, which the 404 status mapping now shares. validateModernRequest() returns early for a message with no usable id, so an id-less request is never validated at all and only its version is read; gating on presence alone would answer such a request under an era it never asked for. MODERN_PROTOCOL_VERSIONS and LEGACY_ONLY_METHODS are exported so the assertions in the next commit can pin them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFy2qPKK546AEFGixoxYoE
The endpoint served zero tools to every 2026-07-28 client for as long as it did because nothing exercised it. jdevalk#186 was found by a person running the conformance suite by hand, and the two branches the previous commit fixes were still there afterwards. A read-through does not catch a missing member on one of two eras; a table of cases does. Same idiom as scripts/test-websub.mjs at the repo root — plain node + node:assert/strict, a check() that counts failures, no framework and no dependency — so this repo keeps one way of writing assertions. Unlike that file it drives the real fetch handler rather than pure helpers, which needs nothing extra: the Worker is a plain `export default { fetch(request, env) }`, it never touches an execution context, and logMcpCall() returns immediately when env.MCP_LOG is absent, so an empty env is enough. No wrangler, no network. 114 assertions. Each was checked by breaking the thing it guards and confirming the run goes red: the era gate, `resultType`, the cache hints, get_checklist's structuredContent, and tools/list back to a bare result all fail it. What it pins is the wire contract, not what the tools compute — a tools/call row asserts the result's shape and that `content` is there, not that `search` ranked anything. Rows are hand-maintained per tool and per return site, because nothing derives how many return sites a tool has: the row for an empty result set exists because that is the branch get_checklist's structuredContent was missing from, and each such row asserts the count it expects to be zero so a content change fails it instead of silently testing the populated branch. The suite also pins that both eras are served the same result members, in both directions. complete() sits inside handleRpc, so a handshake-era client gets `resultType` and the cache hints too. That is legal — 2025-11-25's Result carries `additionalProperties: {}` — and it is a decision rather than an accident, so moving the stamp to the era boundary has to edit those lists rather than quietly change what legacy clients receive. Running the TypeScript sources directly needs a 21-line resolve hook: wrangler bundles the Worker, so its relative imports are extensionless and Node's resolver wants the extension. registerHooks needs Node >= 22.15, above the root's declared >= 22.12. No `engines` field — the floor belongs to one script rather than to the package, and below 22.15 the named import already fails at link time naming the missing export. mcp/README.md says so. Deliberately not in .githooks/pre-commit: that fires on every commit, almost none of which touch mcp/, and it would need this package's separate dependency tree. `pretest` regenerates src/data.json, which is generated and gitignored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFy2qPKK546AEFGixoxYoE
CLAUDE.md's `mcp/` row now names them, and the Commands section gains the package's own scripts. Records the one non-obvious trap: typecheck fails with TS2307 on a fresh clone until build:data has run, because src/index.ts imports the generated, gitignored src/data.json. Also records why they are deliberately not in the pre-commit hook. The Deployment bullet said ci.yml "only runs type-check + build verification", which was already short of what it ran. It now says ci.yml verifies rather than listing jobs, so the next one added will not stale it again. mcp/README.md gains a Tests section and the Node >= 22.15 floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFy2qPKK546AEFGixoxYoE
A proposal, and deliberately the last commit: adding a gate to this repo's CI is the maintainer's call, so drop this one and everything before it still stands. The one line elsewhere that names the job — a sentence in CLAUDE.md — is inside this commit for that reason. mcp/ is a separate package with its own lockfile, so the build job never touched it: the root `npm ci` doesn't install it, and both eslint.config.js and .prettierignore exclude mcp/ deliberately. Its type-check had never run in CI at all. A second job keeps a Worker failure reading distinctly from a site-build failure. The build job is unchanged. build:data is an explicit step: src/index.ts imports the generated, gitignored src/data.json, so `tsc --noEmit` fails with TS2307 on a clean checkout. pretest would regenerate it, but relying on that makes step order quietly load-bearing. The runner is pinned to 22.15 rather than the build job's 22: ts-resolve-hook .mjs needs node:module's registerHooks, added there. `22` resolves to something newer today, so this only matters the day it doesn't — and the failure it prevents is a link-time missing export with nothing to point at. Not added to .githooks/pre-commit: that fires on every commit, almost none of which touch mcp/, and it would need this package's separate dependency tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFy2qPKK546AEFGixoxYoE
9aae8e8 to
9a6b621
Compare
hsw
changed the title
|
Rebased onto Dropped, because you already fixed it
What is leftOnly Placed in One thing worth your eye, deliberately not changed
This is legal and I have left it alone. 2025-11-25's What I did do is pin it in both directions. Each row in the method table carries a Also worth flaggingTwo other tools have a return site with no The four commits are ordered so the last one — the CI job — can be dropped on its own. |
Modern MCP requests for ping and logging/setLevel currently receive successful empty results even though revision 2026-07-28 removed those methods. Return HTTP 404 with JSON-RPC -32601 for those requests, while preserving legacy protocol behaviour.
Adds 114 protocol assertions against the real Worker handler and runs them in the existing MCP CI job, alongside its dependency audit and type-check. The merged workflow has a single MCP job; the new tests do not replace the audit.
Validation: clean npm 10 installs on Node 22.22.3, all 114 assertions, MCP type-check, zero-vulnerability MCP audit, full site build, lint, formatting and agent-skill checks. Updated against current main before validation.