Skip to content

Commit 8572cda

Browse files
Merge pull request #33 from StepenkoAnatoli/fix/mcp-dual-era
The MCP server was unreachable: dual-era, proven against a real client
2 parents 7768469 + d1ff2e2 commit 8572cda

7 files changed

Lines changed: 197 additions & 16 deletions

File tree

‎README.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,9 @@ instead of a command:
157157
```
158158

159159
Two tools: `collect` starts a run and returns its id; `fetch_corpus` takes that id and
160-
returns a link to the validated package. The same token, the same one permission, and the
160+
returns a link to the validated package. The server speaks **both** protocol eras -
161+
`2026-07-28` and `2025-11-25` - because the specification is ahead of every shipped client
162+
and a server only the spec can talk to is one nothing can call. The same token, the same one permission, and the
161163
same rule at the end - the result carries `buildAuthorized`, and it is `false` for every
162164
freshly collected corpus.
163165

‎docs/ARCHITECTURE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ else, and it never touches the corpus:
119119
| `artifact-zip.mjs` | **Reading a ZIP somebody else wrote** — the opposite job from `archive.mjs`, and a separate file on purpose: a writer that also parses hostile input grows a trust boundary in the middle of itself. `openZip(bytes)` → `{ entries, names, problems, has, read }`, plus `checkEntryName`, `readCentralDirectory`, `inspectEntries`, `readEntry` and the exported `ZIP_LIMITS` (entry count, total and per-entry uncompressed bytes, ratio + floor, name length) so tests drive the refusal paths with small fixtures instead of real bombs. **The order of operations is the security property**: every structural refusal — traversal, absolute, drive letter, backslash, NUL, duplicate, case collision, symlink/special entry, encryption, unknown method, counts, sizes, ratio — is decided from the **central directory before one byte is inflated**. The directory is attacker-controlled too, so it is never the only defence: `inflateRawSync` runs with `maxOutputLength`, making the ceiling the runtime's to enforce rather than a number compared against a field the attacker wrote. Names are judged as raw text, never `path.normalize`'d first — normalising would let `a/../../b` become `../b` and then be judged, when the honest answer is that `..` has no business in a research artifact. Touches no filesystem: entries come back as buffers. ZIP64 is refused rather than half-read. | ADR-0032 |
120120
| `artifact-validator.mjs` | **Is this package valid, and does it authorize a build** — two questions, kept apart. `validateArtifact({ file, expectedClientRef })` → `{ status, buildAuthorized, packageId, clientRef, workflowRunId, state, errors, warnings }`, where `status: 'PASS'` means internally consistent and says *nothing* about permission. Offline and read-only by construction: no transport imported, `process.env` never read, nothing from the package executed, no shell, and the only write is a `fs.mkdtemp` copy of `project/` removed in a `finally` — needed because the chain and handoff checks are the kit's existing code (`readCorpus`, `verifyHandoff`) and reuse beats a second implementation of provenance verification nobody tested. Stable error codes (`ZIP-*`, `MANIFEST-*`, `FILE-*`, `LEDGER-*`, `CAPTURE-*`, `AUTHORIZATION-INCONSISTENT`, `SECRET-DETECTED`, `FORMAT-UNSUPPORTED`, `CLIENT-REF-MISMATCH`) and **never a secret value in an error** — the report names the pattern and the line. `authorizationProblems()` is the one implementation of the build rule; the schema encodes it a second time and when the schema catches it first the rule is still reported in its own words. An unsupported format **major** is `INCOMPLETE`, not invalid. A crash is `BLOCKED`, never `PASS`. | ADR-0032 |
121121
| `artifact.mjs` | **Packaging a project without changing it, and deriving every authorization field.** `createArtifact` → `{ bytes, name, manifest, entries, derived }`; `writeArtifact` writes then **re-validates with the consumer's own validator** before reporting success. There is no `--build-authorized` and no option that reaches an authorization field: `deriveState(root)` runs the real gate with `env` defaulting to `{}` so a packaging shell's override cannot become a property of a package that outlives it. `mapClassified` and `briefReviewed` have honest machine answers; `findingsReviewState` answers the third **negatively** — a `Finding` byte-identical to what `firstFinding` re-extracts from the capture was demonstrably never rewritten, which detects the unreviewed case without grading the reviewed one (ADR-0013's line, uncrossed). `packageName` never carries the topic (an artifact listing is world-readable on a public repository) and falls back to `run-<workflowRunId>`; `checkClientRef` refuses rather than sanitises. Inventory sorted by ZIP path, manifest and digest appended **last**, digest over the exact manifest bytes. | ADR-0032, ADR-0031 |
122-
| `mcp.mjs` | **The collector as a Model Context Protocol server**, and the first module in this repository built from an APPROVED BRIEF rather than from a plan - every design choice cites an E-row in `docs/decisions/2026-09-21-agent-interface/`. `handle(message, deps)` is a pure function of a JSON-RPC message plus injected transport, so the whole surface is tested offline. **stdio, not Streamable HTTP**: semantics are identical on both, so the difference is operational, and remote costs the entire OAuth 2.1 chain. **Two tools, not one** - `collect` returns a run id immediately, `fetch_corpus` returns a link - because a server cannot initiate a request, so the alternative is one call held open for minutes and an agent that loses a paid run when its process dies. **A `resource_link`, never embedded bytes**: the spec defines embedding but states no maximum size, so a link is the defensible option. **No tool takes a token** and a test asserts no parameter is credential-shaped. `PROTOCOL_VERSION` is pinned and a mismatch answers `-32022` with `data.supported`, the same discipline as pinning `X-GitHub-Api-Version`. `createStdioLoop` serialises chunk processing through one promise chain - overlapping `data` events were mutating the buffer concurrently, which a test caught. | ADR-0034 |
122+
| `mcp.mjs` | **The collector as a Model Context Protocol server**, and the first module in this repository built from an APPROVED BRIEF rather than from a plan - every design choice cites an E-row in `docs/decisions/2026-09-21-agent-interface/`. `handle(message, deps)` is a pure function of a JSON-RPC message plus injected transport, so the whole surface is tested offline. **stdio, not Streamable HTTP**: semantics are identical on both, so the difference is operational, and remote costs the entire OAuth 2.1 chain. **Two tools, not one** - `collect` returns a run id immediately, `fetch_corpus` returns a link - because a server cannot initiate a request, so the alternative is one call held open for minutes and an agent that loses a paid run when its process dies. **A `resource_link`, never embedded bytes**: the spec defines embedding but states no maximum size, so a link is the defensible option. **No tool takes a token** and a test asserts no parameter is credential-shaped. `PROTOCOL_VERSION` is pinned and a mismatch answers `-32022` with `data.supported`, the same discipline as pinning `X-GitHub-Api-Version`. `createStdioLoop` serialises chunk processing through one promise chain - overlapping `data` events were mutating the buffer concurrently, which a test caught. **DUAL-ERA**: `SUPPORTED_VERSIONS` is `['2026-07-28', '2025-11-25']` and `initialize` is implemented beside `server/discover`, because the specification is ahead of every shipped client - the official SDK at v1.30.0 declares `2025-11-25` as its latest and answered the modern-only first version with `-32601: unknown method initialize`. `initialize` **echoes** a serveable requested version and falls back to **legacy** rather than to this server's preference, because a client throws on a version it does not support. | ADR-0034 |
123123
| `dispatch.mjs` | **Asking the collector to run, from outside GitHub** - the seam an agent or a desktop app uses. `dispatchCollection` (pins `X-GitHub-Api-Version: 2026-03-10` on the REQUEST, not merely in a comment), `waitForRun`, `listArtifacts`, `downloadArtifact`, `unwrapArtifact`, plus `tokenFromEnv` and `redact`. **No token may be passed as an argument** - `tokenFromEnv` is the only way in, because a credential on a command line reaches the shell history, `ps`, and any log that echoes its own command; a test asserts the source contains no token flag. `redact` is PATTERN-based rather than "remove the token we hold", so it also catches a credential the process was never given - one echoed back by a server, a proxy, a redirect. A **204** is a named `NO_RUN_ID` failure explaining that the server served `2022-11-28`, never an empty success: "the run started and I cannot tell which one" is the failure a caller is least able to diagnose from a status code. `waiting` is reported as a status, not a stall - it means a protection rule is holding the job. `unwrapArtifact` opens GitHub's outer ZIP through the same distrustful reader, and passes a hostile or ambiguous container through untouched for the validator to refuse rather than guessing at packaging depth. Every request goes through an injectable `fetch`, so the whole module is tested offline. | ADR-0033, ADR-0032 |
124124
| `brief.mjs` | The phase-1 → phase-2 handoff: renders `research/BRIEF.md` from the corpus, and **owns the brief's shape** — `BRIEF_SECTIONS` (six sections, the two that are judged marked), the markers that distinguish *still the scaffold* from *the drafter wrote this* (`briefState` → `template \| legacy \| draft \| authored`), and the section reader (`briefSection` / `judgedSection`) that `lib/audit.mjs` consumes instead of regexes of its own. The renderer writes headings **from** that definition, so the writer and its readers cannot drift; the scaffold ships `BRIEF_FILE_MARKER`, which the substitution pass leaves alone — the alternative, matching the template's prose, made a sentence of English load-bearing. | — |
125125
| `corpus.mjs` | The research corpus: **one reader** (`readCorpus`, `readCaptures`, `parseTable`, `parseCapture`, `readOverrides`) and **one writer family** (`appendRow`, `upsertRow`, `appendJsonLine`, `writeRaw` lives in collect), plus the **cache decision** over the capture index (`cacheDecision`) — the one freshness predicate the collector, the run, and phase 0 consume. Table machinery: `splitRow`/`escapeCell`/`tableRow`/`repairRowArity`. Malformation is reported, never absorbed. The capture index has one constructor (`captureEntry`, used by `readCaptures` **and** by the collector when it has just written a page) and one writer (`rememberCapture`), so the index a run keeps in memory holds the same shape as the index read from disk — callers store the entry they are handed and shape nothing. The format's **joins** are here too: `traceOf` (evidence row → the fetch that produced it and the capture behind it), `captureOf` (row → capture, via the by-file index the reader builds), and `claimOf` (unknown → the claim it rests on, first cited row, falling back to the unknown's own text). Five consumers re-derived those joins, including the subtle "an empty Raw cell matches on the URL alone" clause; now a format change lands in one place. | ADR-0003 |

‎docs/adr/0034-the-collector-speaks-mcp-over-stdio.md‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,50 @@ The other `GAP` was **split** rather than fudged. How a break is detected and su
9696
design question and is closed. Who governs the specification was never collected and is
9797
dismissed with that said plainly: it moves the planning horizon, not the architecture.
9898

99+
### Amendment, 2026-09-21 — the server is DUAL-ERA, because the spec is ahead of every client
100+
101+
The decision above says "the protocol version is pinned" and pins `2026-07-28`. That was
102+
correct about the specification and useless in practice, and the only thing that could have
103+
revealed it was running the server against a client somebody else wrote.
104+
105+
Driven by the official SDK at v1.30.0, the first version of this server answered, in full:
106+
107+
```
108+
MCP error -32601: unknown method initialize
109+
```
110+
111+
The SDK declares `LATEST_PROTOCOL_VERSION = '2025-11-25'` and supports
112+
`2025-11-25, 2025-06-18, 2025-03-26, 2024-11-05, 2024-10-07`. Every one of those is what
113+
E-05 calls **legacy** — session-based, opening with an `initialize` handshake, which
114+
`2026-07-28` removed. Claude Desktop and Claude Code are built on that SDK.
115+
116+
**A server nothing can call is not a conformant server; it is an unreachable one.** This is
117+
the same failure shape as building the workflow against an npm package named `firecrawl`:
118+
read from documentation, never exercised against reality, wrong in the one way that
119+
matters. Both were found by running the thing, and neither could have been found any other
120+
way.
121+
122+
E-05 already named the remedy, in the corpus, in the brief, and in the paragraph of this
123+
ADR that quotes it — **"a dual-era implementation that supports both"**. I cited the term
124+
and did not implement it, because until a real client refused the connection nothing forced
125+
the question.
126+
127+
So `SUPPORTED_VERSIONS` is `['2026-07-28', '2025-11-25']`, `initialize` is implemented
128+
beside `server/discover`, and one session object per connection holds what the handshake
129+
agreed. Two rules follow from how the official client behaves:
130+
131+
- **Echo the requested version** when it is serveable. The client refuses a reply carrying
132+
a version it does not support — `SUPPORTED_PROTOCOL_VERSIONS.includes(result.protocolVersion)`
133+
or it throws — so answering with this server's preference disconnects it.
134+
- **Fall back to legacy, not to preference,** for a version we cannot place. Answering an
135+
unknown client with `2026-07-28` is a true statement about this server and a guaranteed
136+
disconnection, because no shipped client lists it.
137+
138+
Verified end to end afterwards, through the official client rather than by hand: connect,
139+
`tools/list`, a real `collect` that spent credits, polling `fetch_corpus` through the tool
140+
because the server cannot call back, and a `resource_link` to a package that validated
141+
`PASS` with `buildAuthorized: false`.
142+
99143
## Alternatives considered
100144

101145
**A remote/hosted MCP server.** Deferred with the whole of E-01's OAuth chain, until

‎research-kit/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,7 @@ node research-kit/bin/selftest.mjs # all of it
372372
node research-kit/bin/selftest.mjs gate hook # just these files
373373
```
374374

375-
808 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means
375+
815 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means
376376
the assertions settled (ADR-0021), and each test is raced against a watchdog
377377
(`RESEARCH_KIT_TEST_TIMEOUT`, default 60s).
378378

‎research-kit/bin/mcp-server.mjs‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
// holding the collector workflow. See the front-page README.
2828

2929
import { requireRuntime } from '../lib/runtime.mjs';
30-
import { createStdioLoop, handle, PROTOCOL_VERSION, SERVER_INFO, TOOLS } from '../lib/mcp.mjs';
30+
import { createStdioLoop, handle, SUPPORTED_VERSIONS, SERVER_INFO, TOOLS } from '../lib/mcp.mjs';
3131
import { tokenFromEnv, TOKEN_VARS, redact } from '../lib/dispatch.mjs';
3232

3333
if (process.argv.includes('--help')) {
@@ -36,7 +36,9 @@ if (process.argv.includes('--help')) {
3636
3737
node research-kit/bin/mcp-server.mjs
3838
39-
Speaks newline-delimited JSON-RPC on stdin/stdout. Protocol version ${PROTOCOL_VERSION}.
39+
Speaks newline-delimited JSON-RPC on stdin/stdout.
40+
Protocol versions: ${SUPPORTED_VERSIONS.join(', ')} - modern and legacy, because every
41+
shipped client still opens with the legacy initialize handshake.
4042
Tools: ${TOOLS.map((t) => t.name).join(', ')}.
4143
4244
The token is read from ${TOKEN_VARS.join(' or ')}. There is no token argument: a stdio
@@ -55,13 +57,17 @@ requireRuntime({ node: true });
5557
// so at the top of the log the client already captures.
5658
const { token, from, detail } = tokenFromEnv();
5759
process.stderr.write(token
58-
? `research-kit mcp: protocol ${PROTOCOL_VERSION}, token from ${from}\n`
60+
? `research-kit mcp: protocols ${SUPPORTED_VERSIONS.join(", ")}, token from ${from}\n`
5961
: `research-kit mcp: ${detail}. Tools will refuse until it is set.\n`);
6062

63+
// One connection, one session. A stdio server serves exactly one client, so this object
64+
// is the whole of the session state the legacy handshake establishes.
65+
const session = { version: null };
66+
6167
createStdioLoop({
6268
input: process.stdin,
6369
output: process.stdout,
64-
onMessage: (message) => handle(message),
70+
onMessage: (message) => handle(message, { session }),
6571
onError: (error) => process.stderr.write(`research-kit mcp: ${redact(error.stack ?? error.message)}\n`),
6672
});
6773

0 commit comments

Comments
 (0)