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>
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
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>
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>
Review follow-up on #1983. Copilot was right, and about a hazard this repo
had already proven and then documented away: the claim that ink's ">=19"
peer "keeps npm from hoisting it next to a React the Inspector couldn't
also use" is false. A consumer pinning React 19.0 satisfies ">=19" while
`^19.2.4` nests underneath, so ink renders through their React and the
bundle through ours — the #1952 crash, one level up, breaking TUI startup
rather than just its forms.
Bundling ink closes it (verified: with a `createRequire` banner for the
inlined CJS, since react-reconciler calls `require("react")` and
signal-exit@3 `require("assert")`), but costs ~1.4MB — react-reconciler
and yoga-layout come along. The root `react` range closes it for free
instead: `^19.0.0` is open to the whole major, so npm can satisfy our
React and a consumer's pinned React 19 with one copy, and an external ink
resolves the same copy the bundle does.
Verified against both consumer trees — the reporter's (React 18 + ink 5)
and a pinned React 19.0.0 + ink 6.8.0, which splits under `^19.2.4`.
That makes the range load-bearing while looking like an ordinary version
bump, so the tests pin it: the root range must equal ink's own declared
peer floor. They also assert each exempt package is both external and a
root dependency — external means consumers install it, the mirror image
of the inlined ones, which must not be root-declared.
The docs in all four places now say ink is exempt on cost and never on a
peer range, and name that claim as one we got wrong.
Closes#1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
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
