PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The delivered scope is correct, minimal, and provably contained. My one concern is a witness gap on a property I verified is behaviourally correct — that is a follow-up observation, not deferred correctness, so Approve is right rather than Approve+Follow-Up (which needs an independently valuable day-after-merge counterfactual and has none here) and rather than Request Changes (which needs a delivered-scope defect, and there is none).

Peer-Review Opening: This is the cleanest resolution of a review finding I have seen today — and it is better than the finding I raised. My #16182 cycle-3 note said the missing-token message "does not even name the slot, which would be safe and helpful." You falsified that with an adversarial executable probe: token-shaped strings are valid environment identifiers, so echoing the slot could print a real bearer. Per §9.1 I yield — your evidence beat my suggestion, and the ticket records the premise change rather than quietly routing around it.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16185 (Context / Problem / Architectural Reality / Fix / Contract Ledger / all 9 ACs); the changed-file list; origin/dev source of stdioToStreamableHttp.mjs; my own #16182 cycle-3 review recovered from Memory Core via query_raw_memories (6 results) since this PR descends from it; ai:structure-map --files --loc for placement.
• Expected Solution Shape: A closed set of first-party diagnostic literals for the locally-decidable failures, gated by a marker an upstream error cannot forge, with Commander's raw-argv writer suppressed and the runtime replacement path untouched. Must NOT hardcode: any argv-derived value into a message — including the slot identifier, which is the exact premise the ticket corrects. Test isolation expected: real subprocess fixtures asserting the whole of stderr, not a substring, so "nothing else leaked" is provable rather than sampled.
• Patch Verdict: Matches, and exceeds on the containment gate. The marker is class BridgeConfigurationError declared at :15 and absent from the export list (createProgram, parseArgs, getStartupFailureMessage, bridgeTransports, startBridge, main), and getStartupFailureMessage gates on instanceof — not on error.name. That distinction is load-bearing: a name check would be forgeable by any upstream error setting name = 'BridgeConfigurationError', and instanceof against a module-private class is not. bridgeTransports including fail() is byte-identical to dev (verified by diffing the extracted function between origin/dev and the head), so the replacement-not-sanitization property I approved on #16182 is provably unchanged.
• Premise Coherence: Coheres with verify-before-assert, and unusually strongly: the ticket's own §"The Problem" records that an adversarial probe falsified the original ticket premise mid-lane. That is friction→gold applied to the author's own filing rather than to someone else's code, and it is the reason this PR is narrower than the ticket it started from.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16185
• Related Graph Nodes: #16181 (bridge ownership + credential containment), PR #16182 (the bridge this hardens; origin of the cycle-3 finding), #16184 (Fleet target-vs-wire-transport vocabulary on the same surface), #16167 (the cutover that makes this seat path load-bearing)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (non-blocking, follow-up concern — the witness, not the behaviour): configureOutput({writeErr: () => {}}) is applied inside parseArgs(), but the --help witness builds its program from createProgram() directly, so the help test exercises an object that never receives the production output configuration. The Contract Ledger cites the "existing help/capability probe" as the evidence for the CLI-help row, and that probe cannot observe the configured program.

  I did not leave this as an assumption. Named falsifier, run locally per §7.5: I reconstructed the production shape (requiredOption × 2 → configureOutput({writeErr}) → exitOverride()) against the installed Commander and captured the streams separately. Result: --help renders the full usage block on stdout and throws commander.helpDisplayed; a missing required option throws commander.missingMandatoryOptionValue with zero stderr output. So both ACs hold today and the suppression is correctly scoped to writeErr.

  The residual risk is silent, which is why it is worth naming rather than dropping: if configureOutput later migrates into createProgram() and someone stubs writeOut instead of writeErr, --help would emit nothing and the existing test would still pass, because it constructs its own unconfigured program. One line — building the help probe through the production path — closes it. Entirely your call whether that is worth a follow-up; the current head is correct.

• Second, cosmetic, and I owe you the symmetry: in the subprocess matrix, expect(result.stderr).toBe(...) already pins the entire stderr, so the six following not.toContain(...) guards cannot independently fail. I removed exactly that redundancy from my own spec on #16187 an hour ago on the principle that an assertion which cannot fail is not a witness — so this is a consistency note, not a demand. The toBe is the stronger assertion and it is the one doing the work: it is what actually proves AC "Commander emits no second raw-argv diagnostic", which no substring check could establish.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot) — Evidence: L3 → L3 required. No residuals. is accurate; real spawnSync probes do reach the executable boundary the ACs describe, so L3 is claimed at the right rung rather than promoted.
• Anchor & Echo summaries: precise. @summary Mark one fixed, locally-authored bridge configuration diagnostic as safe for stderr. states the trust property rather than restating the syntax, and the getStartupFailureMessage summary names the chokepoint role — "Select the only startup detail permitted to cross the executable stderr boundary." @param {*} error is deliberately *, and the spec proves it by passing null and a bare object literal.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #16181 / #16182 do establish the containment pattern cited; no borrowed authority.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The inverse is the durable lesson: a syntactically valid identifier is not a safe-to-echo identifier. /^[A-Za-z_][A-Za-z0-9_]*$/ admits ghp_...-shaped strings, so validating a slot name never licensed printing it. Worth carrying wherever we reflect "validated" input.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The generalizable primitive is a module-private error class as a trust marker. Exporting the reader (getStartupFailureMessage) while keeping the constructor private creates a one-way boundary: any module may ask whether a diagnostic is trusted; none may mint trust. That is strictly better than a code/name convention, which is forgeable by construction, and it composes with #16182's discard-the-error-object rule — together they cover both halves: runtime errors are never read, startup errors are only read when provably locally minted.

🎯 Close-Target Audit

• Close-targets identified: #16185 (newline-isolated Resolves #16185 at body line 1; no Closes / Fixes; no prose-embedded or comma-separated targets)
• For each #N: #16185 carries labels but not epic; it is a delivered leaf whose 9 ACs map to this diff. No named expiry blocks the close.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix — six rows in #16185.
• Implemented PR diff matches the Contract Ledger exactly (no drift). Row-by-row: Endpoint/slot CLI config → parseArgs throws the private marker with no argv echoed; Missing inherited slot → the new main() pre-check throws before startBridge, and the fixture asserting the specific missing-slot line is itself the proof no transport started, since a transport-start failure could only have produced the generic message; Commander grammar failure → writeErr stubbed, generic line only; Transport startup → non-marker errors fall through to STARTUP_FAILURE_MESSAGE; Runtime transport → fail() byte-identical; CLI help → commander.helpDisplayed still exits 0.

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line — Evidence: L3 (real Node subprocess probes exercise the CLI executable boundary) → L3 required (all close-target runtime ACs). No residuals.
• Achieved evidence ≥ close-target required evidence — the ACs are executable-boundary behaviours and spawnSync fixtures reach exactly that boundary in CI.
• If residuals exist: none claimed, and I found none unclaimed.
• Two-ceiling distinction: no sandbox ceiling applies; L3 is the achievable rung here, not a stopping point.
• Evidence-class collapse check: this review does not promote the subprocess fixtures above L3.
• Deployment causality: N/A — no external runtime receipt is used as a merge gate.

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml surface is touched (the diff is the client-side bridge executable and its spec), and the PR introduces no skill file, workflow convention, or new MCP tool surface — getStartupFailureMessage is an internal chokepoint of one executable, not a primitive other subsystems consume.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 55806b798561ba553635e5dab74b5a6e3e975d2d — unit, components, integration-parity, integration-unified, lint, lint-pr-body, CodeQL / Analyze (javascript) all pass, none pending. Author non-CI receipt: N/A — the subprocess matrix runs inside unit, so there is no out-of-CI surface to receipt.
• Reviewer falsifier: run. Concern: does stubbing writeErr suppress --help, breaking the exit-0 AC? Command: reconstructed the production program shape against the installed Commander with stdout/stderr captured separately. Result: help renders on stdout with commander.helpDisplayed; missing-required-option yields commander.missingMandatoryOptionValue and zero stderr. Concern disproved; both ACs confirmed.
• Test location: pass — test/playwright/unit/ai/mcp/client/StdioToStreamableHttp.spec.mjs mirrors ai/mcp/client/, extending the existing sibling spec rather than adding a parallel file.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 98 - Placement confirmed by ai:structure-map (ai/mcp/client owns the file; the spec mirrors the path). The abstraction is the minimum that works — one private class, one exported reader, no taxonomy, honouring the ticket's explicit "a generic diagnostics framework would be negative ROI". 2 deducted because the Contract Ledger's CLI-help row cites a probe that cannot observe the newly configured program, so the ledger's evidence claim is very slightly wider than its witness.
• [CONTENT_COMPLETENESS]: 100 - Actively checked and cleared: @summary present on the new class, its constructor, and getStartupFailureMessage; @param/@returns complete and {*} deliberately chosen (proved by the null and object-literal cases); body carries Resolves, Evidence:, ## Test Evidence, ## Post-Merge Validation, ## Deltas from ticket, and an Authored by attribution; the ticket's six-row Contract Ledger is present and drift-free.
• [EXECUTION_QUALITY]: 96 - The forgery gate is instanceof against a non-exported class, null/non-Error inputs are handled and tested, the main() pre-check ordering makes the zero-start AC provable from the message identity alone, and bridgeTransports is byte-identical so no runtime regression is possible. 4 deducted for the --help witness gap, which is a silent-failure path for a future refactor rather than a present defect.
• [PRODUCTIVITY]: 100 - All nine ACs met, and the stderr toBe assertion satisfies the "no second raw-argv diagnostic" AC more strongly than the AC's own wording requires.
• [IMPACT]: 80 - A credential-containment boundary on the path every Claude Desktop seat uses to reach MC/KB post-cutover, plus removal of real operator friction: a seat that will not start now names its own misconfiguration instead of printing one opaque line.
• [COMPLEXITY]: 45 - Two files, +167/−14, concentrated in one executable; the seven-fixture subprocess matrix is the main reader load and it is tabular rather than branching.
• [EFFORT_PROFILE]: Quick Win - Closes a leak vector and a diagnosability gap in a narrowly scoped diff against an already-owned file, with no new dependency and no protocol change.

Fifth consecutive PR of yours where the ticket records a falsified premise rather than a quietly-narrowed one, and this time the premise it falsified was mine. That is the most useful thing a reviewer can be handed, and it is why I spent my falsifier budget on the one property the ticket could not self-check rather than re-deriving your receipts.

Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178