[nissa-seru]

Problem

Anarchid's review of #540 (review) found four gaps, all in the two MCP connection classes. Each one was reproduced on main before this change (see Tests).

1. A call whose arguments JSON can't write (a BigInt, a cyclic object) put nothing on the wire, yet the modern engine reported it as no-response: "whether it took effect is unknown". ObservedTransport.send marked the request handed off before the inner send, and undid that mark only for NotWrittenError. The serializer's TypeError came from inside the inner send: stdio's writeLine(JSON.stringify(…)), or the SDK's HTTP _send. The legacy engine already refused such a request as not sent, but said "Try again later", which fails the same way. This is Modern MCP engine: unserializable arguments read "effect unknown", and a failed HTTP request is also reported as a server error #556's first finding.
2. A failed HTTP request was reported twice. The SDK's HTTP send reports a failure through the transport's onerror and then throws the same error (index.mjs:5817-5819, SDK 2.3.1). ObservedTransport forwarded every onerror to the client, which the connection emits as 'error', which the framework traces as mcpl:server-error. So each failed request reached its caller as its outcome, and also appeared as an error outside any request: one more trace per failure under the framework, and a second log line from a connection with no 'error' listener. This is Modern MCP engine: unserializable arguments read "effect unknown", and a failed HTTP request is also reported as a server error #556's second finding.
3. When a stdio child's pipe failed and the child outlived SIGKILL, the modern engine held the launch only once the reap's close had failed. By then that close had already ended the SDK's pending calls, so they read "this host is reconnecting to it". So did a call made as the connection closed, one tick before reconnecting halted for good. The legacy engine holds the launch first. This is the review's R1-1.
4. A legacy connection redacted each JSON-RPC error answer's log line with a redactor built for that line. For a stdio server the framework launched, the configuration's env is a getter that reads the Discord awareness ledger. So every error answer read and parsed the ledger from disk, since fix(mcp): the modern engine's failed requests, and a connect in flight, in #506's words #540 keeps the configuration on every connection. This is the review's R1-2.

Changes

• ObservedTransport.send writes an observed request's JSON before marking it handed off. A request JSON can't write is refused there, before anything is marked, so a call whose first leg it is reads not-sent, over stdio and over HTTP, with the serializer's words in the host's log line. A later leg keeps what the earlier legs established. The transport is handed the message as before.
• While a request's or a response's send is in flight, ObservedTransport holds what its transport reports, until that send settles. Then it forwards all of it except the error the send threw, which whoever sent it reports: a call through its outcome, the connect through its failure, the SDK through its own report. A notification's send holds nothing, since its sender may swallow its failure, as the SDK's listen teardown does. A report made outside any send, after its send has settled, or by another transport is forwarded at once. The held reports live in an AsyncLocalStorage scope per send that names its transport, the same way wireScope already keeps the request a send belongs to.
• The stdio transport's 'unreaped' passes the reap's failure, and handleLost holds the launch before it emits 'close', once it has started and recorded the reap. It reports the hold after 'close'. The permanent reconnect-failed comes from the reap's continuation, as on the route where the reap finds the child alive, and only if the host hasn't closed the connection meanwhile.
• Legacy sendRequest serializes a request before it keeps anything for it, and refuses one JSON can't write as not sent, in the modern engine's words: "If calling it again fails, ask whoever runs this host." Each of its refusals as not sent, this one, a failed pipe's and a write the transport refuses at once, now rejects before it logs, so a host logger that throws can't replace the refusal.
• A legacy connection keeps one redactor for its error answers' log line, made at its first error answer. serverTextRedactor builds again only after a build that failed, which withholds that line, or once a dial notes something new.

What each part is for

• Writing the JSON before the mark (one statement). It covers the failure before the wire that a call's own arguments can cause. The SDK has other steps before its POST. The Mcp-Name header is base64-encoded when it needs to be, so it can't fail. A bearer header fails only for a token a header can't carry, which an access provider could hand over at a refresh; such a call still reads no-response, as fix(mcp): the modern engine's failed requests, and a connect in flight, in #506's words #540 reads any HTTP send that throws. Marking where each transport writes would cover that too: before writeLine, and in a fetch wrapper given to the SDK. That's a mechanism in both transports for a token no provider should hand out. The cost here is a second JSON.stringify of each tools/call and tools/list request.
  • Not taken: handing the transport the JSON the check wrote, so that a serializer answering differently a second time couldn't send something unchecked. Parsing that JSON back turns JSON.rawJSON numbers into JavaScript numbers, and JSON.rawJSON is how a BigInt is often sent, so it would change what goes out.
• Holding during a send (a scope, a check in onerror, a finally, and a scope for a notification that holds nothing). It tells a send's own failure from anything else by the error object's identity, since the SDK reports and throws the same object. In SDK 2.3.1, every request's and response's sender reports its failure: a request's rejection reaches its caller, and the SDK reports a failed response itself. A notification holds nothing, because the one this connection sends over HTTP, the listen teardown's notifications/cancelled, is sent by a caller that swallows what it throws, so the transport's report is its only one. The connection sends no other notification over HTTP: it pins the modern revision, whose connect sends no notifications/initialized, and it cancels an HTTP request by aborting its stream. Over stdio, the transport never reports the error a send throws, so a notification's failure there is reported once, by whatever sent it. A notification whose sender also reported its failure would be reported twice over HTTP, as on main, but this connection sends none. A report made during a send waits until it settles, and every other report keeps its timing. One cost: when the host closes the connection while a send is in flight, a report made during that send is dropped at settle, since a closed connection emits no more 'error', where on main it went out at once. The scope is per send, so overlapping or nested sends neither release nor hold each other's reports, and a settled send's scope lets go of its transport. It names its transport because a host callback can run in a send's async context while the send is in flight: the SDK delivers a JSON response's messages from inside its send. A report another transport makes there is that transport's.
  • Not taken: dropping repeated reports of one error object in general. Reading the SDK's code, a failed stream resume is reported twice, once in _startOrAuthSse's catch and again by its caller's .catch. This host passes no resumption token, and nothing here tests that path.
• Holding the unreaped launch before 'close' (one line, plus the reap's failure passed through, the hold's report split from its record, and a finally). This is Anarchid's fix. The hold is recorded after the reap starts and before 'close', and reported after 'close', which is where the other route reports a hold too, in a finally. So no listener that throws can skip the reap, the hold, 'close', the hold's report or the permanent failure. It leaves a window: a child that exits between 'unreaped' and the reap's re-check stays held until close(), where before the reap would have found it gone and reconnected. Legacy has the same window. A child that exits after the re-check stays held until close() in both engines, as before.
• The permanent report from the reap's continuation (moved, with the guard the other route has). A 'close' listener that throws can't skip it, and a close() the host makes meanwhile reports the failure itself.
• Legacy serialization first (one try). It's for the words: the outcome was already right.
• One legacy refusal helper (four lines, two callers), and the refused write in the same order. Each refusal rejects before it logs.
• One legacy redactor (one field, one line). It stops the ledger read per answer. It's made at the first answer rather than at connect, so a ledger that's unreadable then still withholds that line, as fix(mcpl): a redaction that can't read its config withholds the text #542 has it. The reconnect-failure line also redacts with the configuration, and keeps building per line: it runs once per failed attempt, and each attempt's spawn reads env anyway.

Tests

• Probes on main 76623a39, before the change, run outside the suite on Linux with Node 24.22.0:
  • Over stdio, a BigInt or a cyclic argument ended the call no-response, "whether it took effect is unknown", with no tools/call on the wire.
  • Over HTTP, the same arguments gave the same, with no tools/call POST, and one connection 'error' each.
  • Over HTTP, a tools/call answered with 500 ended no-response and raised one 'error'. Under the framework, that 'error' was one mcpl:server-error trace beside the host's log line. With no 'error' listener, it was two log lines. A refused connection (fetch failed) did the same.
  • A pipe-failed child outliving SIGKILL: the pending call said "…Check its effect before trying again; this host is reconnecting to it.", and a call made in the 'close' listener said "…This host is reconnecting to it; try again later." The hold's 'error' and the permanent reconnect-failed came after both.
  • A legacy connection read env once per error answer: 1, 2 and 3 reads for three answers. Through the framework, each error answer was one DiscordAwarenessOutbox.batches() call.
• On Linux at this head (tree 1ca803df):
  • mcp-modern-outcomes passes 53 of 53 on Node 24.22.0. On Node 20.20.2 it passes 52, and its JSON.rawJSON test skips itself, since Node 20 has no JSON.rawJSON.
  • mcp-modern-http passes 11 of 11, mcpl-agent-words 20 of 20 and mcp-recorded-text 14 of 14, on both.
  • The 35 other test files that name either connection class, sendToolsCall, protocol: 'modern', unreaped or mcpl:server-error pass on Node 24, 729 tests in all.
• The whole suite, at this head merged with main 6345d27d: the typecheck and build pass, and all 243 test files pass, each in a process of its own: on Node 24.22.0, 4,744 tests pass and 27 are skipped; on Node 20.20.2, 4,743 pass and 28 are skipped, the JSON.rawJSON test among them. CI's stop-with-work-in-flight and conversation-routing pair passes on both.
• CI, at this head merged with main 4fd2bfb9: the four Build & Test legs (Ubuntu and macOS, Node 20 and 24) and the packed install pass in run 39231803388, and the changelog check in run 39231803565.
• Red-first: with main's src and this change's tests, 13 tests fail, each one this change adds or changes, and the rest of those four files passes.
  • mcp-modern-outcomes, 7: "an argument JSON cannot write is refused before the wire…", "a send's own failure is its call's to report…", "a request or a response that isn't a call's leaves what its send threw to its sender too…", "each send holds only what its own transport reported during it…", "a server that closes its stdin and outlives SIGKILL too is held…", and the two "an unreaped launch's loss with a … listener that throws…" tests.
  • mcp-modern-http, 2: "an argument JSON cannot write…" and "a request the server answers with HTTP 500 is reported once…".
  • mcpl-agent-words, 3: "a request JSON can't write is refused…", "a request refused as not sent is refused through its promise when the host's logger throws", and "error answers' log lines read their connection's configuration until a read succeeds…".
  • mcp-recorded-text, 1: "a real discover-then-tools/list 404 is redacted in the host's log line…".
  • Four new tests pass on main too. Each pins something main already does that a wrong fix could break: a notification's report, a call's JSON.rawJSON arguments, another connection's report, and a host closing the connection as an unreaped launch's loss closes it.
• Each piece, removed on its own, fails a test (24 removals, each run on its files):
  • Serializing before the mark: both "argument JSON cannot write" tests.
  • Handing the transport a JSON round trip of the message: the JSON.rawJSON test.
  • Holding during a send: the request-and-response test, the send's-own-failure test, the overlapping-sends test, the HTTP 500 test and the 404 test.
  • Holding only the send's own transport's reports: the two-connection test.
  • Holding only a call's own sends: the request-and-response test.
  • A notification holding as a request does, or holding inside the send it's made in: the notification test, the second after its 60 s wait.
  • Dropping everything held rather than only what the send threw: the request-and-response, send's-own-failure and overlapping-sends tests.
  • A settled send's scope keeping its transport: the send's-own-failure test, after its 60 s wait.
  • Forwarding nothing held after a send that went out: the send's-own-failure test.
  • No hold before 'close': both throwing-listener tests and the extended unreaped test.
  • The hold reported before 'close', or not at all: both throwing-listener tests.
  • The hold's report outside the finally: the throwing-'close' test.
  • No reap for the loss: both throwing-listener tests, among nine failures before the file's 600 s bound cut the run short.
  • A second permanent failure from the reap's verdict: the extended unreaped test.
  • The permanent failure beside a host's close(): the host-close test.
  • No permanent failure: both throwing-listener tests and the extended unreaped test.
  • The permanent failure right after 'close': the host-close test and both throwing-listener tests.
  • The legacy redactor built per answer: the redactor test.
  • Legacy serialization inside the write's try: the legacy refusal test.
  • Each of the three legacy refusals logging before it rejects: the throwing-logger test.

Not verified

• Bun. Nothing here ran under Bun. The hold relies on AsyncLocalStorage reaching the SDK's onerror call inside its send, as wireScope already relies on it reaching send.

Out of scope

• A refused connection (fetch failed) still reads no-response, "whether it took effect is unknown". A failed fetch doesn't tell a connection that was refused from one that broke after part of the request went out, and fix(mcp): the modern engine's failed requests, and a connect in flight, in #506's words #540 settled on reading any throwing HTTP send as handed off.
• The SDK's own double report of a failed stream resume, under "What each part is for".
• The modern engine's failure lines build their redaction per failure. Its configuration is the one the host gave, not the framework's copy with an env getter (framework.ts:22368), so nothing there reads the ledger.
• A host listener that throws inside the reap's continuation, on either route, skips what comes after it there and rejects what lossReaped() returns, as on main.

---

• Changelog fragment added: changelog.d/mcp-540-review-follow-ups.fixed.md.

Fixes #556.

Written by Crispin-2406 (Claude Opus 5.5). Ada-1017 (Claude Opus 5.5) made the design calls, and Nigel-2654 (Claude Opus 5.5) reviewed it. Three rounds of our internal review read it.

Description checked and posted by Lottie-2661 (Claude Opus 5.5).

🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared nissa-seru account.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

This PR fixes MCP request outcomes, duplicate HTTP error reports, and cleanup ordering.

• MCP calls with unwritable JSON are refused before they are sent.
• Modern send failures reach their callers without a duplicate connection error.
• Modern connections hold an unreaped child before closing the session.
• Legacy requests that cannot be sent keep their not-sent answer.
• Legacy error-answer logs reuse a redactor for the connection.

No actionable issues were found. git diff --check passed. Tests were not run because dependencies are not installed.

Acknowledged limits from nissa-seru:

• Double serialization, including JSON hooks changing between writes, is accepted to preserve JSON.rawJSON values without a JSON round trip.
• Reports held during a send may be dropped if the host closes before settlement; closed connections emit no further error events.
• A child that exits after unreaped remains held until close(). Throwing listeners inside the cleanup continuation remain unchanged.
• More precise handoff tracking for invalid bearer headers and refused fetches, plus the SDK's existing duplicate stream-resume reports, are deferred.

_{Reviews (1) · Last reviewed commit: "fix(mcp): a request JSON can't write rea..." · Reviewed by Greptile}