| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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> |
||
|
Cliff Hall
|
f9d893f3d7 |
Merge pull request #1986 from modelcontextprotocol/v2/fix/1984-mantine-transition-timer-leak
fix(test): clear leaked timers so one cannot fail an unrelated CI run |
||
|
cliffhall
|
c0e4b89c8d |
fix(test): treat timer handles as opaque; the numeric guard never matched
Review was right, and I had assumed the handle type rather than checking it. Probed both under happy-dom: setTimeout -> typeof "object", constructor Timeout requestAnimationFrame -> typeof "object", constructor Immediate So `typeof id === "number"` in the clearTimeout wrapper never matched, and an explicitly-cleared timer was never untracked. Both `Set<number>` annotations and `const id: number` were fictions TypeScript could not catch, since the DOM lib DECLARES number while happy-dom RETURNS an object. The effect was benign — the teardown sweep re-clears them and clearTimeout is idempotent — but the sharper problem is that the test asserting this passed for the wrong reason. "clearTimeout doesn't throw" holds whether or not tracking works, so it proved nothing and hid the broken guard. Hold handles as `unknown`, route cancellation through two small helpers that carry the one justified cast each, and drop the numeric guards. The test now asserts the net's own bookkeeping through an exported pendingTimerCount(), and is mutation-checked: restoring the numeric guard fails it (1 failed | 6 passed). That is the second test in this PR that passed without proving its claim. Both had the same tell — the assertion would have held with the feature 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
|
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
|
c4e8ed992b |
fix(test): cancel queued frames before sweeping timers
Review found the net still had the exact hole it was written to close. The afterEach is synchronous, so a queued rAF callback cannot run until it returns — at which point it registers a fresh setTimeout AFTER the sweep has already drained. On a file's last test that timer survives environment teardown. That is the rAF race described in the previous commit message, left open by the fix for it. Track animation frames too, and cancel them BEFORE sweeping timers. Order is the fix: JS is single-threaded and nothing between the two loops yields, so no frame callback can run in between. Mutation-checked — without the cancel pass the new ordering test fails (1 failed | 6 passed); with it, 7 pass. Also from review: - renderWithMantine.tsx still documented the disproven env="test" behavior. It is the file contributors are told to use, so leaving it would have preserved the wrong belief exactly where it does most damage. That was the third copy of this claim in the repo; all three now corrected. - Dropped an unjustified `as unknown as` in the test helper, which broke the repo's own double-cast rule. Inference keeps the handle type clearTimeout accepts. 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
|
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>
|
||
|
cliffhall
|
1e0f94bb31 |
fix(test): clear leaked timers so one cannot fail an unrelated CI run
A window.setTimeout that outlives its test file fires after happy-dom disposes that file's window; React's dispatchSetState then throws an uncaught ReferenceError: window is not defined. Vitest attributes it to whichever file was running, so a single leak fails the ENTIRE run with every test passing — landing on an innocent file under a step named "Enforce per-file coverage gate." Seen twice in CI, both on docs-only branches. Not a Mantine bug, and not a badly-written test. Both hooks do clear on unmount (useTransition's clearAllTimeouts, useLockScroll's effect cleanup). It is a rAF race: clearAllTimeouts cancels the *pending* rAF, but the transition schedules rAF -> rAF -> setTimeout, so if the inner callback is already in flight when the unmount lands, cancelAnimationFrame is a no-op and that callback then schedules a timer after cleanup has run. Nothing owns it. Needs a loaded machine, which is why it only ever appeared in CI. Track every timer and clear whatever is still outstanding after cleanup(). The ordering is load-bearing: legitimate unmount cleanups get their turn first, so only true leaks are dropped. Fake timers pass through untracked, which is correct — a fake timer cannot outlive the environment. Also corrects the claim, in AGENTS.md and in setup.ts's own comment, that env="test" prevents this. It does not: env is read only by Transition.mjs at its render branch, while useTransition runs before that check (hooks cannot be conditional) and still schedules real timers. Measured — opening a <Modal> through renderWithMantine schedules three 200ms timers. Believing that guarantee is why this went unexamined. The regression test is mutation-verified: with the net disabled it fails, with it restored it passes. It deliberately does not assert that Mantine schedules no timers — it does, and that is normal; the contract is that none survive. Closes #1984 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
|
4b9d84d4f4 |
Merge pull request #1968 from modelcontextprotocol/v2/chore/1898-drop-setprojectannotations
chore(storybook): drop the redundant setProjectAnnotations setup file |
||
|
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 |
||
|
Cliff Hall
|
a4a8d4e2f9 |
Merge pull request #1972 from modelcontextprotocol/v2/fix/1888-float-parameter-entry
fix(web): keep the text being typed in a schema number field (#1888) |
||
|
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
|
6fd03d6de4 |
fix(web): don't round unrepresentable numbers, and reset app-switch drafts
Review round 2. Three findings, all real. 1. Precision. Mantine's `NumberInput` returns the raw *string* rather than a number once the value reaches `Number.MAX_SAFE_INTEGER` (see its `isValidNumber` guard), precisely because JS cannot hold it. Parsing that turned `90071992547409910` into `90071992547409904` and would have sent the server a number the user never typed. An inspector must not misreport what it transmits, so such input now reports no value — which is also what this field did before #1888, so nothing regresses. Only the integer part overflows, so long decimals stay parsed. 2. `AppDetailPanel` has the same reuse pattern as `ToolDetailPanel`: `AppsScreen.handleSelect` swaps `selectedAppName` + `formValues` in place without remounting, so switching between apps with a same-named number field could retain an equal-valued draft. It now passes `resetKey` too. 3. An exponent was never reachable: `NumberInput` masks input through `NumericFormat`, which rejects `e`. Dropped that claim from the doc comment rather than implying `"1e"` is a case being handled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
cliffhall
|
24a9786337 |
fix(web): reset a number field's draft text when the form changes entity
Review follow-up. The draft/value re-sync compares parsed numbers, so it cannot see a reset to an *equal* value. `ToolDetailPanel` is not keyed by tool, so its fields are reused across selections: type `-` (draft `"-"`, value `undefined`), switch to a tool with a same-named number field and no default, and the value is `undefined` on both sides — no divergence is detected and the stale `-` stays in the box for the new tool. Add a `resetKey` prop carrying the identity of whatever the form edits, and vary the number field's React key with it so no in-progress text can outlive the entity it was typed into. `ToolDetailPanel` passes the tool name; nested object forms inherit it. The schema object is no substitute — callers rebuild it every render, so its identity is unstable. The regression test drives the switch with `fireEvent` rather than `user.click`: a real click also blurs the input, and Mantine sanitizes an incomplete value on blur, which masked the defect entirely. A paired test asserts a *stable* `resetKey` does not remount, since remounting on every render would wipe the draft and reinstate #1888. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
cliffhall
|
6f47eaa143 |
fix: render a tool result's structuredContent in the Tools screen (#1908)
A tool declaring an `outputSchema` returns its real payload in `structuredContent`; the `content[]` blocks usually only summarize it. v1 rendered that payload as its own inspectable JSON section — v2's ToolResultPanel only ever walked `result.content`, so the payload was dropped with no label, section, or hint that it existed. Adds a `StructuredOutputPanel` group: a collapsible "Structured Output" box (bordered like the existing "Resource Links" box) rendering the payload as pretty-printed, syntax-highlighted, copyable JSON, bounded so a large payload scrolls within itself. ToolResultPanel renders it below the content blocks — and also when `content` is empty (where "No results yet" was simply wrong) and below the alert on an error result, so it is never silently dropped. Adds a `list_items` test-server preset (nested payload, the issue's repro shape) and a `structured-output-http.json` showcase config, documented in the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
51bbe9996b |
fix(web): keep the text being typed in a schema number field (#1888)
`SchemaForm` drove Mantine's `NumberInput` directly off the parent's numeric value. `NumberInput` reports the raw string while the text does not parse, so pressing `.` after `1` produced `"1."`, which the handler collapsed to `undefined` — and because the control is fully controlled, the `value` prop immediately rewrote the box back to `1`. The decimal point was discarded on every keystroke, making `1.5` unreachable. A trailing zero, a lone leading `-`, and an exponent failed the same way. Introduce `SchemaNumberInput`, which holds the in-progress text as the source of truth for what is displayed while the parent still only ever sees a `number | undefined`. Draft and value re-sync only when they genuinely diverge, via `useValueChange` (adjust-state-during-render) rather than an effect, so an external reset still rewrites the box but an in-progress `"1."` is left alone. Also pass `allowDecimal` from the schema, so an `integer` field rejects the decimal point instead of accepting a value its schema forbids — matching what the TUI already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
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
|
d1135dfe7c |
chore(storybook): derive the primary shade from the theme in the guard
The guard restated `primaryShade.light` as a literal 7, so re-pinning the
theme's primary shade would have made it fail even with the preview
annotations correctly applied — a guard that has to be edited alongside a
legitimate theme change just trains people to edit it.
Derive the shade from `theme.primaryShade`, handling both of Mantine's
forms (a bare number and `{ light, dark }`) and falling back to Mantine's
own default of 6 when a theme pins none.
Verified by temporarily re-pinning the theme to `{ light: 8, dark: 8 }`:
the guard still passes, where the hard-coded 7 would have failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
|
||
|
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
|
ca70a7cfa7 |
chore(storybook): drop the redundant setProjectAnnotations setup file
Since Storybook 10.3, @storybook/addon-vitest provisions the preview annotations itself — and *skips* doing so when it finds a setup file calling setProjectAnnotations. So .storybook/vitest.setup.ts was not merely redundant, it was actively opting the project out of the automatic path, which the addon printed a notice about on every run. Remove the file and the `setupFiles` entry from the `storybook` vitest project. A green suite doesn't prove the automatic provisioning works: without the preview annotations, stories would render outside MantineProvider and without App.css, and would very likely still pass. Add src/test/PreviewAnnotations.stories.tsx, which asserts from a play function that the Mantine theme variables and the App.css tokens are actually present in the rendered document, so an unthemed render fails loudly. Verified by emptying `decorators` in preview.tsx and confirming the story goes red. The a11y addon's axe runner can't be introspected from a play function, so it was checked out-of-band: a story with a real violation turned the suite red with the setup file already removed, confirming @storybook/addon-a11y/preview is applied automatically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
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
|
4340283f70 |
test: add a duplicate-tool-names test server for the #1957 repro
Nothing in the repo could serve the shape this fix is about: every preset registers a unique name, and the SDK's `registerTool` rejects a repeat. So the only reproduction was a unit test — there was no way to smoke the real app against a server that returns duplicates. Adds `ServerConfig.duplicateToolNames` (a list of registered tool names) to the composable test server, plumbed through the JSON config, and a showcase config `duplicate-tool-names-http.json`. The named tools are emitted a second time in `tools/list` with the same `name` and a "(duplicate)" title. The repeats are appended after the whole list rather than placed beside their twin. That is both the realistic shape (two tool sources concatenated) and what makes the defect observable: React matches a leading run of same-key children first, so a head-adjacent duplicate happens to line up and the bug hides. Only a separated pair collides in the keyed map and orphans a row — verified both ways against the prod bundle. The `tools/list` override is shared with the existing pagination handler (which already hand-builds the list); with no `maxPageSize` it returns a single unbounded page. Duplication happens before pagination, so a duplicated pair can straddle a page boundary the way a real server's would. Also adds a ToolControls test for this appended ordering, distinct from the reporter's interleaved fixture, and documents the config in the README showcase table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
6f535356b4 |
fix: key tool rows by source position so duplicate names filter correctly
`ToolControls` keyed both the main list and the SEP-2243 excluded list by `tool.name`. A server may legitimately return the same tool name more than once, and colliding React keys let a filtered-out row survive reconciliation: searching narrows the array, React matches the surviving rows against the stale keys, and an unrelated row stays mounted (plus a "two children with the same key" console warning). Stamp each row's position in the unfiltered list before filtering and use `<index>:<name>` as the key, on both lists. The position is captured before the filter, so the key stays stable as the search narrows. No deduplication, no change to what `tools/list` returns, and the name/title search behavior is unchanged. Adds regression tests for both lists using the reporter's fixture. 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
|
a6f233eb6a |
fix: read the list error via useSyncExternalStore; clear correlation maps
Two fixes from Copilot's review of #1953. useManagedListError re-synced React state from the `state` prop inside a useEffect, which renders one frame carrying the PREVIOUS store's error after `state` changes (switching servers) before the effect corrects it — the pattern AGENTS.md forbids. useSyncExternalStore reads the snapshot during render, so a store swap lands in the same frame, and it also closes the window where an error recorded between render and subscribe was missed. The snapshot returns the stored Error instance (or null), so it is referentially stable as the API requires. The `markResponseRejected` correlation maps were never cleared. Ids of requests that never got a response — a timeout, a dropped connection — are the only entries trackResponse can't remove, so they accumulated across reconnects. Cleared on the start-clean connect path, per the convention documented on resetSessionState (one route out, `onerror` with no `onclose`, tears down nothing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
fad8480376 |
fix: only attribute a decode rejection to a Protocol entry
markResponseRejected was called for ANY refresh failure. The id it recovers is "the last response received for this method", which is the failing exchange only when a response actually arrived and was refused while decoding. A transport drop, a timeout, or an abort produces no response frame at all — the last-answered id then still points at an EARLIER successful call, so marking it stamped "Rejected by the Inspector" onto an exchange that worked. That is the same class of lie this issue exists to remove. A server-sent JSON-RPC error is excluded for a different reason: the id would be right, but the failure is the server's and its entry already renders as an error from the error frame. Gated on SdkErrorCode.InvalidResult / UnsupportedResultType — decisions the client made about a frame in hand. The failure is still recorded as list state either way; only the Protocol attribution is gated. Caught in Copilot's review of #1953. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
1e427c05f3 |
fix: mark a rejected standalone response entry as an error too
extractStatus returned none for any non-request entry, so a clientError annotation that landed on a standalone response frame — the fallback messageLogState uses when no request entry was there to fold into, e.g. a trimmed log or a reconnect boundary — rendered with no status badge at all. The clientError check now runs before the request-only lifecycle. Caught in Copilot's review of #1953. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
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 |
||
|
Cliff Hall
|
834fd2d77d |
chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1900)
* chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1838) eslint 9 pulled `minimatch@3` → `brace-expansion@1.1.16`, which carries GHSA-mh99-v99m-4gvg (high, DoS via unbounded expansion length). It appeared in all five lockfiles — the root plus each of the four clients, none of which is an npm workspace, so all five had to move together. Deferred from the pre-2.0.0 sweep because it is dev-scope, not attacker- controlled, and a major bump across five packages was the wrong thing to attempt immediately before an irreversible publish. This is that work. ## What moved - `eslint` `^9.39.4/5` → `^10.8.0` and `@eslint/js` → `^10.0.1`, in all five manifests. - `eslint-plugin-react-hooks` `^7.0.1` → `^7.1.1` in web and tui. Required, not cosmetic: 7.0.1's peer range stops at `^9.0.0`, so installing it alongside eslint 10 fails `ERESOLVE`. - Everything else already declared eslint 10 support and stayed put: `typescript-eslint@^8.56.1` (`^8.57 || ^9 || ^10`), `eslint-plugin-react-refresh@^0.5.2` (`^9 || ^10`), `eslint-plugin-storybook@^10.2.19` (`>=8`). ## Config migration None was needed. All five configs were already flat (`eslint.config.js` with `defineConfig`/`globalIgnores` from `eslint/config`), which is the format v10 keeps and the only one it still supports. The root config's `core/` and shared- surface blocks — including the `_`-prefix `argsIgnorePattern` / `varsIgnorePattern` / `caughtErrorsIgnorePattern` — are unchanged and still gate `lint:core` and `lint:shared`. No rule was disabled, downgraded, or scoped away to make the new eslint pass. v10 surfaced 21 real violations across three rules; all 21 are fixed in the code. ### `preserve-caught-error` (new in `eslint:recommended`) — 15 sites A `throw new Error(msg)` inside a `catch` silently drops the original error. Each now passes `{ cause }`, so the underlying failure survives into the chain. Messages are unchanged, so nothing that asserts on them moves. 12 in `core/`, 2 in `clients/cli`, 1 in `test-servers/src`. ### `no-useless-assignment` (new in `eslint:recommended`) — 2 sites `clients/cli/src/cli.ts`'s `optionArgs` and `test-servers/src/test-server-oauth` 's `valid` were initialized and then unconditionally overwritten on every path. Both are now declared without the dead initializer; TypeScript's definite- assignment analysis covers the remaining paths. ### `react-hooks/set-state-in-effect` — 8 sites (web) These were false negatives the plugin fixed in 7.1.0, not a rule we newly opted into: `set-state-in-effect` has been `error` in `flat/recommended` since 7.0.1, and the web config has always extended it. Every one was the same anti-pattern — resetting or re-syncing local state from a prop inside a `useEffect`, which paints the stale value for one frame and then renders again to correct it. All eight now use a new `useValueChange(value, onChange)` hook (`clients/web/src/hooks/useValueChange.ts`), which is React's documented "adjusting state during render": compare against the previous render's value with `Object.is` and call back *during* render, so React discards the in-progress output before it reaches the DOM. - `MrtrConversation`, `ProtocolEntry`, `TaskCard` — mirror `isListExpanded`. - `NetworkEntry` — same mirror, plus the "Reveal in Network" force-open. The reveal's rAF scroll stays an effect (real external-system work); only the `setIsExpanded(true)` moved. `isExpanded` is now seeded from `isListExpanded || revealed`, since a render-time sync fires on change and so cannot cover an entry that mounts already revealed. - `PromptArgumentsForm`, `ResourceTemplatePanel` — reset on template/prompt switch. - `ServerConfigModal` — reset on open. Keyed on `opened ? initial : undefined`, which collapses "opened flipped" and "initial changed while open" into one value; `initial` is already memoized, so it cannot thrash. `useValueChange` has its own test file, and `ServerConfigModal` gains an open→edit→close→reopen test (the close leg was previously untested, which is what left the new guard uncovered). ## Advisory delta `npm audit`, per package, before → after: | Package | before | after | | ------------------- | -------------------------- | -------------------------- | | root | 9 (1 low, 6 mod, 2 high) | 8 (1 low, 6 mod, 1 high) | | `clients/web` | 19 (3 low, 6 mod, 6 high, 4 crit) | 17 (3 low, 6 mod, 4 high, 4 crit) | | `clients/cli` | 6 (2 low, 4 high) | 4 (2 low, 2 high) | | `clients/tui` | 4 (1 low, 3 high) | 2 (1 low, 1 high) | | `clients/launcher` | 3 (3 high) | 1 (1 high) | `brace-expansion` is gone from all five — `eslint@10` resolves `minimatch@10` → `brace-expansion@5.0.9`. Dropping `@eslint/eslintrc` (removed in v10) also took `js-yaml` with it, clearing GHSA-52cp-r559-cp3m (high) from all four clients as a bonus. The remainder are unrelated and tracked separately. `npm run ci` passes end to end — validate, the per-file ≥90 coverage gate, the build gate, all five smokes (including `smoke:tui`, run locally on a real TTY), and 462 Storybook tests. Also documents the effect rule in AGENTS.md and mirrors it into `.github/copilot-instructions.md`, since it is now something a reviewer cites. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: state useValueChange's referential-stability requirement (#1838) Copilot review on #1900: the hook compares with `Object.is`, so an object or array literal rebuilt in the component body compares unequal every render. Because the callback is what updates state, that is not merely a wasted render — it is an infinite loop. Every current caller passes a primitive (`boolean`, `string`) or an already- memoized value, so nothing changes behaviorally. But the hazard was implicit, and the next caller is the one it would bite. Documented on the hook itself, and one clause added to the AGENTS.md rule with its mirror in `.github/copilot-instructions.md`. No runtime guard: the requirement is the same one a `useEffect` dependency array carries, and here the failure is loud and immediate rather than silent. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): stop asserting the --tool-args-json catch binding is an Error (#1838) Copilot review round 2 on #1900: the `--tool-args-json` parse handler formatted its message with `(e as Error).message`. A `catch` binding is `unknown`, so that cast is an assertion rather than a check — a thrown non-Error would render `--tool-args-json is not valid JSON: undefined`, which points the user at nothing. `JSON.parse` does only throw `SyntaxError` today, so this is not a live bug. But the cast is the kind AGENTS.md asks us not to leave unjustified, the same handler was already being touched here to add `{ cause: e }`, and every other site in this PR uses the guarded form. Now it does too. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: require useValueChange's onChange to be render-pure (#1838) Copilot review round 3 on #1900: `onChange` is called during render, so it is bound by render-phase purity — and the web client mounts under `StrictMode`, which deliberately double-renders in development. A callback doing anything external would run an unpredictable number of times, and concurrent React can abandon an in-progress render entirely. Every current caller already passes a callback that only calls `setState`, so nothing changes. But "runs during render" is the whole point of the hook and the constraint that follows from it was left implicit. Stated on the hook, with the same clause in the AGENTS.md rule and its `.github/copilot-instructions.md` mirror. All three now point at `NetworkEntry` as the worked example of the split: the reveal's force-open is a state update and lives in `useValueChange`, while its `requestAnimationFrame` scroll stays a `useEffect`. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
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
|
7784553674 |
fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808) (#1810)
* fix(auth): offer explicit re-authorize when callback state is lost (#1808) A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state made the SDK throw `AuthorizationServerMismatchError` and the Inspector dead-ended, surfacing the raw SDK text with no path forward. The SDK folds two very different failures into that one error class, so `core/auth/issuerBinding.ts` classifies them structurally: a recoverable "lost authorization state" (the SDK puts prose, not an issuer URL, in the `recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only the first gets a recovery affordance. Web: the callback failure now raises a dedicated banner ("Authorization state was lost" / "Authorize again") whose action clears the stale OAuth state for that server and starts a fresh authorization. A genuine mismatch stays a red, non-dismissible security notification with both issuers named and no one-click retry. Node runners (CLI/TUI): the same classification rewrites the callback rejection into the shared actionable copy instead of the SDK text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(auth): detect the SDK mismatch error via isInstance (#1808) The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares that brand in a `static {}` block, so it lives on the constructor and is never reachable from an instance. The predicate always returned false, so `findIssuerBindingFailure()` never matched and the #1808 recovery banner was inert in production — the callback dead-ended exactly as before. Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand set), with a `name` fallback for a serialization boundary. Correct the module doc, which cited cross-bundle safety as the reason to avoid `instanceof` — the very thing the SDK's branded `hasInstance` provides. Every fixture now constructs a real `AuthorizationServerMismatchError` instead of a hand-rolled object with an own `mcpBrand`, which is why a green `npm run ci` hid this. Adds a test pinning the brand's placement so the feature cannot silently go inert again. Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove the duplicated banner title default, resolve the banner title/action label where the banner is raised, keep the clear-failure notification open, and bound remote-supplied issuers in user-facing copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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> |
||
|
Cliff Hall
|
cea22d86b8 |
feat(web): enforce localhost-only binding; expand loopback origin allow-list (#1796)
Closes #1795. Enforce localhost-only binding in code (the "never 0.0.0.0" rule was only a spec note): refuse an all-interfaces HOST — across every spelling the OS binds as the wildcard (inet_aton decimal/octal/hex, IPv6, IPv4-mapped, zone-scoped, IDNA/Unicode) — unless DANGEROUSLY_BIND_ALL_INTERFACES=true (Docker's sanctioned opt-in). Fixes the reported stdio-connect 403 by expanding the default loopback origin allow-list to all three interchangeable forms. Hardening surfaced across an extended review: closes an IDNA/fullwidth wildcard bypass, a blank-ALLOWED_ORIGINS fail-open, and the CLI/TUI OAuth-callback wildcard bind (loopback-only now, RFC 8252); canonicalizes the origin allow-list the way a browser does; derives frame-ancestors from the real embedder list (IPv6-literal exclusion verified in headless Chromium); strictPort on all three bind points; and preserves the CLI exit-code map + JSON envelope through the mcp-inspector --cli launcher path. Shared host normalization lives in core/node/hostUrl.ts. Docs + tests + smokes throughout. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Cliff Hall
|
a7b2fdb2ef |
test(web): settle real-timer Transitions so the coverage gate can't leak 'window is not defined' (#1786) (#1793)
* test(web): settle real-timer Transitions so the coverage gate can't leak 'window is not defined' (#1786) The web coverage gate was intermittently failing with an unhandled `ReferenceError: window is not defined` thrown from a Mantine `Transition` timer that fired after happy-dom tore down `window` (the #1760 class). The three `ViewHeader.test.tsx` tests that use `renderWithMantineTransitions` (real, env="default" timers) each drove their *primary* transition to completion, but the concurrent *enter* on a sibling cell leaves no unmount to `waitFor`, so its rAF→setTimeout chain could still be pending at teardown and fire post-teardown, failing an unrelated later file's run. Fix: after each such test, flush pending transition timers against the still-mounted tree via `settleTransitions()` (a wait longer than the 300ms header duration + rAF slack), so the settling setState targets a live tree and nothing is left to fire after cleanup unmounts. Also close the related nit: `renderWithMantine`/`renderWithMantineTransitions` now accept a `colorScheme` option, so `ViewHeader.test.tsx` no longer hand-rolls a bare `defaultColorScheme="dark"` MantineProvider (the #1760 anti-pattern). AGENTS.md documents both the option and the settle requirement. Closes #1786 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): address review — export settleTransitions, derive settle from HEADER_ANIM_MS, guard fake timers, correct comments Review feedback on PR #1793: 1. Derive the settle window from the real animation duration: export `HEADER_ANIM_MS` from ViewHeader.tsx and compute `TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200` in the test, so bumping the header duration can't silently make the settle insufficient (the invariant is now structural, not a comment). 2. Move `settleTransitions` into the shared `renderWithMantine.tsx` helper (exported, optional `ms` arg) next to `renderWithMantineTransitions`, since AGENTS.md points every future caller at it — shared test scaffolding belongs in src/test/, not a single test file. AGENTS.md now references the helper. 3. Guard `settleTransitions` against `vi.useFakeTimers()`: it awaits a real setTimeout, so under fake timers it would hang to the test timeout. Throw a clear message instead. 4. Correct the diagnosis comments: the hazard isn't a Mantine timer surviving unmount (its effect cleanup cancels timers) — it's the React state update Mantine's inner rAF schedules outside `act`, resolving post-teardown. Also fix the stale setup.ts matchMedia comment: the reduce-motion stub does not disable transitions (Mantine defaults respectReducedMotion=false); env="test" is the actual #1760 protection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): tighten the review-trail comments (round 2) Comment-accuracy follow-ups from PR #1793 round 2 (test infra only, no behavior change): - DEFAULT_SETTLE_MS: drop the false "Mantine's default header transitions run ~300ms" attribution (300ms is ViewHeader's HEADER_ANIM_MS; Mantine's Transition default is 250ms). Justify 500ms generically — a typical few-hundred-ms transition plus rAF slack. - settleTransitions doc: stop asserting a single escape mechanism as fact. State what's observed (a post-teardown dispatchSetState throwing from resolveUpdatePriority, which runs synchronously inside dispatchSetState), what's known (useTransition has an unmount cleanup), and that the exact escape route isn't pinned down — the act-wrapped drain fixes it regardless. - setup.ts: happy-dom v20 does implement matchMedia, so reframe the block as a deliberate override (pinning prefers-reduced-motion: reduce) rather than a polyfill for a missing API. - Export MantineRenderOptions so callers can name the options type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): resolve the last comment-contradiction + tighten the matchMedia override (round 3) Round-3 review of PR #1793 (test infra only): - setup.ts finding 1: the inner `value:` comment still said "happy-dom doesn't implement it", contradicting the block comment two lines up (which now correctly states happy-dom v20 DOES implement matchMedia and this is an override). Dropped the false clause; kept the part that actually justifies the `as unknown as MediaQueryList` double cast (the stub omits most of the interface). - setup.ts finding 5 (folded in rather than deferred): anchored the `matches` regex to `/prefers-reduced-motion:\s*reduce/` so the override no longer also answers `true` to `(prefers-reduced-motion: no-preference)`. - renderWithMantine.tsx nit 4: the `env="test"` rationale sat on `makeWrapper`, which also builds the `env="default"` wrappers (opposite purpose). Moved it onto `renderWithMantine`, mirroring the `env="default"` rationale on `renderWithMantineTransitions`; `makeWrapper` keeps a one-liner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): make renderWithMantineTransitions own the settle (round 4) Round-4 review of PR #1793 — implements the note-2 hardening in-PR rather than deferring it, plus note 1 and the nit: - Note 2 (settle was a convention AGENTS.md had to police): the real-transitions helper now OWNS the settle. `renderWithMantineTransitions` arms an automatic post-test drain via a helper-registered `afterEach`, so a real-transitions test can no longer leak the #1760 class by forgetting to settle a concurrent enter. Verified empirically that the helper's `afterEach` runs BEFORE the global cleanup() in setup.ts (afterEach is LIFO; setup.ts's cleanup is a setupFile registered first) — the ordering the drain depends on. The three explicit `settleTransitions()` calls in ViewHeader.test.tsx are gone; each render passes `settleMs: HEADER_ANIM_MS + 200` instead, keeping the window duration-derived. The auto-settle skips under fake timers (and no transitions test uses them). `settleTransitions` stays exported for the rare manual mid-body case. - Note 1 (settle formula was duration-only): the helper doc + the new `settleMs` option doc now say the window must exceed the longest JS chain — duration PLUS any enterDelay/exitDelay — plus rAF slack. - Nit: ViewHeader.test.tsx now states why HEADER_ANIM_MS is exported (the settle window must track the animation it settles). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): pin + self-check the auto-settle hook ordering; fix fake-timer doc; keep longest settle (round 5) Round-5 review of PR #1793: - Note 2 (load-bearing hook ordering was unpinned/undefended): the auto-settle's afterEach must run before setup.ts's cleanup(). Added BOTH guards the review suggested. (1) Runtime self-check: renderWithMantineTransitions captures the render container; the afterEach throws a clear error if it's already detached (cleanup ran first). (2) Pinned `sequence.hooks: "stack"` on the unit project. While verifying, found the ordering actually holds in EVERY sequence.hooks mode (stack/list/parallel) because setupFile hooks are outer relative to the helper's import-registered hook — so the comments now state that true mechanism (the pin is defense-in-depth; the container.isConnected check is the real enforcement), rather than claiming it "depends on stack". Full 3533-test unit suite passes under "stack". - Note 1 (fake-timer claim was backwards): AGENTS.md no longer says the leak "would return" under fake timers (fake clocks park Mantine's setTimeout/rAF and are discarded, not drained). The real hazard — the auto-settle silently no-ops, so real-clock work is unprotected — is now stated. The afterEach also console.warns on the fake-timer skip (was silent; the manual settleTransitions throws — this closes that asymmetry). - Note 3 (arming was last-write-wins): armedSettleMs now keeps the longest window via Math.max, so a later short-animation render can't under-settle an earlier long one. - Nit: documented `settleMs: 0` as a working opt-out for a test that drove every transition to completion itself (act flush, no wait). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): post-await self-check + correct the hook-ordering mechanism (round 6) Round-6 review of PR #1793: - Finding 1 (self-check blind in parallel): added the post-await container.isConnected re-check the review suggested (factored both checks into assertConnectedForSettle(container, "before"|"after")). While verifying it, I established the actual mechanism, which differs from what both prior comments and the review assumed: setup.ts's cleanup() is a *setupFile (outer)* hook, and Vitest runs inner afterEach hooks to completion before outer ones in EVERY sequence.hooks mode — so cleanup never races this settle. Verified: ViewHeader's real-transitions tests pass under stack/list/parallel with neither check tripping, and a same-suite `afterEach(cleanup)` probe (where the ordering CAN break) does trip them (pre-check). So sequence.hooks isn't load-bearing here; the comments in renderWithMantine.tsx, vite.config.ts, and AGENTS.md now say that. The "stack" pin stays as documentation of intent / defense-in-depth, and the two checks are the real guard against a future same-level-cleanup regression. - Nit: documented why last-write-wins on armedContainer is fine (cleanup detaches every container at once, so the last is a faithful "cleanup ran" proxy). - Nit: the fake-timer skip warning now interpolates the current test name so it's actionable at teardown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): disarm the auto-settle on self-unmount; concurrency + message nits (round 7) Round-7 review of PR #1793: - Finding 1 (guard misdiagnoses a self-unmount): the detached-tree check fired on ANY detachment and blamed a broken cleanup ordering, but a test calling the unmount() returned by renderWithMantineTransitions also detaches the container (a common pattern here). Fixed at the source: the helper now wraps unmount() to disarm the auto-settle (clear armedSettleMs/armedContainer) — once the test has unmounted there's nothing to settle against a live tree. The guard message also now names the self-unmount cause. Verified: a probe that unmounts a real-transitions render no longer trips the guard; ViewHeader unaffected. - Nit: documented that the module-level arming assumes sequential tests (not safe under test.concurrent/describe.concurrent). - Nit: the fake-timer skip warning falls back to "(unknown test)" when currentTestName is undefined. - Finding 3 (doc weight): trimmed the AGENTS.md bullet to the *rule* (use the transitions variant only for mid-flight assertions; pass a duration-derived settleMs; no fake timers; use the returned unmount()) and pointed at the helper's own comments for the full mechanism, which now carries it at more depth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): downgrade (not disarm) the auto-settle on self-unmount; per-container liveness (round 8) Round-8 review of PR #1793: - Finding 1 (round-7 disarm was too blunt + self-contradictory): disarming on self-unmount removed the drain entirely for the mid-flight-unmount test that most needs #1760 protection, and its "useTransition cleanup applies" rationale contradicted this file's own settleTransitions doc (which says that cleanup can't be relied on to stop the rAF). Now the wrapped unmount() DOWNGRADES: the drain still runs (mechanism-independent — flushes queued rAF/setTimeout while window is alive; a setState on the unmounted tree is a React no-op), only the liveness assertion for that container is dropped. - Finding 2 (disarm undid round-5 max-wins arming): arming is now per-container (a Set of live containers) instead of all-or-nothing. Unmounting one tree drops only its own container; a second still-mounted tree keeps its longer window and its liveness check. The isConnected assertions run over the still-live set. Verified with probes: returned unmount() drains without tripping the guard; a second still-mounted tree is unaffected when the first self-unmounts; a bare mid-body cleanup() STILL trips the guard (it's live, not decoration); ViewHeader unchanged. Comments/AGENTS.md updated so the self-unmount path and the settleTransitions doc no longer contradict each other. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): make the auto-settle drain best-effort so a guard trip doesn't also leak (round 9) Round-9 review of PR #1793 (merge-verdict; optional robustness note): - Finding 1 (pre-settle throw skipped the drain): on the ordering regression the guard exists to catch, throwing before the drain meant the maintainer got BOTH the explanatory error AND an uncaught post-teardown `window is not defined` in an unrelated file. The afterEach now captures a liveness failure, always runs the act-wrapped drain (flushing queued rAF/setTimeout while window is alive), and throws afterward — so the explanatory error stands alone. Verified: a bare mid-body cleanup() still throws the guard error (now after draining); ViewHeader unchanged. - Nits: AGENTS.md now names the reachable bare-detach path (a mid-body cleanup() on a still-armed tree) rather than "a bare RTL unmount"; the settleMs doc notes arming is per-test, so in a multi-render test the longest window wins (a `0` is superseded by a longer sibling render). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): fix the regressed fake-timer doc + guarantee the liveness error survives a drain throw (round 10) Round-10 review of PR #1793 (merge-verdict): - Finding 1 (AGENTS.md fake-timer clause regressed in the round-7 doc trim): it had reverted to "silently dropping the protection", which round 5 had corrected — "silently" is now false (the auto-settle console.warns on the fake-timer skip) and "dropping the protection" reinstates a walked-back framing (fake clocks park Mantine's timers on a discarded clock, so the no-op doesn't obviously reopen #1760). Reworded to match the helper: it warns, and the hazard is that real-clock work is then unprotected, so the test depends on which clock was installed at teardown. - Finding 3 (nit): matched the console.warn string to that same corrected framing, so AGENTS.md and the warning agree. - Finding 2 (nit): a drain-time throw could discard the captured liveness error (the substitution round 9 set out to prevent, one step further). Now the drain error is captured too and the liveness error is preferred, so the explanatory error always wins. Verified a bare mid-body cleanup() still throws the liveness error after the best-effort drain; lint clean (no throw-in-finally); ViewHeader unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): preserve the drain error as cause + route the guard message to the reachable cause (round 11) Round-11 review of PR #1793 (merge-verdict; two optional polish items): - Finding 1: when both a liveness error and a drain-time throw occur, the drain error was dropped entirely. It's now attached as the liveness error's `cause` (when the liveness error is an Error), so an act-surfaced component error from the settling render is preserved without changing which error fails the test. - Finding 2: the guard's error message pointed the reader first at the `sequence.hooks: "stack"` pin, which the file itself documents as defense-in-depth and NOT the ordering guarantee. Reworded to name the actually- reachable cause first — cleanup moved to a same-level afterEach instead of the setup.ts setupFile (outer) hook — and demote the pin to a parenthetical, so a maintainer who trips the check isn't sent to investigate a non-causal setting. Verified: a bare mid-body cleanup() throws the reworded message; eslint clean; ViewHeader unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): guard message states observation before candidate causes; log the preserved drain error (round 12) Round-12 review of PR #1793 (merge-verdict): - Finding 1: the round-11 rewording had dropped the "Expected cause:" hedge and asserted setup.ts's cleanup() as the fact, but the check only observes that a still-live armed tree was detached — it can't identify what detached it. Reworded to state that observation first, then list candidate causes: (a) a bare mid-body cleanup() in the test (the reachable case today), or (b) cleanup moved off the setup.ts setupFile hook to a same-level afterEach. No single cause asserted as fact. - Nits 2a/2b: when both a liveness error and a drain error fire, the drain error is now also console.error'd (in addition to being attached as `cause`), so it's readable regardless of whether the reporter renders `error.cause` and isn't silently dropped even if the liveness error somehow isn't an Error. Verified: a bare mid-body cleanup() throws the reworded message; eslint clean; ViewHeader unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): final comment/string polish on the auto-settle guard (round 13) Round-13 review of PR #1793 (merge-verdict; string/comment polish only): - Finding 1: the console.error text claimed the drain error was "reported as the liveness error's cause" unconditionally, but the cause is only attached when the liveness error is an Error — misleading on the very non-Error path the hoisted log exists to serve. Dropped that parenthetical so the log is true on both paths. - Nit: trimmed the now-duplicated outer comment to just the priority rationale (the inner comment, which mentions the log + instanceof conditional, is the accurate one). - Nit: guard message says "drained against a dead tree" (exact for both the before- and during-settle detach) instead of "drained nothing". - Nit: the message now names renderWithMantine.tsx rather than "the ordering note here", so it's self-contained out of context. eslint clean; ViewHeader unchanged; npm run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): correct the sequence.hooks default claim (it's "stack" in vitest 4, not "parallel") (round 14) Round-14 review of PR #1793 (merge-verdict): - Finding 1: verified against installed vitest 4.1.0 — `resolved.sequence.hooks ??= "stack"` (coverage chunk), so the runtime default is "stack"; the CLI help-text's "parallel" is stale in vitest itself. The vite.config.ts comment claimed the pin overrides a "parallel" default; corrected to say the pin is Vitest 4's own default made explicit (documents intent + guards a future default change), which is a truer reason to keep it than an override. - Nit: tied the two additions together — the post-settle check's same-level race can only arise if someone changes sequence.hooks to "parallel" (same-level hooks run sequentially under "stack"/"list"), so the pin and the post-check now reference each other rather than sitting in opposed positions. ViewHeader unchanged; npm run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): document the isolate:true dependency (empirically confirmed) + fix pre-check-blindness wording (round 15) Round-15 review of PR #1793 (merge-verdict; comment-only): - Finding 1: the auto-settle's module-level afterEach depends on isolate:true (the default) to re-register per test file; under isolate:false the module evaluates once per worker and the hook binds to only the first importing file, so other files arm with no consuming hook — the drain silently never runs and the state leaks across files. Reproduced it (vitest --no-isolate --pool=threads --no-file-parallelism over two real-transitions files leaves one file's arming unconsumed) and documented the assumption + the onTestFinished escape hatch on the arming-state comment. - Nit: the "sequence.hooks is why the pre-check can't see the concurrent detach" clause was wrong — the pre-check is blind because it runs synchronously before the await (a property of where it sits, not the hook mode). Reworded to state that, and that the concurrent case *additionally* needs sequence.hooks: "parallel". ViewHeader unchanged; npm run ci green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
1ea1e30f63 | ||
|
|
adde9ccf94 | ||
|
|
f9d893f3d7 |
||
|
|
c0e4b89c8d | ||
|
|
b70aed6914 | ||
|
|
c4e8ed992b | ||
|
|
24d89d7d0b | ||
|
|
1e0f94bb31 | ||
|
|
9e074aa6f2 |
||
|
|
75f4597281 |
||
|
|
4b9d84d4f4 |
||
|
|
53b619db09 |
||
|
|
f404d50af7 |
||
|
|
0f44288c26 | ||
|
|
a4a8d4e2f9 |
||
|
|
8049b4f7c8 | ||
|
|
6fd03d6de4 | ||
|
|
24a9786337 | ||
|
|
6f47eaa143 | ||
|
|
51bbe9996b | ||
|
|
2a4e93c621 | ||
|
|
b2dfd4099a | ||
|
|
d1135dfe7c | ||
|
|
d08a53027a | ||
|
|
ca70a7cfa7 | ||
|
|
eda8ec0db8 | ||
|
|
0325d81493 | ||
|
|
ddfce0f7ac | ||
|
|
7a74b5eba1 | ||
|
|
e328da43b5 | ||
|
|
4340283f70 | ||
|
|
6f535356b4 | ||
|
|
347173fab7 | ||
|
|
c60563b7d6 | ||
|
|
0b9aee11a3 | ||
|
|
1881e5c73f | ||
|
|
a09f2dcf1d |
||
|
|
bb219dc001 |
||
|
|
a6f233eb6a | ||
|
|
fad8480376 | ||
|
|
1e427c05f3 | ||
|
|
f201032137 | ||
|
|
9644ac7fe8 | ||
|
|
834fd2d77d |
||
|
|
5d91d4acd0 |
||
|
|
7784553674 |
||
|
|
01df9d81bb |
||
|
|
c2af547cc8 |
||
|
|
cea22d86b8 |
||
|
|
a7b2fdb2ef |