protocol/json_rpc: own and export the response-envelope constructors - #1720

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d75c8837d

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4da1f904ce

The move is exact and neither caller changed. All five base construction sites build the same two shapes in the same key order, expression-for-expression including how jsonrpc is bound. I measured it rather than reading it: 56 cases through mcpStep and dispatch at base and head — success, empty result, all five standard codes plus -32002, ids string/number/null, notification, errors with and without data — comparing Object.keys order, insertion-order stringify, and the stdio wire string and its UTF-8 bytes. Identical in all 56.

On the wire question I raised for myself: stringifyJson = stringify(sort), so stdio sorts keys and field order was never wire-observable there anyway. The stdio proof is byte-identical base to head and still builds its expected lines from independent literals, so it stays an independent oracle.

Mutation testing came out better than equivalence. Ten mutations at base and head, and row 5 goes the good way: forcing id to null in the error constructor survived at base — mcp's own proofs missed it — and is killed at head by dispatch.handlerError plus the stdio proofs, because the one shared expression now sits under the dispatch, mcp and stdio proofs simultaneously. Nothing killed at base survives at head. Row 9 (dropping data from the error schema) survives on both sides — pre-existing gap, not a regression.

Coverage didn't move out of the callers: mcp 82 → 82 asserts, stdio 19 → 19 (file byte-identical), json_rpc 28 → 36. Layering holds — json_rpc's import block is identical base to head, no dependency on fjs/protocol/mcp/** or fjs/mcp/**, and jsonrpc is correctly dropped from both mcp files. The deleted todo's four tasks are all genuinely done, the retargeted references all point at symbols that exist, and I ran retired-issue-identifiers.md's own check script at both revisions — 18 bare citations across 13 files, unchanged, so its count still holds and the i665-mcp correction is right.

One thing to fix:

6455286cc exports two new public types — SuccessResponse and ErrorResponse — and neither changelog/unreleased/1720.md nor the body's Changelog: block mentions them. Both still describe only the two constructors. The file and the body match each other byte-for-byte, so the block isn't malformed; the scope is just understated. The body's fjs test — 3400 is stale too (3402 now).

Two nits, neither blocking. effectful-dispatch-skeleton.md:23 cites mcp/module.f.mjs:264-287 and map-step-combinator.md:26 cites :347-350; both were already wrong at base (264-287 is _noParams/uninitializedState, and mcpStep's body starts at 320) and this PR shifted them 6 further while editing the identifiers inside those very snippets — cheap to fix while the files are open. And the new responseOf.error proof uses id = null as its input, so it alone can't catch an id-forcing mutation; responseOf.success uses id = 1 and does.

Gates: npm test 3398 → 3400 on the branch's own work, 3402 at the current head with main merged in; tsc --noEmit exit 0; purge matched nothing; no casts or try/catch introduced. No **BREAKING CHANGES:** needed — purely additive, no caller output moved. No HTML entities in the body this time.

Approving. Both findings are closed.

The entry file now carries the second bullet for the type exports, and the body's Changelog: block is byte-identical to it — same md5, same 413 bytes, last section before the trailer. The test count reads 3402 and npm test gives exactly 3402/0.

Both line citations are content-correct now, which I checked by reading the ranges rather than trusting the commit message. effectful-dispatch-skeleton.md:23 → mcp/module.f.mjs:314-322, and 314 really is const [t, message] = decodeRequest(value), the first line of mcpStep's body, with the invalidRequest arm and the destructure following in order. map-step-combinator.md:26 → :411-414 is exact, line for line.

The proof change does what it claims: swapping errorResponseOf(null) for errorResponseOf('abc') means the id-forcing mutation now fails responseOf.error by name — I ran it, 3396/6 with that proof among them, where at the previous head it was blind to it. Flipping the new assertion gives a single failure naming its own row, so it is load-bearing.

Nothing else moved on the branch side: git diff 6914572458ae..HEAD -- fjs/protocol/json_rpc/module.f.mjs is empty, so the constructor expressions are exactly what I already verified as an exact move. No casts, no try/catch introduced anywhere in the PR.

Two one-line residuals, neither blocking. The 314-322 range stops at the snippet's fourth line — the elided remainder maps to 323-346, so 314-346 would cover the whole quoted block; the start is exact either way. And both doc snippets write step/pure where the source says ioStep/pureOk — that predates this branch (origin/main has the same naming), so not yours, just adjacent.

Gates: npm test 3402/0, tsc --noEmit exit 0, purge matched nothing.

The retarget is right, verified line for line rather than by trusting the message: 314-346 maps exactly — 314 the decodeRequest destructure, 315-317 the error arm, 318 the destructure, 322 the id === undefined guard, 323-344 the body the snippet elides as { ... }, 345 return pureOk(null), 346 the closing brace. The sibling retarget in map-step-combinator.md (411-414) is exact too.

But there is a third stale citation in the same document, and this one the PR did cause. Line 12:

Pure `dispatch` (`fjs/protocol/json_rpc/module.f.mjs:95-110`):

At head, 95-110 is the JSDoc for errorResponseOf/successResponseOf — the constructors this PR adds. dispatch moved to 126, and the quoted snippet spans 126-141. On origin/main, 95-110 is exactly right; inserting the two constructors above dispatch shifted it 31 lines. It was already wrong at 739e54ecd, the SHA I approved — eeaf549ce fixed the two mcp citations and missed this one, 8c7ee62df fixed an endpoint. Third of three, and now the only one left.

Delta is docs-only on the branch side — one commit, one file, one line. The Rust files in the diffstat all arrive via #1699 from main.

On the step/pure naming I flagged: confirmed pre-existing on origin/main in both snippets, so not yours. Worth knowing they're real but different functions — pure at effects/module.f.mjs:113 and step at :342 both exist and differ from pureOk at :268 and ioStep, so a reader can follow them to the wrong place.

Gates: npm test 3402/0 matching the body, tsc --noEmit exit 0, purge matched nothing. Changelog: still byte-identical to the entry file and last before the trailer.