fix(mcp): answer an un-decodable envelope id instead of silently dropping the frame (D47, ub-cnv) - #438
Merged
Merged
Conversation
A JSON-RPC frame whose envelope `id` cannot be decoded falls through rmcp 1.7.0's untagged message union into the Notification variant and is silently dropped: no reply, no diagnostic, and a client that correlates by id waits on an untimed await forever. Duplication is only the MINORITY route in — a `null` id, a wrongly-typed id, a number outside what `RequestId` accepts (including the exponent spelling of an integer), and the `id` key written with a \u escape are each equally silent. A frame with NO `id` member is a legitimate Notification and its silence is CORRECT; it stays explicitly out of scope. D47 answers the class OUT-OF-BAND with `-32600 Invalid Request` on the id RECOVERED from the raw bytes (one valid id, or several all EQUAL), falling back to the id OMITTED when the bytes are ambiguous or the value is no representable `RequestId`, and then DROPS the frame instead of delivering it — which also removes a fatality nobody had connected to this defect: the same frame sent before `initialize` terminated the server with `ExpectedInitializeRequest`. This is the SPEC half, landing first per PROCESS.md §2. It mints a NEW D-id rather than amending D43 because the class decided here is not the duplicate-key class and because it REVERSES D43 clause (6) — a separately-motivated decision under PROCESS.md §3, so both rows carry reciprocal cross-references. Cascade: - PRD §4: the D47 row; the reciprocal SUPERSEDED note on D43 clause (6); NFR-18 widened from ~2 to ~2.35 unbounded per-line passes, with both surviving envelope residuals named rather than implied. - Spine §5.6: the envelope enumeration re-opened member by member (it omitted `id` entirely and stated the `params` case absolutely; both were wrong), and the CLOSED out-of-band list gains its fourth member. - Crate plan, crate CLAUDE.md, implementation plan, roadmap (markdown + the rendered v1.0.1 card, which is outside every doc-lint corpus). - D-range bump D1..D46 -> D1..D47 at every site the PROCESS.md §3 count-free LIST names: the three prose sites plus the live-range knob of all three shipped required check scripts. `docs/PRD.md` D46 row and `implementation-plan.md:40` are NOT bumped — they are frozen records of D46's own cascade. No contract movement: `-32600` is rmcp's transport-level code, not our 36-member taxonomy. `unblock.mcp.v1.9` stands and no `CONTRACT_RE` knob moves. Refs: ub-cnv
…me (D47) The transport now recovers the envelope `id` from the RAW BYTES before the parse, answers `-32600 Invalid Request` on it, and DROPS the frame. `src/envelope_id.rs` (NEW, crate-private) is the predicate: a `DeserializeSeed` over the ROOT object that keeps every top-level `id` member's value verbatim and `IgnoredAny`s every other member, yielding occurrence COUNT, mutual EQUALITY and `RequestId`-recoverability as a trichotomy. Three properties are normative and each carries a unit cell: keys are compared DECODED (a byte prefilter for the literal bytes `"id"` is unsound — it misses the \u-escaped spelling, which the shipped binary already answers on); ROOT LEVEL ONLY (the opposite discipline from `dup_key`, which descends at any depth); and NO short-circuit, which is exactly the property a collector fused into `dup_key`'s root loop would lose. Recoverability delegates to rmcp's OWN `RequestId` deserializer rather than to a hand-written type table, so agreement with the real decoder is structural. Equality is decided on the DECODED `serde_json::Value`, never on raw spans — `"a"` and its \u escape are ONE id and must be recovered, while `1` and `1.0` are unequal and take the conservative omitted arm. `src/wire.rs` gains the branch inside `receive()`'s `Ok(Some(..))` arm, guarded by an EXHAUSTIVE match on `JsonRpcMessage` so an rmcp bump adding a fifth variant is a compile error rather than a silent hole, and so all request traffic pays zero. The shipped `-32700` arm is refactored onto the same `answer_error` helper, which makes the two out-of-band arms identical by construction; both still encode through rmcp's own codec, so there is no hand-rolled byte path. DEVIATION from the implementation package, reported rather than absorbed: the package specifies `answer_error(&self, ..)` and calls the borrow "checked". It does not compile — `Transport::receive` requires a `Send` future and `&Self` is `Send` only if `Self: Sync`, which this transport is not. The helper takes the write half (`&Arc<Mutex<Option<W>>>`) instead, which satisfies both the `Send` bound and the disjoint-borrow requirement against `line`. The doc comment states this so the next reader does not "restore" the simpler signature. Measured over raw stdio, before and after, on the same frames: every class shape went from SILENT to answered, the recovered-id shapes answer ON that id (including the escaped key and the escaped-value pair), a frame with NO id stays silent, the F17 parity drop stays silent, the duplicated-`method` residual is still `-32700` with the id omitted (ub-788, deliberately open), and a clean request afterwards is answered normally. Pre-`initialize`, the same frames went from exit 1 with an `INTERNAL_ERROR` blob to exit 0 with the handshake completing; an id-LESS notification there still exits 1, which is out of D47's class by decision. No contract movement: `unblock.mcp.v1.9` stands, `ErrorCode::ALL` stays 36. Refs: ub-cnv, PRD §4 D47
A single whole-stream byte-identity assertion would stay GREEN with the entire -32600 arm deleted: all 17 shipped corpus entries were re-measured through rmcp's own AsyncRwTransport and NONE of them diverges under D47 — the Notification-delivering entries carry no id, and every id-carrying entry is a Request, an Ok(None) drop or a hard parse error. The risk was VACUITY, not breakage, so the repair is structural rather than a new assertion. - PARITY tier: the 17 kept, now LABELLED so divergences are attributable, plus four never-answered negatives the shipped corpus had no shape for — a NESTED params.id, a well-formed id on a notifications/* method (already a CustomRequest today), a duplicated method whose -32700 still omits the id, and a string VALUE whose bytes spell an id member's key. F17 is commented in place as the deliberate parity drop, whose exclusion is STRUCTURAL (it returns Ok(None) and never becomes a message) rather than a carve-out. - DIVERGENCE tier: the 23 new frames, each asserted on EXACT expected bytes including the recovered-versus-omitted id, because 'differs from rmcp' is satisfied by a mutant writing garbage. - SET-EQUALITY guard: the stream positions where we diverge from rmcp must be exactly the declared tier. Without it, an over-firing predicate that also answered the id-less F8/F12 keeps both other tiers green. Plus kind-coverage as a SET (a count rots; a set cannot be off-by-one against itself) and a corpus non-vacuity cell — including the D23-specific guard that its two id occurrences are NOT byte-identical while their decoded values ARE equal, without which the one cell that kills a raw-span value comparator silently degenerates into a copy of D02. Two shipped code comments that this work established FALSE are corrected here: the NS2 corpus comment generalised a method-dependent outcome past what it tests, and the compatibility-filter comment credited the -32700 with 'not leaving the client waiting' — that reply omits the id, which an rmcp client drops, so the client IS left waiting (the ub-788 residual). Refs: ub-cnv, PRD §4 D47
…shake Three suites, each homed where only it can prove something: - crates/unblock-mcp/tests/envelope_id_duplex.rs (NEW) carries the EFFECT ORACLE. The most plausible wrong implementation of D47 is not 'forget to reply' but 'recover the id, rebuild the frame as a Request, and deliver it' — which answers on the right id, writes plausible bytes, recovers the connection, and passes every channel-only assertion in the whole change. The only thing that distinguishes it is that the tool RAN, so a class tools/call naming a destructive action is asserted to leave the store fingerprint unmoved. - crates/unblock-cli/tests/envelope_id_frames.rs (NEW) drives a real child over real pipes. request_raw correlates BY ID and returns, which is the strongest available proof that the hang is over: on main that call cannot return, because nothing is ever written for that frame. - crates/unblock-cli/tests/mcp_lifecycle.rs gains the pre-handshake half, whose observation channel is client.seen_lines and NOT stdout_snapshot(): capture_stdout() CONSUMES the stdout reader and read_response panics once it is gone, so it cannot be called before the ping_barrier()/initialize() these cells must perform. Written like the sibling D38 cell, every line of interest would be consumed into seen_lines while stdout_snapshot() came back empty, and the two negatives that are the entire pin on 'the pre-handshake death is gone' would pass over an empty collection. Each therefore opens with a non-emptiness guard. The NS4 cell is SPLIT rather than inverted, because it does not go red under D47: its shipped frame carries 424242/999999 — DIFFERING — so the reply takes the omitted arm and both of its assertions stay TRUE. Only its prose was false. A straight inversion would have been wrong AND would have left the recovered arm — the half the unambiguity rule turns on — with zero end-to-end coverage. NS4a drives EQUAL ids (genuinely silent on main, so it is the before/after witness); NS4b keeps the original frame and both original negatives, plus a new assertion that a reply now exists at all. L-N2 pins a defect D47 deliberately does NOT fix — an id-LESS notification before initialize still exits 1 — and is partly redundant with the shipped D38 cell on purpose: that cell provokes its Err with exactly this frame, so a predicate that swallowed no-id frames would turn a D38-era cell red for a reason having nothing to do with D38. The coupling is now documented instead of latent. Harness: RawDuplexClient gains seen_lines (a reply with an OMITTED id is invisible to seen_ids and read_response would loop past it), and the effect oracle moves into tests/common so both duplex suites share ONE copy instead of drifting. Refs: ub-cnv, PRD §4 D47
Mint `scripts/checks/d47-envelope-id-claims.sh`, wire it as a step of the required `doc-lint` job, and add it to the two count-free enumerations of shipped check scripts (`docs/PROCESS.md` section 3 and `docs/plans/ci-cd-and-distribution.md` section 2.1) — including the "newest reference" sentence in both, which must name this script now that the list grew: by the self-row rule a script never pins its own knob, so the D46 sibling stopped covering the enumeration the moment D47 joined it. The gate is deliberately POSITIVE-ONLY. A negative sweep for the framing D47 retires is unfindable in principle once a claim is rewrapped across two lines, so every row is a spelling-independent presence landing: the predicate module and its `scan` entry point, the transport's `-32600` reply constant, the EXHAUSTIVE `JsonRpcMessage` match (a `_` wildcard would compile, pass every cell and silently reopen the hole), the decoded-key rule, the recovered arm's `Some(id)`, the gated shared corpus, the store-effect oracle, the disclosed `-32700` residual, both tracker ids, the rendered roadmap card that sits outside every lint corpus, this gate's own two-sided wiring, and the live D-range at every file the section 3 list names. The `CONTRACT_RE` knob pins `unblock.mcp.v1.9` as UNMOVED: `-32600` is `rmcp::model::ErrorCode`, a transport-level type in another crate, so D47 mints no `ErrorCode` and bumps no contract. An unstated "we didn't bump" is indistinguishable from an oversight. Refs: D47, ub-cnv
Carries ub-788 (the deliberately-unclosed -32700 id-omission residual that the D47 row names) and ub-og3 (the stdout framing-channel defect) into the git record, so the PRD names no id absent from it. Discharges the d47 claims gate's P10 row. That row pins a TRACK-step landing while the script itself is minted in the implementation commit, so the gate was red by construction until this landed — a package sequencing defect the implementer reported rather than worked around by weakening the assertion.
… rows The gate script shipped mode 100644 while all four siblings are 100755, and the required `doc-lint` job invokes every one of them as a bare relative path with no interpreter — so its first CI run would have died with a permission error (exit 126), not a rule failure. Restore the executable bit. Two of its rows were also satisfiable by prose. P4 (the decoded-key rule) and P5 (the recovered-id arm) were presence predicates over a bare token, and both tokens also appear in a doc comment and a test body — so each row stayed GREEN under the exact mutant its own reason text named: rewriting the production recovered arm from `Some(id)` to `None` left the script exiting 0 while still printing that the arm was in the tree. That is a false coverage claim inside a required CI gate, i.e. the class this gate exists to prevent. Both are re-anchored as Q rows on their PRODUCTION lines, the remedy the D46 sibling states verbatim. Proven, not asserted: `Some(id)` -> `None` at wire.rs:326 now exits 1 (vanished anchor); a raw-span key type at envelope_id.rs:192 exits 1 (anchored line fails the requirement); deleting that loop exits 1 (vanished anchor). The P codes are not reused and the survivors are not renumbered — the gap is documented in the header instead, so no prior reference to a P code silently changes meaning. Both floors move to 12: the two rows crossed tables, they were not dropped, and the total stands at 24. Refs: ub-cnv, D47
…s (D47)
The crate plan's `src/envelope_id.rs` row promised three cells that did not
exist, and a plan row is normative prose: it said "plus a proptest that `scan`
agrees with `RequestId::deserialize` for any spliced `Value`" while what shipped
was a fixed 14-value table whose own doc line repeated the "for ANY value"
claim; it named an `Absent` cell for a non-object root and for an `id` inside a
ROOT-LEVEL array element, where the shipped cell drives an array nested under
`params`; and it named an `Unusable` empty array, which the shared corpus spells
`[1]`. Miguel's ruling was to land the tests, not to soften the plan.
- A15 is the property itself, over a bounded recursive `serde_json::Value`
strategy. It is non-vacuous under a mutation a fixed value table cannot
express: truncate a string id inside the collector and A15 is the ONLY cell
that fails. Under the coarser mutant — `RequestId::deserialize` replaced by a
hand-written type table — A15 and A12 fail TOGETHER, which is what should
happen rather than evidence for either cell: they splice the same boundary
values, so a counterexample drawn from that table is a counterexample to both.
- A12 keeps the 14-value table as its named-boundary regression corpus, with
its doc line corrected to claim exactly that.
- A14 covers the root shapes: every scalar root, a string root spelled `"id"`,
an array root, and `[{"id":3}]`.
- The empty-array id joins A5 alongside the corpus's `[1]`.
A14's doc states what it does NOT do, because the opposite reading is the one a
reader will take: the eight visitor arms it exercises are NOT graded by it.
Deleting all eight leaves the whole 168-test suite green — `deserialize_any`
then fails and `scan` maps a failed seed to `Absent` too, so the verdict is
identical either way. That is an equivalent mutant by construction, not a hole
a byte corpus can close.
Refs: ub-cnv, D47
The branch stated the answer-on-the-recovered-id guarantee without a caveat, and there is one. rmcp polls the transport as one arm of an unbiased `tokio::select!` (rmcp 1.7.0 `src/service.rs:805`, the `receive()` arm at `:813`), so a losing poll DROPS that future — and the D47 reply is written inside `receive()`, so on cancellation it is never written, while the frame goes with it because the loop clears its line buffer on the next iteration. The Verify gate's attack lens measured 0/40 lost on an idle connection, 25/40 with four requests in flight and 39/40 with eight. Miguel's ruling was DISCLOSE AND TRACK, not fix, and the reasons are recorded rather than assumed: it is pre-existing (the shipped `-32700` arm has the same shape and the same loss rate on main), and the pre-handshake fatality this arm removes occurs by definition with zero requests in flight — the regime measured at 0/40 — so that half of the fix is unconditional. The transport write path is untouched here. The residual lands in the two places the branch already discloses its other one: the D47 decision row's residual list and the `wire.rs` module doc. The mutex claim is scoped at `answer_error` itself to what it ever established — byte-atomicity, one whole frame per guard — with cancellation named as the separate hazard it does not cover. `ub-TBD` is a PLACEHOLDER at both sites, to be replaced by the tracker id of the issue opened for this residual before the PR is opened. Refs: ub-cnv, D47
The Track step's two same-commit duties for ub-cnv (PROCESS.md §6/§8): the regenerated git record and the wiki run-report, on top of the gated work. The export carries 47 records, including the three issues this work opened — ub-og3 (the non-JSON-RPC blob on the framing channel), ub-nbz (the -32600 lost to a cancelled receive()) and ub-q1u (the libsql stress flake reported rather than swept) — plus the ub-cnv comment thread through the Implement outcome. It is a generated artefact under the D5 export-only model and was produced over the tracker tool, never hand-edited. The report records what the diff cannot: the class predicate and why duplication was only the minority route, the four binding rulings, the two impossibilities escalated rather than absorbed (the explicit-null reply rmcp's own type cannot serialize, and the eight visitor arms that are an equivalent mutant), the full gate history including a delta-verify that failed inside the repair of its own findings, the mutation result (26 of 26 catalogue mutants killed, zero survivors), the four open residuals, and two gotchas worth carrying forward — a required check that pins a landing from a later step is red by construction, and a claims script without its executable bit passes every local `sh` invocation while dying in CI. It lands BEFORE the commit that adds the P15 row, so the row's landing exists in the record at every commit rather than only at the tip. Refs: ub-cnv, D47
The decision row disclosed a FOURTH v1.0.1 residual — the -32600 is lost whenever rmcp cancels the receive() future — under the placeholder `ub-TBD`, which the issue now exists to replace, and left two roadmap sites saying "Three residuals stay OPEN" above a list the same decision had grown. - Both `ub-TBD` sites become `ub-nbz`: the D47 row's clause (8)(v) and the transport module's matching disclosure. A named residual in the top document of the hierarchy must resolve, or it is worse than the vagueness it replaced. - The v1.0.1 card loses its count word entirely rather than gaining a bigger one, in both the markdown roadmap and its RENDERED twin, and both lists gain the concurrency residual. The list is the rule and carries no count — a derived count has already rotted at this exact site five times. The HTML is the published artefact and sits outside every lint corpus, so only the D47 gate's own row can notice it going stale. - Two sentences in the transport's module documentation are aligned with the figures the decision row states: 0 of 40 lost with the connection idle, 25 of 40 with four requests in flight, 39 of 40 with eight, from ONE harness and unreplicated. "Idle: never lost" claimed more than one unreplicated harness measured. Documentation only — no behaviour changes, no contract byte moves. Refs: ub-cnv, ub-nbz, D47
The D47 row now names TWO residual issues, ub-788 and ub-nbz, and the gate pinned only the first. An id a decision row names and the tracker does not carry is a dangling reference in the top document of the hierarchy, which is exactly the failure P10 exists to prevent — so its sibling gets the same treatment. P15 mirrors P10 rather than widening it: one row matching either id would go green with the other dangling. The P-table floor moves 12 -> 13 with the row it counts, and the closing line names both ids instead of one. The row's landing is already in the record — the tracker re-export commit precedes this one deliberately, because a required row that pins a landing from a LATER step is red by construction, which this decision's own gate learned the hard way with ub-788. Verified by running the script the way CI does, as a bare relative path (`sh script` never exercises the permission bit). Refs: ub-cnv, ub-nbz, D47
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
1 participant
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.
Closes
ub-cnv. Decision D47 (v1.0.1 patch slot), spec-first.The defect
A JSON-RPC frame whose envelope request
idcannot be decoded fell through rmcp's untagged message union into theNotificationvariant and was silently dropped — no reply, no diagnostic, no stderr line at default verbosity. An rmcp client correlating by id waits forever:Peer::send_requestuses no options, soawait_responseis an untimed await, and the client drops id-less errors.Duplication was the minority route. Reproduced live on
main, each of these was silent: the same id twice, two different ids,"id":null,"id":{},"id":true, an integer beyond 64 bits, and1e2— while the identical value written100was answered normally. The honest class predicate is the bytes carried a top-levelidand the decode produced a Notification.Worse before the handshake: as the first frame on a connection it killed the process — exit 1, with a non-JSON-RPC blob written onto stdout, the framing channel.
The fix
The owned transport recovers the envelope
idfrom the raw bytes before the parse, answers-32600 Invalid Requeston it, and drops the frame — never delivering it, because delivering is what causes the pre-handshake death.idis a legitimate Notification and stays silent — verified byte-identical on both binaries across nine shapes. Replying to it would violate JSON-RPC's rule against answering notifications.DeserializeSeedwalking only the root object, comparing decoded keys (an escaped spelling cannot slip past) and decoded values, with no short-circuit, delegating the type check to rmcp's ownRequestIddecoder. The variant match is exhaustive, so a future rmcp variant is a compile error rather than a silent hole.Contract-neutral:
CONTRACT_VERSIONandCONTRACT_HASHunmoved, no new error code —-32600is rmcp's transport-level code, not our taxonomy.Before / after, on raw bytes
main-32600on that idnull·1e2·{}-32600, id omittedid-32600, then a successful handshake on the same connection, exit 0Answer-and-drop proven by effect, not by reading: six in-class frames carrying issue-creating payloads were each answered and the subsequent listing returned an empty store.
Gates
Design Review — pass with ten must-fixes → revision → delta-verify FAIL on three defects, each sitting inside the repair of its own item → escalated under the two-iteration rule → Miguel authorised direct closure → closed and verified.
Verify — pass with four must-fixes. 26 of 26 catalogue mutants killed, zero survivors, plus ~30 further mutants across two lenses with disjoint harnesses and no non-equivalent survivor. Fix pass → re-verify PASS.
Full local gate green: fmt, clippy pedantic, 1703 tests,
insta --check, doc-lint (19 docs), knowledge-lint (61 pages), check-layering, all five claims scripts as bare relative paths, and the run-report gate.Two things for the reviewer's eye
1. A coverage claim was corrected rather than shipped. One commit message asserted a property-test cell fails alone under a given mutant. It cannot: a sibling cell splices the same boundary value using the real decoder as its oracle, so both must fail together. The message was reworded (branch unmerged; tree byte-identical before and after —
git diffbetween the old and new tips is empty). The defensible statement: the property test is non-vacuous because under a collector mutation a fixed value table cannot express — truncating a string id in the collector — it is the only failing cell; under the type-table mutant it and the sibling fail together, as they should.2. An equivalent mutant was declared, not faked. The Verify gate asked for eight visitor arms to be graded, on the premise that a missing root-shape cell would do it. That premise was false: deleting the arms and keeping them both end at the same verdict through different branches, so no byte corpus can distinguish them. The fixer said so and wrote the limit into the test's own doc instead of claiming a kill.
Residuals — disclosed, tracked, not claimed closed
ub-nbz— the reply is lost whenever rmcp cancels thereceive()future under concurrent traffic (0/40 idle, 25/40 at four in flight, 39/40 at eight; one harness, unreplicated, recorded as such). Pre-existing: the shipped-32700arm behaves identically. Out of D47's decided scope; Miguel ruled disclose-and-track.ub-788— the-32700arm omits a readable id, so duplicatedmethod/jsonrpcframes still leave an rmcp client pending. Blocked on this PR.ub-og3— the non-JSON-RPC blob on stdout at startup failure (this PR removes one route to it, not the defect).ub-q1u— an intermittent storage stress-test failure observed during the gate; unrelated to this branch.A behaviour is REMOVED and said so plainly: a cancellation notification carrying an un-decodable id currently cancels a matching in-flight request inside rmcp's serve loop; it is now answered and dropped, so that stops happening. A conforming cancellation, which carries no id, is unaffected. Miguel ruled: disclose, do not preserve — preserving it would mean delivering after answering, which reopens the pre-handshake fatality.
Gotchas worth carrying forward
shinvocation and dies in CI, which invokes it as a bare relative path.Run-report:
.knowledge/wiki/runs/2026-08-06-envelope-id-reject.md.