fix(core): register peer-request handlers before connect (#1797)

[cliffhall]

Closes #1797

Problem

Connecting to npx -y @modelcontextprotocol/server-filesystem /tmp, the server's roots/list is answered -32601 Method not found. Connecting to server-everything, the same request returns the configured roots. Same client, same settings.

InspectorClient advertises capabilities.roots (and sampling/elicitation) on the SDK Client at construction, so from the moment connect() sends notifications/initialized the server is entitled to issue those requests. The handlers were registered after the handshake resolved and after fetchServerInfo() and the optional setLoggingLevel() — a window in which the SDK Client has no handler and replies -32601.

server-filesystem asks for roots the instant it is initialized (that is how it learns its allowed directories) and loses the race; server-everything asks later and wins. The consequence is not just a red entry in the Protocol view — server-filesystem silently falls back to its CLI-argument directories instead of the roots configured in the Inspector.

Fix

Extract the four peer-request handler blocks — sampling/createMessage, elicitation/create, roots/list, and the receiver-side tasks/* polls — out of the middle of connect() into registerPeerRequestHandlers(), and call it before client.connect(this.transport).

None of them depend on server capabilities; they read only constructor-set state (this.roots, this.sample, this.elicit, this.receiverTasks). The notification handlers that do gate on this.capabilities (tools/resources/prompts list_changed, etc.) stay where they are, after initialize.

The diff is large but almost entirely the moved block — the behavioural change is the call site plus one dedent level.

Testing

New clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts drives the race deterministically: a fake transport delivers roots/list synchronously from inside the send() of notifications/initialized — the earliest instant any server could ask — and asserts the reply carries result.roots, not an error. It fails on the pre-fix ordering (expected { id: 9001, … } to not have property "error") and passes with the fix.

An HTTP-server integration variant was tried first and rejected: it passed both with and without the fix, because whether the server's request lands before or after the post-connect awaits is network timing. It would not have guarded the regression.

npm run ci green (twice — the second run against the final file).

🤖 Generated with Claude Code

https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 3s —— View job

---

Review: fix(core): register peer-request handlers before connect (#1798)

• Read the diff against origin/v2/main
• Verify the handler move is behaviour-preserving
• Review the new timing test
• Post review

---

Verdict: the fix is correct and well-targeted. No blocking issues. Two low-priority observations below.

Verification I did

I confirmed the diff is a pure move. Stripping indentation and comparing the extracted block against the deleted one, the only differences are prettier re-flows caused by the one-level dedent:

< return Promise.resolve(\n taskResult as unknown as CreateMessageResult,\n);
> return Promise.resolve(taskResult as unknown as CreateMessageResult);
< this.client.setRequestHandler(\n "sampling/createMessage",\n samplingHandler,\n);
> this.client.setRequestHandler("sampling/createMessage", samplingHandler);
< const elicitHandler = (\n request: ElicitRequest,\n): Promise<ElicitResult> => {
> const elicitHandler = (request: ElicitRequest): Promise<ElicitResult> => {

Everything else is byte-identical. The behavioural change is exactly the new call at core/mcp/inspectorClient.ts:1372.

Ordering safety checks that hold:

• this.client exists at the new call site. It is constructed once in the constructor (inspectorClient.ts:628) and never recreated (this.client = appears exactly once in the file), and connect() already hard-guards it at line 1257. So the && this.client narrowings inside the moved block are satisfied.
• Client capabilities are construction-time (clientOptions.capabilities set at :620), so any capability assertion the SDK performs inside setRequestHandler is satisfied before connect() — nothing here needed the handshake.
• Re-entrancy on reconnect/retry is unchanged. registerPeerRequestHandlers() still runs once per connect(), and setRequestHandler overwrites the map entry before installReceiverTaskResponseBypass re-wraps it — so the bypass wrapper can't nest/accumulate across reconnects or across an auth-recovery retry. Same as before the move.
• The call sits inside the existing try (opened at :1366), so a throw during registration still routes through the catch at :1564 (status → error, cached transport dropped).

The doc comment on registerPeerRequestHandlers is unusually good — it records why the ordering is load-bearing and why the notification handlers stay behind. That's the right thing to leave for the next reader.

1. notifications/roots/list_changed is still in the post-handshake block, and it's the same race class (nit)

inspectorClient.ts:1433-1445 registers the roots-list-changed notification handler under a bare if (this.client) — it is the one handler in the post-handshake block that does not gate on this.capabilities. By the PR's own criterion ("nothing here depends on server capabilities → move it"), it qualifies. A server that emits notifications/roots/list_changed immediately after initialized currently has that notification silently dropped, since the SDK's _onnotification finds no handler and falls through.

The impact is far smaller than the roots/list bug (a dropped UI-refresh signal, not a wrong answer on the wire), so I would not hold the PR for it — but it's cheap to fold in now while the seam is fresh, either into registerPeerRequestHandlers() or a sibling registerPeerNotificationHandlers().

(notifications/elicitation/complete at :1539 gates on this.elicit, also constructor state — but it's only reachable after a tools/call, which can't happen during the handshake window, so it genuinely doesn't matter where it lives.)

Fix this →

2. The test pins only roots/list, not the other three handlers (optional)

inspectorClient-peer-handler-timing.test.ts is well-designed — the fake transport delivering roots/list from inside send("notifications/initialized") is the earliest instant physically possible, and the PR description's reasoning for rejecting the HTTP-server variant (it passed both with and without the fix, so it guarded nothing) is exactly right. Placement (src/test/core/mcp/) and naming (inspectorClient-*.test.ts) match the siblings.

The gap: four handler blocks moved, one is asserted. A future refactor could move sampling/createMessage, elicitation/create, or tasks/* back out of registerPeerRequestHandlers and this test would stay green.

The cheap tightening is tasks/list — unlike sampling/elicitation it needs no user interaction to resolve, so injecting it alongside roots/list in the same send() (with receiverTasks: true on the client options, inspectorClient.ts:473) and asserting a non-error reply would lock the ordering for the whole block at ~10 extra lines. Take it or leave it.

Two triviata in the same file: sessionId and setProtocolVersion are declared on InitializedRacingTransport but never used (they're optional on Transport, so they can just go), and expect(reply).not.toHaveProperty("error") is subsumed by the toMatchObject that follows — though keeping it is defensible since it's the assertion that produces the readable pre-fix failure quoted in the description.

Also checked, no issue found

• Handlers now persist on the Client after a failed connect (previously they were never registered on that path). Harmless — the client is reused, and the next attempt overwrites them.
• elicitation/create being live during the handshake means a server could surface a pending-elicitation modal while status is still "connecting". That's the correct behaviour for a spec-conformant client, not a regression.
• No other call site registers request handlers; disconnect() doesn't clear them, and doesn't need to.
  · branch v2/fix-roots-handler-race

[cliffhall]

Both findings taken, in 1d2d44b4. npm run ci green.

1. notifications/roots/list_changed — fixed

You're right that it qualifies by the PR's own criterion, and it turned out to be more than hypothetical: I extended the timing test to assert it, and that assertion fails against 55e25b7c (the round-1 commit) — the notification is dropped and no rootsChange event fires. So it was the same bug, just quieter.

Moved to a sibling registerPeerNotificationHandlers(), called next to registerPeerRequestHandlers() before connect(). Went with the sibling rather than folding it in so the method names stay honest about requests vs. notifications; its doc records why this one is different from the listChanged handlers that stay behind (they gate on this.capabilities, which isn't populated until fetchServerInfo()).

Agreed on notifications/elicitation/complete — reachable only after a tools/call, so the handshake window can't touch it. Left where it is.

2. Test pins only roots/list — fixed

Took the tasks/list suggestion. The fake transport now injects roots/list + tasks/list + the roots/list_changed notification in the same send("notifications/initialized") burst, so moving any part of the block back after connect() fails the test rather than just the roots part.

Left sampling/elicitation out deliberately, and said so in the file comment: they park a pending request awaiting user input, so there's no reply to assert on — asserting "no error reply" for them would pass vacuously.

Triviata: dropped the unused sessionId / setProtocolVersion. Kept expect(rootsReply).not.toHaveProperty("error") for the reason you named — it's the assertion that produces the readable pre-fix failure — and added a comment saying that, so the next reader doesn't delete it as redundant with the toMatchObject.

I re-verified both regression directions after the change: the roots/list assertion fails against origin/v2/main, the notification assertion fails against 55e25b7c.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 1s —— View job

---

Review (round 2): fix(core): register peer-request handlers before connect

• Read the round-2 diff (1d2d44b4) against origin/v2/main
• Verify the new registerPeerNotificationHandlers() seam
• Re-check the widened timing test
• Post review

---

Verdict: round-2 changes are correct and non-blocking-clean. Ship it. The one thing I want to flag is a correction to my own round-1 finding, which the new doc comment has now enshrined.

Verified

1d2d44b4 is another pure move: the notifications/roots/list_changed block extracted from connect() into registerPeerNotificationHandlers() (inspectorClient.ts:1267-1279) is byte-identical in body, including the original inline comments. The behavioural change is the new call at :1402, inside the same try opened at :1394, immediately after registerPeerRequestHandlers() and before client.connect(this.transport) at :1409.

Re-checked the ordering invariants against the final tree:

• One client.connect( call site in the whole file (:1409), so there is no path that reaches the handshake without both registrations.
• Every remaining handler in the post-handshake block genuinely needs to be there. Swept :1471-1560: tools/resources/prompts list_changed gate on this.capabilities?.*.listChanged, tasks on this.capabilities?.tasks, resources/updated on this.capabilities?.resources?.subscribe. The only ungated survivor is notifications/elicitation/complete (:1550, gates on this.elicit) — as agreed, unreachable during the handshake window. So the doc's "the rest of the listChanged handlers gate on this.capabilities" is accurate as written.
• Re-entrancy unchanged — setNotificationHandler overwrites, once per connect().

1. Correction: roots/list_changed is a client→server notification — my round-1 framing was wrong

RootsListChangedNotification is a member of ClientNotification in the MCP spec: the client tells the server its roots changed. This repo does exactly that — inspectorClient.ts:4251 sends it from setRoots(), and messageTrackingTransport.ts:89 lists it among the client-originated notifications ("initialized, progress, roots/list_changed").

So a server emitting it to the Inspector isn't spec traffic, and the inbound handler is defensive coverage for a non-conformant or experimental server — note its body dispatches rootsChange with this.roots, i.e. the client's own already-known roots, so even when it does fire it's a refresh-with-unchanged-data signal. My round-1 claim that this was "the same bug, just quieter" overstated it; the timing test failing against 55e25b7c proves the handler was unregistered, not that any real server exercises the path.

The move is still right — harmless, behaviour-preserving, and it puts the handler with the other constructor-state-only registrations. What I'd change is the doc comment, which now states the inaccurate premise as fact:

* `notifications/roots/list_changed` is the only one: a server may emit it as
* soon as it is initialized, and a notification with no registered handler is
* silently dropped by the SDK

That reads authoritatively enough that the next person will believe servers send this. Worth softening to what's actually true: the spec sends this notification the other way, the inbound handler exists as defensive coverage, and it's registered pre-handshake for consistency with the request handlers so nothing is dropped if a server does send one. Same for the test's section comment.

Fix this →

2. expect(transport.notificationRejected).toBe(false) is a vacuous assertion (nit)

inspectorClient-peer-handler-timing.test.ts:76-83. The SDK's Protocol._onnotification does not throw for an unregistered method — it falls through to fallbackNotificationHandler and otherwise returns, and the handler itself is invoked off the synchronous onmessage call. So the try/catch in deliver() can never observe a dropped notification: the flag is false in the post-fix run and was false in the pre-fix run. Your own bisect confirms it — the assertion that failed against 55e25b7c was rootsChanges, not this one.

Which makes the comment inverted:

} catch {
  // An unhandled notification would surface here rather than on the wire.
  this.notificationRejected = true;
}

It would not. expect(rootsChanges).toEqual([roots]) is the load-bearing assertion and it's correct — I'd drop the flag and the claim, keeping the try/catch (if at all) as an unannotated safety net. Secondary reason to reword: the catch is broad enough to also swallow a synchronous throw from delivering roots/list / tasks/list, where the failure would then surface as a puzzling expect(rootsReply).toBeDefined() rather than the real stack.

Fix this →

3. Two triviata

• replies also collects the client's outgoing requests. The fallback branch keys on "id" in message && typeof message.id === "number", which matches responses and any request the Inspector originates. Harmless today (server capabilities are {}, so nothing is sent, and ids 9001/9002 can't collide with the SDK's counter), but narrowing to "result" in message || "error" in message would make the map mean what its name says.
• if (!this.client) return; at :1268 is unreachable — the method is private with one call site, in connect() after the hard if (!this.client) throw at :1256. The pre-move code carried the same dead branch (if (this.client) { … }), so it's coverage-neutral and not a regression. Mentioning only because AGENTS.md's policy for provably-dead defensive guards is a justified /* v8 ignore … */ rather than silence.

Round-1 items — both resolved

The tasks/list addition is exactly the right pick (no user interaction, so a real reply to assert on), and leaving sampling/elicitation out with the reasoning written into the file is better than including them vacuously. Unused sessionId/setProtocolVersion gone; the not.toHaveProperty("error") kept with a comment explaining why it isn't redundant — good.

I could not run the suite here (this checkout has no node_modules), so I'm taking the reported green npm run ci at face value; everything above is from reading the code.
· · branch v2/fix-roots-handler-race

[cliffhall]

All three taken, in 80168576. npm run ci green.

1. Correction accepted — and you're right

I verified it against the installed SDK rather than taking it on trust: RootsListChangedNotificationSchema is a member of ClientNotificationSchema and absent from ServerNotificationSchema (@modelcontextprotocol/core/dist/auth-*.mjs). So the client sends it, servers don't — my "same bug, just quieter" framing overstated what the failing test proved. It proved the handler was unregistered, not that anything real sends it.

Reworded the method doc to say what's true: it's a ClientNotification we send from setRoots(), the inbound handler is defensive coverage for a non-conformant server, and it dispatches rootsChange with our own already-known roots — a refresh signal carrying no new data. It's registered pre-handshake because it gates on no server capability, not because servers emit it. Softened the test header the same way.

The move itself stands, for the reason you gave.

2. notificationRejected was vacuous — removed

Confirmed from the bisect rather than by reading the SDK: the flag assertion sits before the rootsChanges one in the test body, and the pre-fix run failed on rootsChanges — so the flag was false on both sides, exactly as you said. Dropped the flag, its assertion, and the inverted comment.

Also took your secondary point and removed the try/catch entirely rather than keeping it unannotated: it would have swallowed a genuine synchronous throw from delivering roots/list / tasks/list and resurfaced it as a confusing toBeDefined() failure.

3. Triviata

• replies narrowed to "result" in message || "error" in message, so it means what its name says.
• Added the justified /* v8 ignore next -- … */ on the unreachable if (!this.client) guard, per the AGENTS.md dead-guard policy. Left registerPeerRequestHandlers alone — its && this.client narrowings are inside per-feature conditions, so annotating them would be noisier than the guard is worth.

On the un-run suite

Fair flag. For the record, I re-ran both bisect directions after these edits: the roots/list assertion fails against origin/v2/main, the rootsChange assertion fails against 55e25b7c, and the full npm run ci is green on the final tree.

One process note in case it matters for a future round: my first attempt at that bisect had a shell quoting bug ($ref:core/... parsed as one word) that wrote an empty file, so both "failures" were bogus. I caught it and redid it with the refs quoted — the results above are from the corrected run.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 16s —— View job

---

Review (round 3): fix(core): register peer-request handlers before connect

• Read the round-3 diff (80168576) and the final tree
• Verify the reworded docs against the code they describe
• Re-check the final timing test
• Post review

---

Verdict: all three round-2 items are resolved correctly. Ship it. One leftover doc nit below, which is pre-existing text the round-3 reword now sits directly on top of — worth 30 seconds while you're in the file, not worth another round on its own.

Verified

80168576 is doc + test-hygiene only; no behavioural change. Re-checked the final tree rather than just the delta:

• this.client is assigned exactly once (inspectorClient.ts:628) and never reassigned, and there is one client.connect( call site (:1417) — reached only past the hard if (!this.client) throw at :1293. So both registerPeerRequestHandlers() (:1409) and registerPeerNotificationHandlers() (:1410) run on every handshake path, and the v8 ignored guard at :1276 is genuinely unreachable as annotated. No connect() recursion on the auth-recovery path either — the retry is driven by the caller, so there's no path that re-enters client.connect() behind the registrations' backs.
• The setRoots claim in the new doc is accurate — :4260 sends notifications/roots/list_changed from setRoots(), with its own comment noting the server needn't advertise support. That's the client→server direction, matching messageTrackingTransport.ts:89.
• Everything left in the post-handshake block still needs to be there. Re-swept :1471-1576: tools/resources/prompts list_changed gate on this.capabilities?.*.listChanged, tasks on this.capabilities?.tasks, resources/updated on this.capabilities?.resources?.subscribe, elicitation/complete on this.elicit.url (post-tools/call, so unreachable in the handshake window). The doc's "the remaining listChanged handlers gate on this.capabilities" holds as written.
• The multi-line /* v8 ignore next -- … */ form is established in this file (:3149, :3182) and in core/auth/node/oauth-callback-server.ts:154, so the annotation matches house style. Agreed on leaving registerPeerRequestHandlers unannotated — its && this.client narrowings are the second conjunct of per-feature conditions, so each would need its own ignore for very little.
• Round-2 items: the notificationRejected flag, its assertion, and the inverted comment are gone, and the try/catch went with them — right call, it would have masked a real synchronous throw from delivering roots/list/tasks/list. replies now narrows on "result" in message || "error" in message. The reworded method doc and test header both say what's true.

1. The inner comment on the roots handler now contradicts the doc four lines above it (nit)

inspectorClient.ts:1280-1284 — the handler body carries its original comments, moved verbatim:

// Dispatch event to notify UI that server's roots may have changed
// Note: rootsChange is a CustomEvent with Root[] payload, not a signal event
// We'll reload roots when the UI requests them, so we don't need to pass data here
// For now, we'll just dispatch an empty array as a signal to reload
this.dispatchTypedEvent("rootsChange", this.roots || []);

Two problems, both of which the round-3 work elsewhere just corrected:

• "server's roots" is the ownership framing this round fixed. The roots are ours; the doc at :1264 now says so explicitly.
• "we'll just dispatch an empty array" is not what the line does — it dispatches this.roots || [], which is non-empty whenever roots are configured. Your own new test proves it: expect(rootsChanges).toEqual([roots]) at inspectorClient-peer-handler-timing.test.ts:134 passes with a one-element roots, which the comment says is impossible.

So a reader who scrolls past the (now careful) method doc lands on four lines telling them the opposite. Collapsing them to one accurate line — "re-dispatch our known roots as a refresh signal; the payload carries no new data" — would close the loop on what this round set out to do.

Fix this →

2. Also looked at, nothing to raise

• this.roots || [] at :1284 vs this.roots ?? [] at :1210 — identical behaviour for Root[] | undefined (an empty array is truthy), so purely cosmetic; not worth touching in a fix PR.
• The test's expect(rootsChanges).toEqual([roots]) asserts exactly one dispatch, and no other path dispatches rootsChange during connect() — so the exact-match is a stronger and still-correct assertion than a toContainEqual would be.
• installReceiverTaskResponseBypass (:884) re-wraps by overwriting the map entry that setRequestHandler just installed, so the wrapper still can't nest across reconnects now that registration happens earlier. Unchanged from round 1.

On the process note

Worth having written down — a bisect that silently wrote an empty file is exactly the failure mode where both directions "confirm" whatever you expected. Catching and redoing it is the right outcome, and saying so is more useful than a clean-looking result would have been. I still can't run anything here (no node_modules in this checkout), so the green npm run ci and the corrected bisect are taken as reported; everything above is from reading the final tree.
· branch v2/fix-roots-handler-race

[cliffhall]

Taken, in dd262892. Comment-only; npm run ci green.

You're right that both halves were wrong, and the second one is the sharper catch: this.roots || [] dispatches a non-empty array whenever roots are configured, and the test I added in this very PR asserts exactly that (expect(rootsChanges).toEqual([roots]) with a one-element roots). So the comment claimed something the adjacent test disproves. Collapsed the four lines to one:

// Re-dispatch our already-known roots as a refresh signal for the UI —
// the payload carries no new data (see the note on ownership above).

That's the last of the review items. Agreed on leaving || [] vs ?? [] alone — identical for Root[] | undefined, and churn in a fix PR.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 29s —— View job

---

Review (round 4): fix(core): register peer-request handlers before connect

• Read the final tree (dd262892) against origin/v2/main
• Verify the round-3 comment fix
• Re-check the whole four-commit stack for anything missed
• Post review

---

Verdict: the PR is done. Nothing blocking, nothing left over from rounds 1–3. One adjacent finding below that is pre-existing and out of scope — I'd open it as a follow-up issue rather than fold it in here.

Round-3 item: verified closed

dd262892 is 2 insertions / 4 deletions, comments only. The replacement reads accurately against the code it sits on: the handler dispatches this.roots || [], the doc four lines up establishes the ownership, and the new line no longer claims an empty payload. Good.

Final-tree sweep

Re-derived the invariants from scratch rather than trusting the earlier rounds:

• One client.connect( call site (inspectorClient.ts:1415), reached only past the hard if (!this.client) throw at :1290. Both registrations (:1407, :1408) precede it inside the same try. No path reaches the handshake unregistered.
• The registration guards exactly mirror the capability guards. roots/list registers on this.roots !== undefined (:1208); capabilities.roots is advertised on the identical condition (:588). Same for sampling (:562/:1073), elicitation (:566/:1147), and tasks (:591/:1215). So there is no residue of the original bug shape — no method is advertised without a handler, or vice versa. This is the check I'd most want to hold after a move like this, and it does.
• installReceiverTaskResponseBypass is safe to run pre-connect. It only reads/rewrites this.client._requestHandlers (:884-912) and touches no transport state, so running it earlier changes nothing. It still overwrites the entry setRequestHandler just installed, so no wrapper nesting across reconnects.
• Nothing left in the post-handshake block belongs forward. :1471-1576 all gate on this.capabilities?.* except notifications/elicitation/complete, which gates on this.elicit.url and is only reachable post-tools/call.
• Test reads correctly. The "result" in message || "error" in message narrowing now makes replies mean what it's named; the try/catch and notificationRejected are gone; the three assertions (roots/list reply, tasks/list reply, rootsChange dispatch) each pin a distinct piece of the moved block.

1. Pre-existing, not this PR: setRoots() after construction never registers a roots/list handler — the CLI hits this

Flagging because it is literally the #1797 symptom (-32601 on roots/list), and this PR just created the seam that makes it a two-line fix. It is not a regression and I would not change it here.

setRoots() (inspectorClient.ts:4243) documents itself as "Note: This will enable roots capability if it wasn't already enabled" and does if (this.roots === undefined) this.roots = []. But both the capability advertisement (:588) and the handler registration (:1208) already ran — the former at construction, the latter at connect(). So flipping this.roots afterwards enables neither.

Web is unaffected: App.tsx:375 / :2298 always pass roots (defaulting to []), so the condition holds at construction. The CLI is not — clients/cli/src/cli.ts:144-158 omits roots entirely. Which makes this reachable:

mcp-inspector --cli --method roots/set --roots-json '[{"uri":"file:///work"}]'

run-method.ts:306 calls setRoots(), which sends notifications/roots/list_changed to the server (:4256) — an explicit invitation to re-fetch — and a server that takes it up gets -32601. The user asked for roots, the CLI reports success, and the server can't read them.

Two candidate fixes, both cheap now: register the roots/list handler unconditionally (it already answers this.roots ?? [], so an unconfigured client returns an empty list, which is a better answer than -32601), or have setRoots() register it when it flips this.roots from undefined. Either way the capability advertisement stays construction-time, so a strictly-conformant server still wouldn't ask — but "we told it our roots changed and then refused to list them" is the wrong end state, and the stale setRoots doc comment should go with it.

Open a follow-up →

2. Nothing else

• No stale test elsewhere depends on the old post-handshake ordering (the only two changed files are the ones in the diff, and the move is behaviour-identical for any server that asks later — which is every existing test's server).
• getRoots() (:4236) and the MRTR embedded roots/list auto-answer (:2374) both read this.roots directly and are untouched by the move.
• The /* v8 ignore next */ on the :1275 guard matches the multi-line house form used elsewhere in the file.

On the four rounds

Each round's response verified the claim against the code rather than accepting it — including checking my round-1 framing against the installed SDK and finding it wrong, and catching your own bisect's quoting bug and redoing it. The doc comments that came out the other end say things that are true and load-bearing, which is the part that survives longest. Good work.

I still can't run the suite here (no node_modules in this checkout), so the green npm run ci is taken as reported; everything above is from reading the final tree.
· branch v2/fix-roots-handler-race

[cliffhall]

Handled in this PR rather than as a follow-up (0a38fe67) — but your suggested fix doesn't work, so the shape is different. npm run ci green.

The finding is real — I reproduced it

Wrote the test first, on a client constructed without roots: setRoots() → server sends roots/list → -32601. Exactly as you described, and the CLI reaches it (cli.ts:144 omits roots, run-method.ts:306 calls setRoots()).

But "register roots/list unconditionally" is not implementable

I tried it, and it fails at connect:

SdkError: Client does not support roots capability (required for roots/list)
  ❯ Client.assertRequestHandlerCapability client/client.ts:1530
  ❯ Client.setRequestHandler shared/protocol.ts:1683

The SDK asserts the matching client capability inside setRequestHandler, so an unconditional registration throws for every client built without the option — including the CLI, i.e. it would break the exact path we're fixing. And registerCapabilities refuses to run post-connect ("Cannot register capabilities after connecting to transport"), so your second option — have setRoots() register the handler — hits the same assertion.

The conclusion is stronger than the finding: a client that omits roots at construction can never legally serve roots/list. There's no seam in core to fix it from.

So it's fixed at the layer that can fix it

clients/cli/src/cli.ts now seeds roots: [], exactly as web does (App.tsx:2298 always passes it, defaulting to []). Capability negotiated at initialize, handler registered at connect, roots/set serviceable.

The core registration stays gated on the constructor value, but now carries a comment recording why it can't be unconditional — otherwise this is a change someone reasonably tries again.

You were right about the stale setRoots() doc, and more so than the nit implied: "will enable roots capability if it wasn't already enabled" is not merely stale, it's false, and the if (this.roots === undefined) this.roots = [] under it was dead anyway — the next line overwrites this.roots unconditionally. Both gone.

Tests

• CLI (cli.test.ts): drives --method tools/call --tool-name list_roots against an HTTP test server carrying createListRootsTool(), so the server really asks the CLI's client for roots. Verified load-bearing — with the roots: [] seed removed it fails on the -32601 error text.
• Core: the timing test gains a case pinning that roots set after connect are served with current values (the handler reads this.roots live), on a client that advertised roots: [] up front — the CLI's new shape.

Two self-inflicted things the gate caught, worth recording

1. constructor(private readonly burstOnInitialized = true) — a parameter property, rejected under erasableSyntaxOnly (TS1294). Rewritten as an explicit field.
2. My first version of the new core test asserted the reply after a single await Promise.resolve(). It passed in isolation and failed in the full run — the handler is async, so the reply lands some microtasks later. Replaced the guesswork with injectRequest() returning a promise that resolves when the matching reply is captured; re-ran the file 3× clean.

Neither was in a previous "green" claim — both were caught by npm run ci before pushing.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 4m 19s —— View job

---

Review (round 5): fix(cli): advertise roots so roots/set is actually serviceable

• Read the round-5 diff (0a38fe67)
• Verify the SDK-assertion claim that reshaped the fix
• Check the CLI roots: [] seed and its test
• Check the new core timing case
• Post review

---

Verdict: the reshaped fix is right, and your correction of my round-4 suggestion is correct. One should-fix before merge — the CLI seeds roots: [] when it has the user's configured roots in hand two lines away, so the user-visible half of #1797 is still unfixed on the CLI path.

Your correction stands, with one option you didn't list

Registering roots/list unconditionally does throw — assertRequestHandlerCapability gates request-handler registration on the client's own declared capabilities, and registerCapabilities refuses post-connect. I couldn't run it here (no node_modules), but the stack you pasted is the SDK's documented behaviour and the conclusion follows: a client built without roots can never legally serve roots/list. Fixing it at the client layer is correct.

The third option, for the record: InspectorClient could default this.roots = options.roots ?? [] in its own constructor, fixing every embedder at once. You'd be trading away the documented opt-in (types.ts:920: "If provided (even if empty array), the client will advertise roots capability"), so I don't think it's obviously better — but it's the difference between "web and CLI remembered" and "nobody has to remember." Relevant to finding 2 below.

1. Should-fix: the CLI ignores the roots the user actually configured

clients/cli/src/cli.ts:155 hard-codes roots: []. Two lines down it passes serverSettings — which already carries the user's roots:

• StoredMCPServer.roots is the persisted per-server roots field (core/mcp/types.ts:177), written by the web Server Settings form.
• mcpConfigToServerEntries lifts it into InspectorServerSettings.roots on read (core/mcp/serverList.ts:228, roots: stored.roots ?? []).
• The CLI gets that via loadServerEntries → selectServerEntry → selected.settings (cli.ts:935), and hands it to the same constructor call.

Web does exactly what's missing here (clients/web/src/App.tsx:2298):

roots: cleanRoots(savedSettings?.roots ?? []),

So today: a user configures roots for filesystem in the Inspector, runs mcp-inspector --cli --server filesystem …, the server asks roots/list, and gets []. Which lands exactly where the PR description says the bug lands — "server-filesystem silently falls back to its CLI-argument directories instead of the roots configured in the Inspector." I checked the upstream source to be sure: updateAllowedDirectoriesFromRoots only replaces the allowed dirs if (validatedRootDirs.length > 0), so an empty answer is discarded and the CLI-arg dirs stand. The -32601 is gone; the outcome is not.

This matters more than it looks because the CLI is one-shot. --method roots/set sets roots on a connection that finally { disconnect() } tears down a few statements later — whether the server's re-fetch even lands is a race, and nothing survives to the next invocation. The config file is the only durable way to give a CLI run its roots, and that's the path the seed currently drops. Same one-liner also makes --method roots/list (run-method.ts:287, prints getRoots()) report something other than [].

roots: cleanRoots(serverSettings?.roots ?? []),   // cleanRoots from @inspector/core/mcp/serverList.js

cleanRoots is already shared core (serverList.ts:99) and is what the write side uses, so this keeps CLI and web answering roots/list with byte-identical content for the same file. The comment above the field should lose "empty until --method roots/set fills them in" with it.

Fix this →

2. Same gap in the TUI, and it's the same one-liner (follow-up, not this PR)

clients/tui/src/App.tsx:331 constructs its client from an opts object with no roots key at all — grep -rn roots clients/tui/src is empty. It builds opts from savedSettings (...(savedSettings && { serverSettings: savedSettings })), so it has the same configured roots sitting right there and drops them the same way.

Lower severity than the CLI: the TUI has no roots UI and never calls setRoots(), so nothing over-promises — it just silently doesn't support the capability, which is at least honest (and, per the filesystem server's oninitialized, gets the "client does not support MCP Roots, using allowed directories from server args" path). But a user who set roots in the web UI and then opened the TUI against the same mcp.json reasonably expects them to apply. Worth an issue; the fix is finding 1's line, or the constructor default from the top of this review.

Verified

• The [] seed itself is safe. The obvious worry with advertising an empty roots list is a server clamping to zero allowed paths. Checked upstream src/filesystem/index.ts: the length > 0 guard means empty is discarded and current settings are kept. One behaviour delta, niche enough to ignore: a filesystem server started with no dir args used to hard-fail in oninitialized ("Server cannot operate…") when the client didn't advertise roots, and will now proceed with zero allowed dirs instead.
• The dead-code removal is genuinely dead — if (this.roots === undefined) this.roots = [] was followed immediately by this.roots = [...roots]. And the doc it sat under was worse than stale: it promised a capability upgrade the SDK forbids. Both corrections read accurately now, and the registerPeerRequestHandlers comment records the constraint at the place someone would retry it.
• The new core test is well-built. injectRequest() resolving on the captured reply is the right replacement for microtask-counting — good catch on the isolation-vs-full-run divergence, that's the classic shape of that bug. The burstOnInitialized: false variant keeps the two cases from interfering. It pins the live read of this.roots (construction-time [], post-setRoots answer), which is exactly the invariant the CLI now depends on.
• The CLI test is load-bearing and correctly shaped — createListRootsTool makes the server drive roots/list (server.server.listRoost() → renders or catches into an isError result), so it exercises the real handler path rather than getRoots(). Both the isError and -32601 assertions earn their place. It will also be the natural place to extend for finding 1.
• No CLI test or smoke asserts on the negotiated client capabilities, so the new advertisement breaks no snapshot.

3. Two test-file nits

• waiters (:56) is declared after the constructor while burstOnInitialized (:49) is declared before. It works — instance fields initialize in declaration order before the constructor body runs — but it reads like a hazard to anyone checking. Move it up with the other fields.
• injectRequest never deletes its waiter and has no timeout, so a regression that stops the client replying hangs to the vitest timeout rather than failing on an assertion. Fine for a two-case file; worth a Promise.race with a short reject if this transport gets reused.

On the two self-inflicted catches

Both worth having written into the PR — the erasableSyntaxOnly parameter-property rejection and especially the "passed in isolation, failed in the full run" microtask assumption. The second is the one that would have come back as a flake weeks later attributed to something else.

I can't run the suite here (no node_modules in this checkout), so the green npm run ci is taken as reported. Everything above is from reading the final tree, plus the upstream server-filesystem source, which I did fetch.
· branch v2/fix-roots-handler-race

[cliffhall]

All three taken, in b952142c. npm run ci green.

1. Should-fix — you're right, and this was the half that mattered

The roots: [] seed fixed the wire error and left the outcome broken, which is precisely what the PR description says the bug is. Your check of updateAllowedDirectoriesFromRoots (validatedRootDirs.length > 0, so an empty answer is discarded) is what makes it concrete: the CLI would have answered roots/list politely with nothing and let the server keep its own defaults.

Now roots: cleanRoots(serverSettings?.roots ?? []), using the shared helper the write side uses, so CLI and web answer identically for the same file. Comment updated — the "empty until --method roots/set" framing is gone.

Your point about the CLI being one-shot is the part I'd have missed: roots/set sets roots on a connection that finally { disconnect() } tears down almost immediately, so the config file isn't merely a way to supply roots, it's the only durable one. That reframes this from a nicety to the actual feature.

New CLI test writes a config with a roots entry and asserts the configured uri comes back from list_roots. Verified load-bearing: reverting to roots: [] fails it on expected … to contain 'file:///configured'.

2. TUI — folded in here rather than filed

I'd normally take the follow-up, but a known-wrong client sitting in tree behind an issue is worse than a three-line commit, and it's literally finding 1's line. clients/tui/src/App.tsx now passes roots: cleanRoots(savedSettings?.roots ?? []).

Your reasoning for the lower severity is right — the TUI never calls setRoots(), so it over-promised nothing — but "set roots in the web UI, open the TUI against the same mcp.json, they don't apply" is a real surprise, and it's gone now.

On the constructor-default option: agreed it's not obviously better. types.ts:920 documents the opt-in explicitly ("If provided (even if empty array), the client will advertise roots capability"), and defaulting to [] would make every embedder advertise a capability it may not want to serve. Three call sites that each pass their own roots is the honest shape; I left the opt-in alone.

3. Nits

• waiters hoisted up with the other field declarations.
• injectRequest now races a 1s reject naming the method and id, and deletes its waiter on both paths — a client that stops replying fails with that message instead of a bare vitest timeout.

On the []-is-safe check

Worth recording your finding that a filesystem server started with no dir args used to hard-fail in oninitialized when the client didn't advertise roots, and now proceeds with zero allowed dirs. With this commit the CLI advertises real roots whenever they're configured, so the case narrows to "no dirs on the command line and no roots in mcp.json" — nothing useful was going to happen there either way.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 4m 10s —— View job

---

Review (round 6): fix(cli,tui): advertise the roots configured in mcp.json

• Read the round-5 diff (b952142c) against origin/v2/main
• Verify the CLI + TUI cleanRoots wiring
• Check the new CLI test and the timing-test nits
• Post review

---

Verdict: the user-visible half is now actually fixed, and folding the TUI in was the right call. Nothing blocking. Four small things below, all comment/doc/robustness — none of them changes behaviour.

Verified

• All three production construction sites now agree. new InspectorClient appears exactly three times outside tests — clients/cli/src/cli.ts:145, clients/tui/src/App.tsx:337, clients/web/src/App.tsx:2286 — and all three now pass cleanRoots(<settings>?.roots ?? []) through the same shared helper the write side uses (core/mcp/serverList.ts:99, also used by inspectorSettingsToStoredFields at :382). So the "answer roots/list with identical content for the same mcp.json" claim holds by construction, not by coincidence, and there is no fourth site left to forget.
• The round-4 fix is preserved on the ad-hoc path. serverSettings is optional in callMethod, so a positional-URL run (no config file) still yields roots: [] — capability negotiated, handler registered, roots/set serviceable. The change strictly widens what's advertised; it never drops back to omitting the option.
• The TUI's insertion point is safe. roots sits after ...(savedSettings && { serverSettings: savedSettings }) and before the protocolEra spread, and no spread in that literal carries a roots key, so nothing can clobber it.
• The new CLI test is genuinely load-bearing. createListRootsTool (test-servers/src/test-server-fixtures.ts:358) drives server.server.listRoots(), so file:///configured can only reach the output through the client's real roots/list handler reading the lifted serverSettings.roots — not through getRoots() or any local echo. The isError assertion also keeps the -32601 path from passing as a string match.
• The round-5 nits are both closed. waiters is hoisted to the field block, and injectRequest now races a 1s reject that names the method and id. The waiters.delete(id) on the resolve path (:119) is right, and rejecting a promise that already resolved is a no-op, so the timeout can't turn a passing test red.

1. clients/cli/README.md:81 still enumerates the lifted settings without roots

When a server is loaded from a --catalog/--config file, its per-server settings (headers, connection/request timeouts, and OAuth) are applied to the connection — the same resolution the TUI uses.

That list is now incomplete, and roots are the least discoverable member of it: there is no CLI flag for them (--method roots/set is per-invocation and dies with the connection), so the config file is the only durable way to give a run its roots — exactly the point you made in the round-5 reply. A user reading this paragraph has no way to learn the capability exists. Per AGENTS.md's doc-maintenance rule, this is the sentence to update. The trailing "the same resolution the TUI uses" is, happily, still true after the TUI commit.

Fix this →

2. The new test's comment describes the CLI as it was one commit ago (nit)

clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts:178:

// does; this pins that, and the `roots: []` seed the CLI now passes
// (`clients/cli/src/cli.ts`) is what makes the handler exist at all.

The CLI stopped passing a [] seed in this very PR — cli.ts:158 passes cleanRoots(serverSettings?.roots ?? []), which is [] only when nothing is configured. The point the comment makes is still exactly right (passing the option at all, even empty, is what makes the handler exist), so it's a two-word fix: "the roots option the CLI now always passes ([] when nothing is configured)". Flagging it only because comments-that-contradict-adjacent-code is the thread that ran through rounds 2–4 of this review, and this is a fresh one.

Fix this →

3. cleanRoots trusts the shape from disk, and CLI/TUI now newly evaluate it (low)

core/mcp/serverList.ts:100 does roots.filter((r) => r.uri.trim() !== ""). Nothing between the file and that call validates the shape — mcpConfigToServerEntries is a plain stored.roots ?? [] passthrough (:228) with no runtime check, and StoredMCPServer (core/mcp/types.ts:92) is a compile-time type over hand-editable JSON. So in mcp.json:

"roots": [{ "name": "Work" }]      // → TypeError: Cannot read properties of undefined (reading 'trim')
"roots": "file:///work"            // → TypeError: roots.filter is not a function

Web has carried this exposure since it started calling cleanRoots at connect; the CLI and TUI acquire it here, having previously ignored the field entirely. Failure mode is a crash at connect, not a wrong answer, and the trigger is a hand-edit — so low severity and clearly not this PR's job. But a typeof r?.uri === "string" filter (plus an Array.isArray bail) inside cleanRoots would close it for all three clients in one place, which is a better shape than three call-site guards. Follow-up issue, not a change here.

Open a follow-up →

4. Two triviata

• The injectRequest timeout is never cleared on success. :68 arms a 1s timer per injected request and only unref?.()s it — the ?. is there because this file runs in the unit project (happy-dom), where setTimeout may return a plain number with no unref. The callback only touches a Map and constructs an Error, so it can't reproduce the Flaky test teardown: Mantine Transition timer fires after happy-dom window teardown (window is not defined) #1760 window is not defined teardown failure — but a clearTimeout on the resolve path at :119 is one line and keeps the file out of that class entirely, which this repo has been bitten by before.
• setRoots()'s doc (core/mcp/inspectorClient.ts:4250) says "despite what this comment used to claim." The correction is right and worth keeping, but the next reader can't see the claim being corrected. "This does not enable the roots capability… pass roots at construction" says the same thing without the archaeology.

Also checked, nothing to raise

• The CLI test's as unknown as MCPServerConfig matches the two neighbours in the same file (:429, :475) — roots is a StoredMCPServer field, not an MCPServerConfig one, and the helper's parameter is typed to the latter. The block comment above the test explains the test rather than the cast, but the cast's justification is one screen up in an identical form, so per AGENTS.md's "unavoidable, with justification" carve-out this is consistent with house practice rather than new debt. (Widening createTestConfig to Record<string, StoredMCPServer> would delete all three casts — cleanup, not this PR.)
• No existing CLI/TUI test or smoke asserts on the negotiated client capabilities, so the new advertisement breaks nothing.
• The MRTR embedded-roots/list auto-answer (inspectorClient.ts:2381) reads this.roots ?? [] independently of capability, so it stays consistent with the handler for all three clients.

I can't run the suite here (no node_modules in this checkout), so the green npm run ci is taken as reported; everything above is from reading the final tree.
· branch v2/fix-roots-handler-race

[cliffhall]

All four taken, in b8e02e1d. npm run ci green.

1. CLI README — fixed

Right call under the doc-maintenance rule, and your framing is the reason it matters: roots are the one lifted setting with no flag, so a user who doesn't read the config-file paragraph has no way to discover the capability exists. Added them to the enumeration plus a sentence saying the file is the only durable route, with the server-filesystem case named so the "why would I want this" is answerable.

2. Stale test comment — fixed

Fair, and you're right that this is the same thread as rounds 2–4. Reworded to make the point without the outdated specifics: passing the roots option at all is what makes the handler exist; the CLI now always passes it, empty only when nothing is configured.

3. cleanRoots hardening — folded in, not filed

Same reasoning as the TUI last round: it's a known crash-at-connect that this PR is what newly exposes on two of the three clients, and the fix is smaller than the issue describing it would be. Non-array bails to []; an entry without a string uri is dropped with a warning. Done in the shared helper, so all three clients get it — agreed that's the better shape than call-site guards.

Both of your reproductions are now tests ("roots": [{"name":"Work"}] and "roots": "file:///work"), plus a non-string-uri case.

4. Triviata

• clearTimeout on the resolve path. You're right that the callback itself can't trip Flaky test teardown: Mantine Transition timer fires after happy-dom window teardown (window is not defined) #1760 — but "armed timer outlives the test" is the class, and the comment now says that's why the line exists rather than leaving it looking redundant.
• Dropped the "despite what this comment used to claim" archaeology. The correction reads on its own now; git has the history.

On the as unknown as MCPServerConfig note

Agreed it's consistent with the two neighbours rather than new debt, and that widening createTestConfig to Record<string, StoredMCPServer> would delete all three. Left alone — it's a test-helper signature change touching unrelated tests, which doesn't belong in this PR.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 24s —— View job

---

Review (round 7): review: round-6 fixes — document roots in the CLI README, harden cleanRoots

• Read the round-6 diff (b8e02e1d) against origin/v2/main
• Verify the hardened cleanRoots and its tests
• Check the README + comment fixes
• Post review

---

Verdict: all four round-6 items are closed correctly. One should-fix: the cleanRoots hardening validates uri but not name, and name throws on the very next line — so the crash-at-connect class this commit set out to close is still open for one field. I reproduced it.

1. Should-fix: cleanRoots still throws on a non-string name

core/mcp/serverList.ts:118. The new filter guards uri, then the .map immediately does:

const trimmedName = r.name?.trim();

?. guards null/undefined, not "is a string". So a hand-edited mcp.json gets past the new gate and dies one line later:

"roots": [{ "uri": "file:///a", "name": 42 }]        // TypeError: r.name?.trim is not a function
"roots": [{ "uri": "file:///a", "name": { "x": 1 } }] // same

I ran the shipped function body against both inputs to be sure — both throw. Reachable from every call site the commit message names (cli.ts:158, tui/src/App.tsx:312, web/App.tsx:2298), plus the web settings-save path (App.tsx:3981), and there is no schema validation upstream: mcpConfigToServerEntries is still a plain stored.roots ?? [] passthrough (serverList.ts:243).

It's the same severity and the same fix shape as the uri case — drop the field (or the entry) with a warning rather than throwing:

const rawName = r.name;
if (rawName !== undefined && typeof rawName !== "string") {
  console.warn("Dropping non-string `name` on root:", r);
}
const trimmedName = typeof rawName === "string" ? rawName.trim() : undefined;

Worth doing here rather than filing: it's three lines inside the function this commit just rewrote, and leaving it means the doc's "the shape is validated at runtime too" (:99-102) overstates what the code does — the failure mode this PR spent a round eliminating for uri is still live for name.

Fix this →

Verified

• The uri hardening itself is right and its tests are load-bearing. typeof r?.uri !== "string" also covers a null/undefined entry in the array, Array.isArray covers the "roots": "file:///work" string case, and the three new tests in serverList.test.ts:106-140 assert both the surviving output and the warning. Suppressing console.warn via vi.spyOn in beforeEach/afterEach matches AGENTS.md's "suppress expected error output" rule.
• The doc comments on cleanRoots and setRoots are accurate (modulo finding 1's overstatement). The setRoots reword reads on its own now — the archaeology is gone, and the substantive claim ("[] is enough — pass it at construction") is the actionable half.
• The clearTimeout fix is correct and can't flip a green test red. waiters.get(id)?.(message) runs the wrapped resolver which clears the timer before resolving, and the resolver is deleted on the same path — so a late second reply for the same id is a no-op rather than a double-resolve. The comment now says why the line exists, which was the point.
• The README addition is the right two sentences. "no roots flag, and --method roots/set applies only to that one short-lived connection" is the discoverability fact, and naming server-filesystem answers "why would I want this." The trailing "the same resolution the TUI uses" in the sentence above is still true post-b952142c. Checked the TUI and web READMEs: neither enumerates per-server settings or mentions roots, so nothing else went stale.
• Nothing in connect() moved since round 3. git diff dd262892..HEAD -- core/mcp/inspectorClient.ts is comments plus the dead-code removal in setRoots, so the round-4 ordering sweep still holds.
• The registerPeerRequestHandlers comment records the constraint at the retry point — that's the note that stops someone re-attempting the unconditional registration, and it's placed where they'd be standing.

2. Two triviata

• cli.test.ts:331 — "cli.ts now seeds roots: []" is the same wording the round-6 fix corrected in the web test. It happens to be true for that test (ad-hoc URL → no serverSettings → []), so it isn't wrong, but "always passes the roots option, empty on the ad-hoc path" would match the neighbouring test and not read as pre-b952142c.
• inspectorClient.ts:1209 cites (:588) for the capability advertisement. Line-number references in comments rot on the next insertion above them; capabilities.roots in the constructor names the same thing durably.

Also checked, nothing to raise

• The new cleanRoots warnings can't get chatty: it runs once per connect and once per settings save, not per render or per keystroke.
• inspectorSettingsToStoredFields (:397) passes settings.roots, required Root[] on InspectorServerSettings, so the non-array bail is dead on the write path and live only on the read-from-disk path — which is where it's needed.
• The as unknown as Root[] casts in the three malformed-input tests are the standard "feed the function what disk can actually hand it" shape and carry the reproduction inline as a comment.
• Coverage: the new branches in cleanRoots are all exercised by the three tests; r?.uri on a literal null entry isn't, but serverList.ts has ample headroom on the per-file gate.

I can't run the suite here (no node_modules in this checkout), so the green npm run ci is taken as reported. Finding 1 I did execute — as a standalone copy of the shipped function body, not through the suite.
· branch v2/fix-roots-handler-race

[claude]

Claude finished @cliffhall's task in 7m 50s —— View job

---

Review (round 51): final tree after 69b0a0cd

• Read the recent commits (rounds 46–50) and the final tree
• Verify the connect-start sweep and the unified best-effort close
• Final sweep of the stack
• Post review

---

Verdict: the round-50 changes are correct and the connect-start sweep closes a real gap. Nothing blocking. One finding I'd fold in (an undocumented ordering dependency the sweep created), one test gap, and three doc items — the largest of which is the same enumeration-drift class round 23 flagged, in four places, all made stale by the commit that added the caller.

Verified

• The onerror-without-onclose route is real and the sweep is the right place for it. attachTransportListeners' onerror (inspectorClient.ts:824-844) sets status = "error" and dispatches, and stops — it doesn't touch baseTransport, so connect() reaches if (!this.baseTransport) at :1621 with a live transport and reuses it. Neither teardown path runs, so before this commit both the peer queue and the raw-wire map crossed the session boundary. App.tsx:4020-style derivation of the modal from queue length with no status gate is what makes the queue the sharper of the two, as the comment says.
• Both sweeps are genuinely idempotent, so the overlapping routes are unaffected. clearAndAnnouncePendingPeerRequests early-returns on an empty queue (:1563-1568); rejectPendingRawWireRequests clears its map (:2304) and each entry's own timeout deletes its id before rejecting (:2287), so a re-reject is a no-op on a settled promise.
• The sweeps run before any await in connect(), which is what makes the new raw-wire test deterministic under timeout: 50 — the 50 ms raw-wire timer is cleared in the same tick as the connect() call, so it can't win the race and produce a wrong rejection message.
• closeSubscriptionBestEffort absorbs both arms at all three sites. Inside an async function the try catches a synchronous close() throw and await catches the rejection; resetSubscriptionStream's void closeSubscriptionBestEffort(closing) (:1472) therefore can't produce an unhandled rejection, and the two refreshModernSubscription sites (:4721, :4737) are awaited. The fold is a real fix at :4721, where the old await previous.close().catch(() => {}) would let a synchronous throw both abandon a stream whose reference was already dropped and abort the refresh before its replacement listen() opened.
• resolveModernLogLevel really is one derivation, and its two inputs can't drift. The client re-derives from this.serverSettings (:1521) and web from activeServer.settings; both web writers that call setServerSettings (App.tsx:3447 with a rollback at :3480, and :3993) persist the same object through updateServerSettings, so the two sources stay in step.
• Round 49's table-driven pair pins resetSubscriptionStream's arms from the right side — seeding a close() that throws and one that rejects, then asserting a downstream teardown step (taskInputAbortControllers abort) still ran. That's the assertion that catches the escape, not just the absence of a rejection.

1. The sweep created an ordering dependency between two adjacent calls, and nothing says so

connect() runs resetSessionState() (:1590) and then clearAndAnnouncePendingPeerRequests() (:1605). Those read as independent, and the order is load-bearing:

ElicitationCreateMessage.cancel() resolves synchronously (elicitationCreateMessage.ts:117-123), and for a task-augmented elicitation resolvePromise is the record callback from :1279-1289, whose last statement is this.upsertReceiverTask(updated). Same for the sampling twin via its reject arm (:1215-1227). upsertReceiverTask (:1092-1098) is a no-op only because the record is already gone — resetSessionState() → clearReceiverTasks() (:1510) emptied the map three lines earlier.

Swap the two calls and the record is still present, so upsertReceiverTask → emitReceiverTaskStatus (:1071) → this.client.notification({ method: "notifications/tasks/status", … }) fires — for the outgoing session's task, on the transport the new session is about to reuse, and then clearReceiverTasks() drops the record anyway. Not catastrophic (same transport, same server, it did create the task), but it's a stray frame attributable to a session that's ending, emitted from the middle of a connect.

The dependency is worth a clause at the sweep, because the comment block above it explains why the sweep exists and gives no reason for it to sit after the reset — which is exactly the invitation to hoist it above for readability.

Fix this →

2. Four enumerations went stale in the commit that added the third caller

clearAndAnnouncePendingPeerRequests now has three call sites — :813 (crash), :1605 (connect start, new), :1911 (connect catch) — and the third is not a teardown path at all; it's a route in. Every doc that enumerates it still says two, or says "teardown":

site	current text
inspectorClient.ts:1550-1551	"for the paths that drop a queue without going through disconnect() — a failed connect() and a mid-session transport close"
inspectorClient.ts:1534-1535	"emits them immediately for the paths that end a connection without going through disconnect()"
elicitationCreateMessage.ts:105-107	"clearPendingPeerRequests(), which serves all three teardown paths"
samplingCreateMessage.ts:91-93	"clearPendingPeerRequests(), which serves all three teardown paths"

Plus rejectPendingRawWireRequests's one-liner (:2298): "(on disconnect/teardown)", which now also runs at connect start.

The two class-level ones matter most: cancel()'s doc is where someone asks "who calls this and when," and it now answers with a count that is wrong and a category ("teardown") that excludes the new caller. Round 23's diagnosis applies directly — an enumeration that counts will keep drifting; naming the category ("every route out, plus the top of connect() as a backstop for the route in that settles nothing") doesn't.

Fix this →

3. The synchronous-throw arm the fold newly fixed at the refresh sites is untested

Round 49 pinned both failure modes for resetSubscriptionStream (the table-driven pair at inspectorClient-peer-handler-timing.test.ts:1033-1078). Round 50 extended the same protection to refreshModernSubscription's two sites — and that's where the fold actually changed behaviour, since await previous.close().catch(() => {}) never caught a synchronous throw. No test drives it: grep -rn "modernSubscription" clients/web/src/test returns only the two seeded-stub sites in this file and the era-suite's read at inspectorClient-subscriptions-era.test.ts:291-294.

So reverting :4721 to the old form restores the described bug — a stream leaked with its reference already dropped, and the refresh aborted before its replacement opens — with a green suite. subscribeToResource on a modern connection is the reachable driver, so the test is a subscribe with a poisoned modernSubscription seeded first, asserting the replacement listen() still happened.

Fix this →

4. Two comment nits

• The new raw-wire test borrows a safety argument that doesn't hold for it. inspectorClient-peer-handler-timing.test.ts:1004-1006: "As in the crash-path case above: … safe only because no SDK request is issued after connect." That test issues no second connect; this one calls connect() twice, and the second one issues an initialize under the client-wide 50 ms budget. It's safe for a different reason — ElicitAfterConnectTransport.send() answers initialize synchronously from inside send (:297-312), so no timer is ever in play. The precise version matters here for the same reason the original comment spelled it out at :686-693: a fixture that later answered initialize a macrotask later would flake, and this comment says there's nothing to check.

• resetSessionScopedUiState's new comment gives "the default" two meanings four lines apart. clients/web/src/App.tsx:1226-1227 says clearing activeServerIdRef "would silently drop this back to the default" — but the no-server branch is : null, i.e. Off, and :1231 says so explicitly ("Only 'no server at all' means Off"), while :1230 uses "the default" to mean DEFAULT_MODERN_LOG_LEVEL. "…would silently drop this to Off" is the fix.

Fix this →

5. Also checked, nothing to raise

• closeSubscriptionBestEffort's doc opens "Every close in this file goes through here" (:249). There are two this.client?.close() calls that don't (:1157, :1959); the following clause scopes it to the three stream sites, so it reads correctly in context — just an over-broad opener. Not worth a round.
• SamplingCreateMessage.cancel() rejects with "Connection torn down", which is now also the message a connect-start sweep produces, where nothing is being torn down. Reaches only the receiver-task payload promise (marked handled at :1056) or an SDK error response on a transport that is about to be re-initialized, so it's cosmetic.
• The connect-start sweep dispatches before status = "connecting" (:1711), so a pendingElicitationsChange listener sees the outgoing status. usePendingClientRequests only stores the array, and App.tsx derives the modal from length with no status gate, so nothing reads the pair — which is the whole point of the fix.
• The two dispatch payloads are the internal arrays by reference (:1570-1574), consistent with every other dispatch site in the file rather than an outlier.
• The rest of the stack is unchanged since round 45 — one client.connect( call site (:1725) with both registrations before it (:1717-1718) inside the same try; six capability flags all derived from the built capabilities object with every gate reading a flag; four cleanRoots entry points with both Keep: guards intact and no constructible input that throws; both cancel() implementations settling without onRemove.
• Coverage. closeSubscriptionBestEffort's catch is covered from the resetSubscriptionStream side; the two new tests cover both new sweep call sites and both arms of the announce guard.

On fifty-one rounds

Round 50's own framing is the useful one: "the routes out do not cover every route in." Finding 1 is that observation applied one level further — the sweep's placement now depends on a call three lines above it, and the reason lives only in the interaction. Finding 2 is the enumeration class again, and it's now five docs describing a set that grew by one in the same commit; naming the category instead of counting is what stops the sixth occurrence.

This checkout has no node_modules, so I couldn't run the suite; the reported gate results are taken as reported. Everything above is from reading the final tree and diffing origin/v2/main...HEAD.
· branch v2/fix-roots-handler-race

[cliffhall]

All four taken, in 6cf9ae04. Full gate green.

1. The ordering dependency — stated and pinned

You traced this correctly and it's the sharpest kind of finding: two adjacent calls that read as independent, where the second is a no-op because of the first. ElicitationCreateMessage.cancel() resolves synchronously into the record callback, which ends in upsertReceiverTask; that finds nothing only because clearReceiverTasks() emptied the map three lines up. Hoisted for readability — exactly the invitation the comment block was extending — it emits a notifications/tasks/status for the ending session's task onto the transport the connect is about to reuse.

The clause is now on the sweep. I also took the test you suggested, since a comment saying "must stay after" is the kind of thing a refactor reads past: it queues a task-augmented sample, drives the onerror-without-onclose route, clears the first session's frames, reconnects, and asserts no notifications/tasks/status went out. Verified against the hoisted order — it fails there.

2. Five stale enumerations

Right, and the count was the lesser problem: cancel()'s doc is where someone asks who calls this and when, and it answered with a category ("teardown paths") that excludes the new caller entirely. All five now name the category — every route out, plus the top of connect() as a backstop for the one route in that settles nothing — with the "not a count, that set has grown" note on the two class-level ones so the sixth occurrence doesn't recur.

3. The refresh-site arm — tested, and confirmed load-bearing

This was the right thing to push on. Round 50's message claimed the fold fixed :4721, and nothing exercised it, so the claim rested on reading. Table-driven pair in the era suite now: subscribe, swap the live stream for one whose close() throws (and one that rejects), subscribe a second URI to force a re-list, and assert the refresh resolved with an acknowledged replacement stream and both URIs subscribed. Driven through subscribeToResource rather than the private path, so it's the reachable route.

Checked rather than assumed: reverting :4721 to await previous.close().catch(() => {}) fails the synchronous arm and passes the rejecting one — which is exactly the asymmetry the fold removed.

4. Both nits

• The raw-wire test's timeout justification was borrowed and wrong for it — that test does connect twice, so an initialize is issued under the 50 ms budget. Now says why it's actually safe: the fixture answers initialize synchronously from inside send(), so no timer is in play, and a fixture answering a macrotask later would race it.
• App.tsx:1226 now says clearing the ref "would take the no-server branch below and silently drop this to Off," so "the default" keeps one meaning in that block.

Also folded in your :249 note — the opener is now "All three stream closes go through here," which is what the rest of the sentence already scoped it to.

Gate

validate ✓, coverage ✓, verify:build-gate ✓, smoke ✓, Storybook ✓.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 5m 24s —— View job

---

Review (round 52): final tree after 64bf2b99…6cf9ae04

• Read the diff against origin/v2/main and the commits since round 33
• Verify the connect-start sweep, its ordering guard, and closeSubscriptionBestEffort
• Final sweep of the whole stack
• Post review

---

Verdict: rounds 47–51 land correctly and the diff is converged. Nothing blocking — the PR is ready. One finding worth 30 seconds, because the comment justifying the new sweep argues against the settle the sweep deliberately performs; two nits after it.

Verified

• The round-51 ordering claim is literally true, and I checked the mechanism rather than the prose. upsertReceiverTask (inspectorClient.ts:1092-1098) early-returns when the record is absent, and a task-augmented peer request's error callback is record.rejectPayload(error) followed by upsertReceiverTask(updated) (:1216/:1226, :1292/:1301) — so hoisting the sweep above clearReceiverTasks() really would emit a notifications/tasks/status for the outgoing session's task onto the transport the connect is about to reuse. The test that notices it (:1000) asserts on sentNotifications after zeroing the first session's frames, which is the right scoping.
• The onerror-without-onclose route is real as described. attachTransportListeners' onerror (:824-844) sets status and dispatches, and touches nothing else — no client.close(), no _transport null, no queue clear. connect() then skips transport creation because this.baseTransport is truthy (:1634), which the created === 1 assertion in each of the three new tests pins as the premise rather than assuming it.
• Both sweep helpers are idempotent as claimed, so the routes that already ran them are unaffected: clearAndAnnouncePendingPeerRequests guards on a non-empty queue (:1567), and rejectPendingRawWireRequests clears its map, with a settled promise's re-reject a no-op.
• The sweep sits after the status === "connected" early return (:1588), so a redundant connect() on a live connection can't cancel live peer requests — that ordering is load-bearing and easy to get wrong.
• closeSubscriptionBestEffort absorbs both failure modes for real. try { await subscription.close() } catch {} inside an async function catches a synchronous throw and a rejection, so the returned promise always resolves — which is what makes the void at :1472 safe. The three call sites (:1472, :4738, :4754) are the only ones; the round-51 table-driven test drives the middle one from the public surface (a re-list over a poisoned stream), which is the site where the fold actually changed behaviour.
• The clearModernReconnectTimer move fixes a genuinely orphaned doc — it had been sitting between refreshModernSubscription's doc block and its signature.
• The closed rejection-arm comment is accurate: onModernSubscriptionClosed (:4793-4830) only assigns state, dispatches, and arms a timer, so "cannot throw today" holds, and scoping the .catch to the rejection rather than chaining it is the right call for the reason given.
• Rounds 1–46 still hold. One client.connect( call site (:1738) with both registrations before it (:1730-1731) inside the same try; six capability flags all derived from the built capabilities object at :702-710, no gate re-deriving from an option; cleanRoots reached from the constructor (:562), setRoots (:4588), serverList.ts:411, and all three clients (cli.ts:164, tui/App.tsx:312, web/App.tsx:2318).

1. The sweep's own comment names as the harm the write the sweep performs

inspectorClient.ts:1601-1604:

The peer queue is the sharper of them — the web pending-request modal is derived from its length with no status gate, so it outlives the session, and answering it writes a response for the old request id onto the reused connection.

That last clause describes what clearAndAnnouncePendingPeerRequests() does four lines below it. elicitation.cancel() resolves the SDK request handler's promise, the SDK writes { id: <old id>, result: { action: "cancel" } } to _transport — and on this route _transport is still the previous, live MessageTrackingTransport, because onerror never nulled it. So the sweep writes a response for the old request id onto the connection the connect is about to reuse, immediately.

That behaviour is right, and the ordering makes it better than the comment implies: the write lands at :1618, before dropCachedTransport() (:1630) and before the re-initialize (:1738) — i.e. on the still-open pre-handshake connection, which is the best moment available. It's the same "settle, don't discard" invariant rounds 24–25 established (accept-then-silence is worse for the peer than a decline).

Which is why the clause is worth fixing rather than ignoring: as written it reads as an argument that the frame should not be emitted, and the change it invites — clear without settling — is precisely the regression this PR spent two rounds removing. What the sweep actually prevents is the stale modal and a user-authored answer landing arbitrarily later, after the new initialize, for a request the previous session raised. One clause ("…and a user answering it later would write their answer for the old request id onto the new session — the sweep answers it now, with a cancel, while the old connection is still the one on the wire").

Fix this →

2. closeSubscriptionBestEffort's "at each site" claim holds at two of the three (nit)

:250-256:

All three stream closes go through here, because at each site the caller has already dropped its reference to the stream, so an escaping failure abandons a stream that may still be open on the server and takes the caller's remaining work with it — teardown that most callers of disconnect() would never see skipped, or the re-listen that was about to replace the stream being closed.

Both consequences hold at :1472 (resetSubscriptionStream) and :4738 (the re-listen's first close). At :4754 — the superseded just-opened stream — the reference was never stored, and there is no remaining work: the next statement is return. The consequence there is different and smaller: the throw/rejection would propagate out of refreshModernSubscription to whichever subscribeToResource call was already superseded, rejecting a call whose replacement had already succeeded.

Still worth wrapping, and the enumerated reasons are the load-bearing ones. Same shape as the parity claims rounds 15/18/32 scoped: an "each site" that holds at two. A scope clause ("…at the third the stream was never stored and the failure would instead reject a subscribeToResource a newer refresh had already superseded") is the whole fix.

Fix this →

3. The two release-obligation tests are split by the three sweep tests (nit)

The file groups coherently by route — :905 (closes a live stream the next connect drops), :953 (clears the peer request), :1000 (no stale task status), :1035 (rejects the raw-wire entry) are all the same onerror-then-reconnect route, which is a better organizing principle than by collection.

The odd one out is the table-driven pair at :1080 (continues teardown when the dropped stream's close() …), which is the disconnect() side, not that route — and the header pairs it with :905 as the two halves of one property ("That release is best-effort in both directions"). Moving it directly after :905 puts the pair together and leaves the route group intact. Cosmetic, and the fifth time this residue has appeared (rounds 20, 25, 28, 31) — the underlying cause is that the file has two valid axes, route and category, so it may be worth a line in the header saying which one the ordering follows.

4. Also checked, nothing to raise

• Concurrent connect() while "connecting" would sweep a queue the in-flight handshake had populated — but resetSessionState() already had that exposure before this commit, and nothing in the tree issues overlapping connects. Pre-existing re-entrancy, not widened in a way worth guarding here.
• The cancel's response can't produce an unhandled rejection — the SDK's _onrequest catches its own send failure, and void payloadPromise.catch(() => {}) still covers the task-augmented rejecter.
• resetSubscriptionStream's fire-and-forget close races nothing — it is dispatch-ordered after both state mutations, and a later dropCachedTransport() closing the transport underneath it is absorbed by the helper.
• The new sweep's pendingElicitationsChange fires while status is still "error" (status → "connecting" at :1724). usePendingClientRequests tracks only lengths, so there's no ordering dependency.
• The header's new caveat paragraph is accurate — "those paths are every way out, which is not every way in", and the end-clean clears staying put because a disconnect-event consumer has to see them empty, which a sweep on the way back in cannot provide. That's the right reason for keeping both.
• Coverage should be neutral-to-positive: three new tests over two new call lines plus a table-driven pair over the folded helper, and both arms of each new branch are exercised.

This checkout has no node_modules, so I could not run the suite; the reported gate results are taken as reported. Everything above is from reading the final tree and diffing origin/v2/main...HEAD.
· branch v2/fix-roots-handler-race

[cliffhall]

All three taken, in aba28283. Full gate green.

1. The comment argued against the settle it performs

This is the one I'm glad you traced to the wire rather than reading past. You're right: cancel() resolves the SDK handler's promise, _transport is still the previous live transport on this route, so the cancel frame for the old id goes out at the sweep — before dropCachedTransport() and well before the re-initialize. So the clause I wrote as the harm is a description of the fix.

And your reason for fixing it rather than shrugging is the right one: as written, the change it invites is clear-without-settling, which is the exact regression rounds 24–25 removed. The clause now names what is actually prevented — a user-authored answer landing arbitrarily later, past the re-handshake, for a request the previous session raised — and says plainly that the sweep instead answers it now, with a cancel, while the old connection is still on the wire, which is the best moment available.

2. "At each site" held at two of three

Correct, and the third is different in kind, not just degree: the superseded stream was never stored and the next statement is return, so neither named consequence applies — an escaping failure would only reject a subscribeToResource whose replacement had already succeeded. The doc now says that instead of generalizing, and says why it's wrapped anyway (one rule, one shape).

3. Ordering, and the axis note

Moved — the disconnect()-side pair now sits directly after its connect()-side twin, so the two halves of the release obligation read together and the onerror-then-reconnect route group is contiguous again.

Took the header line too, since as you note this residue has recurred: the file now says which axis the ordering follows (route), and that a case which is the counterpart of one in another group is placed next to its counterpart rather than with its route. That's the rule the file was already following implicitly — writing it down is what stops the sixth occurrence.

Gate

validate ✓, coverage ✓, verify:build-gate ✓, smoke ✓, Storybook ✓.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 5m 3s —— View job

---

Review (round 53): final tree after aba28283

• Read the round-52 diff and the final tree
• Verify the reworded sweep comment and the scoped close doc
• Re-check the moved test block and its back-references
• Post review

---

Verdict: the sweep-comment reword is right and the doc's new scoping is 2/3 correct. One finding: the site the round-52 reword singles out as "teardown skipped" is the one site where that cannot happen — and the test written for it doesn't pin the protection either. Nothing here changes runtime behaviour; the code has been converged for many rounds.

Verified

• The sweep comment now names the right hazard. On the onerror-without-onclose route client.close() never ran, so _transport is still set and elicitation.cancel() / SamplingCreateMessage.cancel() really do put a frame on the wire for the old id. Calling that out as the settle-don't-discard rule at the best available moment — rather than as the harm — is the correct framing, and it closes the reading that would have invited the clear-without-settling regression rounds 24–25 removed.
• The ordering invariants still hold. One client.connect( call site (:1745) with registerPeerRequestHandlers() (:1737) and registerPeerNotificationHandlers() (:1738) before it inside the same try. The connect() sweep sits after resetSessionState() (:1597 → :1625-1626), which the :1617-1624 comment and the sweeps the queue without reporting the outgoing session's task test both pin.
• The :987 back-reference was fixed by the move. Pre-move the loop sat at :1080, whose two predecessors were sweeps the queue… and rejects an in-flight raw-wire request… — neither gives a seeding rationale. It now sits under aborts a paused task-input wait… (:883) and closes a live listen stream… (:912), which give exactly the two rationales it cites. Correct as written.
• The header's ordering note is honest about the exception it creates — grouping by route, with the stream-release pair kept together across routes — so the placement itself is a documented decision rather than drift.

1. The close doc's "two of three" puts the resetSubscriptionStream site in the wrong group

core/mcp/inspectorClient.ts:249-253 names two harms for the two "caller already dropped its reference" sites: teardown that most callers of disconnect() would never see skipped, or the re-listen that was about to replace the stream. Mapping them to the call sites:

site	shape	what an escaping failure does
:1475 resetSubscriptionStream	void closeSubscriptionBestEffort(closing) — fire-and-forget, last statement	unhandled rejection
:4745 refreshModernSubscription	await …, re-listen follows	rejects the method → re-listen skipped ✅
:4761 superseded-generation discard	await …, return follows	rejects an already-superseded subscribeToResource ✅ (the new "milder" clause)

:1475 can't produce the harm the doc assigns it, and the reason is structural: closeSubscriptionBestEffort is async, so a synchronous throw from a third-party close() is captured into the returned promise rather than propagating — and the call site voids that promise. Strip the helper's try/catch and resetSubscriptionStream still returns normally, disconnect() still runs :2013-2030, and nothing is skipped. What you get instead is an unhandled rejection.

That matters more than a mis-sorted example, because this PR already established (rounds 25–26, the payloadPromise fix) that an unhandled rejection is the process-fatal class in Node — the reason void payloadPromise.catch(() => {}) was worth putting at the source. The doc doesn't mention that harm anywhere, so the one site whose failure mode is the severe one is described as having the mild one. The local comment at :1471-1474 has the same tension inside four lines: "fire-and-forget because nothing downstream depends on it" and "disconnect() calls this from the middle of a straight-line teardown that runs outside any try" can't both be the reason — the first is why the second doesn't apply.

Fix this →

2. Consequence: the disconnect()-route close loop's assertions hold with the protection removed

inspectorClient-peer-handler-timing.test.ts:960-1006 asserts await expect(client.disconnect()).resolves.toBeUndefined() and that a downstream teardown step ran (controller.signal.aborted). Its comment states what it means to pin:

an escaping failure would skip the raw-wire rejects, the paused task-input aborts, the in-flight tool-call abort and clearReceiverTasks() — silently, since most callers catch disconnect(). […] a close() that fails does not take the rest of teardown with it.

By finding 1, that can't happen at this site regardless of the helper's catch: both close variants become a rejected promise from an async helper, the void drops it, and disconnect() proceeds. So both halves of the loop pass with the try/catch deleted — the assertions are satisfied either way. (Vitest would likely still redden the run by reporting the unhandled rejection, so it isn't invisible; but it fails as an unattributed run-level error, not as the assertion that names the mechanism — which is the distinction rounds 15/25/32 kept applying to this file.)

What the loop does pin is a different regression: unwrapping the call to a bare void closing.close(), where the sync-throw half would then propagate. Worth keeping for that, but the assertion for the stated claim is "no unhandled rejection" — an unhandledrejection / process.on("unhandledRejection") spy around the disconnect(), or dropping the void and awaiting the reset so a rejection is observable.

The parallel is exact with the round-12 fixture and the round-5 HTTP variant: an absence assertion is only as good as the proof the thing could have been present.

Fix this →

3. The move traded one broken back-reference for another (nit)

:1009 — "Same route as the test above, for the other collection it strands" — the test immediately above is now the disconnect()-route close loop, not the onerror-route closes a live listen stream the next connect drops (:912) it means. :1017's "Counted for the same reason as above" is now two tests up rather than one (vaguer, so it survives, but it's the same drift).

Fixing the referents is a two-word change. The more durable read is that this file has now broken a positional reference twice — :987 before this commit, :1009 after — for the same reason round 17 rewrote the header to stop counting tests: naming the test ("the same route as closes a live listen stream the next connect drops") is immune to the next insertion in a way "the test above" isn't, and the header's own ordering note guarantees more insertions between route groups.

Fix this →

4. Two triviata

• inspectorClient.ts:1611 is 106 chars in a comment block whose other lines wrap at ~78 — the reword spliced a sentence and left the seam. Prettier's proseWrap: preserve won't reflow it and format:check won't complain. Same class as rounds 26/28/32; the fix is the same (reflow the paragraph in one pass). The file's pre-existing floor is high (there are 118- and 167-char lines), so this is cosmetic.
• "the old connection is on the wire until dropCachedTransport() below" (:1610-1611) reads as though that drop always runs. It's conditional on :1629-1636 (HTTP + OAuth + authorized + no auth provider + not enterprise-managed), so on the stdio route the tests use it never fires and the transport stays live through the reconnect. The claim is true-but-understated rather than wrong.

5. Also swept, nothing to raise

• closeSubscriptionBestEffort really does absorb both modes — the await is inside the try, so a synchronous close() throw and a rejected promise both land in the same catch. The helper itself can never reject.
• :4745's re-listen claim is exact — the reference is dropped at :4743 and the client.listen(...) at :4754 is the work an escaping failure would skip.
• The disconnect() clear-before-announce ordering is unchanged — clearPendingPeerRequests() at :1998 above the disconnect dispatch at :2005, matching the crash path; the raw-wire reject stays at :2020 with its documented "the user asked to disconnect" rationale.
• The connect() sweep is idempotent on the overlapping routes as its comment claims — clearAndAnnouncePendingPeerRequests guards on a non-empty queue, rejectPendingRawWireRequests clears its map and re-rejecting a settled promise is a no-op.
• Coverage is neutral for aba28283 — comments plus a moved test block.

On fifty-three rounds

Round 52 set out to scope two over-broad claims, and got one of them: the sweep comment now names the right hazard. The close doc's scoping picked the wrong site to call mild — and the interesting part is that the site it mis-describes fails in the mode this PR itself proved is fatal in Node, with a test whose assertions don't distinguish the two. That's the same "an absence assertion needs proof the thing could have been present" check that caught the round-5 and round-12 fixtures, arriving last at the newest test in the file.

This checkout has no node_modules, so I couldn't run the suite; the reported gate results are taken as reported. Findings 1 and 2 are from the code's structure (an async function converts a synchronous throw in its body to a rejection; void on the returned promise drops it), not from execution.
· branch v2/fix-roots-handler-race

[cliffhall]

All four taken, in 904cdcff. Full gate green.

1. The mis-assigned site — and it was the severe one

You're right, and the mechanism is exactly as you state it: closeSubscriptionBestEffort is async, so a synchronous throw in its body is captured into the returned promise, and resetSubscriptionStream voids that promise. Strip the try/catch and disconnect() still runs to completion — nothing is skipped. What you get is an unhandled rejection, the class this PR itself put void payloadPromise.catch(() => {}) in place for.

So the doc had it backwards where it mattered most. Each site now carries its own consequence rather than sharing a pair of harms: the fire-and-forget reset (unhandled rejection — fatal in Node, a console error in the browser), the re-listen close (replacement listen() skipped, non-empty set with no stream), and the superseded discard (rejects an already-superseded call). And you're right that the local comment contradicted itself in four lines — "nothing downstream depends on it" is precisely why the straight-line-teardown reason doesn't apply there. It now says the void is what makes a failure unable to reach the caller, and that going unhandled is what's left.

2. The test proved nothing at that site

Correct — both assertions hold with the guard deleted, for the same structural reason. It now also installs an unhandledRejection listener around the disconnect() and asserts nothing fired, yielding one macrotask first so Node's checkpoint has run.

Verified rather than assumed: with the helper's try/catch removed, both arms fail — and on the expect(unhandled).toEqual([]) line, so the failure names the mechanism rather than arriving as an unattributed run-level error. The two teardown assertions stay, for the regression you identified them as actually guarding: unwrapping the call to a bare void closing.close(), where the synchronous half would propagate.

3. Positional references, again

Both now name their referent — closes a live listen stream the next connect drops and aborts a paused task-input wait when the session ends — rather than "the test above" / "as above." Your framing is the right one: the header's own ordering note guarantees more insertions between route groups, so a positional reference in this file has a known expiry. Second breakage, so it's the rule now, not the incident.

4. Both triviata

• Reflowed the spliced paragraph in one pass, and split it at the natural break so the idempotence argument is its own paragraph.
• The dropCachedTransport() claim is now scoped: the old connection is on the wire here and stays so at least until that conditional drop — which on a stdio server never runs, so the same transport carries through the re-handshake. Understating it in the direction of "the transport is live" was the wrong way to be imprecise in a comment about writing to it.

Gate

validate ✓, coverage ✓, verify:build-gate ✓, smoke ✓, Storybook ✓.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 5m 3s —— View job

---

Review (round 54): final tree after 904cdcff

• Read the round-53 diff and the final tree
• Verify the per-site harm doc and the new unhandled-rejection assertion
• Final sweep of the stack
• Post review

---

Verdict: round-53 closes its item correctly, and the new assertion is genuinely the one that fails — I traced both arms. Nothing blocking; the PR is ready. One finding, on the doc paragraph this commit rewrote: it gives each site "its real harm," and the third bullet names one caller where there are two — on the second of which the harm is a false error shown to the user, not the mildest case.

Round-53 item: verified, including the claim I'd have doubted

The interesting part of this commit is the assertion split, and it holds up:

• With closeSubscriptionBestEffort's try/catch deleted, both arms produce an unhandled rejection — the sync arm too, because the helper is async, so the throw becomes a rejection of the returned promise, which the void at inspectorClient.ts:1487 drops. Nothing propagates, so disconnect() still resolves and controller.signal.aborted is still true. The listener is the only assertion that moves. Exactly as the commit says.
• With the call unwrapped to a bare void closing.close(), the arms split: the sync throw escapes resetSubscriptionStream() (called at :2029, outside any try, and before the task-input aborts at :2040), so both resolves.toBeUndefined() and the abort assertion fail; the rejected-promise arm goes unhandled and the listener catches it.

So the two assertion sets are complementary rather than one being vestigial, which is what the comment claims and the reason to keep both. The setTimeout(0) is also the right yield — Node emits unhandledRejection at the end of the microtask drain, so a macrotask hop is sufficient and not merely probable.

The doc restructure is the substantive half, and its premise is right: grouping the void-ed site with the "takes the caller's remaining work with it" sites had the one site with the process-fatal failure mode documented as having the mild one.

Final sweep

Re-derived on the final tree rather than trusting the earlier rounds:

• The original fix is intact. One client.connect(this.transport) (:1762), with registerPeerRequestHandlers() (:1754) and registerPeerNotificationHandlers() (:1755) before it inside the same try.
• Six capability flags, one derivation — all assigned at :715-723 off the built capabilities object, and every gate reads a flag (:1202, :1279, :1352, :1366, :1904, :4625). No gate re-derives from a constructor option.
• Teardown call sites match the documented three-path story: clearAndAnnouncePendingPeerRequests at :826 (crash), :1642 (connect start-clean sweep), :1948 (connect catch); clearPendingPeerRequests at :1588, :2015; rejectPendingRawWireRequests at :834, :1643, :2037.
• The start-clean sweep's ordering constraint is honoured — it sits after resetSessionState() (:1608) at :1642-1643, which is what keeps a cancelled task-augmented peer request from emitting notifications/tasks/status onto the transport this connect is about to reuse.
• resolveModernLogLevel is the single derivation for both sides — resetSessionState() (:1537) and App.tsx:1236/:2360 — so the client's _meta stamp and the Logs control can't disagree.
• Both cancel() implementations still settle without onRemove, so clearPendingPeerRequests's for…of can't splice mid-iteration.

1. The third bullet's "mildest of the three" is mild only for one of its two callers

inspectorClient.ts:263-265:

the superseded-generation discard — awaited, but return follows, so an escaping failure would only reject a subscribeToResource whose replacement had already succeeded. The mildest of the three.

refreshModernSubscription has three callers, and the discard at :4778 can reject into all of them:

caller	what a rejection does
subscribeToResource (:4945)	caught, rolls back the optimistic add + stream state, rethrows — as the bullet describes
unsubscribeFromResource (:4989)	caught, rethrown as Failed to unsubscribe from resource with no rollback
reconnect timer (:4883)	.catch(() => this.onModernReconnectFailed()) — counted as a failed re-list and retried

The middle one is the reason to raise this. unsubscribeFromResource's own comment says the removal is deliberately kept even when the re-listen fails ("the stale URI simply lingers in the server's honored filter") — so on that path a close() throw surfaces a false failure to a user whose unsubscribe actually stuck and whose replacement stream is fine. That's a worse consequence than the one the bullet assigns to the site, and it's the site the bullet calls mildest.

Two smaller precision points in the same two bullets:

• "whose replacement had already succeeded" — the generation bump only proves a newer refresh started. It can still fail at its own listen(), in which case the discard's rejection is a second failure on top rather than a redundant one. "whose replacement had already superseded it" is what the code guarantees.
• Bullet 2 omits the reconnect caller too. "Skips the re-listen and leaves a non-empty subscription set with no stream" is right for the user-initiated paths; on the reconnect path the same escape is absorbed into the backoff run and retried — self-healing, i.e. a third consequence at that call site, not the one stated.

Given the commit's own thesis is that the sites' consequences "are not interchangeable," naming the second caller (and softening the "had already succeeded") is the same correction one level along.

Fix this →

2. The unhandledRejection capture is global but attributed locally (nit)

inspectorClient-peer-handler-timing.test.ts:1023 asserts unhandled is empty — i.e. that nothing anywhere in the process rejected unhandled during the window. Within a sequential test file that's almost always this site, but the assertion's failure message would blame this test for a rejection a prior test left pending and Node reported late (this file has several tests that arm and race timers). Filtering to the fixture's own reason makes the attribution match the claim, at no cost to the regression it catches:

expect(unhandled.filter((r) => String(r).includes("close blew up"))).toEqual([]);

3. The listener registration sits outside the try that removes it (nit)

:988 registers, :1013 opens the try, :1024 removes in finally. Between them: new InspectorClient(...), await client.connect(), and two private-state seeds. Any throw there leaks the listener into the rest of the file, where it silently accumulates into an array nobody reads (and the loop makes two of them). Moving the process.on to the first line of the try, or wrapping from registration onward, closes it.

4. Also checked, nothing to raise

• process is available under happy-dom, and vitest's own unhandledRejection handler is additive — so the new listener neither suppresses vitest's reporting nor depends on it. The comment's "ends a Node process by default" is scoped to production, correctly; in-test Node won't die precisely because listeners exist.
• The round-52/53 back-reference de-positioning holds — the two "same route as the test above" references now name closes a live listen stream the next connect drops and aborts a paused task-input wait when the session ends, both of which exist under those exact titles.
• The softened dropCachedTransport() claim is right: that drop is gated on isHttpOAuthConfig() (:1648-1656), so on stdio it never runs and the same transport does carry through the re-handshake.
• resetSubscriptionStream's dispatch ordering is still the documented one — both axes moved before either dispatch, so no listener can observe an empty set with an active stream.
• The roots/list_changed gate reads rootsListChangedCapabilityAdvertised (:4625), the predicate the SDK asserts, not the wider capabilities.roots presence.
• Coverage is neutral-to-positive this round — the doc is free, and the new listener/assertion add no production lines.

On fifty-four rounds

The code has been converged since the mid-teens; what the last dozen rounds have been finding is prose that asserts more than the code guarantees, and this round is the same shape one level in — a paragraph written specifically to stop over-generalising, which generalises over one caller of two. Worth doing because unsubscribeFromResource's false error is a real (if narrow) user-visible consequence sitting under the label "mildest of the three"; not worth another round after that.

This checkout has no node_modules, so I couldn't run the suite; the reported gate results are taken as reported. Everything above is from reading the final tree and diffing origin/v2/main...HEAD.
· branch v2/fix-roots-handler-race

[cliffhall]

All three taken, in 88cbd1ac. Full gate green.

1. "Mildest of the three" was true of one caller out of three

You're right, and the one it misses is the one that matters: unsubscribeFromResource deliberately keeps the removal when a re-listen fails ("the stale URI simply lingers in the server's honored filter"), so a close() throw there rethrows Failed to unsubscribe from resource for an unsubscribe that actually stuck and a replacement stream that is fine. A false failure shown to the user is not the mildest consequence in the list — it's arguably the worst of the ones enumerated.

Both bullets now name their callers rather than generalizing over them:

• the discard — mild from subscribeToResource (rolls back, rethrows), self-healing on the reconnect path, and the false-error case from unsubscribeFromResource named explicitly;
• the re-listen close — the "non-empty set with no stream" outcome holds for the user-initiated callers, with the reconnect caller's absorb-and-retry called out.

And the generation-bump claim is softened to what the code guarantees: a newer refresh had started. It may still fail at its own listen(), which makes an escaping failure here a second one rather than a redundant one — the exact distinction that turns "harmless" into "compounding."

The self-referential note is fair. A paragraph written to stop over-generalizing, generalizing over a caller set, is the same error one level in.

2. Global capture, local attribution

Filtered to close blew up. The regression it catches is unchanged — re-verified that both arms still fail with the helper's try/catch removed, in the filtered form — and a rejection some earlier test left pending that Node reports inside this window no longer reddens this test with someone else's failure.

3. The listener leak window

Moved: process.on is now the first statement of the try whose finally removes it, so the constructor, the connect() and both private-state seeds are inside the guarded region. As you note the loop makes two of them, so a throw in setup would have leaked a pair per run into an array nobody reads.

Gate

validate ✓, coverage ✓, verify:build-gate ✓, smoke ✓, Storybook ✓.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 54s —— View job

---

Review (round 55): final tree after 88cbd1ac

• Read the round-54 diff and the final tree
• Verify the three-caller harm attribution
• Re-check the unhandledRejection test change
• Post review

---

Verdict: round-54's correction is right, and the caller-by-caller attribution is now accurate for two of the three sites. One finding: the bullet it left as "mild from subscribeToResource" understates that site for the same structural reason it just fixed for unsubscribeFromResource — and there the harm has a variant that is live today, independent of the wrapping. Not a merge blocker; the code change it would prompt is pre-existing scope.

Round-54 items: verified

• The unsubscribeFromResource correction is exactly right. :5000 awaits the refresh, and the outer catch (:5006) rewraps anything escaping into Failed to unsubscribe from resource:. Since the removal was already committed and dispatched at :4997-4999 — deliberately, per its own comment — a failure there really does report a failed unsubscribe for one that stuck. And the superseded case makes it purely spurious: the newer refresh built its filter from the set after the removal, so the server honors the removal. Both halves hold.
• The re-listen bullet's reconnect clause is right. The reconnect caller is this.refreshModernSubscription(true).catch(() => this.onModernReconnectFailed()) (:4894), which counts the failure and reschedules at the next backoff — so it is absorbed and retried, not a set left with no stream.
• The softened generation claim is the accurate one. generation !== this.modernListenGeneration proves only that ++this.modernListenGeneration ran in a newer refresh (:4766), which happens before its listen() — so the newer refresh may still fail at :4783. "Started, not succeeded" is precisely what the check witnesses.
• The test's listener scoping does what the comment claims. process.on is the last statement before the try and everything that can throw (construction, connect(), the two casts) is now inside it, so no path leaves the listener attached. (The commit message says "registered inside the try"; the code has it immediately above. The intent is met — only the message is loose.)
• The filter is a real improvement. unhandledRejection is process-wide and vitest's own handler stays installed, so an unfiltered toEqual([]) could fail on a rejection another test left pending and Node reported late. Filtering to the fixture's own reason removes that without weakening the regression signal: with the helper's try/catch gone, the reason is close blew up.

1. subscribeToResource's rollback is unconditional, so on a superseded refresh it de-registers a URI the server did honor

The new bullet reads:

Mild from subscribeToResource (which rolls the optimistic add back and rethrows) and self-healing on the reconnect path, but not from unsubscribeFromResource …

The rollback is what makes it not mild. :4957-4966:

} catch (error) {
  // Roll back the optimistic add + stream state so both stay consistent
  // with the (unchanged) server filter.
  this.subscribedResources.delete(uri);
  this.dispatchSubscriptionsChange();
  if (this.subscribedResources.size === 0) {
    this.setModernStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE);
  }
  throw error;
}

The comment's premise — "the (unchanged) server filter" — is exactly what supersession falsifies. The newer refresh built its filter from the set including the URI this call had already added, so on success the server is honoring it. Rolling back then leaves subscribedResources missing a URI the live stream carries: the UI shows it unsubscribed while notifications/resources/updated for it keep arriving. That's a silent divergence, strictly worse than the spurious error round 54 called out at the other site — same shape as unsubscribeFromResource, mirrored, and the doc has it the other way round.

Two further points, which is why I'd raise it rather than leave it as prose:

• It is reachable without unwrapping anything. The escape doesn't have to come from close() — a rejecting listen() at :4783 propagates the same way, before the supersession check ever runs. Interleave a user subscribe with the reconnect timer (:4883-4895, i.e. precisely when the server is flaky): user adds A, gen 1, awaits listen; the timer fires, gen 2, re-lists {A} and succeeds; listen1 rejects; the catch deletes A, size hits 0, and it sets INACTIVE_SUBSCRIPTION_STREAM_STATE — while modernSubscription holds a live acknowledged stream. That is the empty-set-with-an-active-stream combination resetSubscriptionStream's doc (:1469-1471) says the rest of the file treats as impossible, arrived at from the other end.
• It's pre-existing — this PR introduced closeSubscriptionBestEffort and the three call sites; the generation check and the rollback both predate it (git diff origin/v2/main...HEAD touches neither). So the code fix is out of scope by the standard the earlier rounds used. The doc claim is new in 88cbd1ac, and it's the half that's in scope: it currently tells the next reader this site is the safe one.

The minimal code fix is a supersession check the caller can make: capture this.modernListenGeneration before the call and skip the rollback when it has advanced by more than the one bump the call itself makes — leaving the optimistic add in place, since a newer refresh owns the filter and may have honored it.

Fix this →

2. Both parameterized arms reject with the same reason, so the filter can't tell them apart (triviata)

The for (const [label, close] of …) loop gives both arms new Error("close blew up"), and the filter keys on that string. So the mechanism that excludes foreign rejections cannot exclude the sibling arm's — if the sync-throw arm ever left one pending and Node reported it during the rejected-promise arm, it would land in the filtered array.

No practical consequence: in the passing state neither arm produces an unhandled rejection, and in the regression state both fail regardless. Distinct messages (close blew up (sync) / (async)) would make the filter as precise as its comment claims, at the cost of a second interpolation.

3. Also swept, nothing to raise

• resolveModernLogLevel is a single derivation with two consumers, and the undefined → null mapping stays at the web boundary (App.tsx:1236-1240, :2360) rather than becoming a second derivation. The re-seed branches on the server, not its settings, which is right — a hand-written mcp.json entry with no settings node is the common case and the default is what both the seed and the client use.
• resetSubscriptionStream's ordering claim holds: the set and the generation move, then both dispatches, then the void close — so no listener can observe an empty set with an active stream, and the fire-and-forget close is after the dispatches as its comment says.
• The void-vs-await distinction across the three close sites is real, not stylistic: at resetSubscriptionStream the helper being async is what converts a synchronous throw into a rejection the void drops, so the wrapping there buys "not unhandled" rather than "teardown continues" — which is exactly what the test asserts and why its two teardown assertions are kept for the neighbouring regression instead.
• clearReceiverTasks is called from both connect() and disconnect(), so the auth-recovery retry (same instance, new session) can't inherit records — and the doc says why that isn't redundant.
• The rest of the stack still holds. One client.connect( call site with both peer-handler registrations before it inside the same try; six capability flags all derived from the built capabilities object with every gate reading a flag; both cancel() implementations settle without onRemove; four cleanRoots entry points with both Keep: guards intact and no constructible input that throws.

On fifty-five rounds

Round 54's move was to stop assigning one harm to a method with three callers. The finding above is that same move applied to the bullet it left standing: subscribeToResource is described by what it does (rolls back) without asking whether that action is correct under the condition the bullet is about. The rollback's own comment asserts the premise supersession removes — which is the comment-contradicts-adjacent-code shape this review has been chasing since round 2, now one level in, on a catch block.

This checkout has no node_modules, so I couldn't run the suite; the reported gate results are taken as reported. Everything above is from reading the final tree and diffing origin/v2/main...HEAD.
· branch v2/fix-roots-handler-race

[cliffhall]

Both taken, in edb01fcc — including the code fix. Full gate green.

1. The rollback is the thing that makes that site severe

You're right, and I'm taking the code change rather than only the doc, because the doc claim and the bug are the same mistake: the catch asserts "the (unchanged) server filter", and supersession is precisely the condition where that's false. The newer refresh built its filter from the set including this URI, so on success the server honors it — and rolling back then leaves the set missing a URI the live stream carries. Worse than the mirrored spurious error at the other site, because it's silent.

Your reachability argument is what settles it: no close() failure needed, a rejecting listen() propagates the same way, and the interleaving that produces it — a user subscribe racing the reconnect timer — is by construction what happens when the server is flaky. The empty-set-with-an-active-stream endpoint you traced is the same combination resetSubscriptionStream's doc calls impossible, arrived at from the other end, which is the strongest form of the argument.

The fix is the generation gate you describe: read modernListenGeneration before the call, and skip the rollback when it has advanced past this call's own single bump. The error still reaches the caller — the caller's failure is real, the filter's contents aren't the caller's to correct once a newer refresh owns them.

Test in the era suite: two overlapping modern subscribes, the first listen() held open and then rejected after the second acknowledges, asserting the first URI survives and the stream stays acknowledged. Verified load-bearing — with the gate forced open it fails.

On scope: you're right that the generation check and the rollback both predate this PR. I'm fixing it here anyway rather than leaving a follow-up, since the doc line asserting the site is safe is new in 88cbd1ac and I'd rather not ship a correct comment over an incorrect catch.

2. The filter couldn't exclude its sibling

Fair — both arms threw the same reason, so the mechanism that excludes foreign rejections was blind to the one rejection most likely to arrive late. Each arm now carries its own (close blew up (sync) / (async)) and the filter keys on the arm's, so it's as precise as its comment claims.

Gate

validate ✓, coverage ✓, verify:build-gate ✓, smoke ✓, Storybook ✓.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 5m 8s —— View job

---

Review (round 34): fix(core): don't roll back a subscribe a newer refresh superseded

• Read the round-33 diff (edb01fcc) against origin/v2/main
• Verify the supersession gate and its test
• Final sweep of the stack
• Post review

---

Verdict: the gate is the right fix for the divergence it targets, and the test is load-bearing. One finding: the case the doc itself flags — the newer refresh may also fail — now converges on a stuck state where the pre-gate code converged on a clean one. Not a merge blocker; a nit and two triviata after it.

Verified

• generationBefore + 1 is the correct encoding. refreshModernSubscription bumps once, synchronously, at :4768 (const generation = ++this.modernListenGeneration) before any await — the only pre-bump code is if (!this.client) return (which doesn't throw) and clearModernReconnectTimer(). So in the catch the generation is exactly generationBefore + 1 unless something else advanced it.
• The gate also covers a case the comment doesn't claim. A disconnect() → reconnect → re-subscribeToResource(uri) sequence leaves the stale in-flight call's catch to run against a new session that legitimately holds the URI; pre-gate it would have deleted a freshly-subscribed URI. Strictly better than the same-session case it's written for.
• The new test can't pass by accident. previous is null on a fresh connect, so client.listen() is reached synchronously inside subscribeToResource(...) — failFirst is assigned before const first returns, no tick needed, consistent with this file's no-microtask-counting discipline. expect(first).rejects… is attached before the supersession so the rejection is never unhandled, and /listen boom/ still matches through subscribeToResource's "Failed to subscribe to resource: …" wrapper. Without the gate the superseded call deletes RESOURCE_URI and the toEqual([URI, URI_2]) fails.
• The distinct-reason change is correct and closes a real hole. Both arms of the close() loop previously threw "close blew up", so the process-wide unhandledRejection filter could not exclude the sibling arm's late-reported rejection — only a foreign one. (sync) / (async) fixes that, and the reason is now threaded from the same tuple that builds the error, so the two can't drift.
• The rest of the stack still holds. One client.connect( call site with both peer registrations before it inside the same try; six capability flags all derived from the built capabilities object with every gate reading a flag; resetSubscriptionStream still moves the set and the state together and announces only after both; closeSubscriptionBestEffort still absorbs both failure modes at all three sites.

1. When the superseding refresh also fails, the stream state is left stuck at connecting

Two overlapping subscribes whose listen() calls both reject:

gen G   A: subscribeToResource(URI1) → set={URI1}, state {active,connecting}, gen→G+1, listen hangs
gen G+1 B: subscribeToResource(URI2) → set={URI1,URI2}, state {active,connecting}, gen→G+2, listen rejects
        B catch: G+2 <= G+1+1 → rolls back URI2; size is 1, so no INACTIVE; throws
        A catch: G+2 <= G+1  → false → skips rollback; throws

Final state: subscribedResources = {URI1}, modernStreamState = {active:true, status:"connecting"} (from :4951), modernSubscription === null, no reconnect timer armed — scheduleModernReconnect is only reachable from onModernSubscriptionClosed (needs a stream) or onModernReconnectFailed (needs the reconnect path), and neither ran. So the UI shows URI1 subscribed with a permanent Connecting… badge, no resources/updated will ever arrive, and nothing self-heals short of another subscribe/unsubscribe or a reconnect.

Pre-gate, the superseded call's rollback emptied the set and set INACTIVE — both errors surfaced and the state converged. Either ordering of the two failures produces the same end state, and the trigger is the one the commit message names as the realistic path: "exactly when the server is flaky."

This is precisely the gap the doc block already records two hundred lines up — "what the generation bump proves is that a newer refresh had started, not that it succeeded — it may yet fail at its own listen()" — applied to the new gate rather than to the discard it was written about.

The cheap resolution doesn't need the three-way (pending / succeeded / failed) distinction, because it belongs in the branch that does know: the rollback branch runs only when this call owns the filter and has just failed, so it can reconcile the state instead of only handling size === 0:

if (this.subscribedResources.size === 0) {
  this.setModernStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE);
} else if (this.modernSubscription === null) {
  // Stream gone but subscriptions remain — the same state
  // `onModernSubscriptionClosed` reports for that shape.
  this.setModernStreamState({ active: true, status: "ended", honoredUris: [] });
}

ended is already the documented badge for "stream gone, URIs still subscribed" (README, onModernSubscriptionClosed:4854), so this reuses an existing state rather than inventing one — and it makes the badge honest without claiming the subscription failed.

Fix this →

2. The new comment names one of the two things that advance the generation, and mislabels the combination it prevents (nit)

Both halves are in the block this commit wrote:

• "anything beyond that means a newer refresh started" — resetSubscriptionStream also bumps (:1484), reached from disconnect() (:2041) and resetSessionState() (:1538). So a disconnect or a session reset lands in the same skip branch. Skipping is still right there — the reset already cleared the set and set INACTIVE, so there is nothing to roll back — but it's right for a different reason than the one given, and it's the stronger of the two benefits (it's what stops a stale catch from deleting a URI a new session legitimately holds).
• "an empty set with an active stream, the combination resetSubscriptionStream exists to prevent" — that combination is the set and the state disagreeing, and the rollback can't produce it: emptying the set sets INACTIVE in the same branch. What the unguarded rollback actually leaves is a live modernSubscription open on the server with an empty set — a different triple from the one that helper guards. The harm as described one clause earlier ("the UI showing it unsubscribed while its resources/updated keep arriving") is exactly right; only the cross-reference is.

Same shape as rounds 10/14/15/18 — a precise claim that holds on a neighbouring axis rather than the one named.

Fix this →

3. Two triviata

• <= can only ever be === here. The generation is monotonic and bumped synchronously, and the one path that skips the bump (!this.client) also skips the throw — so this.modernListenGeneration < generationBefore + 1 is unreachable in the catch. === would state the invariant the comment describes ("a refresh bumps the generation exactly once"); <= reads as though the counter could be behind. Defensive either way, no behaviour difference.
• generationBefore + 1 encodes a cross-method coupling as arithmetic — the gate's correctness depends on refreshModernSubscription bumping exactly once, in another method 200 lines away. The comment mitigates it by stating the coupling, which is the right minimum; capturing the generation from the callee instead would make it structural, at the cost of a signature change I wouldn't take in this PR.

4. Also checked, nothing to raise

• unsubscribeFromResource's spurious "Failed to unsubscribe" is still there and the reworded doc bullet is honest about it. Its state lie is a different one — the previous acknowledged state survives a failed re-listen with modernSubscription already null, so the badge reads Listening with no stream. Entirely pre-existing (this commit touches neither), and finding 1's reconcile-in-the-failure-path shape would cover both if you'd rather do it once.
• Not suppressing the error on the superseded path is the right call, and consistent: the doc's own point is that the bump doesn't prove the newer refresh succeeded, so claiming success on its behalf would be the mirror mistake. The caller's own operation did fail.
• The reconnect path is unaffected — refreshModernSubscription(true) is driven through .catch(() => this.onModernReconnectFailed()), which never enters subscribeToResource's catch.
• The as unknown as McpSubscription in the sibling close-failure test is the pre-existing poisoned-stream fixture, unchanged by this commit and justified in place.
• Coverage. The new gate's true arm is exercised by the existing "rolls back the optimistic add when listen() fails" test and its false arm by the new one; the peer-handler change is a string swap on already-covered lines.

On thirty-four rounds

The gate correctly identifies that a rollback's premise is falsified by supersession. What finding 1 says is that the replacement premise — "the newer refresh owns the filter, so leave it to say what the server honors" — is only true when that refresh succeeds, which is the caveat this file already wrote down for the neighbouring case. The fix is one branch in the code path that does know the answer.

This checkout has no node_modules, so I could not run the suite; the reported gate results are taken as reported. Everything above is from reading the final tree and diffing origin/v2/main...HEAD.
· branch v2/fix-roots-handler-race

[cliffhall]

All three taken, in 2a3e002d. Full gate green.

1. The both-fail case — reconciled

You're right, and the trace is exact: the superseded call correctly skips its rollback, the superseding one rolls back only its own URI, and the state is left at the optimistic connecting from :4951 with modernSubscription null and no timer armed. Nothing self-heals, because scheduleModernReconnect is only reachable from a stream that closed or a reconnect that failed and neither happened. So the gate traded a spurious-but-converging outcome for a stuck one, on exactly the flaky-server path it was written for.

Taken as you framed it — in the branch that does know the answer. The owning call now calls a small reconcileModernStreamStateAfterFailedRefresh() instead of only handling size === 0: empty set → INACTIVE; non-empty with no stream → {active: true, status: "ended"}. Reusing ended rather than inventing a state is the right call for the reason you give — it's already what onModernSubscriptionClosed reports for that shape and what the README documents as the badge, so it's honest about the stream without claiming the subscriptions failed.

I took your offer to do it once: unsubscribeFromResource gets the same ownership gate and the same reconcile. It rolls nothing back by design, but its failed re-listen left the badge reading Listening over no stream — the same lie in a different register. Two tests, both verified to fail with the reconcile removed.

2. The comment named one bumper of two, and the wrong invariant

Both corrected.

resetSubscriptionStream bumps too, from disconnect() and the start-clean reset — and you're right that skipping there is the stronger case, since it's what stops a stale catch from deleting a URI a new session legitimately re-subscribed. The comment now enumerates both bumpers with why the rollback is wrong under each.

And the cross-reference was wrong: emptying the set sets INACTIVE in the same branch, so set and state never diverge there. What the unguarded rollback actually leaves is a live stream on the server with an empty set — a different triple from the one resetSubscriptionStream guards. Dropped the borrowed invariant and kept the accurate clause about resources/updated still arriving.

3. Triviata

<= → ===. You're right that the weaker comparison reads as though the counter could be behind, when the bump is monotonic and synchronous and the one path that skips it also skips the throw. === is the invariant the comment states.

On the arithmetic coupling: agreed on both halves — it is a cross-method coupling, and the comment stating it is the right minimum here. Changing refreshModernSubscription's signature to return its generation would make it structural, but that's a wider change than this thread should carry.

Gate

validate ✓, coverage ✓, verify:build-gate ✓, smoke ✓, Storybook ✓.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 6m 57s —— View job

---

Review (round 34): final tree after 2a3e002d

• Read the tree at HEAD and the diff against origin/v2/main
• Verify the two newest fixes (edb01fcc supersession gate, 2a3e002d stream-badge reconcile)
• Final sweep of the whole stack
• Post review

---

Verdict: both new fixes are correct and their tests pin them. Nothing blocking — the PR is ready. One finding worth a look: the catch that now cites the "empty set with an active stream" invariant by name exposes exactly that combination through its own dispatch order — and the ordinary last-URI unsubscribe does too, pre-existing. Two nits after it.

Verified

• The gate is exact, not merely conservative. refreshModernSubscription bumps modernListenGeneration synchronously at :4769, and generationBefore is read synchronously immediately before the call — so nothing can interleave between the read and the bump, and === generationBefore + 1 is precisely "nothing else advanced it since". The tightening from <= is right: <= before was unreachable (the bump always happened) and > before + 1 is the supersession case. Both bumpers are named in the comment, and the second one (resetSubscriptionStream:1485) really does make the rollback wrong for the reason given — it clears the set, so a stale catch would delete a URI a new session holds.
• reconcileModernStreamStateAfterFailedRefresh closes a real stuck state. With no throw source in refreshModernSubscription other than listen() (closeSubscriptionBestEffort swallows both failure modes, so the two close sites can't propagate), a gated catch always lands with modernSubscription === null. Neither onModernSubscriptionClosed nor onModernReconnectFailed can have run, so nothing else would ever move the badge off the optimistic "connecting" — the commit's description of the both-fail case is accurate, and {active: true, status: "ended"} matches what :4855 already reports for the same shape.
• The two new tests are load-bearing and fail for the right reason. In "ends the badge when both overlapping subscribes fail" the superseding call's own gate passes (G+2 === G+1+1), so it rolls back RESOURCE_URI_2 and reconciles; the first call's gate then fails (G+2 ≠ G+1), so RESOURCE_URI survives. Attaching expect(first).rejects before the supersession is what keeps that rejection handled — same discipline as the raw-wire test.
• The round-54 test hygiene holds. The unhandledRejection listener is registered inside the try whose finally removes it, and the filter on reason distinguishes the two arms ("(sync)" vs "(async)" don't cross-match) as well as a foreign rejection.
• The whole stack still holds. One client.connect(this.transport) call site (:1775) with both registrations before it (:1767-1768); six capability flags all derived at :728-736 from the built capabilities object, every gate reading a flag; resolveModernLogLevel is the single derivation, paired between resetSessionState:1549 and the web re-seed (App.tsx:1237, :2360); cleanRoots' two Keep: guards intact.

1. The catch cites the "active stream, empty set" invariant, then dispatches through it

inspectorClient.ts:5015-5017:

this.subscribedResources.delete(uri);
this.dispatchSubscriptionsChange();               // ← set may now be empty…
this.reconcileModernStreamStateAfterFailedRefresh(); // …state still says active/"connecting"

resetSubscriptionStream goes out of its way to avoid exactly this (:1489-1492):

Announced only once both have moved: a listener that ran between them would see an empty set with an active stream — the combination this helper exists to prevent, so its own dispatches must not expose it.

And the comment 15 lines above this dispatch names the same combination as the harm the gate exists to avoid. So on the last-URI rollback, a resourceSubscriptionsChange listener that reads getResourceSubscriptionStreamState() synchronously sees the state the file calls impossible. The fix is a one-line hoist — delete → reconcile → dispatch — which mirrors resetSubscriptionStream's own order (state first, then the set). The reverse intermediate ("inactive stream, non-empty set") is the benign direction and is already exposed by the optimistic add at :4983.

Not observable through today's consumers, which is why this is a finding and not a defect: ResourceSubscriptionsState keeps its own copy of each axis, updated from two separate client events, and React batches both setStates in one turn, so no render sees the pair.

The same violation is on a happy path, pre-existing: unsubscribeFromResource:5050-5053 deletes the URI and dispatches before awaiting the refresh that sets INACTIVE — so unsubscribing the last resource exposes empty-set-plus-active-stream on every successful call, not just on failure. That's unchanged from origin/v2/main, so it's legitimately out of scope; but it means resetSubscriptionStream's claim is a local guarantee rather than the global invariant its wording implies. Worth either fixing both orderings or scoping that sentence, since three separate comments in this PR now lean on it.

Fix this →

2. The reconcile gives up on the first failure — the only "stream gone, URIs live" path that never retries (optional)

Every other route to status: "ended" with a non-empty set is either an expected close (local/graceful at :4848) or a give-up after the cap (onModernReconnectFailed:4946). The new site reaches it after a single failed listen(), with a non-terminal status and URIs still subscribed — so those subscriptions are dead with nothing armed to recover them, and in the both-fail case the surviving URI was never honored by any server at all while the UI shows it subscribed.

The user's cheapest remedy doesn't work either: clicking Subscribe on that URI hits if (this.subscribedResources.has(uri)) return (:4977) and silently succeeds. Recovery needs an Unsubscribe→Subscribe toggle.

scheduleModernReconnect() in the non-empty branch would reuse machinery that already fits: a user-initiated refresh reset modernReconnectAttempts to 0, so it starts at the base delay; the timer bails on terminal status or an empty set; and the cap lands on the same "ended" badge. That turns "displayed subscribed, never honored, no retry" into either a stream that makes it true or an ended badge after a real attempt. It would need the two new tests' expectations updated ("reconnecting" then a recovery/give-up), and the error still reaches the caller either way.

Genuinely a judgment call — the current state is honest, and arguing scope discipline on a PR this long is reasonable. Raising it because the closeSubscriptionBestEffort doc's own caveat ("the bump proves a newer refresh had started, not that it succeeded") is what leaves that URI unhonored, and this is the one branch positioned to do something about it.

Fix this →

3. if (this.modernSubscription === null) in the reconcile is provably dead (nit)

:4892. Both call sites gate on generation === generationBefore + 1, and the only throw inside refreshModernSubscription is listen() — reached only after this.modernSubscription = null at :4773. The superseded-discard close() can't propagate (closeSubscriptionBestEffort catches both failure modes) and its branch requires the generation to have advanced, which the gate excludes. So under the gate the field is always null, the method's own doc asserts as much ("a failed re-listen leaves modernSubscription null with URIs still subscribed"), and the false arm is permanently uncovered — with silence as its behaviour, i.e. the stale badge this commit fixed.

AGENTS.md's policy for a provably-dead defensive guard is a justified /* v8 ignore … -- <reason> */ rather than leaving it unmarked; the alternative is dropping the condition, since the callers' gate establishes it. Either beats a branch no test can reach.

Fix this →

4. Also checked, nothing to raise

• The closeSubscriptionBestEffort doc's three bullets still match the code after the gate change: bullet 2's harm ("a non-empty subscription set with no stream") is still what an escaping close would produce, and bullet 3 now correctly says both user-initiated callers gate their failure handling rather than calling one of them the mild site.
• The reconcile duplicates onModernSubscriptionClosed's ended literal rather than sharing it (active: true hard-coded inside a size !== 0 branch vs size > 0) — equivalent, and the doc says the reuse is of the vocabulary rather than the code. Fine.
• No consumer reads both axes synchronously — ResourceSubscriptionsState tracks subscribedUris and streamState off two separate client events and useResourceSubscriptions holds two useStates, so finding 1 has no rendered symptom today.
• resetSessionState's five-member contract still pairs with disconnect(), and modernLogLevel is the documented paired (re-derive / blank) member rather than a duplicate.
• Test placement. The two new cases sit in modern stream internals with the other gate/badge tests, which is where the file's grouping puts them; the unsubscribe one among subscribe cases is consistent with that block's existing mix.

On thirty-four rounds

The two newest commits are the same discipline the chain has been running on, applied inside the subscription cluster: edb01fcc found that a rollback's premise was falsified by supersession, and 2a3e002d found that the resulting gate left a state nothing would ever correct. Finding 1 is the next step out — the invariant those commits cite is honoured by the helper that names it and not by the two sites that lean on it, one of them on a happy path that predates the PR.

This checkout has no node_modules, so I couldn't run the suite; the reported gate results are taken as reported. Everything above is from reading the tree at 2a3e002d and diffing origin/v2/main...HEAD.
· branch v2/fix-roots-handler-race

[cliffhall]

All three taken, in e4b8692e. Full gate green. Wrapping the review thread here — the last two rounds have been closing nits on comments about fixes, and the code is converged.

1. The catch dispatched through the invariant it cites

Correct, and the placement made it worse than an ordinary ordering slip: the comment fifteen lines above names that exact pair as the harm the gate exists to avoid, and then the rollback announces the emptied set with the state still reading connecting. Reordered to delete → reconcile → dispatch, matching resetSubscriptionStream's state-then-set order.

I took the happy-path half too rather than scoping the sentence. You're right that it's pre-existing, but three comments in this branch now lean on that invariant, and "local guarantee" is a weaker thing to be leaning on. unsubscribeFromResource now moves the stream to inactive when it removes the last URI, before announcing — the re-listen sets the same state on arrival, so this only closes the round-trip-wide window.

Asserted from the consumer's side, as you suggested: a resourceSubscriptionsChange listener that reads the stream state synchronously, over both sites, with expect(seen.filter(s => s.size === 0 && s.active)).toEqual([]). Verified load-bearing against the un-hoisted unsubscribe.

2. Taken — the reconcile now retries

Your framing settled it: every other route to "stream gone, URIs live" either expects the close or has exhausted the cap, and this was the one that had made no attempt at all — over a URI that, in the both-fail case, no server ever honored, with the user's obvious remedy (click Subscribe again) silently early-returning.

scheduleModernReconnect() in the non-empty branch, as you sketched. It fits without modification: a user-initiated refresh already zeroed modernReconnectAttempts so it starts at the base delay, the timer bails on terminal status or an emptied set, and the cap lands on the same ended badge. So the state becomes true or ends after a real attempt. The caller still gets its error — the retry is about the subscriptions, not the call. Both tests updated to assert reconnecting and, for the overlapping case, recovery to acknowledged.

3. The dead guard

Resolved by construction rather than annotated — with the non-empty branch now calling scheduleModernReconnect(), there is no modernSubscription === null check left to be permanently true.

Gate

validate ✓, coverage ✓, verify:build-gate ✓, smoke ✓, Storybook ✓ (108 files / 460 tests).