| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Cliff Hall
|
4e4aa49db1 |
Merge pull request #2037 from modelcontextprotocol/v2/feat/1906-auth-token-url-overrides
feat(auth): Auth URL and Token URL overrides in Server Settings |
||
|
cliffhall
|
d830213796 |
fix: address round-2 review — stale disconnect, lint-project gap, coverage
Copilot round 2 (PR #2039) surfaced no new visible comments but six suppressed ones. Four were actionable. `clients/tui/src/App.tsx` — a stale-completion race in the new banner: start disconnecting server A, switch to B, then let A's disconnect reject, and A's failure appears under B. Rapid same-server retries had the same shape, and the stale attempt is the one most likely to reject. Claim a monotonic attempt token before awaiting and publish only if it is still current and the server has not changed; `handleConnect` bumps the same counter so a connect retires an in-flight disconnect. The rejection is still owned when superseded — just not published. The regression test drives a deferred rejection across a server switch, and fails without the guard. `tsconfig.lint.json` — the root type-aware block matches `test-servers/src/**/*.tsx` but the parser project omitted that extension, so such a file would have failed with "not found in any of the provided project(s)" instead of being linted. Added, and AGENTS.md now states the invariant: the include must stay a superset of the lint globs. `clients/web/server/{server,vite-hono-plugin}.ts` — the `open()` rejection branch was untested, and duplicated. Both were fixed by removing the duplication: `openBrowser` now lives beside `printServerBanner` in `web-server-config.ts`, with one implementation to test. Two copies of a one-line `.catch` is two chances for one to drift back to a bare `open(url)`. The dev plugin is not booted in tests — it returns early under VITEST by design — but it calls the same tested helper. `core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close path, the entire reason the helper exists, had no test. Exported it (as this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`) and covered both legs, asserting no unhandledRejection fires. Also corrects AGENTS.md, which said each client lists "both" tsconfig projects — true for cli/tui/launcher, but web lists four leaf projects. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
364d8c9dbc |
feat(auth): per-server authorization and token URL overrides
Adds two optional per-server fields — `oauth.authorizationUrl` and `oauth.tokenUrl` — that override the endpoints authorization-server metadata discovery resolved, so a server can be pointed at a development or staging authorization server without changing what it advertises. SDK v2 routes both endpoints through the discovered metadata document and neither through OAuthClientProvider, so the override is applied by wrapping the client's base fetch and patching the metadata document in flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather than only the auth fetch — also covers the discovery the SDK runs from inside the transport on the 401/refresh path. The fields round-trip through mcp.json, the /api/servers routes, the runner (CLI/TUI), and the Server Settings → Authorization panel, where a value that is not an absolute http(s) URL is flagged inline; at runtime such a value is dropped with a single warning rather than failing the connection. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
Cliff Hall
|
0f31de83af |
Merge branch 'v2/main' into v2/fix/1909-salvage-malformed-list-items | ||
|
Cliff Hall
|
23847a61c7 |
Merge branch 'v2/main' into v2/fix/1909-salvage-malformed-list-items | ||
|
Cliff Hall
|
9c4356fb8c |
Merge branch 'v2/main' into v2/feat/2018-custom-authorization-params | ||
|
cliffhall
|
55734f3776 |
fix: accessible names for the authorization-parameter rows (#2018)
Copilot round 2 (suppressed comments). Five of the seven findings were the accessibility of the new row list and one stray cast; all are addressed here. - The reserved-key reason is now the key input's `error` **string** rather than a bare boolean plus free-standing text. Mantine renders it in the input's own error slot and wires the `aria-describedby` association, so a screen reader learns *why* the field is invalid; a boolean only set `aria-invalid`. This also deletes the duplicate `ReservedParamError` text and its Stack wrapper. - Each control carries a per-row `aria-label` — the section heading and the placeholders are not programmatically associated with the inputs, so the key box, the value box, and a remove button announcing only "X" were indistinguishable to assistive technology. The label names the row by its key, falling back to "row N" while the key is still blank. - Dropped an unnecessary `as unknown as Record<string, unknown>` in the servers-route integration test: `MCPConfig.mcpServers` is already typed as `Record<string, StoredMCPServer>`, so the entry's `oauth` reads directly. Three tests cover the new behavior (accessible names, the blank-key row-number fallback, and the aria-describedby association), each verified to fail against the pre-fix component. No change to the reserved-key rule or the EMA boundary. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
77ff30bbc9 |
fix: stop the exclusion scan writing the rendered list's report (#1909)
Copilot round 10. The scan no longer publishes its malformed entries into the tools/list report at all. Round 9 only stopped it overwriting the aggregate's; the remaining case is unsalvageable for a stronger reason than 'two different requests': - a FAILED strict aggregate runs salvage, which writes the report itself; - a SUCCESSFUL one means every entry parsed, so the rendered list contains no malformed entries. So by the time the scan finds something, either the aggregate already reported it with indices matching the screen, or the aggregate succeeded and what the scan found is provably not in the list being displayed. Publishing could only ever name entries the user cannot see. Nothing is lost: listToolsForScan still marks its own refused response, so the finding surfaces on the Protocol entry for the exchange that carried it, where it is true. The per-page accumulation and raw-entry offset existed only to feed the removed publish, and go with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
efd0bf2b7c |
fix: let the aggregate own the tools/list report, and harden the scan (#1909)
Copilot round 9. The exclusion scan is a separate, uncached tools/list exchange, so its entries need not be the ones rendered — the server can answer differently, and a cache-served aggregate is a different list version. It was overwriting the aggregate's report wholesale, replacing accurate indices with indices into a list nobody is looking at. The aggregate now owns the report; the scan publishes only when the aggregate left nothing, which is the cache-served case that made it publish at all (round 2). A conforming re-fetch no longer swallows the mark: the list can change between the strict page and the lenient one, and a clean second response says nothing about the first, which was refused. It is marked with the original decode error when there is nothing per-item to name. The scan's fallback re-fetch now goes through invokeMcpClient, like the strict call beside it and like salvageList's walk, so an auth challenge on that request can still be recovered instead of being swallowed by listAllTools's best-effort scan catch. Also fixes listTools dropping an empty-string cursor (truthiness rather than !== undefined) — its own fallback leg already preserved it, so the two legs of one scan page disagreed about which page they had fetched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
7db807b0de |
fix: return the era-parsed tool, and hide aggregate reports in paginated mode (#1909)
Copilot round 8. The era contract validated with the wire schema but returned the raw input, so the wire schema's normalization was discarded — both era schemas are z.object, which strips unknown top-level fields as the strict codec does, so salvaged tools could carry fields the strict path drops. The parsed output is now what gets normalized and returned. This also simplifies the bridge: issues come straight from the wire parse with their paths intact, so the custom-issue forwarding (and its early describeIssue application) is gone. The malformed-entry report is written by the aggregate walk's salvage, but in paginated mode the tools/prompts/resources panels render the paged stores, which neither write nor clear it — leaving a stale warning above a freshly fetched page it does not describe, with indices into an aggregate the user is no longer looking at. Those reports are now suppressed in paginated mode; resource templates keep theirs, having no paged store. Suppressing rather than adding per-page salvage: pointing at the wrong entry costs more than staying quiet, and per-page salvage is a feature, not a display fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
dbda70795b |
fix: make the salvage item contract match each era in both directions (#1909)
Copilot round 7. The era contract was derived from the neutral ToolSchema plus one override, which diverged from the real wire schemas both ways. Measured against SDK 2.0.0 on a real connection: - modern accepts `inputSchema.properties: 5` and `required: [1]`; ours rejected them, so a malformed sibling made salvage DROP a conforming tool and report it as malformed — a false accusation from the screen whose job is to say which entry is wrong. - legacy rejects `outputSchema.properties: 5` / `required: [1]`; ours accepted them, readmitting entries the strict path refused. - modern rejects a non-string `inputSchema.$schema`, legacy accepts it; ours accepted on both. Each era is now expressed as its own wire schema, bridged to Tool by `eraToolItemSchema`. The bridge exists because both eras accept values the exported Tool type cannot describe (the SDK returns them typed as Tool anyway), so the wire schema's output type is irrelevant and z.custom<Tool> supplies the caller's type — which is what avoids an `as unknown as` here. Forwarded issues keep their path so the report still names the field. The full measured matrix is pinned table-driven in listSalvage-era.test.ts: each row asserts the SDK's verdict AND that our contract agrees, so a divergence that closes or widens fails loudly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
67972adc4e |
fix: bound the salvage walks and mark the scan's refused response (#1909)
Copilot round 6. Bound both fallback walks with LIST_MAX_PAGES, which is now also passed to the SDK client as `listMaxPages` so the strict aggregate and its fallback share one value by construction. A server emitting endlessly UNIQUE cursors never trips the repeated-cursor guard, so the salvage re-walk and the SEP-2243 exclusion scan would page and accumulate without bound. Hitting the cap aborts rather than returning a truncated list as a complete one. Mark the exclusion scan's refused response on every fallback-failure path. `salvageList` can rethrow bare because ManagedListState marks from the error it receives; the scan has no such backstop — listAllTools catches and logs a failing scan by design — so an unmarked rethrow left a response the client demonstrably refused rendering as a clean exchange. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
b25ece8ded |
feat: per-server custom OAuth authorization-request parameters (#2018)
Real deployments gate IdP selection, prompt behavior, or audience on a query
parameter the OAuth/OIDC core specs never standardized (Keycloak's
`kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's
`audience`), and none of them could be driven from the Inspector.
The Inspector never builds the authorize URL — the SDK's `auth()` does, and it
exposes no hook for extra parameters — so the merge happens at the one seam
that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`.
It lives in `core/`, so web, CLI, and TUI all inherit it.
- `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`,
`code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`,
`response_type`, `scope`, `state`), the key validator the form renders
inline, and the merge itself. Reserved keys are dropped with a warning
rather than silently overriding PKCE / the state binding / RFC 8707;
blank-key rows are skipped; the caller's URL is never mutated.
- Applied to the authorization request only — nothing wires it into the token
request.
- Persisted as `oauth.authorizationParams` (a string record) on the per-server
config beside `scopes`, edited as key/value rows in Server Settings ->
Authorization ("Additional authorization parameters"), with an inline error
per offending row plus a section-level warning.
- The enterprise-managed (EMA) leg redirects to a *different* authorization
server (the enterprise IdP), so it goes through the explicit
`redirectToExternalAuthorization` entry point, which skips the merge — a
`kc_idp_hint` or `audience` meant for the MCP server's AS must never be
appended to the IdP's authorize URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
71cbe27d83 |
fix: address Copilot round 5 on the list-salvage path (#1909)
Both findings are the same shape: the fallback accepting what the strict path
would not.
- `ModernResultEnvelopeSchema` typed `ttlMs` as any number, where the codec
requires a non-negative integer. `ttlMs: -1` (or `0.5`) plus one bad entry
would salvage, reporting the entry and hiding the envelope violation. Tightened
to `z.int().min(0)`.
- The item schema was the SDK's era-NEUTRAL `ToolSchema`, and the negotiated
era's wire schema is stricter in ways the SDK does not export. Verified
against SDK 2.0.0 on a real connection: on legacy, a tool with
`outputSchema: { type: "array" }` is refused by the strict aggregate but
accepted by the neutral schema; on modern, `execution` is STRIPPED by the
strict path but kept by the neutral one. Either way a salvaged list could show
— and let the user call — a tool the connection's era says isn't valid.
`toolItemSchemaForEra` closes both. Replicating rules that live in the SDK's
internals is a real cost; `listSalvage-era.test.ts` is what makes it safe to
pay, pinning each rule to the strict path's OBSERVED behavior so a divergence
that closes or widens fails loudly instead of rotting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
8356033d58 |
fix: strip the enum null sentinel, and make the preset test cover its wiring
Copilot round 4: no new visible comments, two in the suppressed block. Both
valid.
1. The `type: [T, "null"]` encoding keeps its keywords at the top level, so a
nullable enum written that way carries the null *inside* the list:
`{ type: ["string", "null"], enum: ["envio", "recebimento", null] }`. The
collapse left it there, which broke both renderers in different ways — the
web dispatcher handed `null` to Mantine as option data, and the TUI's
all-strings check rejected the whole enum and fell back to a plain text
field. Neither is the dropdown the schema asks for.
The collapse now strips a `null` member whenever it flattens, since that
fact has already moved onto `nullable`. An enum of nothing but `null`
drops out entirely: it offers no value a dropdown could show, so the plain
widget serves the field better than an empty select. Covered in core and
end-to-end in both dispatchers.
2. The new integration test claimed in its own header to pin the registry and
config wiring, then called `createNullableFieldsTool()` directly — so a
misspelt preset case or a config naming a dead preset would have left it
green. A fair "this does not test what it says" catch. It now resolves the
checked-in `nullable-fields-http.json` through `loadConfig` →
`resolveConfig`, matching `duplicate-tool-names.test.ts`, and asserts the
resolved tool names before booting the server.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
573b14e14e |
fix: address Copilot round 4 on the list-salvage path (#1909)
Two findings, both about the fallback accepting more than it should. - The lenient page schema validated pagination and nothing else, so a response carrying a malformed entry AND an unrelated top-level violation salvaged: the entry was reported and the other violation vanished with the strict error that caught it. The page schema is now DERIVED from the method's real result schema with only its item array relaxed (`lenientListPageSchema`), so every other field — `_meta`, `nextCursor`, anything a future revision adds — is still validated. The modern era envelope the raw-wire path bypasses (`resultType`, `ttlMs`, `cacheScope`) is checked explicitly for the same reason. Salvage is also narrowed to `InvalidResult`: `UnsupportedResultType` means the codec doesn't recognize what KIND of frame arrived, so it isn't a list with a bad entry and reporting an item-level reason would hide a protocol failure. Marking the Protocol entry still applies to both, hence two predicates. - On every rethrow path the re-walk had already answered the method, so the caller's own `markResponseRejected` (in `ManagedListState.refresh`) landed on the lenient page that succeeded and left the refused response clean. The correlation is restored before rethrowing — deleted when nothing had answered before, so the outer mark is a no-op rather than a lie. `nextCursorOf` / `rawItemsOf` narrow the page's fields instead of casting: the runtime-keyed `extend()` loses the shape, and asserting it back would be exactly the unjustified cast the last round removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
34ba8256a4 |
fix: address Copilot review round 3 — typed consts, null admission, preset coverage
Three findings (one inline, two in the suppressed block), all valid.
1. `toConstOptions` still stringified every non-null `const`, so
`items: { anyOf: [{ const: 1 }, { const: 2 }] }` submitted `["1"]` where
the schema says `[1]`. Same wrong-type-on-the-wire problem that keeps a
numeric `enum` off the select path, so it gets the same answer: only a
string `const` is selectable, anything else falls to the JSON editor
where the value keeps its type.
2. `hasMissingRequiredFields` derived null admission from the *renderer's*
collapse, which is narrower on purpose — it only flattens a two-member
union. So `anyOf: [string, number, null]` renders through the JSON
fallback (where `null` is typeable) yet was still marked missing,
disabling submit on a value the schema accepts. Null admission is a
validity question, not a rendering one, so it is now its own predicate:
`admitsNull` in core, recognizing `nullable`, `type: "null"`,
`type: [..., "null"]`, and a null branch anywhere in an `anyOf`/`oneOf`
of any size.
3. Nothing exercised the `record_shipment` preset — the web and TUI tests
build the `anyOf` shape by hand, which verifies the renderers but
*assumes* the premise the whole fix rests on: that Zod's `.nullish()`
emits `anyOf: [<branch>, { type: "null" }]` with the enum on the branch.
A Zod output change or a registry typo would leave every unit test green
while the documented reproduction server quietly stopped reproducing.
Added an integration test that boots the preset over HTTP, asserts the
wire shape, and runs it back through the collapse — following the
`duplicate-tool-names.test.ts` precedent, which exists for the same
reason (#1957).
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
aea3f6d4c2 |
fix: address Copilot round 3 on the list-salvage path (#1909)
The visible review was clean; all five findings were in the suppressed block, two of them real defects. - The exclusion scan's aggregate offset was wrong twice over: `excluded.length` counts only invalid-header tools, and on page one it also folded in the current page's valid count. A reported index therefore pointed at a position that doesn't exist — worst for an unlabeled entry, whose index is all the user has. Replaced with a raw-entry offset advanced by each page's valid + malformed counts. - The scan's own refused response was never marked. It is a distinct exchange from the aggregate's, and the lenient re-fetch moves the method correlation off it, so its Protocol entry rendered as a clean success the client had in fact refused. Marked with the captured id, as `salvageList` already does. - Three double casts in the test fixture are gone: `ListPage` now models raw wire data (`items: unknown`, `nextCursor?: unknown`), which is what a server built to emit spec-forbidden shapes actually serves. Declaring the validated type and casting malformed values through it asserted the opposite of what these tests exist to check. The cursor map narrows to `string` at the one place a cursor addresses a page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
1889e04135 |
fix: address Copilot round 2 on the list-salvage path (#1909)
All four findings taken (one visible, three in the suppressed block), plus the
`excludedTools` sibling leak the round-1 reply had deferred:
- `rawItemsOf` read a missing or non-array list member as an empty page, so a
top-level violation (`{ tools: "invalid" }`) on a later page was discarded
and the call returned a silently truncated list. It now returns `undefined`
for "not a list at all", and the walk rethrows the original strict error.
- A failure of the re-walk ITSELF (a malformed `nextCursor`, a connection lost
mid-walk) escaped in place of the original error, whose response is the one
`rejectedResponseId` was captured for. The walk is now wrapped: the original
surfaces, the secondary is logged so it stays diagnosable.
- The exclusion scan's lenient fallback discarded its malformed entries despite
the comment claiming they were reported. Observable when the aggregate is
served from cache: the salvage never runs, so nothing else reports them. The
scan now returns them and `refreshExcludedTools` reports them — only ever
adding, so a clean scan can't clear what the salvage path just wrote.
- An empty-string cursor is a valid cursor, and `cursor ? {cursor} : {}` dropped
it, re-requesting page one and duplicating its entries until the
repeated-cursor guard fired. Both walks now test `!== undefined`.
- `excludedTools` had the same session leak fixed for `malformedListItems` last
round: cleared only on an explicit `disconnect()`, so the `onerror`-with-no-
`onclose` route carried the previous server's set into the next connection.
Cleared on the start-clean path too — safe because every `listAllTools`
recomputes it, so this can only drop a stale set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
54c6088114 |
fix: address Copilot review on the list-salvage path (#1909)
Three real defects it caught, plus the tests it asked for: - The strict `tools/list` aggregate is filtered by the SDK for invalid `x-mcp-header` annotations (SEP-2243), but the salvage path was not: one ordinary malformed tool would trip the fallback and readmit every invalid-header tool beside it. Salvaged tools now go through the same scan. - `markResponseRejected` resolves "the last response received for this method", and the re-walk answers that same method — so the mark landed on a salvage page that succeeded while the actually-invalid response rendered clean. The correlation id is now captured before the re-walk. - The report was cleared only on an explicit `disconnect()`. The route out that tears down nothing (`onerror` with no `onclose`) carried it into the next connection, so it is now cleared on the start-clean `resetSessionState()` path as well. Fixing the first exposed two more, both found by the new test: - The modern (2026-07-28) codec validates a result against the spec schema before the caller's, so a lenient `client.request` is rejected exactly as the strict one was — there was no salvage on the modern leg at all. It now goes through the raw-wire channel that exists for the modern `tasks/*` methods. `withModernEnvelope` is split out of `withModernTaskEnvelope` so a list request doesn't claim the tasks extension the user may have disabled. - A malformed tool also blanked the SEP-2243 excluded set, leaving a tool that vanished for an invalid header with no explanation — #1909's own complaint one level down. The scan walk now tolerates a malformed sibling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fe5b622fd1 |
fix: salvage a list result's valid entries when one is malformed (#1909)
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.
`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.
Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.
`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
1ea1e30f63 |
fix: address Copilot review — real latch coverage, consistent docker docs
- The `retriedDynamic` latch test never made the retry fail: a real `listen(0)` always succeeds, so it produced one EADDRINUSE and passed with the latch deleted. Mock `node:http` so every listen fails, asserting exactly two listen attempts and a settled result. Verified by mutation: removing the latch now hangs the test instead of passing it. - The migration guide's Docker notes still described a dynamic sandbox port and told readers to pin a custom one, contradicting the new fixed 6275 two sections later. - Narrowed "or any container" in the troubleshooting entry, which overstated the scope and contradicted the PR's own scope note. It now names the IPv4-forwarder case and explicitly excludes the official image and a plain loopback -p. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
adde9ccf94 |
fix: bind loopback by address and pin the sandbox port so containers work
Two defaults that each break the Inspector inside a container. #1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted to the NAME `localhost`, and `server.listen(port, host)` resolves a name through `dns.lookup` and binds the SINGLE address it gets back. Since Node 17 the default result order is `verbatim`, so on a glibc Linux host — whose /etc/hosts maps localhost to both 127.0.0.1 and ::1 — that is `::1`. The server bound IPv6 loopback only and refused every IPv4 client, including the dev container port forwarder. It presents as an endless spinner rather than a connection error because the forwarder accepts on the host side before failing to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit HOST=localhost is still honored as typed. #2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox listened on an OS-assigned port, so it changed every run and could never be named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an OS-assigned port (loudly) if it is taken — the collision a dynamic default never had. Verified in the reporter's exact image (mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox. Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both defaults in the web README, the migration guide, and the root README's Docker section. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
b70aed6914 |
test(fix): probe a single-source route when waiting for the dead transport
Review feedback on #1987: `waitForDeadTransport` polled `/api/mcp/send`, which answers `transport_error` from two branches — the `isTransportDead()` short-circuit and the `catch` around a rejected `send()`. Only the first implies the transport is marked dead, so the stop condition could not tell them apart from the response alone. That was safe in practice: `StdioClientTransport.send` rejects only when `_process` is undefined, and the transport clears `_process` and calls `onclose` in one synchronous block, so the `catch` branch is not observable before `markTransportDead()` has run (confirmed empirically — 0/25 probes returned early). But the safety rested on an SDK-internal ordering this test neither states nor controls, and if it changed the send test would silently cover the `catch` branch instead of the short-circuit it names — a regression with no failing run attached to it. Poll `/api/mcp/auth-state` instead. For a session connected without an authState there is no auth provider, so `setAuthState` always throws and `transport_error` there has exactly one source: the dead-transport short-circuit. The route never awaits a transport response, so it cannot hang either. Same cost, exact condition, no dependence on SDK internals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks |
||
|
cliffhall
|
24d89d7d0b |
fix(test): wait for the dead transport instead of sleeping 300ms
connectDeadSession spawned a real node subprocess whose only job is to die,
then slept a flat 300ms for the death to propagate. That budget has to cover a
cold spawn(), node's startup, its exit, the parent observing the close, and
onclose marking the session dead — comfortable on an idle box, not under CI
contention or a machine running several agents.
When the budget was missed the session was still live, so isTransportDead() was
false and /api/mcp/send fell through to `await responseWait` for a tools/list
reply the dead child can never send. Nothing bounds that wait, so the test hung
to the project's full 30s ceiling. The timeout IS the symptom of losing the
race, which is why it never presented as a clean assertion failure.
Poll the observable condition instead. Two design points that are load-bearing:
- The probe is a NOTIFICATION (method, no id). requestIdForSendWait returns
undefined for it, so the route never awaits a response and cannot hang.
Probing with a request would reproduce the exact bug being fixed.
- Not /api/mcp/events, which looks like the natural "transport died" signal:
opening that stream on a dead transport calls sessions.delete(sessionId),
so the send under test would then answer 404 instead of transport_error.
Both terminal branches of the route answer transport_error — the
isTransportDead() short-circuit and the catch around a send() that threw on the
dead pipe — and either proves the subsequent real send cannot reach the hanging
path, so both are valid stop conditions.
Verified by reproducing the bug deterministically: with the wait reduced to 0
(the budget always missed) the test times out at 30s; with the poll it passes
10/10. Fixing the helper rather than the test also covers its second call site,
the auth-state test, which sits on the same race and had simply not lost it yet.
connect-crash.test.ts:147 is deliberately left alone. #1985 proposed giving it
the same treatment, but readSseEvents stops at transport_error, which arrives
whenever the child dies — so its 200ms sleep cannot hang, and changing it would
add risk for no gain.
Closes #1985
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
Cliff Hall
|
9e074aa6f2 |
Merge pull request #1946 from modelcontextprotocol/v2/fix/firefox-sse-stream-hang
fix: prime SSE streams so Firefox resolves the events fetch |
||
|
Cliff Hall
|
75f4597281 |
Merge pull request #1967 from modelcontextprotocol/v2/fix/modern-listen-stream-opens-on-listchanged
fix: open the modern listen stream whenever the filter is non-empty (#1920) |
||
|
Cliff Hall
|
53b619db09 |
Merge pull request #1958 from modelcontextprotocol/v2/fix/unhandled-rejections-progress-tests
fix: settle the deliberately in-flight tool calls in the progress tests |
||
|
Cliff Hall
|
f404d50af7 |
Merge pull request #1963 from modelcontextprotocol/v2/fix/proxy-protocol-version-header
fix: forward the negotiated Mcp-Protocol-Version through the remote proxy |
||
|
cliffhall
|
0f44288c26 |
Merge branch 'v2/main' into v2/fix/tool-row-keys
Both sides added a showcase config row and section after the pagination entry; kept both, structured output first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
8049b4f7c8 |
test: cover the structured-output cap and the list_items showcase end to end
Two gaps from the Copilot review on #1975: - The `Large` story asserted nothing, so dropping the section's `mah` (or applying it to the wrong element) would still pass CI. Its play function now measures the real ScrollArea viewport: the visible height stays at the cap and the overflow is scrollable rather than clipped. - Nothing exercised the `list_items` fixture, so a preset-registration typo would leave the documented showcase broken with a green suite. A new integration test loads `structured-output-http.json` through loadConfig/resolveConfig — covering the file, the registry entry, and the fixture — connects, calls the tool, and asserts both the summary block and the nested structuredContent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
2a4e93c621 |
fix: only announce the connection if it is still live after the stream open
Copilot review round 4: every await before the `connect` dispatch is a window for a `disconnect()` or a transport close to overtake the connect, and the listen round-trip widened it. Announcing anyway restarts every managed list refresh against a session being torn down or already dead. The dispatch is now gated on `status === "connected" && !disconnecting` — the second because an explicit teardown claims ownership before it awaits `client.close()`, leaving the status untouched until that block finishes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
b2dfd4099a |
docs: correct three comments the connect-ordering change invalidated
Copilot review round 3 (no new code findings, three stale-comment ones): - `openModernListenStreamOnConnect`'s generation guard was justified by a `connect` listener racing it — which the previous commit made impossible by dispatching `connect` after this call. The guard is still right, for reasons that survive the reorder: `statusChange` has already fired, and a concurrent `subscribeToResource` or `disconnect()` (whose `resetSubscriptionStream` bumps the generation) can supersede it. - `reconcileModernStreamStateAfterFailedRefresh`'s JSDoc still described its branches as "nothing subscribed" vs "URIs live"; they are the empty vs non-empty *filter* now, which differ exactly on the listChanged-only path. - The superseded-failure test carried the same stale `connect` rationale. Comments only; no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
d08a53027a |
fix: dispatch the connect event after the listen stream is up
Copilot review round 2: opening the stream after `dispatchTypedEvent("connect")`
left a real gap. The managed list states start their initial `refresh()` from
that event, so `tools/list` could go out ahead of `subscriptions/listen` — and a
list the server changed in that window would notify nobody, leaving the UI
stale with no way to notice.
The handler registration, the initial logging level, and the stream open now all
run before the `connect` dispatch, so no consumer can act on the connection
until the notification channel is established (or its retry armed). Costs one
listen round-trip on a modern connect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
|
||
|
cliffhall
|
eda8ec0db8 |
fix: guard the connect-time listen failure on the refresh generation
Copilot review: `openModernListenStreamOnConnect`'s catch reconciled unconditionally, unlike the subscribe/unsubscribe call sites. The `connect` event is dispatched before the stream opens, so a listener subscribing to a resource can start and acknowledge a newer refresh while this one is still awaiting its `listen()` — and reconciling anyway would arm a reconnect that tears down a healthy stream. Apply the same generation test. Also consolidates the tests' private-member access onto the file's existing `internals()` helper (one justified double cast, hoisted and documented) and makes the failure stub bump the generation the way the real method does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
0325d81493 |
fix: open the modern listen stream whenever the filter is non-empty (#1920)
On the modern (2026-07-28) era every server→client notification rides the `subscriptions/listen` stream, but the Inspector only ever opened that stream from a resource-subscribe click. A tools-only server advertising `tools.listChanged` therefore had no path to an open stream, so `notifications/tools/list_changed` could never arrive — even though `buildSubscriptionFilter()` already modelled the opt-in. The trigger now matches the filter it builds: the stream opens (and stays open, and reconnects) whenever the built filter carries anything — subscribed URIs or an enabled list-change opt-in the server advertises. That mirrors the SDK's own `ClientOptions.listChanged` auto-open, which opens on a non-empty config ∩ capability intersection. `connect()` opens it once the handlers are registered; a failure there is handed to the reconnect machinery rather than failing the connect. `ResourceSubscriptionStreamState.active` deliberately keeps its narrower meaning — "at least one URI is subscribed" — because it drives the Subscriptions section's badge, which has nothing to report for a listChanged-only stream. That also preserves the invariant the rest of the file is written against: an empty subscribed set is never announced alongside an `active` stream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
ddfce0f7ac |
fix: reject a non-string protocolVersion, and drop the test double casts
Copilot review follow-ups: - `applyProtocolVersion` took the value straight from an unvalidated JSON body and went to `RegExp.test`, which coerces — `123` and `true` both matched the token pattern and reached `Transport.setProtocolVersion` with the wrong type. It now takes `unknown` and checks `typeof === "string"` first, with cases for the coercible values. - Replace `as unknown as Transport` in the new session fixtures with a `makeTransport()` helper returning a structurally valid `Transport`, so an interface change stays visible to the tests. - Drop `as unknown as typeof fetch` in the new transport test — `vi.fn<typeof fetch>()` is already the right type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
cliffhall
|
7a74b5eba1 |
fix: forward the negotiated protocol version to the remote backend transport
The web client's SDK Client owns the `initialize` handshake, so the `setProtocolVersion()` call it makes after initialize lands on RemoteClientTransport — a pass-through to the Node backend, not the real HTTP transport. The backend's StreamableHTTPClientTransport therefore never learned the negotiated version and stamped no `Mcp-Protocol-Version` on any request after initialize, starting with `notifications/initialized`. A stateful server that requires the header rejects that notification with HTTP 400. RemoteClientTransport now records the version and rides it on every `/api/mcp/send`; the backend applies it to its transport before the send, so that request and everything the transport issues afterwards (the standalone SSE GET, the session DELETE) carry the header. The version is validated against a token pattern on the backend so a client can't push arbitrary bytes into an upstream header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
cliffhall
|
e328da43b5 |
test: cover the duplicateToolNames server option over a live connection
The ToolControls regressions build their `Tool[]` by hand, so they would still pass if the new server option broke — config loading, resolveConfig, the ordering, the title marker, or its composition with pagination. Copilot flagged exactly that gap. Adds an integration test that connects a real InspectorClient to a real streamable-HTTP test server and asserts the wire shape: repeats appended (not adjacent — the ordering the defect depends on), only the appended copy marked "(duplicate)", an unregistered name ignored, an empty list a no-op, and duplication running before the page slice so a pair straddles a boundary. The last case loads `duplicate-tool-names-http.json` through loadConfig + resolveConfig, so the JSON plumbing the manual repro uses is covered too. Verified non-vacuous: mutating withDuplicates to place repeats adjacent fails four of the five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
347173fab7 |
fix: absorb only the teardown's own close, not any ConnectionClosed
Review feedback (round 3). The predicate absorbed *any* CONNECTION_CLOSED, while the doc comment claimed it absorbed only the teardown's — so a transport dropping on its own after the progress notifications but before `disconnect()` would have been swallowed. These tests assert only on the notifications, so that regression would have passed green. Gate the absorption on a teardown flag owned by the helper, which now returns an `InFlightCall` exposing `disconnectAndSettle()`. Keeping the flag inside means it can't be raised too early (which would silently reopen the hole) and the settle can't be forgotten; call sites also collapse from two statements to one. Verified: a pre-teardown ConnectionClosed now fails the test and stays observed (no run-level error), while a teardown-time one is still absorbed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
c60563b7d6 |
fix: observe the derived promise so an unexpected rejection can't go unhandled
Review feedback (round 2). Narrowing the handler in the previous commit fixed the silent-pass but reintroduced #1947's own failure class on the unexpected path: `then` returns a *derived* promise, and the re-throw rejects that one rather than `call`. The caller doesn't await it until after `disconnect()`, so a rejection arriving while the test is still waiting on progress notifications sits unobserved for seconds and Node reports it as an unhandled rejection. Attach an observer to the derived promise the moment it exists. Nothing is swallowed — it stays rejected, so the caller's `await` still fails the test. Verified both directions with a probe rejecting with a non-ConnectionClosed error and awaiting after a 1.5s gap: without the observer the run reports "Unhandled Rejection" + "Errors 1 error"; with it, the test fails and the run stays clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
0b9aee11a3 |
fix: only absorb the teardown's own rejection in settleInFlight
Review feedback: the blanket rejection handler also converted a genuine `callTool` failure into a pass. That matters here because these tests assert on the progress notifications, which arrive before the result — so a call that emitted all three progress events and then failed would have satisfied every assertion and been swallowed. Narrow the handler to absorb only `SdkError` / `CONNECTION_CLOSED` and re-throw anything else. The handler is still attached at call time, so an unexpected rejection stays *handled* and surfaces as an ordinary test failure at the `await inFlight` after `disconnect()` rather than as another unhandled rejection. Fulfillment is still accepted: whether the call beats the teardown is a race, so asserting either outcome would reintroduce the original bug as a flake. Verified the narrowed predicate is actually exercised rather than dead code — both call sites reject with SdkError/CONNECTION_CLOSED on every run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
1881e5c73f |
fix: settle the deliberately in-flight tool calls in the progress tests
Two progress tests fire `client.callTool()` without holding the promise so they can assert on the notifications the call streams while it is still in flight. `disconnect()` then closes the SDK client, which rejects every pending request with "Connection closed" — with nothing holding the promise that lands as an unhandled rejection, which vitest counts as a run error and fails `npm run ci` at `coverage` even though all tests pass. Add a `settleInFlight()` helper that attaches the handler at call time (not after the assertions, which would leave a window the rejection can escape through) and returns a promise the test awaits after `disconnect()` so teardown stays ordered. Neither outcome is asserted: the call may legitimately complete before the teardown or reject with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
Cliff Hall
|
a09f2dcf1d |
fix: load @napi-rs/keyring lazily so an unsupported platform degrades instead of crashing (#1905) (#1948)
* fix: load @napi-rs/keyring lazily so an unsupported platform degrades `@napi-rs/keyring` ships one prebuilt binary per platform triple and throws on import where it has none. On Android / Termux there is no `@napi-rs/keyring-android-arm64`, so the static top-level import in `core/auth/node/secret-store.ts` threw during module evaluation and the Inspector exited at startup with "Cannot find native binding" — before any of the keychain-unavailable handling in that same file could run. Replaces the static import with a cached dynamic one. The outcome is cached, not just the module, so a box without a binary doesn't re-attempt (and re-throw) resolution on every secret operation — `expectedSecretFields` means that would otherwise be once per server per `GET /api/servers` — and so `set` can name the underlying cause in its `KeychainUnavailableError`. An unloadable package now folds into the same degradation contract as an unreachable keychain: `get` returns null, `delete` and `deleteAllForServer` no-op, and `set` throws `KeychainUnavailableError` (not double-wrapped — the load failure is already typed by the time the catch sees it). The class doc comment now enumerates all three ways "unavailable" can arrive, since each escaped the contract at some point. The shared `vi.mock` stub can't express this — it models a keyring that loads — so the new tests build on `vi.resetModules()` + `vi.doMock` with a throwing factory and re-import the module, covering the load failure, each method under it, and the load-once caching. Verified the dynamic import survives bundling in all three clients (cli, tui, and the web runner) rather than being inlined back to a static one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F * fix: steer a missing native binding to a reinstall hint `KeychainUnavailableError` gave the Linux libsecret / gnome-keyring advice for every cause, including the one where the keychain is fine and the @napi-rs/keyring platform package is what's missing — an unsupported triple (#1905) or npm's optional-deps bug dropping it on a supported one (npm/cli#4828, the Windows report on #1852). Detect the loader's "Cannot find native binding" phrasing and point those users at a reinstall / npx-cache clear instead. Ported from the superseded #1943, which fixed this alongside the same lazy-load change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: name a bad keyring namespace instead of "is not a constructor" The lazy import accepted whatever the resolution produced. The named exports arrive through CJS interop, so a resolution that stopped yielding them — a default-only export upstream, a bundler changing interop, a platform where named-export detection fails — would hand us `undefined`, and `new mod.AsyncEntry(...)` would throw a TypeError inside the very try that implements graceful degradation. Shape-check the resolved module and treat a bad shape as unavailable. Scope, because it is narrower than it first appears: this does NOT prevent a bad shape from emptying the secret list. `get` returns null either way — that is its read-tolerance contract, and the TypeError lands in the same catch a dead keychain does. What changes is the diagnosis. Without the check the only signal is `set` reporting "keyring.mod.AsyncEntry is not a constructor", which reads like an Inspector bug; with it, `set` names the real problem once, at the load boundary, in the same actionable 503 as every other unavailability. Catching the silent-empty-list case itself would need a round-trip against the unmocked package, which nothing in the suite does today. The member access sits inside the try because reading a missing export is not always a harmless undefined — a Proxy-backed namespace can throw, as vitest's module mocks do — and letting that escape would reject the cached promise, which is otherwise guaranteed to always resolve. Of the four new tests only the cause-message one fails without the guard; the rest pin the surrounding contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * docs: correct the test header's count of unavailability entry points The shape check added a fourth way "unavailable" can arrive, and the enumeration in secret-store.ts was updated to match, but the test file's header still said three — so it read as an exhaustive list that silently omitted the case a whole describe below it covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: give the wrong-shape cause its own remediation hint The shape check added a cause that fell through to the libsecret advice — so a packaging mismatch told the user to install a keyring daemon, which cannot help, and on Windows or macOS points somewhere that does not exist. That is the same wrong-advice bug the native-binding branch was added to fix, reintroduced by the guard itself. Hint selection moves into `hintFor`, where libsecret is the explicit fallback rather than the default, and the shape failure becomes a distinct `KeyringModuleShapeError` matched by type — it is our own error, so there is no reason to re-parse text we wrote (the native-binding branch matches a string only because that message comes from napi-rs). Also retitles the wrong-shape test: it claimed the guard avoids "returning null secrets", but `get` returns null either way by the read-tolerance contract. The title now says what actually differs — `set` hard-fails — matching the correction already made in the comments. Both raised by Copilot review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: give both shape-check failure paths the packaging hint The shape check can fail two ways — the members are absent, or reading them throws — and only the first carried KeyringModuleShapeError. The catch path returned the raw error, so a Proxy-backed namespace throwing on access fell through to the libsecret advice: the same wrong-remedy bug as the previous two commits, one branch further along. Both paths now carry the type. KeyringModuleShapeError takes a detail string plus a standard `cause`, so the throwing path reads "its exports could not be read: <original>" and keeps the underlying failure legible while still earning the packaging hint. Three instances of one mistake in a row (a new cause added without deciding which remediation it inherits) says the shape was wrong, not just the branch: making libsecret an explicit fallback in `hintFor` did not stop me adding a second exit that skipped it. The tests now pin the invariant directly — one asserts both construction paths produce the same hint — so the next cause that forgets is a failing test rather than a reviewer's catch. Raised by Copilot review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
bb219dc001 |
fix: construct AsyncEntry inside the try so keychain degradation engages (#1848) (#1945)
* fix: construct AsyncEntry inside the try so keychain degradation engages `KeyringSecretStore` built its `AsyncEntry` outside the `try` in `get`, `set`, and `delete`. `AsyncEntry::new` performs the platform-store setup (on Linux, the Secret Service connect with a keyutils fallback) and throws when no backend is reachable, so the documented degradation contract never engaged for a construction-time failure: the raw keyring error escaped, 500ing every `GET /api/servers` on a box without a Secret Service (the published container, which has no D-Bus session). `expectedSecretFields` always includes the OAuth slot, so `rehydrateConfig` constructs an entry for every server — the default seeded catalog was enough to 500 the first list load, before any server was added. And because the escaping error wasn't a `KeychainUnavailableError`, it bypassed both the routes' 503 translation and the `migratePlaintextSecrets` skip branch, yielding a generic 500 instead of the actionable message. Moving construction inside the existing `try` restores the contract for that failure mode: `get` returns null, `delete` no-ops, and `set` is the only operation that throws — as `KeychainUnavailableError`, which is what the 503 translation and the migration skip both match on. The test stub's constructor could not fail, which is why the coverage gate never saw the gap. Adds a `constructorThrows` hook alongside the existing method-level `failures` flags and covers all three methods plus `deleteAllForServer` under it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F * test: pin why the OAuth secret field is unconditional `expectedSecretFields` always includes the OAuth slot, and #1848's report frames that as why the 500 fired so broadly — which invites a follow-up that skips the keychain read for servers with "no OAuth config". That change would lose data. `extractSecretsFromStored` deletes the `oauth` block outright when `clientSecret` was its only property, so such a server carries NO marker on disk that a secret exists — the keychain is the sole record, and the unconditional slot is what finds it again. Gating the read on a disk-visible `oauth` block would silently stop rehydrating exactly that shape. The existing tests do fail if the slot is made conditional (verified by applying the change: 4 red), but only under names that explain nothing about the consequence — `always lists the OAuth slot first` reads like a tautology worth updating rather than a trap. Add a round-trip case that states the consequence, so the next reader sees the field is load-bearing rather than defensive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
f201032137 |
fix: surface list-fetch failures instead of swallowing them
A list load that failed — a transport error, or a result the SDK's era codec rejects as invalid — reported nothing. The connect-time refresh is fired with no caller to await it (`void this.refresh()`), so its rejection became an unhandled rejection: the header said "Connected", the panel rendered an empty list indistinguishable from a server that has none, and the failed exchange showed in the Protocol panel as a clean success. Two surfaces now tell the truth: - ManagedListState records the failure as observable state (`getError` + an `errorChange` event) and still re-throws, so App's auth-recovery wrapper keeps working. The four `useManaged*` hooks expose it via a shared `useManagedListError`, and the Tools/Prompts/Resources sidebars render a `ListLoadError` alert with the reason and a Retry. - A response the client refused is attributed back to its Protocol entry (`markResponseRejected` → `responseRejected` → `MessageEntry.clientError`), which now renders an Error status and a "Rejected by the Inspector" alert rather than the green resultType badge alone. The SDK gives no request id with a decode failure, so the id is recovered by correlation; see the comment on `markResponseRejected` for why that is exact. Also stops `listAllTools` from dropping a failing excluded-tools walk on the floor — it stays non-fatal but is logged. Closes #1953 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
9644ac7fe8 |
fix: prime SSE streams so Firefox resolves the events fetch
Firefox does not hand a streaming `fetch()` response to JS until the first *body* byte arrives; Chromium resolves the promise as soon as the headers do. Both SSE endpoints flushed headers immediately and then stayed silent until there was something to report, which deadlocked `/api/mcp/events`: `RemoteClientTransport.openEventStream()` awaits that fetch *before* the MCP client sends `initialize`, so no `initialize` -> no event to report -> no body byte -> the fetch never resolves. The web UI hung on "Connecting..." forever with nothing in the UI, the console, or the node logs. Write an inert `:` SSE comment the instant each stream opens. Conforming parsers ignore a comment line, so this unblocks the read without inventing a wire event and needs no client-side protocol change. `X-Content-Type-Options: nosniff` was also proposed on the issue but does not fix it -- verified against Firefox 153, the fetch still never resolves with the header set and no body byte. The delay is not MIME sniffing. The priming write is deliberately ordered *after* each handler registers its consumer (the session event consumer; the file-watch subscriber plus `ensureWatcher()`). Callers treat the arrival of the stream's first bytes as proof they are subscribed, so priming first would hand out that proof across an `await` and drop an edit made in the gap. `/api/servers/events` gets the same treatment: its fetch was equally stuck on Firefox until the first real change event, which for an unedited `mcp.json` never comes. Its client-side reader counted any `\n\n` as a change, so the inert frame would have fired a spurious background re-fetch on every connection -- it now skips frames carrying no `event:`/`data:` field. The transport's own `parseSSE` already drops comment frames correctly and needed no change. Verified end to end in real Firefox against a live streamable-HTTP test server: before, stuck on "Connecting..." past 25s; after, initialize -> notifications/initialized -> tools/list, connected. Closes #1858 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
Sam Morrow
|
5d91d4acd0 |
fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1847)
* fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1846) A strict modern (2026-07-28) HTTP server (e.g. the GitHub MCP Server) rejects a tools/call whose argument carries an `x-mcp-header` annotation when the matching `Mcp-Param-*` request header is missing (-32020 "header mismatch"). The Inspector never sent that header. The SDK only mirrors inside `client.callTool()` (and skips it entirely in a browser environment), but the Inspector routes tools/call through `client.request()` for manual MRTR driving (#1704), so the SDK never mirrors for us — on any transport. On top of that, the remote (web) transport dropped per-request headers before they reached the backend's upstream fetch. - core/json/xMcpHeader.ts: port the SDK's non-public `buildMcpParamHeaders` + value encoding (Base64 sentinel for non-ASCII/whitespace/empty values) and add `mcpParamHeadersForTool`. - InspectorClient.attemptToolCall: on a modern connection, build the headers from the tool's declarations + call arguments and attach them to the tools/call request options. `Protocol.request` forwards `headers` to the transport and preserves them across MRTR retry legs, so the direct StreamableHTTP transport applies them. Fixes CLI/TUI. - RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest: forward per-send headers and apply them to the backend's upstream transport.send. The browser can't set cross-origin headers, but the Node backend can — where the Inspector's real request is issued. Fixes web. Verified end-to-end against the live GitHub MCP Server (modern HTTP). Closes #1846 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: harden backend header allowlist, fix breadcrumbs, trim comments Address Copilot review on #1847: - server.ts: restrict client-supplied per-send headers to the `Mcp-Param-` prefix (string values only) via `mcpParamHeadersOnly` before applying them upstream, so the per-send channel can't inject arbitrary headers (e.g. Authorization). `settings.headers` remains the channel for configured upstream headers. Covered by new unit tests. - Fix stale `#1810` breadcrumbs → `#1846` in the two new tests. - Trim the verbose mirroring comments in inspectorClient/types/remote transport. - Drop the redundant base64-on-the-wire integration test (encoding is unit- tested; wire delivery is proven by the verbatim test), remove two unneeded test casts, and cover the boolean `true` branch. Not changed (declined with reason on the PR): buildMcpParamHeaders remains a faithful port of the SDK's helper and does not re-validate values against the declared JSON Schema type — arg types are enforced by schema validation on both sides, and matching the SDK's conforming wire output is the module's goal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: mirror Mcp-Param-* on the task-augmented tools/call too Address the maintainer review on #1847: - `callToolStream` (the "Run as task" path) built its own request options and never mirrored, so a task-augmented `tools/call` on an annotated tool was still rejected by a strict modern server with -32020. It now mirrors like the plain path. - Extract the two blocks both `tools/call` entry points had duplicated — `convertStringToolArgs` (the string-arg coercion) and `applyMirroredParamHeaders` (the SEP-2243 mirroring) — so the paths can't drift again. That duplication is why the task path was missed. - Regression test: `callToolStream` against the modern `get_weather` server asserts the POST carries `Mcp-Param-City` (verified failing without the fix). - README: the modern-network showcase claimed mirroring is skipped in the browser so the web client gets -32020, and that CLI/TUI mirror. Both are now wrong — the Inspector mirrors itself, on every client. AGENTS: note that xMcpHeader.ts now also builds the wire headers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t * docs: correct the stale "browser omits Mcp-Param-*" copy Both statements predate this PR and are now the opposite of true, per the wire capture on #1847 (proxy-observed upstream headers, web leg included). - ToolDetailPanel: the mirrored-headers note claimed the browser omits `Mcp-Param-*`. It now says the web client's headers are applied by the Node backend that issues the upstream request. - createRemoteFetch: the header comment claimed an `x-mcp-header`-annotated tool is "uncallable from the web client against a strict modern server" — the exact wrong conclusion to reach while debugging a -32020. It now records that the Inspector builds the headers itself, and that they do NOT travel this `/api/fetch` proxy path: they ride the transport's per-send `headers` through `/api/mcp/send`. Copy only — no test asserted the old note text (ToolDetailPanel.test.tsx matches the section title only). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd |
||
|
Cliff Hall
|
01df9d81bb |
fix(core): start OAuth on a modern/auto-era connect (#1805) (#1809)
* fix(core): start OAuth on a modern/auto-era connect (#1805) The SDK's `server/discover` negotiation probe runs before anything else on `protocolEra: "auto" | "modern"`, and its classifier reports whatever the transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved to `data.cause`. That buried both connect-time auth signals: the remote path's `AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the authorization URL) and a direct transport's `AuthChallengeError`. An OAuth-protected server that authorized fine on the legacy era instead produced a dead-end "Version negotiation probe failed" — no redirect, and the connection status flipped to `error` despite `isConnectAuthRecoveryError` existing to prevent exactly that. - `findNestedAuthError()` walks `cause` / `data.cause` for either typed error. Not gated on the SDK's error code or message, so it survives rewording. - `connect()` unwraps the rejection at the earliest point, ahead of `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the outer status guard, and every client's connect error handling — so web, CLI, and TUI are all fixed without a client-side change. - On the direct path, enable `interceptAuthChallenges` for the probing eras even with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely, no status and not even a cause for the walk to find. Verified against a live dual-era server that answers 401 on both `initialize` and `server/discover`: `auto` and `modern` now reject with `AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the fix is load-bearing — reverting either fails the new integration tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om * docs(core): mark the era-probe challenge interception as a removable workaround Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as #1807). Make its lifespan explicit so whoever picks up #1807 knows this block is the thing to delete, rather than having to re-derive it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): resolve connect() when auth recovery is satisfied silently Review findings on #1809: - connect() built its handshake promise once, outside the runConnect closure withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg re-awaited the already-rejected promise and rejected a connect() whose client was in fact connected. Short-circuit the retry when already connected, and skip the post-connect block the nested connect already ran (a duplicate `connect` event re-triggers every list-state manager's refresh). - Document the accepted 403 side effect of enabling interceptAuthChallenges with no stored tokens, and why every `{ pin }` counts as probing. - Replace the private-method double cast in the era-probe test with an observable assertion on the transport options, and split the non-auth failure case per era instead of a message alternation that passed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): key the connect retry leg off attempt, bound nested recoveries Second-pass review findings on #1809: - The satisfied-recovery short-circuit keyed off live connection status, so an onclose landing between the nested connect() and the retry leg fell through to the rejected handshake promise and reported the stale 401 as the reason the session died. withDirectAuthRecovery now passes `attempt` to its operation, so connect() identifies the retry leg structurally and reports the teardown when the recovered session is already gone. - Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a satisfied outcome recovers via a nested connect() with a fresh counter, so a server that keeps rejecting freshly refreshed tokens recursed without bound. Counted across the nesting boundary, capped at 3. - Strengthen the regression test (server info populated by the nested connect, no duplicate statusChange) and add the two missing cases: a failed reconnect still rejects, and the recursion bound terminates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * test(core): cover the recovery teardown branch; fix a duplicated case Third-pass review findings on #1809, both about tests added in the previous commit: - The retry leg's teardown error had no coverage, so a regression to the old status fall-through would have stayed green. Covered deterministically: the nested connect dispatches `connect` after setting "connected", so closing the live transport from that listener flips the status before the retry leg runs. Asserts the error identity, which is the point of the change. - "still rejects when the reconnect underneath the recovery fails" used a challenging transport for every connect, so it walked the same bounded chain as the recursion-bound test rather than covering a non-auth reconnect failure, and its comment claimed an "error" status the auth path never reaches. Now fails the reconnect with a plain error and asserts both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: rinormaloku <rinormaloku37@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
c2af547cc8 |
fix(core): register peer-request handlers before connect (#1797) (#1798)
* fix(core): register peer-request handlers before connect (#1797) The roots/sampling/elicitation capabilities are advertised on the SDK `Client` at construction, so from the moment `connect()` sends `notifications/initialized` the server may issue any of those requests. The handlers, however, were registered after the handshake resolved and after `fetchServerInfo()` — leaving a window in which the SDK answered `-32601 Method not found`. `server-filesystem` asks for `roots/list` the instant it is initialized (that is how it learns its allowed directories) and lost the race, so it silently fell back to its CLI-argument directories instead of the roots configured in the Inspector. `server-everything` asks later and did not. Extract the four peer-request handler blocks into `registerPeerRequestHandlers()` and call it before `client.connect()`. None of them depend on server capabilities — only on constructor-set state. The notification handlers that do gate on `this.capabilities` stay in `connect()`, after `initialize`. Regression test drives the race deterministically with a fake transport that delivers `roots/list` synchronously from inside the `send()` of `notifications/initialized`; it fails on the pre-fix ordering. Closes #1797 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-1 fixes — pre-connect roots/list_changed, wider timing test (#1797) Finding 1: `notifications/roots/list_changed` was the one handler in the post-handshake block that gates on nothing but constructor state, so it sat in the same race — a server emitting it right after `initialized` had it silently dropped (no wire error; the symptom is a missed UI refresh). Moved to a new `registerPeerNotificationHandlers()` called next to `registerPeerRequestHandlers()` before `connect()`. Finding 2: the timing test asserted one of the four moved handler blocks. It now injects `tasks/list` and the `roots/list_changed` notification in the same burst as `roots/list`, so moving any of the block back after `connect()` fails the test. Sampling/elicitation stay out — they park a pending request awaiting user input and have no reply to assert on. Dropped the unused `sessionId`/`setProtocolVersion` transport fields. Verified the added assertions are load-bearing: the notification assertion fails against the previous commit, the `roots/list` one against `origin/v2/main`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-2 fixes — correct the roots/list_changed premise (#1797) The round-1 doc comment claimed a server may emit `notifications/roots/list_changed` at initialize. It can't, normally: `RootsListChangedNotificationSchema` is a member of `ClientNotificationSchema` and absent from `ServerNotificationSchema` — the client sends it (setRoots does, at :4251). The inbound handler is defensive coverage for a non-conformant server, and dispatches `rootsChange` with our own already-known roots. Reworded the method doc and the test's header so neither states the false premise; the move itself stands (it gates on no server capability, so it belongs with the request handlers). Also from review: - Dropped the vacuous `notificationRejected` flag and its inverted comment. The SDK does not throw for an unregistered notification, so the try/catch could never observe one — confirmed by bisect, where the pre-fix failure was always `rootsChanges`, never the flag. Removing the catch also stops it swallowing a genuine throw from the injected requests. - Narrowed `replies` to responses (`"result" in message || "error" in message`) so it can't also collect client-originated requests. - Annotated the unreachable `if (!this.client)` guard with a justified `v8 ignore` per the AGENTS.md dead-guard policy. Re-verified both regression directions: the `roots/list` assertion fails against `origin/v2/main`, the `rootsChange` one against `55e25b7c`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-3 fix — drop stale comments inside the roots handler (#1797) The four inline comments moved verbatim with the handler contradicted the method doc reworded one round earlier: they said "server's roots" (they are ours — it is a ClientNotification) and "we'll just dispatch an empty array", which is not what `this.roots || []` does when roots are configured. The new timing test asserts exactly that non-empty dispatch. Replaced with one accurate line. Comment-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(cli): advertise roots so roots/set is actually serviceable (#1797) Round-4 review found the same -32601 symptom on a second path: the CLI constructed its InspectorClient with no `roots` option, so `capabilities.roots` was never advertised and no `roots/list` handler was registered — yet `--method roots/set` calls `setRoots()`, which announces `notifications/roots/list_changed` to the server. A server taking up that invitation got "Method not found", after the CLI reported success. The review's suggested fix — register `roots/list` unconditionally — is not implementable: the SDK asserts the client capability inside `setRequestHandler` ("Client does not support roots capability"), so it throws during connect for any client built without the option, and `registerCapabilities` refuses to run after connect. A client that omits `roots` therefore cannot legally serve `roots/list` at all. Fixed at the layer that can fix it: `cli.ts` seeds `roots: []`, matching web (`App.tsx` always passes it). The core registration stays gated on the constructor value, now with the SDK constraint documented so the next reader doesn't retry the unconditional version. `setRoots()`'s doc claimed it "will enable roots capability if it wasn't already enabled" — it cannot, for the reasons above. Corrected, and dropped the dead `if (this.roots === undefined) this.roots = []` the very next line overwrote. Tests: a CLI test drives `list_roots` over an HTTP test server (verified load-bearing — without the seed it fails on the -32601 error text), and the core timing test gains a case pinning that roots set after connect are served with the current values, not the constructor's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(cli,tui): advertise the roots configured in mcp.json (#1797) Round-5 review: seeding `roots: []` fixed the -32601 but not the outcome. `server-filesystem` discards an empty roots answer (`validatedRootDirs .length > 0`) and keeps its CLI-argument directories — which is the symptom this PR set out to fix. The user's roots were already in hand two lines away, on `serverSettings.roots` (lifted from mcp.json by `mcpConfigToServerEntries`). Now passed through `cleanRoots`, the same shared helper web uses, so both clients answer `roots/list` byte-identically for the same config. This matters most on the CLI because it is one-shot: `--method roots/set` sets roots on a connection torn down a few statements later, so the config file is the only durable way to give a run its roots. Same gap in the TUI (`App.tsx` built its options with no `roots` key at all), fixed the same way. The review suggested a follow-up issue for it; folding it in here instead, since it is the identical line and splitting it would leave a known-wrong client in tree. Also from review: hoisted `waiters` up with the other fields, and `injectRequest` now rejects after 1s naming the method and id rather than hanging to the vitest timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-6 fixes — document roots in the CLI README, harden cleanRoots (#1797) - clients/cli/README.md enumerated the settings lifted from a catalog/config file without roots. Roots are the least discoverable member of that list — there is no flag, and `--method roots/set` dies with the connection — so the file is the only durable way to supply them. Said so. - cleanRoots() trusted its input's shape. `Root[]` is a compile-time type over hand-editable mcp.json, and as of this PR all three clients feed it straight from disk, so `"roots": [{"name":"Work"}]` or `"roots": "file:///work"` threw at connect. Non-array bails to []; an entry without a string uri is dropped with a warning. Hardened in the one shared place rather than three call sites. Review suggested a follow-up issue; folded in for the same reason as the TUI fix last round. - Test comment said the CLI passes a `roots: []` seed — true one commit ago. - injectRequest clears its timeout on reply (an armed timer outliving a test is the #1760 teardown class), and setRoots' doc drops the "despite what this comment used to claim" archaeology. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-7 fix — cleanRoots also throws on a non-string name (#1797) The round-6 hardening guarded `uri` and left `name` throwing one line later: `r.name?.trim()` guards null/undefined, not "is a string", so `"roots": [{"uri":"file:///a","name":42}]` still died at connect — the same crash class, one field over, reachable from all three clients and the web settings-save path. Reproduced against the shipped function body before fixing. A bad `name` now costs the name, not the root (it is optional, and the uri is still usable), with a warning. The doc no longer overstates what is validated. Also from review: the CLI test comment said "`cli.ts` now seeds `roots: []`" — true only of that ad-hoc test's path; and the registration comment's `(:588)` line reference is now a durable name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): normalize roots in setRoots() too (#1797) Round-8 review: this PR made cleanRoots the single normalizer for roots at connect and at save, but setRoots — the third way roots enter the client — did a bare `this.roots = [...roots]`. Web cleans at its call site; the CLI does not, so `--method roots/set --roots-json '[{"name":"Work"}]'` stored a root with no uri, advertised it in the roots/list reply, and echoed it back as success. cleanRoots is idempotent, so calling it inside setRoots covers the CLI without disturbing web's pre-clean, and all three entry points now agree. Review filed this as an optional follow-up; folding it in for the same reason as the TUI and cleanRoots items — it is smaller than the issue describing it. Test asserts the malformed entry is dropped and the valid one survives; verified load-bearing against the bare-spread version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-9 fixes — normalize roots in the constructor too (#1797) The round-8 commit claimed "all three entry points now agree"; review pointed out the constructor is a fourth, and the only one still raw. All three clients clean at their call site, so nothing was broken — but the invariant was enforced by convention rather than by the class, at the very entry point this whole thread started from. `this.roots` now runs through cleanRoots, with a ternary so the `undefined`-vs-`[]` distinction that gates capabilities.roots survives. Also from review: - The CLI roots/set test provoked a cleanRoots warning without suppressing it (AGENTS.md). Suppressed, and the spy now asserts the drop was reported rather than silent. - `enables roots when previously undefined …` in coverage-backfill was named after behaviour round 5 established does not exist. Renamed, with a comment on what that client actually can and cannot do. - setRoots dispatches a copy to rootsChange, as getRoots() already did, so a listener can't push into the list we advertise. - run-method.test.ts's connectStdio now passes `roots: []` like cli.ts, so the helper matches the client shape production builds. Verified load-bearing: the constructor test fails against the raw assignment. validate / coverage / build-gate / smoke / storybook all green (run as separate stages — full `npm run ci` kept being killed mid-coverage by the environment). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-10 fixes — copy at both rootsChange dispatches (#1797) Round-9 claimed setRoots' copy closed the "a listener can't push into the list we advertise" hole. It closed one of two sites: my reply conflated the notification handler's *dispatch* with the roots/list *request* handler — the latter is serialized to the wire immediately and is fine, the former hands listeners the internal array. Both copy now. Also from review: dropped the `({}) as never` in the new constructor test for the real factory the sibling tests use (this test never connects, so it is never invoked), and the needless `async` on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-11 doc fixes; merge v2/main (#1797) - cleanRoots' opening sentence still framed it as the settings-form normalizer, contradicting the paragraph below it that says every client feeds it from disk. It has four callers now; says so. - Marked the two runtime guards `Keep:` with the reason. The `Root[]` parameter narrows both to `never`, so "the compiler says this can't happen" is the argument someone will use to delete the checks rounds 6-7 installed. The reason now lives where the deletion would happen. - Noted the constructor-normalization case in the timing test's header, so it is discoverable from a file otherwise about the initialized race. Also merged origin/v2/main (#1796, loopback binding), which had advanced by one commit and touched the smoke scripts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): clear queued peer requests when connect() fails (#1797) Round-12 review found a second-order consequence of the handler move. Before it, registration happened after every await in connect(), so the moment a peer request could be queued the connect could no longer fail — queue and failure path never overlapped. Registering before the handshake puts both the handshake and the post-connect `setLoggingLevel` inside that window. The catch sets status to error and drops the transport but never cleared pendingSamples/pendingElicitations; only disconnect() does, and connect() reaches it solely on the connect-timeout path (not the default). So a server that elicits at `initialized` and then fails the connect left a live pending-request modal on a dead connection — web derives it from the queue lengths with no status gate — and the elicitation's cancel() never fired, hanging any awaitUrlElicitation waiter. Extracted disconnect()'s clear-and-cancel into clearPendingPeerRequests() and called it from the catch, dispatching the change events so the UI drops the modal. CLI/TUI are unaffected either way (sample/elicit off). Test note: the first version of this test passed with and without the fix — its `requestedSchema` omitted `properties`, so the SDK rejected the inbound params and nothing was ever queued. Fixed the fixture; it now fails without the change with a queued ElicitationCreateMessage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): clear the peer-request queue on a mid-session crash too (#1797) Round-13 review, two items. The test asserted only `getPendingElicitations()`/`getPendingSamples()`, which read the arrays directly — but the arrays are not what removes the modal. `usePendingClientRequests` tracks its own state off the pendingSamplesChange/pendingElicitationsChange events, so a regression that cleared without dispatching would strand a live modal on a dead connection and stay green. Both tests now assert the events fired with an empty payload; verified by deleting just the dispatches, which fails them. The connect-failure fix left the other way a connection ends uncovered: a mid-session crash goes through the transport's `onclose`, which sets status and fires `disconnect` but never touched the queues. Same two hazards, and arguably more reachable. `clearAndAnnouncePendingPeerRequests` wraps the clear plus its events, guarded on a non-empty queue so the paths that overlap (a connect failure whose dropCachedTransport also fires onclose) announce once; called from both. New test queues an elicitation, fires onclose, and asserts the queue and the event — fails without the change. Review flagged the crash path as a follow-up; folding it in, as with the earlier out-of-scope items — it is the same seam and three lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-14 fixes — make both teardown paths agree (#1797) - The crash path's comment claimed "cleared before the `disconnect` event so a consumer sees an empty queue" while disconnect() cleared *after* its own dispatch — an invariant stated as a property and held at one of two sites, the same shape as the round-10 finding. Moved disconnect()'s clear above its status/disconnect block so both paths agree; its change events stay batched with the other teardown dispatches. Full gate green, so nothing depended on the old ordering. - clearPendingPeerRequests' doc still named the `connect()` failure path as a caller; that path now goes through clearAndAnnouncePendingPeerRequests. Names the two real callers. - The mid-session test waited on `await Promise.resolve()` — the microtask-counting pattern this file replaced with injectRequest two rounds ago, working today only because the enqueue happens exactly one tick deep. Waits on the client's own `newPendingElicitation` instead. - The file header still called the constructor case "the last case"; it is third of five, and the two teardown cases went undescribed. Rewritten. - Restored the missing blank line between two tests, and noted why the sampling-event assertion holds for a queue that never held anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-15 fixes — scope the parity claim to the arrays (#1797) Round-14 made the two teardown paths agree on when the queue is cleared, and the comment then claimed parity flatly. It holds for the arrays but not the events: the crash path announces before `disconnect`, while disconnect() batches its change events with the rest of its teardown, so they land just after. Since the events are what drives the modal, the distinction matters to the next reader. Scoped the comment rather than un-batching — un-batching would trade a documented ordering for a real behaviour change with no gain. Same rule as round 14, one level down: check the other site, then check the other axis. Also gave the new `newPendingElicitation` wait the timeout that `injectRequest` documents, so a regression that stops the enqueue fails at the wait instead of hanging to the vitest timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * test: pin disconnect()'s clear-before-announce ordering (#1797) Round-14 moved clearPendingPeerRequests() above disconnect()'s status block so a `disconnect` consumer sees an empty queue on both teardown paths, and round 15 documented it — but nothing enforced it. The existing `disconnect` listeners only count events, and the two teardown tests cover the crash and connect-failure paths, neither of which goes through disconnect(). So the one behavioural change in the stack was untested in both directions, and the tidy that undoes it (regrouping the clear with the batched change events) would have stayed green. Test records the queue length from inside a `disconnect` listener; the fixture's close() never fires onclose, so it exercises disconnect()'s own ordering rather than the crash path. Verified against the pre-round-14 position: `expected 1 to be +0`. Folded the newPendingElicitation wait into a shared helper now that two tests use it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * test: pin disconnect()'s announce as well as its clear (#1797) Round-16 pinned the array half of disconnect()'s teardown and left the events unpinned — and nothing else in the suite covers them, so deleting disconnect()'s two change-event dispatches would have left a live modal after a user-initiated disconnect with the suite green. That is the path most likely to have a modal open when it runs. Asserted after the await rather than inside the disconnect listener, since disconnect() batches the events deliberately; verified by deleting the dispatches (`expected undefined to be +0`). Header rewritten by category rather than position: it has now gone stale twice in three rounds because it counted tests ("the last two") and enumerated paths that a new test falsified. It names the three teardown paths and what distinguishes them instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-18 doc fixes — scope the three-path claim, re-attribute cancel() (#1797) The header I rewrote last round claimed all three teardown paths clear before announcing. True only if "announcing" means the `disconnect` event: the connect-failure path emits none at all, and both it and the crash path clear after their `statusChange`. Scoped to what's true. Same shape as rounds 10/14/15 — a parity asserted in prose that holds on one of the axes it could mean — which is now the fourth time, in a sentence I wrote. ElicitationCreateMessage.cancel()'s doc still credited `disconnect()` teardown; this PR made clearPendingPeerRequests() its only caller, serving three paths. Re-attributed, and promoted the no-`onRemove` note to its own paragraph with the consequence spelled out — it quietly became a three-caller invariant during this PR, and "don't splice mid-iteration" reads like a style note until you know it would skip every other entry and leave those promises unsettled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): gate roots/list on what was advertised, not on this.roots (#1797) Round-19 review found a latent wedge, reproduced before fixing: a client constructed without `roots` that later calls setRoots() has this.roots defined, so registerPeerRequestHandlers tried to register the handler on every subsequent connect() — and the SDK throws "Client does not support roots capability" from setRequestHandler when the capability was never advertised. That throw lands before the handshake, so the client can never reconnect. Worse than the -32601 this PR started from: the connection is gone rather than answering one method badly. Not a regression (the same gate lived after the handshake before) and unreachable from the three shipped clients, which all pass roots. But core/ is a library surface, and the failure is unrecoverable. Gate now reads a construction-time `rootsCapabilityAdvertised` — the thing the SDK actually asserts on — so this.roots stays free to change. setRoots' doc says what does happen on such a client: the roots are stored and readable, but no server can ask for them. Also scoped the header's claim that the connect-failure path emits no `disconnect`: true of connect()'s own catch, but on a real transport dropCachedTransport's close() fires onclose first, which does dispatch one — making the catch's own call the auth-recovery backstop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-20 — derive the roots gate from the advertisement itself (#1797) Round-19's fix restored the invariant but not the derivation: rootsCapabilityAdvertised was computed from `options.roots` alongside the capability block rather than from it, so the two encodings of "did we advertise roots" could drift. Adding any term to the advertisement condition would leave the flag true with `capabilities.roots` absent — the same unrecoverable wedge, reintroduced silently and in the throwing direction. Now read off the built capability object after it is assembled, so divergence is structurally impossible rather than merely absent. Verified the reconnect test still fails against the old `this.roots` gate. Also: named the capability-lifetime category in the test header (the new test was none of the three it listed), moved that test below the teardown group so the group stays contiguous, and rewrapped the over-long comment line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): gate elicitation/create on what was advertised too (#1797) Round-21 review asked round-20's question of the capability next door, and there the two derivations already disagreed — no future edit needed. `elicit` is typed `boolean | { form?, url? }`, so `{}` or `{ form: false, url: false }` is valid, enables no mode, and advertises no `capabilities.elicitation` — while the registration gated on `this.elicit` being truthy. Reproduced before fixing: CONNECT ERROR: Client does not support elicitation capability (required for elicitation/create) Since #1797 moved registration before the handshake, that throw means the client cannot connect at all. Same wedge as the roots one, one capability over, and reachable through a documented option value rather than a future edit. Gated on a new elicitationCapabilityAdvertised, derived from the built capability object exactly as the roots flag now is. Test constructs with `{ form: false, url: false }` and asserts connect() resolves; verified load-bearing. Also made the four construction-time capability inputs (sample, elicit, receiverTasks, advertisedExtensions) `readonly`. The round-19 bug was a mutator making a construction-time fact stale, so a future setter for any of these should be a compile error rather than a review catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): derive tasks.requests and the remaining gates from the advertisement (#1797) capabilities.tasks.requests declares which server→client requests we accept as tasks, but was built from `receiverTasks` alone — so `{ receiverTasks: true, elicit: false }` told the server it could send a task-augmented elicitation/create while advertising no elicitation capability and (since the last commit) registering no handler. A server taking the invitation gets -32601: the advertise-then-refuse shape this PR opened with, and the last place it survived. Now built from capabilities.sampling / capabilities.elicitation, both decided just above, with the key omitted entirely if neither applies. Also made the remaining three registration gates read the built capability object like the two from the last two rounds — sampling, tasks/*, and the URL-elicitation completion handler. None could diverge today, but a mixed convention means the next reader has to re-derive which gates are safe, and adding one condition to an advertisement block is exactly how elicitation broke. Five gates, one rule now. Test asserts a receiverTasks+elicit:false client advertises tasks.requests with sampling and without elicitation; verified load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-23 — pin the omitted-requests arm, drop the count again (#1797) The `requests` key is omitted when neither sampling nor elicitation is advertised, but every `receiverTasks: true` construction in the tree left one of them enabled, so that arm was unexercised — and it would not have shown as a coverage failure, since one uncovered branch in a ~4900-line file sits well inside the per-file gate. Second client in the same test asserts `tasks` survives (list/cancel are still serviceable) while `requests` is absent rather than empty; verified against `requests: taskRequests`. The header regained a numeral four commits after round 17 rewrote it by category to stop exactly that, and it was already stale. Dropped, and named the third category — advertise-only-what-you-serve, which is about the capability object rather than a registration gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): settle queued samples on teardown instead of discarding them (#1797) Round-24 review. Round 13 recorded the samples-vs-elicitations asymmetry as deliberate on two grounds: nothing internal awaits a sample, and the transport is gone by then. The first still holds; the second is false on one path, and it is a path this PR created. `connect()`'s catch drops the transport only when `!transportHasAuthProvider`, so on the HTTP+OAuth path — web's ordinary authenticated case — it is retained and the caller reconnects over it. A sampling request queued during the handshake was then discarded with the connection still live: no response frame ever written, and the server waits forever for a reply to a request we accepted. Accept-then-silence, which is worse for the peer than the -32601 this PR opened with. SamplingCreateMessage.cancel() mirrors the elicitation one — settles (rejects, what the UI's decline path already sends and the SDK turns into an error response) without calling onRemove, since the caller iterates the queue and clears it. clearPendingPeerRequests loops it like the elicitations. Also corrected two comments that assumed the catch always tore the transport down, and noted that the retention is gated on transportHasAuthProvider alone — independently of isConnectAuthRecoveryError, which gates only the status hold. Test asserts settlement rather than a wire frame, since on the disconnect() path the transport is closed before the settle; a later respond() throwing "already resolved or rejected" is the proof. Verified load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): mark receiver-task payload promises handled (#1797) Round-25 review. clearPendingPeerRequests iterates every entry in pendingSamples, and two shapes live there: the plain server-request one, whose reject reaches the SDK handler, and the task-augmented one, whose reject reaches the receiver task's payloadPromise. That promise only gets a consumer when the server polls tasks/result, so a task sitting in input_required has no handler attached — and the settle added last commit surfaced as an unhandled rejection. Not hypothetical: the repo already worked around it test-side, with a helper that reaches into private state to attach no-op catches, whose comment describes this exact race. Fixed at the source instead — one `void payloadPromise.catch(() => {})` at creation, which also covers the pre-existing cancelReceiverTask path. Removed the now-redundant helper and its two call sites. Verified: dropping the source line makes that suite report two unhandled rejections. Corrected SamplingCreateMessage.cancel()'s doc — its "no response frame is ever written" rationale is true of the plain shape only; a task-augmented sample already answered the wire with a CreateTaskResult, so settling it is about not leaving the task in input_required limbo. Nits: generalized the enqueue wait helper to both events so the sampling wait gets the timeout the elicitation one has; moved the sampling teardown test back into the contiguous teardown group; named the settle property in the header; fixed the fixture doc that still claimed to record replies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-26 — drop the comment that outlived its call (#1797) The suppressReceiverPayloadRejections call went last commit; its two-line comment stayed, describing a pre-attach that no longer happens and crediting the test for handling a rejection the source now handles. Removed. Also reworded the surviving .catch() in inspectorClient.test.ts: it is still needed, but as a value capture for the assertion, not for unhandled-rejection suppression. Rewrapped the teardown paragraph in the test header — my round-25 edit left a 104-char line, and two hand-fixes left it ragged, so it is reflowed in one pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-27 — skip the roots/list_changed the SDK would refuse (#1797) Review reported setRoots() emitting roots/list_changed on a client that never advertised roots, inviting a re-fetch we'd answer -32601. Probed it: the premise is wrong. The SDK asserts the notification capability too — `notification()` rejects with "Client does not support roots list changed notifications" and nothing reaches the wire. No spec violation and no invitation; what actually happened is that setRoots provoked that rejection and logged it as "Failed to send", implying a transient failure where the truth is structural. Kept the early return on that corrected basis: it stops us provoking a rejection we know will come, and warns accurately instead. Doc and comment say the SDK is the enforcer rather than claiming this guard is what keeps the wire clean. The test pins the end state (no invitation for roots we can't serve) and passes either way — recorded as such rather than presented as a regression guard, since it isn't one. Also moved the advertise-category test below the two gate cases, matching the order the header introduces them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-28 — gate the notification on the predicate the SDK asserts (#1797) The roots/list_changed guard read `capabilities.roots !== undefined`, but the SDK asserts `capabilities.roots.listChanged` for the notification — wider than the flag. Identical today (roots are only ever advertised as `{ listChanged: true }`), but a conditional advertisement would un-gate it silently, which is the round-21 shape again. Split into rootsListChangedCapabilityAdvertised, mirroring urlElicitationCapabilityAdvertised, which already reads its assertion's sub-field; rootsCapabilityAdvertised keeps the roots/list registration, where presence is what setRequestHandler checks. Also moved the new test out of the gate pair it had split — it is a runtime-emission case, not a registration-gate one — and extended the header's converse-category paragraph to cover notifications, which indexes it without inventing a category. Reflowed the setRoots doc paragraph in one pass (111-char line from the last commit, plus a ragged orphan from an earlier edit). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-29 — chase the corrected premise into clients/cli (#1797) Round 27 corrected "setRoots announces roots/list_changed to a server that then gets -32601" — the SDK refuses the notification from a client that never declared it, so nothing reached the wire. That reword happened in core/, but the same premise had been written into the two comments justifying the CLI's roots seed, and they still asserted it. Both now state the real pre-fix failure: a server asking for roots on its own got -32601, and roots/set could not announce at all. Also fixed the warn message under the round-28 gate: it named the roots capability while the gate is on roots.listChanged — precisely the pair that split was made to distinguish, so a client advertising `{}` would have been told it advertised nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): reject in-flight raw-wire requests on a mid-session crash (#1797) rejectPendingRawWireRequests had one caller — disconnect() — so the crash path settled the inbound peer-request queue this PR added and left the outbound raw-wire map alone. That map holds the modern tasks/* frames the SDK's era gate refuses to route, so the SDK's chained _onclose doesn't know about them: a Tasks-tab poll in flight when the server died waited out its own 30s timeout and reported a timeout for what was a crash, on a connection that had already dispatched `disconnect`. Pre-existing — both the map and the crash path predate this PR. Fixing it here because the invariant it violates is one this PR wrote down: "every path that ends a connection has to clear that queue, announce it, and settle each entry", in clearAndAnnouncePendingPeerRequests' doc and the timing test's header. One channel was true of one path out of three. Test starts a raw-wire request and fires onclose; without the call it hangs to the vitest timeout, which is the symptom. Checked the two siblings and left both: taskInputAbortControllers is already unblocked via the elicitation cancel the crash path performs, and activeToolCallAbortController rides an ordinary client.request the SDK settles itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-31 test fixes — justify the cast, drop the no-op tick (#1797) - The new test's `as unknown as` had no justification, which AGENTS.md explicitly prohibits. It is unavoidable — rawWireRequest is private and every public route is gated on isTasksExtensionNegotiated(), which needs a modern handshake the fixture doesn't perform — so that reasoning is now written down instead of left to be re-derived. - `await Promise.resolve()` was a no-op: rawWireRequest registers its entry in the promise executor, synchronously, before any suspension. Dropped, which also makes the test pin that ordering rather than paper over it — and stops the third recurrence of the microtask-counting pattern this file's own helper doc argues against. - Moved the test into the teardown group and extended the header's teardown paragraph to cover the outbound direction, which it framed entirely as inbound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-32 — separate the outbound teardown claim from the inbound one (#1797) My round-31 header edit spliced the outbound raw-wire passage into the middle of the inbound claim, so "There are three: a failed connect(), a mid-session transport close, and an explicit disconnect()" ended up reading as scoping the outbound settle — which is two by design, since nothing populates the map before the handshake. Round 31 established that scoping so the code wouldn't imply an impossible state; the prose then un-scoped it. Split into its own paragraph with its own scope clause, and reflowed both in one pass rather than nudging breaks. Also raced the raw-wire assertion, for the reason the file's own wait helper documents: without the teardown rejection the test waited out the request timeout and reported a bare vitest timeout. It now fails with "raw-wire request not settled by teardown" — verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-33 — extract the timeout race, mark the failure-path promise handled (#1797) Three copies of "reject after 1s so a regression fails where it happened" had accumulated, each re-explaining itself. Extracted `withTimeout`, used by the enqueue wait and the raw-wire assertion; injectRequest keeps its own since its timeout also deletes the waiter entry. On the failing path the raw-wire test left the production 30s timer armed, so a regressed run got the intended failure plus a stray unhandled rejection up to 30s later — the armed-timer class the file's own comment names, on the failure path. `settled` is marked handled before the race. Verified: with the fix removed the run fails with "raw-wire request not settled by teardown" and no unhandled rejection. Recorded why disconnect() drains the SDK's in-flight requests but rejects the raw-wire map outright: the user asked to disconnect, and a Tasks poll is not worth delaying that for. Pre-existing and deliberate, now written down where the asymmetry is visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-34 — drop a no-op guard and the false mechanism it asserted (#1797) Round 33 claimed the raw-wire test could leave `settled` rejecting unhandled on a regression run, and round 33's fix encoded that as `void settled.catch(() => {})` with a comment explaining it. Both were wrong: `Promise.race` attaches a rejection handler to every input, so `withTimeout(settled, …)` on the next line already marks it handled. Verified with a standalone script — no unhandledRejection fires, and the pre-fix version was equally safe. Removed the line and the comment. The comment mattered more than the line: it told the next reader that `Promise.race` does not mark its losers handled, which is false and would cost them elsewhere. Kept the true half by setting `timeout: 50` on that test's client, so a regression leaves no 30s timer armed and fails with `Raw request "tasks/get" timed out after 50 ms` — more self-describing than the raced generic message. Verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-35 — the drain comment described a trade-off nobody made (#1797) Last round's comment said raw-wire requests "get no share of the drain above: the user asked to disconnect, and a Tasks poll is not worth delaying that for" — framing a decision. Verified it isn't one: `safeDisconnectTimeout` defaults to 0 and the only caller anywhere that passes a value is a single test, so nothing is drained in production; and the drain polls the SDK's `_responseHandlers`, which never holds raw-wire ids, so those were never eligible regardless. Reworded to what is true. The framing came from my own round-33 reply, which described the drain as running on every disconnect — a comment faithfully encoding a half-read mechanism, which is the same failure as the `Promise.race` one it followed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-36 — the race comment outlived the mechanism its own commit replaced (#1797) `timeout: 50` made the request settle in ~50ms, so the 1s race can never win and a regression fails on the assertion (`timed out after 50 ms` vs the expected /Connection closed/) — which the commit message said, while the comment beside it still credited the race. Reworded: the short timeout is what fails fast and names itself, the race is the backstop for a promise that never settles at all. Race kept; it covers the case where the request's own timer is broken too. Third comment in three rounds encoding a mechanism the same commit changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-37 — scope the timeout comment, align the bypass guards (#1797) `timeout: 50` is the client-wide request timeout, not a bound on the raw-wire request; it is inert in that test only because the test issues no SDK request after connect. Noted, so someone adding one later knows it would inherit a 50ms budget rather than reading the comment and looking elsewhere. The two installReceiverTaskResponseBypass guards read `this.receiverTasks` while the five gates around them read the advertisement. Installing the bypass mutates the Protocol's _requestHandlers map, so it is registration — same as the setRequestHandler it follows. Equivalent today; aligned so the convention is uniform rather than something the next reader has to re-derive per site. The genuinely-runtime `receiverTasks` reads are left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-38 — one predicate for the whole bypass mechanism (#1797) Round 37 moved the bypass *install* guard to the advertisement and left the three branches it controls on the raw option, splitting one immutable fact across two sources. The dead direction is the harmful one: if the tasks advertisement ever acquires a condition — the exact edit that produced the elicitation wedge — then advertised goes false while receiverTasks stays true, the bypass is not installed, and the handler still returns `{ task }` into the SDK's validating wrapper, which rejects it. That is the breakage the bypass exists to prevent, and before round 37 it could not happen because install and branch read the same field. All three now read tasksCapabilityAdvertised. `receiverTasks` is left where it belongs: assigned once, and driving the advertisement. Also noted that the wrapper's own check is redundant with its install guard and kept deliberately, since it must agree with the handler branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): don't carry receiver tasks into the next session (#1797) Round-39 review found the last member of the enumeration rounds 12-14 and 31 built: receiverTaskRecords was cleared only by disconnect(). The crash path doesn't clear it, the connect() failure catch doesn't, and connect() never reset it — so a record created in one session survived into the next connect() on the same instance, and `tasks/list` is answered from that map. A new server would be told about a task it never created. Reachable in web (the only client with receiverTasks): a task-augmented sampling/createMessage arriving in the handshake window creates a record; if that connect then fails with a 401, App.tsx retries connect() on the same instance. Cleared at the top of connect() rather than only on the crash path — the auth-recovery retry means ending a session isn't the only way a new one begins, and starting clean covers every route in. disconnect() keeps its clear via the same extracted helper. Pre-existing — the crash path never cleared these — but the handshake window this PR opened is what lets a receiver task exist before connect resolves, and this was the one member left outside the invariant the PR documented. Test verified load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): reset all session-scoped state on connect, not just receiver tasks (#1797) Round-40 review: receiverTaskRecords was one of five collections cleared only by disconnect(). Two of the other four have real symptoms on the same crash-then-reconnect path: - subscribedResources — the modern subscribeToResource early-returns on a uri already in the set, so after a reconnect the user's Subscribe click silently sends nothing to the new server while the UI keeps listing it. - cancelledTaskIds — a leftover id mislabels a *new* task sharing it as cancelled rather than failed, and server task ids are often short. taskInputAbortControllers leaked un-aborted controllers. modernLogLevel had the opposite asymmetry: disconnect() cleared it so it couldn't leak (#1629), but nothing restored it, so a reconnect after an explicit disconnect silently lost the user's configured level while a reconnect after a crash kept a mid-session override. It is now recomputed from serverSettings the way the constructor does, so both routes agree. clearReceiverTasks became resetSessionState, called from the same place. Tests cover the two symptomatic siblings and now assert receiver tasks through the `tasks/list` handler — what a server actually sees, and the symptom itself — which also removed the unjustified `as unknown as` the previous commit added. The remaining cast is justified in place, since cancelledTaskIds has no public reader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(web): re-seed the Logs control when the client re-seeds its own level (#1797) Round-41 review caught a consequence of the last commit. `resetSessionState` restores the client's modern log level from the server setting at connect; `resetSessionScopedUiState` still blanked the web control to null on every disconnect. On the one path that reconnects the *same* instance — auth recovery — the control then read Off while every modern request carried `_meta` logLevel "debug", the default. A tool whose job is showing what you send should not disagree with itself about what it sent. The control now re-seeds from the active server's settings exactly as setupClientForServer does, instead of blanking. Also from review: - Added the tests the restore never had, covering both halves: a mid-session override does not survive a reconnect, and a disconnect no longer drops the configured level (the #1629 half, and the actual behaviour change). - resetSessionState's doc claimed "every piece of state scoped to one connection", which invites assuming membership rather than checking it. Now states the rule: state a new session would misread; what only needs settling on the way out stays with the teardown paths. - Header: session-scoping paragraph moved below the outbound one to match test order, and it now names all five members rather than three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * refactor(core): one derivation for the modern log level (#1797) Round-42 review: the configured level was computed independently in four places — twice in InspectorClient (constructor, resetSessionState) and twice in App.tsx (the seed and the re-seed). The client and the Logs control agreed only because four sites happened to compute the same expression, and any drift reproduces exactly the symptom the previous commit fixed: the control reading one level while every modern request stamps another. Extracted resolveModernLogLevel into core/mcp/types.ts beside DEFAULT_MODERN_LOG_LEVEL; all four call sites use it, with the web mapping undefined→null at its own boundary (display, not a second derivation). Also from review: - disconnect()'s comment still credited itself with preventing the #1629 leak, which resetSessionState now prevents. It says what the line does now — read "not opted in" while disconnected — and notes the web control deliberately shows the configured level in that window. - resetSessionState's doc claimed an un-aborted taskInputAbortControllers entry "leaks a paused poll loop". Both registration sites release in a finally, so nothing leaks permanently; the abort closes the window between a crash and the unwind. Softened, and it now has the test it lacked — the only member of the five without one. - Noted in App.tsx that the re-seed depends on activeServerIdRef still holding the outgoing id (it syncs in a passive effect), so an eager clear would silently drop it to the default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): reset the subscription stream state with the set it derives from (#1797) Round-43 review: resetSessionState cleared subscribedResources but left modernStreamState, so a crash-then-reconnect on the same instance held an empty set alongside `{ active: true }` — a combination the rest of the file treats as impossible, since onModernSubscriptionClosed and onModernReconnectFailed both compute `active` from `subscribedResources.size > 0`, and the comment at :4712 asserts a terminal drop clears both. No symptom today only because the UI mirror resets itself independently, in another file — which is the arrangement this PR has spent its rounds removing. Clearing it here also announces the reset; that set's every other mutation dispatches. Also from review: - The web re-seed treated "no server found" as the default level rather than Off, because resolveModernLogLevel returns the default for absent *settings*. Reachable via a second disconnect after a crash, and via removing the active server. Now distinguishes the two. - Converged the two injectRequest copies: withTimeout takes an optional onTimeout, so both drop their waiter on the timeout path. The newer copy had lost exactly the cleanup that was round 33's reason for leaving the original alone. Test seeds the stream state a live subscription would have left; verified load-bearing (the earlier version passed either way, since the fixture never activates a stream). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(web,core): correct the re-seed branch; share the subscription-stream reset (#1797) Round-44 review. My round-43 fix traded one wrong branch for another: it read `?.settings`, which is undefined both when no server matched *and* when the server has no settings node — and the second is the common case (a hand-written mcp.json entry never opened in Server Settings). Those servers showed Debug while connected and Off after disconnect, which is exactly the client/UI disagreement rounds 41-42 removed, with no modernLogLevelChange event to reconcile it. Now branches on the server. Worth noting no test would have caught it: App.tsx is a documented exclusion from the coverage gate. Also from review: - resetSessionState cleared subscribedResources without announcing it, two lines below a comment arguing that every other mutation of that set dispatches. And it took two statements of a six-statement cluster disconnect() clears together — leaving modernReconnectAttempts (a new session's first drop would back off as if it were the old one's nth), modernListenGeneration (the bump that makes an in-flight re-listen bail), and a stale modernSubscription. One resetSubscriptionStream helper now serves both, announcing both axes; disconnect() keeps only the live stream's best-effort close. That also removed the file's one assignment to modernStreamState that bypassed its setter. - The stream-state test pinned the value, not the dispatch — the half the UI actually tracks. Now asserts both; verified against a bare field assignment. - onModernSubscriptionClosed's comment claimed the disconnect reset always clears the set; on a crash it doesn't until the next connect. Says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-45 — reattach the orphaned doc, order the announcements (#1797) Extracting resetSubscriptionStream last round inserted it *and its doc* between resetSessionState's doc and resetSessionState — so two doc blocks stacked, the new helper took the nearest, and the 23-line block explaining each member's symptom attached to nothing. Dead text where it sat, and the most load-bearing comment in the batch. Moved, not reworded. That is round 32's finding in production code, from the same cause: inserting anything between a doc block and its method is a move that needs checking, prose or code. Also from review: - resetSubscriptionStream announced the cleared set before updating the stream state derived from it, so a listener in between saw an empty set with an `active` stream — the combination the helper exists to prevent, exposed by its own dispatches. Both fields move first, then both are announced. The onModernSubscriptionClosed comment now says that window is never observable, which is newly true. - The test header called the log level "cleared"; it is re-seeded, which is the #1629 half of the test twenty lines below. Says "reset", with the distinction spelled out, and the paragraph reflowed in one pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * fix(core): close the listen stream the connect-path reset drops (#1797) Round-46 review: resetSubscriptionStream's doc claimed disconnect() is the only caller with a stream worth closing, and the helper did a bare `modernSubscription = null`. Not guaranteed on the connect path — an `onerror` without an `onclose` leaves the transport up, connect() reuses it, and the reset then drops the last reference to a stream still open on the server. Nothing can close it afterwards: the `closed` handler bails on the bumped generation, and `closed` never resolves for a live stream. Before the extraction the surviving reference was eventually closed by refreshModernSubscription as `previous`, so this was newly unclosable. The helper now closes it itself, best-effort, after the dispatches so the round-45 ordering is unaffected; disconnect() drops its two hand-rolled lines. Also from review: - The session test pinned the subscription set's value but not its dispatch — the half the UI tracks, and the gap round 43 closed for the stream axis one line away. Both now asserted; verified load-bearing. - resetSessionState's doc said disconnect() "clears all of this", which is true by duplication for two of the five members; it now says a sixth has to be added in both places. And "the teardown paths" handle the in-flight tool call only on the disconnect route — on a crash it is callTool's own finally. Named. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-47 — fix the doc's count, catch the closed-promise chain (#1797) - resetSessionState's doc said "two members through the same helpers, the other two hand-rolled" for a body with five. It is two and three — and the member the enumeration dropped is the one that is *paired* rather than duplicated: modernLogLevel is re-derived here and blanked in disconnect(), deliberately. Both corrected. - `subscription.closed.then(...)` had no rejection handler, unlike every other promise around that stream. Newly reachable: before the last commit a connect-path reset dropped the reference with `closed` permanently pending, so the chain never settled at all; closing the stream is what makes it settle. Added, with the Node-terminates reasoning the file already applies to payloadPromise. - The listen-stream test exercised transport reuse without asserting it — the half that makes "still open on the server" true. Counts factory calls now, so the premise fails loudly if a future change recreates the transport rather than silently ceasing to apply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-48 — scope the catch, guard the close, index the test (#1797) - The .catch comment claimed the connect-path close made a *rejecting* `closed` reachable. It doesn't: closing resolves it. What that close newly reaches is the handler running at all, where the reference used to be dropped with `closed` pending forever. And chaining after `.then` also swallowed handler throws — silently abandoning a re-listen on the path whose job is recovering a dropped stream. Now the second argument to `.then`, scoped to the rejection, with the comment saying which half is which. - `closing?.close()` reaches into third-party code from `disconnect()`'s straight-line teardown, none of which is inside a `try` — a synchronous throw would skip the raw-wire rejects, the task aborts, the receiver-task clear and nine dispatches, silently, since most callers catch a disconnect(). Best-effort now against both failure modes. - Header indexes the listen-stream test: same category, but a release obligation rather than a misread one. - Its comment described the generation guard, which the stub can't exercise; marked as production behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr * review: round-49 — pin the best-effort close, unorphan the doc (#1797) Round 48 made `resetSubscriptionStream`'s close best-effort against both a rejected promise and a synchronous throw, but nothing exercised either arm: both `modernSubscription` stubs in the tree resolve. On the `disconnect()` path an escaping failure would silently skip the raw-wire rejects, the paused task-input aborts, the tool-call abort and `clearReceiverTasks()` — the findings rounds 13 and 30 closed, re-opened at once with a green suite. Two table-driven cases now seed a `close()` that throws and one that rejects, and assert teardown continued past it. Also: the `.then` comment gave opposite verdicts on one mechanism (an unhandled rejection is named unacceptable, then chosen for a handler throw) — reworded to say the handler cannot throw today and why the shape is defensive; and `refreshModernSubscription`'s doc block, orphaned two methods up since before this branch, moved onto the method it describes. Closes #1797 * review: round-50 — sweep the queues start-clean, unify the close (#1797) `resetSessionState`'s doc excluded the peer-request queues and the raw-wire map because "the teardown paths handle them" — true of the paths that run. An `onerror` without an `onclose` runs none of them: it flips status to "error" and leaves `baseTransport` cached, so a `connect()` on the same instance reuses a live transport. That is the route the round-46 stream close was built on, and it strands these two the same way — the web pending-request modal is derived from the queue length with no status gate, so it outlives the session, and answering it writes a response for the old request id onto the new connection. Both are now swept start-clean beside the reset (both helpers are idempotent, so the routes that already ran them are unaffected), the end-clean clears stay put for the `disconnect`-event consumer, and both docs say which is which. Also folds the three subscription `close()` sites onto one `closeSubscriptionBestEffort` helper, so all three absorb a synchronous throw as well as a rejection — at `refreshModernSubscription`'s first site the reference is already dropped, so a throw there both leaks a live stream and aborts the refresh before its replacement is opened. Closes #1797 * review: round-51 — pin the sweep's ordering, name the category (#1797) The connect-start sweep must sit after `resetSessionState()`, and the two calls read as independent. Cancelling a task-augmented peer request settles it synchronously into the record callback, which ends in `upsertReceiverTask` — a no-op only because `clearReceiverTasks()` just emptied the map. Hoisted, it would emit a `notifications/tasks/status` for the outgoing session's task onto the transport the connect is about to reuse. Stated at the call and pinned by a test (which fails on the hoist). Adding that third caller also made five docs stale, all describing the old set by count or as "teardown" — the class round 23 diagnosed. 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. And the site where round 50's fold actually changed behaviour — `refreshModernSubscription`'s first close, where the reference is already dropped and a `.catch()` never caught a synchronous throw — is now tested from the public surface (a re-list over a poisoned stream), table-driven over both arms. Verified to fail on the pre-fold form. Closes #1797 * review: round-52 — say what the sweep prevents, scope the close doc (#1797) The sweep's comment named as its harm the exact write the sweep performs: on this route the SDK's transport is still live, so cancelling a queued peer request emits a response for the old id immediately — which is right, and is the settle-don't-discard rule, at the best available moment. As written it read as an argument against emitting the frame, inviting the clear-without- settling regression rounds 24–25 removed. It now names the real hazard: a *user*-authored answer landing arbitrarily later, past the re-handshake. `closeSubscriptionBestEffort`'s "at each site" held at two of three — the superseded-generation discard stored nothing and has nothing following it, so its escaping failure would only reject an already-superseded `subscribeToResource`. Scoped, still wrapped. Also moves the `disconnect()`-side close pair next to its `connect()`-side twin, and states which axis the file's ordering follows, since route and category both describe it and cut differently. Closes #1797 * review: round-53 — give each close site its real harm, pin it (#1797) The doc grouped `resetSubscriptionStream`'s close with the "takes the caller's remaining work with it" sites. It can't: the call is `void`-ed onto an `async` helper, so even a synchronous `close()` throw becomes a dropped rejection — `disconnect()`'s teardown runs regardless. The harm there is an *unhandled* rejection, which this PR already established (the payloadPromise fix) is the process-fatal class in Node — so the one site with the severe failure mode was documented as having the mild one. Each site now carries its own consequence, and the local comment no longer gives two reasons that cancel each other. The test had the same gap: both its assertions hold with the helper's try/catch deleted. It now also asserts no unhandled rejection fired, which does fail without the guard (verified on both arms); the teardown assertions stay, since they still catch unwrapping the call to a bare `close()`. Also names the two back-references the round-52 move left positional, and softens the "on the wire until dropCachedTransport()" claim — that drop is conditional and never runs on stdio. Closes #1797 * review: round-54 — the mildest site isn't mild from every caller (#1797) `refreshModernSubscription` has three callers, and the superseded-generation discard's failure reaches all of them. From `unsubscribeFromResource` — which deliberately keeps the removal when a re-listen fails — it surfaces "Failed to unsubscribe" for an unsubscribe that actually stuck, which is worse than the consequence the doc assigned to the site it called mildest. The re-listen bullet had the mirror gap: on the reconnect caller the escape is absorbed into the backoff run, not a set left with no stream. Both now name their callers, and the generation-bump claim is softened to what it proves (a newer refresh *started*). Test: the `unhandledRejection` listener is process-wide, so the assertion now filters to this fixture's own reason rather than failing on a rejection some other test left pending; and it is registered inside the `try` whose `finally` removes it, so a throw in the setup can't leak it into the rest of the file. Re-verified the filtered form still fails on both arms with the helper's try/catch removed. Closes #1797 * fix(core): don't roll back a subscribe a newer refresh superseded (#1797) `subscribeToResource`'s rollback assumes what its comment says — that the server filter is unchanged — which is exactly what supersession falsifies. The newer refresh built its filter from the set *including* this URI, so on success the server honors it; rolling back leaves the set missing a URI the live stream carries (the UI showing it unsubscribed while its resources/updated keep arriving) and, if it was the only one, an empty set with an active stream — the combination `resetSubscriptionStream` exists to prevent, reached from the other end. Reachable without any close() failure: a rejecting `listen()` propagates the same way, which a user subscribing while the reconnect timer re-lists — i.e. exactly when the server is flaky — can produce. The rollback is now gated on the generation not having advanced past this call's own bump; the error still reaches the caller either way. Test drives two overlapping subscribes with the first `listen()` rejecting, and fails without the gate. The doc bullet that called this the mild site is corrected with it: both user-initiated callers report a failure that did not happen, in mirrored ways. Also gives the two close-failure arms distinct reasons so the unhandled-rejection filter can exclude the sibling arm as well as a foreign rejection. Closes #1797 * fix(core): reconcile the stream badge when a re-listen this call owned fails The supersession gate left the both-fail case stuck: two overlapping subscribes whose listens both reject leave the superseded URI in the set (its rollback correctly skipped) while the state keeps the optimistic "connecting" — no stream, no reconnect armed (neither onModernSubscriptionClosed nor onModernReconnectFailed ran), so the badge never changes and no resources/updated ever arrive. That is the caveat this file already wrote down for the neighbouring case — the bump proves a newer refresh *started* — applied to the gate itself. Fixed in the branch that knows the answer: the call that still owns the re-listen reconciles the state instead of only handling an emptied set, reusing the "ended" badge onModernSubscriptionClosed already reports for "stream gone, URIs still subscribed". unsubscribeFromResource gets the same treatment — it rolls nothing back by design, but its failed re-listen left the badge reading Listening over no stream. Also: the gate's comment named one of the two things that bump the generation (resetSubscriptionStream also does, from disconnect and the start-clean reset) and cross-referenced the wrong invariant; both corrected, and `<=` tightened to `===`, which is what the monotonic synchronous bump actually guarantees. Closes #1797 * fix(core): order the subscription writes, retry instead of stranding Two sites emptied the subscription set and announced it before the stream state caught up — the last-URI rollback in subscribeToResource's catch, and (pre-existing) the ordinary last-URI unsubscribe, which dispatches a round-trip before the re-listen sets INACTIVE. Both exposed an empty set with an active stream: the pair resetSubscriptionStream orders its own writes to prevent, and which three comments in this branch now cite. State before the announce at both, asserted from the consumer's side. And the reconcile added last round settled for an honest but dead badge: it was the only route to "stream gone, URIs live" that had made no attempt at all, over subscriptions the server may never have honored, recoverable only by an Unsubscribe/Subscribe toggle (a fresh Subscribe early-returns). It now hands them to the reconnect machinery, which already fits — base delay after a user-initiated refresh, bails on a terminal status or an emptied set, and lands on the same "ended" badge past the cap. That also removes the provably-dead null check the reconcile carried. Closes #1797 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
4e4aa49db1 |
||
|
|
d830213796 | ||
|
|
364d8c9dbc | ||
|
|
0f31de83af |
||
|
|
23847a61c7 |
||
|
|
9c4356fb8c |
||
|
|
55734f3776 | ||
|
|
77ff30bbc9 | ||
|
|
efd0bf2b7c | ||
|
|
7db807b0de | ||
|
|
dbda70795b | ||
|
|
67972adc4e | ||
|
|
b25ece8ded | ||
|
|
71cbe27d83 | ||
|
|
8356033d58 | ||
|
|
573b14e14e | ||
|
|
34ba8256a4 | ||
|
|
aea3f6d4c2 | ||
|
|
1889e04135 | ||
|
|
54c6088114 | ||
|
|
fe5b622fd1 | ||
|
|
1ea1e30f63 | ||
|
|
adde9ccf94 | ||
|
|
b70aed6914 | ||
|
|
24d89d7d0b | ||
|
|
9e074aa6f2 |
||
|
|
75f4597281 |
||
|
|
53b619db09 |
||
|
|
f404d50af7 |
||
|
|
0f44288c26 | ||
|
|
8049b4f7c8 | ||
|
|
2a4e93c621 | ||
|
|
b2dfd4099a | ||
|
|
d08a53027a | ||
|
|
eda8ec0db8 | ||
|
|
0325d81493 | ||
|
|
ddfce0f7ac | ||
|
|
7a74b5eba1 | ||
|
|
e328da43b5 | ||
|
|
347173fab7 | ||
|
|
c60563b7d6 | ||
|
|
0b9aee11a3 | ||
|
|
1881e5c73f | ||
|
|
a09f2dcf1d |
||
|
|
bb219dc001 |
||
|
|
f201032137 | ||
|
|
9644ac7fe8 | ||
|
|
5d91d4acd0 |
||
|
|
01df9d81bb |
||
|
|
c2af547cc8 |