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>
Review follow-ups on #1983 (Copilot's suppressed comments).
Removing `ink-form` from the root dependencies left `overrides.ink-select-input`
with nothing to override: no root dependency reaches that package any more, and
the regenerated lock carries no entry for it. The override that matters lives in
`clients/tui/package.json`, where the build resolves `ink-form` — and it is what
puts `ink-select-input@6` (rather than the React-18-era v5) into the bundle.
The README paragraph above the new bundling section still described the Ink
components and `App.tsx` as an interim coverage exclusion. #1501 lifted that:
`vitest.config.ts` now gates all of `src/**`, excluding only the pure
`src/tui-servers.ts` re-export. Corrected to match the config it describes.
Closes#1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
Review follow-ups on #1983.
The patch's tests called the string helper directly, so nothing exercised
the esbuild `onLoad` filter that decides whether the helper runs at all. A
filter that stopped matching would leave the build green, the tests green,
and the typo shipped — the silent no-op the throwing helper exists to
prevent, reintroduced one level up. The plugin is now driven the way
esbuild drives it (register, then invoke) against the real `ink-form`
module path, and a second test pins the filter's scope: it matches that
path and a Windows-separator one, and rejects `Form.js`,
`ink-select-input`'s own `SubmitButton.js`, and a first-party
`src/SubmitButton.jsx`.
The plugin type came from `import type { Plugin } from "esbuild"`, but
`esbuild` is tsup's transitive dependency, not one this client declares —
it typechecked only while npm happened to hoist it, and would break on a
lock change that nested it. Derive it from tsup's own
`Options["esbuildPlugins"]` instead of adding a dependency for a type.
Closes#1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
The hint under a form with unfilled required fields reads "you have not
competed yet". The string is upstream's, hardcoded in
`ink-form/lib/SubmitButton.js` with no prop to override it, and `ink-form`
was last published in 2024 — but it renders in the Inspector's tool,
prompt, and resource test forms, so it is ours to fix in practice.
Bundling `ink-form` (previous commit) is what makes it patchable: an
esbuild `onLoad` hook rewrites the label as the file enters the bundle.
The hook throws when the string is absent rather than passing the source
through, so an `ink-form` upgrade cannot silently retire the patch or
leave it aimed at a string that no longer exists — the tests drive it
against the real dependency source for the same reason.
Reported upstream as lukasbach/ink-form#14.
Closes#1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
`ink-form` and `ink-scroll-view` were externalized by the TUI bundler and
both declare a loose `react` peer (">=18"). Installed under a project that
has its own React 18, npm hoists them to that project's root — where the
peer is satisfied — while the Inspector's React 19 nests underneath. The
bundle then renders through React 19 while those two call hooks on React
18, whose dispatcher is null, so opening a tool test form (or any scroll
view) dies with "Cannot read properties of null (reading 'useState')".
Inline them instead: their `import "react"` is emitted into the bundle and
so resolves from the build directory exactly like the bundle's own, with
npm's placement no longer part of the decision. This also pins their
transitive deps to what this repo resolved — notably `ink-select-input@6`
via `overrides`, which npm ignores for a package installed as a dependency,
so consumers were getting the React-18-era v5.
`ink` stays external: it can't be bundled (its CJS `signal-exit@3` fails
ESM interop with `Dynamic require of "assert"`) and doesn't need to be —
its ">=19" peer keeps npm from hoisting it next to an unusable React.
Both packages are dropped from the root `dependencies`, since the tarball
now ships their code rather than having consumers install them.
`clients/tui/__tests__/tsupConfig.test.ts` is the durable guard: the
failure is invisible in this repo and in every smoke (a dev install has
one React) and only appears once the package is installed under another
project, so the invariant is asserted against the config instead — every
dependency declaring a `react` peer must be bundled unless exempted.
Closes#1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
Took v2/main's project-layout line and quality-gate table (adding
verify:dep-lockstep), keeping this branch's migration-guide mention and
prettier table alignment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
README: kept v2/main's showcase table including the new structured-output
row. AGENTS: kept v2/main's newer type-label and triage rules, rendered in
this branch's markdown formatting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
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
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
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
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
Closes#1964.
- README's Docker recipes dropped v1's `127.0.0.1:` prefix, so
`-p 6274:6274` published the Inspector on every host interface. The
container's `HOST=0.0.0.0` governs the container's interfaces, not the
host's, so `DANGEROUSLY_BIND_ALL_INTERFACES` never covered this. Verified
against a real container: from a LAN peer, `GET /` served the injected
`MCP_INSPECTOR_API_TOKEN`, and that token then unlocked `/api/*` with no
`Origin` header (server.ts:241 allows origin-less requests). Restore the
prefix on both recipes, on the port-remap examples, and in the web README.
- Adding a server failed inside the container the moment you mounted a
volume to keep it. Docker seeds a named volume's ownership from the
image's directory at the mount point, and creates it `root:root` when
that directory is absent — so the non-root `node` user got
`EACCES ... open '/home/node/.mcp-inspector/mcp.json.tmp-*'`. Create the
dir in the image, owned by `node`, and document the volume recipe (plus
the bind-mount `--user` caveat) — without a volume the catalog lives in
the writable layer and `--rm` throws it away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
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
`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
The four v2 SDK packages (`client`, `core`, `server`, `server-legacy`) plus
`ext-apps` were declared in the root manifest *and* again in clients/web, cli,
and tui, so each client installed its own copy. Node resolution walks up and the
root install is on every client's chain — and the root manifest is already the
documented source of truth for what ships — so the per-client entries were
duplicates of a decision made at the root, and they had already drifted: before
this change the tree carried ext-apps 1.7.4 at the root and 1.7.5 under
clients/web, dragging in two copies of the v1 `@modelcontextprotocol/sdk`
(1.29.0 and 1.30.0) through ext-apps' peer dependency.
Nothing imports the v1 SDK; it is not in any manifest of ours, only a peer of
ext-apps, so consolidating ext-apps is the only lever over it. After a clean
install there is now exactly one copy of each, at the root. `npm update` then
moves ext-apps to 1.7.5 and its SDK peer to 1.30.0, whose widened
`@hono/node-server` range (`^1.19.9 || ^2.0.5`) also drops a nested duplicate of
that package.
`express` needed a real home to make this work. `test-servers/src` imports it,
but no manifest declared it: it was reaching `clients/cli/node_modules` only as
a peer of `express-rate-limit` under `@modelcontextprotocol/server-legacy`, so
removing that entry took express with it and every cli test that spawns a test
server failed to resolve it. It is now a root devDependency — test-servers is
root-owned code with no manifest of its own — and `vitest.shared.mts` resolves
it from the root, joining `yaml`, which was already the precedent for exactly
this case. The clients/web duplicate is dropped; its only reference is a
type-only import covered by `@types/express`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
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
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
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
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
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
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
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
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
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
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
Closes#1896
`clients/web`'s `tsc -b` exhausted the 4GB default heap at zod 4.4.x. The
cause is not a zod regression nor a pathological inference site of ours: it
is two copies of zod at *mismatched versions* inside one tsc program.
`clients/web/tsconfig.test.json` compiles `test-servers/src`, whose files
resolve zod (and the SDK's zod) from the **root** install, alongside web's
own sources, which resolve from `clients/web/node_modules`. At 4.3.6 vs
4.4.3 TypeScript must relate two structurally-distinct declarations of every
`@modelcontextprotocol/*` schema type, which is exponential over a recursive
generic surface — `TS2589 Type instantiation is excessively deep`, then OOM.
Bumping root zod to 4.4.3 and changing nothing else returns the build to
baseline: 11.4s and 2,567,882 instantiations, against 11.2s / 2,567,503 at
4.3.6. Two copies at the same version are harmless; skew is what explodes.
So drop the `~4.3.6` hold on `clients/web` and move all four manifests and
lockfiles to zod 4.4.3 together, and add `verify:dep-lockstep` to `validate`
to keep them there. The guard derives its candidate set from what `core/`
and `test-servers/src` import, compares the committed lockfiles' top-level
entries, and fails deny-by-default on any skew not in the annotated
`TOLERATED_SKEW` allowlist. It joins the existing mutual-vouch ring so
dropping it from `validate` is caught by `verify:format-coverage`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
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
`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
Closes#1822
Adds docs/v1-to-v2-migration.md — the map for users arriving from v1:
- CLI flag mapping (every v1 flag survives; lists what v2 adds)
- `--config` vs `--catalog` semantics with before/after examples
- Node engine bump (>=22.7.5 → >=22.19.0)
- what no longer ships (the three sub-packages; v2 is one tarball)
- the three behavior changes most likely to bite: exit code 5 on a
failing tool call, target-must-come-first under --cli, and the
reversed `--` separator
- env-var mapping (MCP_PROXY_AUTH_TOKEN → MCP_INSPECTOR_API_TOKEN,
MCP_PROXY_FULL_ADDRESS removed, SERVER_PORT repurposed), the v1 UI
config settings, web query params, and Docker
Linked from the root README (banner + docs list), the CLI README, and
docs/mcp-server-configuration.md; AGENTS.md's docs/ tree entry updated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
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
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
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
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
* 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>
* 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>
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
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
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
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
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
