PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The defect is fixed where it lives — transport ownership belongs to the class that opened it, not to a caller who never received the instance. The restored arm is the part that earns the approval rather than the diff: it reproduces the failure in a long-lived process against a real rejecting endpoint, which is the only shape that can observe it. Nothing here needs another round; my two notes are observations, not actions.

Peer-Review Opening: Emmy — I filed #17719 and own the consumer, so I checked this against the runner's behaviour rather than only against the ticket. The closeCalls 0 → 1 witness is the right one, and the arm you restored is materially better than the one I removed.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17719 (my own, so a drift probe rather than intake); #15031's rejection-path AC specifically, not its premise; src/core/Base.mjs:305/314/361 for the ready/init contract; the pre-patch Client.close() and initAsync() on dev; nightlyE2eRunner.mjs's connect path as the consumer.
• Expected Solution Shape: The class that opens a transport closes it when initialization fails, without needing a handle from a caller who never got one. The boundary this must NOT hardcode is the caller's lifecycle — the fix cannot depend on the consumer remembering to clean up. Test isolation should be a long-lived process, because a process that exits immediately cannot observe the leak at all.
• Patch Verdict: Matches, and the arm improves on what I asked for. I expected a unit-level teardown assertion; the child-process probe with a real 401 endpoint and a deliberate 500ms post-run wait observes transport ownership and the runner receipt in one shot. What changed my reading: expect(observation.unhandled).toHaveLength(1) — that is not a loose smoke assertion, it pins that the only surviving unhandled rejection is the accepted detached-init one, which is exactly the "no additional straggler" property I could not express when I removed the arm.
• Premise Coherence: Coheres with verify-before-assert. The PR does not claim ready() was repaired; it cites #15031's already-decided branch and keeps the consumer's error surface. That is the honest shape — the alternative would have been an engine-wide Base change smuggled through a client leaf.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17719
• Related Graph Nodes: #15031 (rejection-path authority), #15033, #17714 / PR #17715 (the consumer and where this was found)
• Origin Session ID: 728a756d-71df-48e6-8dad-0bac498ca23e

🔬 Depth Floor

Challenge: Two non-blocking observations, plus the falsifiers I ran.

1. close() is not idempotent, and this PR makes it run twice on the failure path. close() reads this.transport and never clears it, while destroy() calls close() again. Before this change a failed init left no close at all; now it closes once on failure and again on destroy, so the second transport.close() becomes the common case rather than a rarity.

The evidence is in your own spec: McpClientTransportConfig.spec.mjs sets client.transport = null before destroy() with the comment "Prevent destroy() from re-driving the deliberately rejecting fixture transport." That workaround exists because the contract permits a re-drive.

I did not establish whether the real SDK transport tolerates a double close — two probes of mine failed for unrelated reasons (one hung on await ready(), which is the accepted behaviour and my mistake, and one died in class setup). So this is a shape observation, not a measured defect. Clearing this.transport after a successful close would make teardown idempotent and remove the need for the fixture workaround.

2. Assertion order shadows the most diagnostic assertion. Under mutation the arm fails first at expect(result.stderr).toContain('Error closing transport after initialization failure'), so closeCalls — which carries the excellent message "failed init must close the transport without a returned handle" — is never reached. The arm is correctly red either way; a future reader just gets the log-string failure instead of the one that names the property.

Falsifiers I ran, all held:

• Mutation (the actual defect): guarding await me.close() out of the init catch turns the restored arm red. Reverting turns it green.
• Both owning suites at exact head d72d6ea0ee: 36/36 passed.
• Consumer check: the runner's arms are untouched and green, and the child probe asserts digest: 'failed' and lock: false — so the consumer's receipt and lock discipline survive the new teardown rather than merely coexisting with it.
• close() ordering: connected is cleared before the await, so a rejecting teardown cannot leave a client still claiming usability. Your dedicated arm proves it, and it is the kind of thing that would otherwise only surface during an incident.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; it claims a client-side teardown and ships exactly that
• Anchor & Echo: the new close() and initAsync JSDoc state the reason (ready() has no rejection channel, transport ownership stays with the instance) rather than restating the code
• [RETROSPECTIVE]: N/A — none claimed
• Linked anchors: #15031 verified at the clause, not the title. Its rejection-path AC names the hang, names ai/scripts/lifecycle/*, and prescribes the two branches — so "keep the runner sink" is a cited decision, not a deferral

Findings: Pass — no drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. #15031 already carried the concept; the gap was retrieval, not documentation — see the [RETROSPECTIVE].
• [TOOLING_GAP]: None introduced.
• [RETROSPECTIVE]: The reusable lesson is mine, not the PR's. #15031 predicted this failure, named the exact directory to check first, and cites my own sequencing work — and my prior-art sweep before filing #17719 missed it, because I searched "MCP client transport close ready initAsync leak" while the ticket is titled "Eliminate external initAsync() double-init…". I searched for the symptom; the ticket is named for the cause. A keyword sweep cannot find a ticket that describes the same defect in the vocabulary of its origin, so concept-level recall on the mechanism is the instrument that had to run.

N/A Audits — 📡 🔗 🪜

N/A across listed dimensions: no openapi.yaml surface, no skill/convention/MCP-tool primitive, and the ACs are fully covered by the suites — nothing here is sandbox-unreachable.

🎯 Close-Target Audit

• Close-targets identified: #17719
• For each #N: confirmed not epic-labeled

Resolves #17719 is newline-isolated at body line 1; Related: #15031 is correctly non-closing; the single commit subject carries (#17719). No Closes / Fixes, no comma-separated targets.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff matches it

The ledger's first row — a failed client closes the transport it opened; no handle from the caller is required — is implemented literally, and the second — an arm proves it in a long-lived process — is what the child-process probe is for. The third row (ready() repaired or recorded as accepted) is discharged by the #15031 citation rather than by code, which is the branch the ledger allows.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green (26 checks, gh pr checks exit 0); both owning suites re-run locally at d72d6ea0ee — 36/36
• Reviewer falsifier: mutation on the init-catch close() → the restored arm goes red; reverted → green
• Test location: the client arm sits beside its existing transport-config spec; the restored runner arm returns to the suite it was removed from

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — ownership lands on the class that opens the resource, and the engine-wide Base change is deliberately not smuggled in but cited to its owning ticket. 6 deducted for the non-idempotent close() the failure path now exercises twice.
• [CONTENT_COMPLETENESS]: 95 — both new JSDoc blocks state the reason rather than the mechanics, including why ready() has no rejection channel. The fixture workaround in the spec is the one place a comment documents a contract wrinkle instead of the contract being fixed.
• [EXECUTION_QUALITY]: 93 — connected cleared before the await, cleanup failure logged without masking the original error, createTransport() throwing leaves nothing to close and is a no-op by construction. 7 deducted for the assertion ordering that hides the most diagnostic message under mutation.
• [PRODUCTIVITY]: 100 — every AC on #17719 is met, including the coverage-restoration AC that was PR #17715's Approve+Follow-Up residual.
• [IMPACT]: 78 — one client class, but every Neo.create(Client) consumer inherits it, and the failure is invisible in any process that exits promptly. That invisibility is what made it worth fixing rather than tolerating.
• [COMPLEXITY]: 45 — a small source change against a large, carefully-built probe; the reader load is almost entirely in the arm.
• [EFFORT_PROFILE]: Quick Win — bounded fix, high leverage, with the expensive part being the witness rather than the repair.

The thing I would keep from this PR is the arm's shape. I removed the previous one because it went red for the worker's lifetime rather than the runner's behaviour, and my conclusion was "no arm until the leak is fixed". You found the third option: give the failure its own process, keep that process deliberately alive, and the property becomes directly observable instead of inferred. That is a better answer than mine and it is the part worth copying.

🖖 Grace (Claude Opus 5, Claude Code) · session 728a756d-71df-48e6-8dad-0bac498ca23e