- `--` under --cli: everything before the separator IS forwarded to the
target, flags included (cli.ts:536-541), so "no way to pass a
leading-dash argument" was wrong. Show the correct placement instead
of sending users to a catalog file.
- Stdout: v2's emitResult appends "\n" where v1's awaitableLog wrote the
JSON with no terminator, so the output is not byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
From the suppressed comment on Copilot's second pass of #1962. Under-
approximating is the dangerous direction here: a package the scan misses
never enters the candidate set, so its skew passes the guard silently.
Three forms went unmatched. `import x = require("pkg")` and a bare
`require("pkg")` are the ordinary import syntax in `.cts` — which the
previous commit just admitted to the scan, so leaving them out would have
been an inconsistency introduced by that widening. A dynamic import carrying
import attributes (`import("pkg", { with: … })`) was missed because the
pattern required the closing paren. A static template literal
(`import(\`pkg\`)`) was missed for want of a backtick in the quote class.
Backticks are accepted only in the *call* forms. A `from` clause requires a
string literal, so allowing them there would buy nothing while reopening the
prose hazard the anchored specifier pattern closes: inline code in comments
is written with backticks throughout this codebase, and admitting it inflated
the derived set from 18 names to 41 with prose fragments (`tools`,
`connect()`, `messages`). Inert today, since a name absent from every
lockfile contributes nothing — but a prose word colliding with a real package
name would silently widen the set. Pinned with a regression test.
The real set is unchanged at 18, so these forms add no package today; they
close the syntax against the one that arrives later.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
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
- Sandbox port: the Apps tab needs a second, dynamic listener, so
"no second port" was wrong for Docker/remote deployments. Qualify the
architecture section, add a pinned-port Docker recipe, and add a
troubleshooting entry.
- Node floor: npm only warns EBADENGINE unless engine-strict is set, so
npx does not refuse to run on an older Node.
- v1 default list: the web UI persisted its list in localStorage; only
the on-disk default catalog is new.
- The v1→v2 --server example needs --cli (and a --method) to select an
entry; note there is no one-entry web equivalent.
- New behavior change #4: an ambiguous URL path (neither /mcp nor /sse)
now errors instead of falling back to SSE.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
Both from Copilot's review of #1962.
The candidate-set enumeration matched only `.ts`/`.tsx`, while
`verify:format-coverage` and `verify:typecheck-coverage` both gate all four
TypeScript extensions. No `.mts`/`.cts` exists under `core/` or
`test-servers/src` today, which is exactly why the omission would have gone
unnoticed until a shared dependency arrived through one and skewed — the
failure this guard exists to prevent. Extract the rule as the exported
`isSharedSourceFile` predicate, cover all four extensions, and pin it with
regression tests (including path-boundary anchoring, so a `core-internal/`
sibling isn't swept in).
The README's `validate` row also omitted `test:scripts` and claimed typecheck
was cli/tui only; launcher runs one too, and web typechecks via `tsc -b`
inside its `build`. Restate the row to match the actual script chain.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
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
Copilot's third pass (suppressed comment) caught the remaining hole in the
child-process handling: `spawn()` reports a failure to start via an `error`
event, not `exit`. With no `error` listener Node throws it uncaught, replacing
the smoke's diagnostic with a raw stack — and because `exit` never fires in that
case, the readiness poll would otherwise spin for the full 30s before reporting
a timeout that misattributes the cause. `close` is now listened to alongside
`exit` for the same reason: it fires in cases `exit` does not.
prod-web-server.mjs already documents this exact hazard for the launcher child;
this brings the test-server child in line.
Both new branches verified by hand:
- unspawnable executable → "could not spawn the MCP test server (…): spawn
/nonexistent/node-binary ENOENT", immediately, instead of an uncaught throw
- missing module → "MCP test server exited early" with the loader stack, fast,
with no 30s hang
Full `npm run ci` green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
Copilot's second pass (suppressed comments) caught a real gap: the smoke
listened only for `pageerror`, which is the *synchronous* half of the
uncaught-crash class. Its async twin — an unhandled rejection, or a failed
dynamic import (this app lazy-loads chunks) — is not a `pageerror`; Chromium
reports it on the console channel. So an async crash during the app-open path
could leave the smoke green.
smoke-web-browser.mjs already solved this; this now mirrors it exactly — same
FATAL_CONSOLE pattern, same split between hard failures and diagnostics, so
benign noise (a font-CDN miss, a React warning) still can't flake CI. Fatal
console errors are now included both in the catch-block diagnostics and in the
final assertion, which is where they were missing.
Confirmed working on a passing run: a benign 409 subresource error is reported
as a non-fatal diagnostic rather than failing the smoke.
Full `npm run ci` green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
Copilot review on #1934, both comments actionable:
- smoke:web:app defaulted to 6299 — the same port smoke:web uses — while its
own comment claimed the port was "distinct … so back-to-back runs can't
collide". The three web smokes run sequentially in `npm run smoke`, so it
passed, but a slow teardown, a TIME_WAIT socket, or a parallel run would
EADDRINUSE it. Moved to 6297 (smoke:web 6299, smoke:web:browser 6298) and
rewrote the comment to name the actual values rather than assert distinctness.
- The showcase table tells readers to connect with Protocol Era = Modern unless
noted, and mcp-app-http.json is legacy-only. That was stated in the prose
below but not in the row, so a reader scanning the table would try Modern
first. Marked the row.
Verified: smoke:web:app passes standalone on 6297, and the full `npm run smoke`
chain passes with all three web smokes on distinct ports.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
`smoke:web:browser` stops at first paint and never connects to a server, so
everything downstream of the connect — the Apps tab, the sandbox controller,
the UI-protocol bridge — was unexercised by any smoke. That is the code #1859
broke, and nothing would have caught a regression in it.
smoke:web:app boots the same prod --web server, spawns the mcp-app-http.json
composable server, and drives connect → open app → widget ready through one
deep-link navigate. The assertion is the documented data-app-status="ready"
contract, which the renderer only reports once the widget has loaded inside the
sandbox iframe AND completed its bridge handshake — so a single attribute
covers the proxy being served, the UI resource loading, and the handshake.
Two mechanics found by running it, both silent-failure shaped:
- server-composable.ts announces readiness on stderr, not stdout, so watching
stdout alone times out with an empty diagnostic. Both streams are scanned.
- the bound port is not the configured one — createTestServerHttp resolves via
findAvailablePort(), which walks upward when the port is taken. The smoke
parses the announced URL instead of assuming 3130.
Scope: this runs against the repo build tree like every other smoke, so it would
NOT have caught #1859 itself (a packaging failure; the file is always present
in-repo). pack:verify owns that dimension — the two are complements, and the
header comment says so. A cheap structural pre-check does assert the proxy page
sits where sandbox-controller.ts resolves it, so a move/rename fails fast.
Both failure modes verified by hand: with clients/web/static removed, the
pre-check fires; with the pre-check bypassed, the data-app-status assertion
catches it independently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
The `mcp_app_demo` tool and `mcp_app_demo_widget` UI resource presets already
existed in the preset registry, but no config wired them into a server — so
there was no runnable way to reach a rendered MCP App, and therefore no way to
exercise the sandbox proxy path that #1859 broke.
`mcp-app-http.json` composes the two over plain streamable-HTTP. With it, the
Apps tab renders a real widget and the packaging bug is reproducible on demand:
without clients/web/static in the tarball the widget area shows "Sandbox not
loaded: ENOENT … /clients/web/static/sandbox_proxy.html" instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
`clients/web/static/sandbox_proxy.html` is a committed source file, not a
build artifact, and it was never listed in the root package.json "files"
allowlist. `sandbox-controller.ts` reads it at runtime as
`<runner dir>/../static/sandbox_proxy.html`, so it resolved fine in-repo but
was absent from every published tarball — the read threw and the controller
served its "Sandbox not loaded" fallback page, breaking the Apps tab for
anyone running `npx @modelcontextprotocol/inspector`.
v1 shipped the equivalent as `server/static` in both the root and the server
workspace "files" lists; that entry has no counterpart in v2.
Add `clients/web/static` to the allowlist, and assert it in pack:verify twice
over — once in the tarball packlist and once on disk after install, since what
the runtime needs is the path *relative to* clients/web/build rather than mere
presence in the tarball.
No .npmignore change is needed: clients/web/.gitignore does not list `static`,
so the nested-gitignore packlist hazard that hid `build/` does not apply here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
The version label says which line work belongs to; nothing said what KIND of
work it is. In practice that meant reaching for bug or enhancement and
pressing everything else into one of them — a migration guide and a
tsup->tsdown swap both landing as "enhancement", which degrades the label to
"not a bug" and makes filtering by it meaningless.
Every issue created or triaged now carries exactly one of bug, enhancement,
documentation, chore, or question, independent of its v1/v2 label. Adds a
table of what each is and is not for, and states the anti-pattern explicitly
so the two familiar labels don't absorb the other three. A PR needs no type
label — it is classified through the issue it closes, the same way it is
tracked through that issue's board card.
Wires the requirement into the three places that would otherwise drift: the
"not created until" callout, triage pass 1 (an outside reporter can set
neither label, so both are applied in the same step), and the board audit,
which gains an "open, no type label" check and a matching row in the
invariant table.
Note `chore` did not exist as a repo label and was created for this; the
existing `dependencies` label is Dependabot's and scoped to PRs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
A board card in Done asserts that the work shipped, and Done is read as the
record of what a milestone actually delivered. Nothing said so, so an issue
closed as a duplicate or not-planned was being moved to Done like any other
close — which makes that record wrong in a way no later reader can detect:
counting Done cards can no longer tell a shipped fix from a report closed as
a duplicate of one.
New rule: exactly two things earn a card a place in Done — its PR merged, or
it is a parent whose last sub-issue closed. Every other close (duplicate,
won't fix, not planned, obsolete, superseded) means nothing shipped, so the
card is deleted instead. Deleting a card touches only the board; the issue
keeps its labels and comments and stays searchable, so nothing is lost.
Also notes that the close reason is the machine-readable form of the same
distinction, and that `gh issue close --reason` cannot express `duplicate` —
it accepts only completed/not planned, so duplicate must be set through the
API or the web UI's "Mark as duplicate" (which also records a duplicate-of
link).
Adds the invariant to the board audit as a tenth check. That required the
audit to read closed issues too, so its fetch moves to --state all with
stateReason, `open(n)` becomes a state test rather than "present in the map",
and --limit must now clear the repo's total issue count, not just the open
ones. The two wrong-board label checks are scoped to open issues in the same
pass — over all history they flag four long-closed cards whose labels nobody
intends to rewrite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Follow-up on the same issue: the triage rules were clear on the happy path
but three gaps meant a bare "triage new issues" would diverge from what a
maintainer actually does.
Define triage by STATE, not authorship. An issue needs triage when it
arrives with no board card and no milestone — whoever filed it. Framing it
as "externally-filed" was wrong: a maintainer opening an issue by hand in
the GitHub UI lands in exactly the same state, because write access makes
the board reachable, not automatic. What puts an issue in Todo is somebody
performing the approval, not who owns the account. Renamed the section and
reworded every dependent passage accordingly.
Fix the discovery query, which diffed open issues against board #28 only.
Verified against the live boards: it reports #1929 as unboarded when that
issue is correctly carded on #11, so following it literally double-boards
the issue — recreating a defect a past sweep introduced. Now diffs against
the union of both boards, filtered by .content.repository (an org project
can hold other repos' issues; #11 carries one from modelcontextprotocol/
servers), and prints each hit's destination, since an unboarded issue that
already has a milestone is approved and belongs in Todo, not Incoming.
Add "The board audit" — the other drift classes a single-issue rule can't
catch (double-boarded, non-Issue items, statusless cards, the Incoming ⇔
milestone invariant in both directions, wrong board for label, missing
version label, missing Priority), as a table of invariant + fix plus one
runnable jq check that should print 0 across the board. Both snippets were
executed verbatim against the live boards.
Add "Recording the score". The rubric claimed its reasoning "survives in a
form someone can argue with later", but the board stores only the result
and is private, so nothing kept that promise. Triage now posts the axes,
the bonuses claimed, and the total as a comment.
Also scope pass 2 explicitly to a human — deciding what ships in which
release is not inferable from a rubric — and correct the stale note about
three statusless cards on #11.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
AGENTS.md had no rule linking a milestone to a board Status, and its two
existing rules contradicted each other: every new issue was to be milestoned
at create time, yet new items were to start in Incoming, with Todo reserved
for work a maintainer had approved. If every issue is milestoned at birth,
"has a milestone" carries no approval information and nothing records the
moment of sign-off.
Make assigning a milestone the approval act, giving one invariant:
Incoming <=> no milestone; everything past it <=> milestoned. External
reports and maintainer-filed issues then differ only in whether approval has
already happened -- an outside reporter cannot set a label, milestone, or
board field, so their issue arriving with none of them is normal rather than
a defect to fix on arrival.
- scope the "not created until five things" callout to maintainer-filed issues
- split the Status bullets into the two cases
- scope the milestone mandate to issues you create, stating the external
exception explicitly
- add a "Triaging externally-filed issues" section: the two passes, plus a
gh/jq snippet that diffs open issues against the board to find unboarded ones
- reword both prose descriptions of the Incoming/Todo line
- reword the rubric's "Linked to a milestone" bonus as "already approved" and
note it is the re-scoring case only; under the old rules it was a constant
offset on every issue, shifting every band boundary down by one
- update both gh recipes (#28 and #11) to default to Todo, with an Incoming
variant for triage
No mirror to .github/copilot-instructions.md: board and milestone mechanics
are on that file's deliberately-absent list.
Closes#1930
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
main's copy of .github/workflows/main.yml carried three release fixes
v2/main never took -- #1831 (least-privilege default GITHUB_TOKEN
scope), #1834 (derive the npm dist-tag from the version) and #1836
(publish via npm OIDC trusted publishing).
Diffing the file both directions shows v2/main has NOTHING main lacks:
its only unique content is the superseded publish step,
run: npm publish --access public --provenance
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
which is exactly what #1836 replaced. Every other line already matched.
So this is a verbatim copy of main's file rather than a hunk-by-hunk
forward-port -- fewer ways to get it wrong, and it leaves the two
branches byte-identical (`git diff origin/main -- .github/workflows`
is now empty).
Not a back-merge: only this one file is taken, so none of main's
pre-swap v1 lineage enters v2/main's ancestry (see #1868).
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1838)
eslint 9 pulled `minimatch@3` → `brace-expansion@1.1.16`, which carries
GHSA-mh99-v99m-4gvg (high, DoS via unbounded expansion length). It appeared in
all five lockfiles — the root plus each of the four clients, none of which is an
npm workspace, so all five had to move together.
Deferred from the pre-2.0.0 sweep because it is dev-scope, not attacker-
controlled, and a major bump across five packages was the wrong thing to attempt
immediately before an irreversible publish. This is that work.
## What moved
- `eslint` `^9.39.4/5` → `^10.8.0` and `@eslint/js` → `^10.0.1`, in all five
manifests.
- `eslint-plugin-react-hooks` `^7.0.1` → `^7.1.1` in web and tui. Required, not
cosmetic: 7.0.1's peer range stops at `^9.0.0`, so installing it alongside
eslint 10 fails `ERESOLVE`.
- Everything else already declared eslint 10 support and stayed put:
`typescript-eslint@^8.56.1` (`^8.57 || ^9 || ^10`),
`eslint-plugin-react-refresh@^0.5.2` (`^9 || ^10`),
`eslint-plugin-storybook@^10.2.19` (`>=8`).
## Config migration
None was needed. All five configs were already flat (`eslint.config.js` with
`defineConfig`/`globalIgnores` from `eslint/config`), which is the format v10
keeps and the only one it still supports. The root config's `core/` and shared-
surface blocks — including the `_`-prefix `argsIgnorePattern` /
`varsIgnorePattern` / `caughtErrorsIgnorePattern` — are unchanged and still gate
`lint:core` and `lint:shared`.
No rule was disabled, downgraded, or scoped away to make the new eslint pass.
v10 surfaced 21 real violations across three rules; all 21 are fixed in the code.
### `preserve-caught-error` (new in `eslint:recommended`) — 15 sites
A `throw new Error(msg)` inside a `catch` silently drops the original error.
Each now passes `{ cause }`, so the underlying failure survives into the chain.
Messages are unchanged, so nothing that asserts on them moves. 12 in `core/`,
2 in `clients/cli`, 1 in `test-servers/src`.
### `no-useless-assignment` (new in `eslint:recommended`) — 2 sites
`clients/cli/src/cli.ts`'s `optionArgs` and `test-servers/src/test-server-oauth`
's `valid` were initialized and then unconditionally overwritten on every path.
Both are now declared without the dead initializer; TypeScript's definite-
assignment analysis covers the remaining paths.
### `react-hooks/set-state-in-effect` — 8 sites (web)
These were false negatives the plugin fixed in 7.1.0, not a rule we newly opted
into: `set-state-in-effect` has been `error` in `flat/recommended` since 7.0.1,
and the web config has always extended it. Every one was the same anti-pattern —
resetting or re-syncing local state from a prop inside a `useEffect`, which
paints the stale value for one frame and then renders again to correct it.
All eight now use a new `useValueChange(value, onChange)` hook
(`clients/web/src/hooks/useValueChange.ts`), which is React's documented
"adjusting state during render": compare against the previous render's value
with `Object.is` and call back *during* render, so React discards the
in-progress output before it reaches the DOM.
- `MrtrConversation`, `ProtocolEntry`, `TaskCard` — mirror `isListExpanded`.
- `NetworkEntry` — same mirror, plus the "Reveal in Network" force-open. The
reveal's rAF scroll stays an effect (real external-system work); only the
`setIsExpanded(true)` moved. `isExpanded` is now seeded from
`isListExpanded || revealed`, since a render-time sync fires on change and so
cannot cover an entry that mounts already revealed.
- `PromptArgumentsForm`, `ResourceTemplatePanel` — reset on template/prompt
switch.
- `ServerConfigModal` — reset on open. Keyed on `opened ? initial : undefined`,
which collapses "opened flipped" and "initial changed while open" into one
value; `initial` is already memoized, so it cannot thrash.
`useValueChange` has its own test file, and `ServerConfigModal` gains an
open→edit→close→reopen test (the close leg was previously untested, which is
what left the new guard uncovered).
## Advisory delta
`npm audit`, per package, before → after:
| Package | before | after |
| ------------------- | -------------------------- | -------------------------- |
| root | 9 (1 low, 6 mod, 2 high) | 8 (1 low, 6 mod, 1 high) |
| `clients/web` | 19 (3 low, 6 mod, 6 high, 4 crit) | 17 (3 low, 6 mod, 4 high, 4 crit) |
| `clients/cli` | 6 (2 low, 4 high) | 4 (2 low, 2 high) |
| `clients/tui` | 4 (1 low, 3 high) | 2 (1 low, 1 high) |
| `clients/launcher` | 3 (3 high) | 1 (1 high) |
`brace-expansion` is gone from all five — `eslint@10` resolves `minimatch@10` →
`brace-expansion@5.0.9`. Dropping `@eslint/eslintrc` (removed in v10) also took
`js-yaml` with it, clearing GHSA-52cp-r559-cp3m (high) from all four clients as
a bonus. The remainder are unrelated and tracked separately.
`npm run ci` passes end to end — validate, the per-file ≥90 coverage gate, the
build gate, all five smokes (including `smoke:tui`, run locally on a real TTY),
and 462 Storybook tests.
Also documents the effect rule in AGENTS.md and mirrors it into
`.github/copilot-instructions.md`, since it is now something a reviewer cites.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: state useValueChange's referential-stability requirement (#1838)
Copilot review on #1900: the hook compares with `Object.is`, so an object or
array literal rebuilt in the component body compares unequal every render.
Because the callback is what updates state, that is not merely a wasted render —
it is an infinite loop.
Every current caller passes a primitive (`boolean`, `string`) or an already-
memoized value, so nothing changes behaviorally. But the hazard was implicit,
and the next caller is the one it would bite.
Documented on the hook itself, and one clause added to the AGENTS.md rule with
its mirror in `.github/copilot-instructions.md`. No runtime guard: the
requirement is the same one a `useEffect` dependency array carries, and here the
failure is loud and immediate rather than silent.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(cli): stop asserting the --tool-args-json catch binding is an Error (#1838)
Copilot review round 2 on #1900: the `--tool-args-json` parse handler formatted
its message with `(e as Error).message`. A `catch` binding is `unknown`, so that
cast is an assertion rather than a check — a thrown non-Error would render
`--tool-args-json is not valid JSON: undefined`, which points the user at
nothing.
`JSON.parse` does only throw `SyntaxError` today, so this is not a live bug. But
the cast is the kind AGENTS.md asks us not to leave unjustified, the same
handler was already being touched here to add `{ cause: e }`, and every other
site in this PR uses the guarded form. Now it does too.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: require useValueChange's onChange to be render-pure (#1838)
Copilot review round 3 on #1900: `onChange` is called during render, so it is
bound by render-phase purity — and the web client mounts under `StrictMode`,
which deliberately double-renders in development. A callback doing anything
external would run an unpredictable number of times, and concurrent React can
abandon an in-progress render entirely.
Every current caller already passes a callback that only calls `setState`, so
nothing changes. But "runs during render" is the whole point of the hook and the
constraint that follows from it was left implicit.
Stated on the hook, with the same clause in the AGENTS.md rule and its
`.github/copilot-instructions.md` mirror. All three now point at `NetworkEntry`
as the worked example of the split: the reveal's force-open is a state update
and lives in `useValueChange`, while its `requestAnimationFrame` scroll stays a
`useEffect`.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
GHSA-p63j-vcc4-9vmv (**critical**) — `@vitest/browser` Browser Mode provider
commands accept a file path from the browser and act on it without checking the
`allowWrite` permission gate. Patched in 4.1.10; `clients/web` was pinned at
4.1.0.
Dev-scope only — `@vitest/*` is absent from the published `files` allowlist, and
Browser Mode here runs this project's own Storybook play functions, never a
third-party page. That is why #1837 deferred it rather than attempting a
toolchain upgrade immediately before an irreversible publish.
## The knot, and what actually unties it
`^4.1.0` already admits 4.1.10, so the declared ranges were never the problem.
The family is welded by *exact* peer pins — `vitest` pins
`@vitest/browser-playwright`, which pins `@vitest/browser` — so nothing moves
alone.
The issue predicted that an explicit coordinated `npm i -D` would fix it. It
does not. With `clients/web/package-lock.json` in place, npm ERESOLVEs even when
every member of the family plus the Storybook packages are named in one command
and `node_modules` is deleted first:
Found: @storybook/addon-a11y@10.2.19
Could not resolve dependency: dev @storybook/addon-a11y@"^10.5.5"
Conflicting peer dependency: storybook@10.5.5
npm resolves against the existing lock tree and will not move the whole
constellation at once. **Regenerating `clients/web/package-lock.json` is the
only thing that unties it** — a clean resolve places all four at 4.1.10 with no
peer complaints at all.
## Versions
vitest 4.1.0 -> 4.1.10
@vitest/browser 4.1.0 -> 4.1.10 (transitive)
@vitest/browser-playwright 4.1.0 -> 4.1.10
@vitest/coverage-v8 4.1.0 -> 4.1.10
storybook + @storybook/* 10.2.19 -> 10.5.5
eslint-plugin-storybook 10.2.19 -> 10.5.5
@chromatic-com/storybook 5.0.1 -> 5.2.1
`clients/{cli,tui,launcher}` were already resolving 4.1.9 (no browser pins, so
they floated freely) and were never vulnerable. Their declared `^4.1.0` still
*permitted* 4.1.0 on a fresh resolve, so the floor is raised to `^4.1.10` in all
four manifests — same reasoning as the vite floor in #1841. Pinning only the
lockfile would have left a regression path open.
## The vite 8 peer override
Resolved, as a consequence rather than a workaround.
`@joshwooding/vite-plugin-react-docgen-typescript@0.6.4` declared
`peer vite@"^3 || ^4 || ^5 || ^6 || ^7"` against our vite 8. It is not a direct
dependency — it arrives through `@storybook/react-vite`, which at 10.5.5 depends
on `^0.7.0`, and 0.7.0 adds `^8.0.0` to that range. So taking Storybook 10.5.5
for the peer set drops the override too; nothing is forced.
## Two holds the regeneration forced, each with a tracked issue
Regenerating the lock floats every other in-range dependency to current. Most of
that is welcome — it is what takes `clients/web` from 19 advisories to 1 — but
two floats broke the gate and are unrelated to this security fix:
- **zod 4.3.6 -> 4.4.3** makes `clients/web`'s `tsc -b` die with
`FATAL ERROR: Ineffective mark-compacts near heap limit` at the ~4GB default.
Reverting zod alone makes it exit 0, so the attribution is unambiguous.
Held at `~4.3.6`. See #1896.
- **eslint-plugin-react-hooks 7.0.1 -> 7.1.1** enables `set-state-in-effect`,
which fails `lint` on 8 pre-existing violations across 7 components. Fixing
them is a real refactor with interaction-behavior consequences, not a
suppression. Held at `~7.0.1`. See #1897.
Both `~` pins are deliberate and temporary, and both issues say so — they are
constraints to remove, not preferences.
Storybook 10.5.5 also reports that `setProjectAnnotations` is applied
automatically since 10.3, making `.storybook/vitest.setup.ts` redundant. Left in
place: `./preview` carries the Mantine decorator and the a11y annotations drive
the play-function assertions, and if automatic provisioning missed either, the
462 stories would render unthemed with a11y checks inert and very likely still
pass. That silent-failure risk does not belong in a security bump. See #1898.
## Audit delta
`clients/web`, where the advisory lived:
before 19 (4 critical, 6 high, 6 moderate, 3 low)
after 1 (1 low — esbuild-in-tsup, Windows dev server only)
All four critical entries — `vitest`, `@vitest/browser`,
`@vitest/browser-playwright`, `@vitest/coverage-v8` — are gone. The other 14 are
transitive dev-tree advisories (hono, ws, js-yaml, flatted, brace-expansion, ...)
cleared as a side effect of the clean resolve. Elsewhere: cli 6 -> 5,
launcher 3 -> 2, tui 4 -> 4, root unchanged at 9 (its lock is untouched). Those
remainders are pre-existing and unrelated.
## Verification
`npm run ci` green end to end on the upgraded stack — the point of caring here,
since this *is* the test infrastructure:
- coverage gate (>=90 per file, all four dimensions) passes on all four clients;
no file regressed, so no gate was lowered and no `v8 ignore` added
- 4802 web tests, 304 cli, 282 tui, 5 launcher
- 462 Storybook play functions across 109 files, on vitest 4.1.10 Browser Mode
- verify:build-gate, and all five smokes including `smoke:tui` (real TTY here)
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: replace the markdown bug template with issue forms (#1844)
Issues are the only intake channel now that external pull requests are off
(#1820), so the chooser is where the contribution policy has to be stated —
GitHub gives a would-be contributor no explanation when the "New pull request"
button isn't there.
The one existing template was a legacy **markdown** template, whose fields are
prose headings a reporter can delete or ignore. Most reports still arrive
missing the client and the transport, which are the two facts that decide where
to look. Issue forms enforce them: this replaces `bug_report.md` with
`1-bug_report.yml`, where client (web/cli/tui/core), version line (v1/v2),
Inspector version, Node version, transport, MCP server, repro, expected, and
actual are all `validations.required`. It auto-labels `bug`, and its intro says
maintainers implement the fix, so a prompt plus screenshots beats a diff —
with a field for exactly that.
Adds `2-feature_request.yml` (auto-labels `enhancement`), which puts the
problem statement first and marks the solution optional, since the problem is
what survives when a specific solution turns out not to fit.
Neither form applies a `v1`/`v2` label: GitHub can't map a form answer to a
label, so the version line is a required dropdown and a maintainer still labels
at triage. v1 takes security fixes only, so routing that answer early is the
point.
`config.yml` disables blank issues and carries the contact links. **There is
deliberately no security *template*** — a template still opens a public issue,
which is precisely what a vulnerability report must not do. The redirect is a
contact link straight to the private advisory form (`/security/advisories/new`,
verified enabled on this repo) plus one to `SECURITY.md`, which covers the v1
line too. The remaining links deflect the recurring misfiled classes: the
specification repo and the TypeScript SDK for reports that aren't about this
tool, the docs site, the contribution policy, and `#inspector-dev`.
Also fixes three links the #1884 rename left dangling — `SECURITY.md` and the
PR template still pointed at `CONTRIBUTORS.md`, which no longer exists on this
branch. A policy nobody can open is the same as no policy.
Validated against GitHub's issue-forms schema (unique ids, allowed `type`s,
`markdown` blocks carrying no `id`/`validations`, `checkboxes` marking
`required` per option rather than under `validations`).
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: address Copilot review on the issue forms (#1844)
- PR template said `v2` targets `main`; the v2 base branch is `v2/main`.
Correct on its face, and the one line in that template a reader acts on.
- Scoped the Node floor on the bug form to v2, since the form serves both
lines and v1's floor is older — reading `>= 22.19.0` as universal would
make a legitimate v1 report look out of support.
- Both the CONTRIBUTING.md paragraph and the AGENTS.md section now say the
chooser is served from the **default branch**, so neither claims a form
added on `v2/main` is live before the milestone merge.
* docs: re-wrap the CONTRIBUTING chooser paragraph (#1844)
Round-2 Copilot nit: the previous wrap left a dangling "The" at end of
line, splitting a phrase mid-sentence in the raw markdown.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: clarify the protocol-era prompt on the bug form (#1844)
Round-3 Copilot nit. "the protocol era (legacy / modern) if you selected
one" reads as referring to a field on this form, which has none — the era is
selected on the *connection*, in the Inspector's server settings. Reworded to
"the protocol era you connected with (auto / legacy / modern)", which also
picks up `auto`, the default the previous list omitted.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: stop describing the feature form as sharing the bug form's fields (#1844)
Round-4 Copilot nits, and it was right three times over — I had described
"client, version line, transport" as the required set for *both* forms in
CONTRIBUTING.md, AGENTS.md, and the config.yml header. The feature form has
neither a version-line dropdown nor a transport field, and cannot have the
first: v1 takes security fixes only, so a feature request is v2 by
construction.
Each of the three now distinguishes the two forms rather than generalizing
from the bug form.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat: declare v2 statically on the feature form (#1844)
Round-5 Copilot nit caught a real contradiction: the AGENTS.md paragraph said
no form can apply a version label, then asserted the feature form "is always
labeled v2". Both halves can't be true.
Resolved in the direction that makes the claim true rather than softer. A
form's `labels:` is static, which is only an obstacle when the value depends
on a reporter's answer — and for the feature form it doesn't: v1 takes security
fixes only, so a feature request is v2 by construction. So it now declares
`["enhancement", "v2"]` outright, which also satisfies AGENTS.md's
label-at-create-time rule without a triage step.
The bug form still can't, and shouldn't: its version line is genuinely the
reporter's to answer.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: add Incoming board status, adopt Priority field, document triage rubric (#1891)
Board #28 had no state for "filed but not yet triaged" — Todo means
maintainer-approved, so parking unreviewed issues there falsely asserted
sign-off. Adds an Incoming status ahead of Todo.
The board's Priority single-select existed but was unused (0 of 264 cards)
and undocumented, with P0/P1/P2 options matching no vocabulary in the repo.
Re-optioned to Urgent/High/Medium/Low and documented alongside Status.
Completes the priority rubric: fills in the score bands (12+/9-11/6-8/<=5),
expands both axes into scored tables, and fixes the truncated signal-indicator
line. Severity alone tops out at High by design.
Also drops the "V2 Go Live" status reference — that option is not in the
field's option list.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: correct the Done option id and harden the deleted-column guidance (#1891)
The Done Status option was deleted from board #28, blanking the Status of
all 247 cards that held it. Restored from a pre-change item-list snapshot;
verified all 247 are Done again and no other card was disturbed.
A recreated option never regains its old id, so Done is now 259d6aab rather
than 248a3910 — updated in the option table and the merge step.
The hazard callout recommended the web UI as the safe path, which is what
made this look safe. It is not: deleting an option blanks its cards in the
UI exactly as it does via the API, with no undo. Corrects that, adds a
snapshot-first rule, and documents the recovery recipe used here — including
the grouping check that proves the orphaned set is exactly the deleted
option's cards before re-applying.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: add Incoming to the v1 board and document its recipes (#1891)
Board #11 had the same gap as #28 — no state for "filed but not yet
triaged" — so a v1 security-fix issue had nowhere to sit that didn't
assert maintainer approval. Adds Incoming there too.
AGENTS.md carried no v1 board recipes at all, only a link, so anyone
boarding a v1 issue had to discover the project and field ids by hand
and was liable to reach for #28's (which are rejected, but only after
the attempt). Adds a V1 board section with its ids, states that Incoming
is the default status for new items on both boards, and notes that
Priority is v2-only — #11 has no such field.
The Incoming option was added with all four existing option ids echoed
back, and a before/after item-list diff confirms all 74 cards kept their
Status. Three cards on #11 carry no Status; that predates this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: distinguish the board Priority from the org-level issue field (#1891)
An issue page shows two fields named "Priority" and nothing syncs them.
Ours is the project field on board #28. The other is a GitHub issue field
(IFSS_kgDOAdAWeg) defined at the modelcontextprotocol org and shared by
every repo in it, alongside Effort/Start date/Target date. Same name, same
four option names, unrelated storage — #1891 sat at Urgent in Fields and
High on the board simultaneously.
Adds a table naming both, states that no pass-through exists in either
direction, and says to leave the org-level one alone: it is not repo-scoped,
so deleting it would strip Priority from every other org repo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: mark the org Priority field untrusted and cap it at a +1 signal (#1891)
Both boards are private (public: false, verified), so maintainer-assigned
Status and Priority are invisible to reporters — board priority is a
working queue, not a published commitment.
The org-level Fields -> Priority is the opposite: public on the issue page
and not part of triage. A value there is a reporter's preference, not an
assessment, so it is untrusted input. It now feeds the rubric as a sixth
signal bonus — flat +1 for Urgent or High, identical for both — and never
maps to a band. Urgent needs 12, so nothing a reporter types reaches it
alone; the issue must already sit at 11 on maintainer-assessed axes.
Supersedes the earlier "it is noise" framing: the signal is worth keeping,
it just cannot be allowed to decide the outcome. Also corrects the band
arithmetic (2-15 -> 2-16) now that a sixth bonus exists, and notes that
scores recorded on 2026-08-01 cite the old /15 denominator.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: drop the /15 denominator caveat now the recorded scores are restated (#1891)
The 51 triage comments were rewritten from /15 to /16, so the note saying
they cite the old denominator no longer describes anything. No total moved:
the reporter-set signal only scores for Urgent or High, and the one scored
issue carrying an org-field value (#1826) had Low, which earns nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: describe the board-option hazard per field, not Status-only (#1891)
Copilot review on #1892 flagged three places that still read as
Status-only now that the section covers Priority too:
- the version-label bullet said a v2 issue needs "a board card with a
Status"; it needs a Status and a Priority (v1 needs only a Status,
since board #11 has no Priority field)
- the updateProjectV2Field hazard said a bad edit "orphans the Status
of every card"; it orphans whichever field was edited
- the recovery recipe is genuinely Status-specific (reads .status,
writes the Status field id), so say so and give the two
substitutions for a Priority deletion
Verified `gh project item-list --format json` exposes both .status and
.priority before documenting the .priority swap.
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>
* fix(scripts): await the pack:verify web child's exit before removing its work dir
`pack-and-verify.mjs` had the same kill-then-remove shape that made
`smoke:tui` flake with ENOTEMPTY in #1801: `verifyWeb()`'s
`finally { stop() }` only *delivered* SIGTERM, then the main flow
`rmSync`'d the work dir immediately, and `fail()` did the same back to
back.
Applies the #1814 treatment, factored into a shared
`scripts/lib/child-cleanup.mjs` so the two scripts can't drift:
- `stopChild()` — SIGTERM, await the child's `exit`/`close`, escalate to
SIGKILL after a grace period, and ultimately proceed with a warning so
a wedged child can never hang the script. `verifyWeb()`'s `stop()` now
goes through it and is awaited, so the child is gone before the caller
removes `work`.
- `removeSafe()` — `rmSync` that warns instead of throwing. Used on both
success-path removals (a leftover temp dir must never fail a run that
passed) and in `fail()`, where an ENOTEMPTY could otherwise bury the
real diagnostic under an rmSync stack and skip the "tarball retained
at ..." hint. `fail()` stays synchronous — it is called from sync
contexts and ends in `process.exit()` — so it best-effort signals the
child; `removeSafe()` is what makes that safe.
`smoke-tui.mjs` adopts `removeSafe()` for its `cleanup()`. Its `done()`
keeps its bespoke wait (documented inline): that one must keep waiting
for `close` *after* `exit` on a shorter re-armed deadline so the quoted
output is complete, which `stopChild()`'s first-of-exit/close contract
deliberately does not do.
Adds `scripts/lib/child-cleanup.test.mjs` (10 cases, `node --test` via
`npm run test:scripts`) covering the SIGTERM/SIGKILL escalation, the
`close`-only spawn-failure path, the never-hang give-up, late events
after giving up, and that `removeSafe` warns rather than throws.
`npm run ci` passes, and `npm run pack:verify` (not in the gate — needs
network) was run end to end and passes with no cleanup warnings.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(scripts): normalize stopChild's graceMs and correct removeSafe's JSDoc
Copilot review round 1 on #1895.
- `stopChild()` trusted `graceMs` as given, but call sites build it as
`Number(process.env.X ?? 5000)` — a typo yields `NaN`, which
`setTimeout` treats as `0`. That would fire SIGTERM and SIGKILL in the
same tick and print "did not exit within NaNms", collapsing the whole
escalation silently. New exported `normalizeGraceMs()` falls back to
`DEFAULT_EXIT_GRACE_MS` for anything non-finite or non-positive, with
two tests: the table of bad values, and an end-to-end one asserting a
`NaN` grace period still leaves only SIGTERM sent a turn later.
- `removeSafe()`'s `@returns` said "true if the path was removed", which
is inaccurate for a missing path (`force: true` makes it a no-op
success). Reworded to say what it actually reports.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 2.0.0-rc series shipped these dependency bumps to `main` only, so
`v2/main` was developing against older versions and would have met them
for the first time at the milestone merge.
- `@hono/node-server` ^1.19.14 -> ^2.0.12 (root, clients/web) — a major.
v2.0.0's only breaking changes are dropping Node 18 (the repo already
requires >=22.19.0) and removing the `@hono/node-server/vercel` adapter
(unused here). The used surface — `serve()`, `serveStatic`, `ServerType`
— is unchanged, so no source edits were needed.
- `vite` ^8.0.0 -> ^8.1.5 (root, clients/web) and ^8.0.16 -> ^8.1.5
(clients/tui) — the security bumps from #1841.
`npm install` at the root (postinstall cascades into every client)
regenerated the three touched lockfiles. `npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore: restore repo files dropped by the v2 tree swap
A sweep of the pre-swap tree (ac3c1a12) against v2/main turned up 14 files the
tree swap dropped and that were never restored to either branch. Most are
correctly gone (v1 CI workflows, v1 sample-config, husky hook, two 0-byte
files, v1-specific prettier config). These four are worth having:
- .github/dependabot.yml — rewritten, not restored. v1's covered only
github-actions at /; this covers github-actions plus the six npm manifests
v2 actually has (root + four clients), monthly rather than weekly, grouped
to one PR per ecosystem/directory, targeting v2/main, and labeled `v2` so
the version-label rule holds for bot PRs too.
- .github/ISSUE_TEMPLATE/bug_report.md — rewritten. v1's asked for an
"Inspector Version e.g. 0.16.5" and had no client or version field. Under
the issues-only policy this template is the primary intake, so it now asks
the routing questions first (v1 vs v2, which client) and points at the
prompt-not-a-diff path.
- .gitattributes — restored byte-for-byte (package-lock.json
linguist-generated=true). More useful in v2 than v1: there are now six
lockfiles, and they dominate dependency-bump diffs.
- CODE_OF_CONDUCT.md — restored byte-for-byte, so GitHub surfaces it as a
community standard. Left unformatted deliberately: it is verbatim upstream
Contributor Covenant text, v1 listed it in .prettierignore for that reason,
and no v2 format glob reaches root markdown.
CONTRIBUTING.md is deliberately not restored here — it is handled by renaming
CONTRIBUTORS.md to that name, which also lights up GitHub's contributing
banner without leaving two files to keep in sync.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs(dependabot): state the documented target-branch/security-update rule
Copilot review: the comment read as if `target-branch` had no bearing on
security updates. Per GitHub's Dependabot options reference, an entry whose
`target-branch` names a non-default branch is not applied to security
updates at all — so the schedule/labels/groups here shape version updates
only, and security PRs continue to be raised against the default branch.
Comment-only; the configured settings are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: correct the branch description and the labeling ask
Two suppressed Copilot comments, both correct:
- dependabot.yml called `main` "release-only". It isn't — README.md and
AGENTS.md both describe it as the legacy v1 implementation taking security
and bug fixes only. Reworded to match, keeping the point that matters
(Dependabot reads config from the default branch).
- bug_report.md asked reporters to label the issue `v2`/`v1`, which most
external reporters have no permission to do. It now asks only for the
version checkbox and says a maintainer labels it at triage.
Comment/prose only; no configured Dependabot setting changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs(dependabot): describe main as release-only, not "legacy v1"
Reverts the branch description introduced in c7fbf350. That change was made to
match README.md:17 and AGENTS.md — but those are precisely the stale sentences
PR #1866 exists to correct. `main` has not been the legacy v1 implementation
since the go-live tree swap; it is the release branch holding the latest
released v2, and the deprecated v1 line lives on `v1/main`.
So the comment now describes the real model: v2/main is the develop branch,
main is release-only and receives milestone merges, v1/main is maintenance and
takes security fixes only.
The rest of c7fbf350 stands — the bug_report.md change (asking reporters for
the version checkbox rather than to apply a label they lack permission to set)
was a genuine catch.
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>
* ci: restore the Claude Code workflow, lost in the v2 tree swap
Closes#1850.
Restored byte-for-byte from ac3c1a12 (sha256
1c44f2d9d51c936adb40c9acefd4b44628fe7810360e2f8196034347f9d2602d,
3592 bytes). No content changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* ci: restore .mcp.json, which claude.yml's --mcp-config points at
The restored workflow runs with `--mcp-config .mcp.json` and
`--allowedTools "Bash,mcp__mcp-docs"`, so without this file the mcp-docs
server never registers and those tools do not resolve.
Lost in the same tree swap: .mcp.json survives only on v1/main, and is
absent from both main and v2/main. Restored byte-for-byte from 47f92841
(sha256 b0f0b8ea0d6c6e4bd9d19270db8c4ed13a951c6a7d8fa9a7b76d0d12ddd2491f,
123 bytes). No content changes.
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>
Closes#1864.
SECURITY.md was restored to main in #1843 but never added to v2/main.
Because v2/main is the develop branch merged into main at milestones, and
the two share no common git ancestor, the next milestone merge would
silently remove the security policy from the default branch a second time.
Copied byte-for-byte from main (sha256
bbc8aaa5a33d481f902c1b7ac260a4c9c0d41dac5bb2916607222dd205dda924,
2633 bytes). No content changes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: say "security fixes only" for v1 and unpin the PR-template links
The v1 maintenance policy was stated two different ways across the docs —
"security fixes only" in CONTRIBUTORS.md, "bug fixes" in README.md. The
#1819 PR triage told 120 contributors that v1 is security-fixes-only, so
that is the wording we keep.
Also make the PR template's two CONTRIBUTORS.md links relative instead of
pinned to blob/main/. `main` is release-only, so those resolved against
the last released tree rather than the branch the reader is on.
The remaining AGENTS.md occurrences are left alone deliberately: PR #1866
rewrites those exact lines and lands first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: drop the README change; #1866 owns that line
The "Repo status" callout edit here patched `(bug fixes only)` →
`(security fixes only)` on the callout as it exists on v2/main. But #1866
replaces that entire callout, because the surrounding sentence is false today:
it still says "The `main` branch is the legacy v1 implementation", when `main`
now holds the latest released v2.
So this edit both collides with #1866 on the same line and, taken on its own,
would preserve the false statement while correcting only the parenthetical.
#1866 already sets the v1 policy wording to "security fixes only" across
AGENTS.md and README (ee020ff2), so nothing is lost by dropping it here.
This PR is now exactly its intended half: the two branch-pinned
`blob/main/CONTRIBUTORS.md` links in .github/pull_request_template.md, made
relative. That file is untouched by #1866, so the two PRs no longer overlap on
any line.
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>
* docs: rename CONTRIBUTORS.md to CONTRIBUTING.md (#1883)
GitHub's contributing-guidelines banner — shown when someone clicks
"New issue" or opens a pull request — keys on the exact filename
CONTRIBUTING.md. Naming the file CONTRIBUTORS.md left that prompt dark
precisely where the "open an issue, not a PR" policy needs to land.
Renamed with `git mv` so history follows the file; contents are
unchanged (its heading already reads "Contributing to MCP Inspector").
Updated the one in-repo reference, in AGENTS.md. The two links in
.github/pull_request_template.md are deliberately untouched — PR #1879
makes them relative so they follow the rename.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: drop the AGENTS.md link edit; #1866 owns that line
This PR edited the `See [CONTRIBUTORS.md](./CONTRIBUTORS.md)` link in
AGENTS.md, which is the exact line #1866 rewrites — guaranteeing a one-line
conflict when this merges after it.
Handing that line to its owner instead: #1866 now writes the link as
`CONTRIBUTING.md` directly, so this PR is a pure `git mv` and the two touch no
common line. No conflict to resolve at merge time.
The tradeoff is a short window between the two merges where AGENTS.md links to
a filename that doesn't exist yet. That's deliberate and bounded: #1866 is
first in the merge order and this is third.
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>
* docs: converge README Test servers section with main's restructure
Transplants main's restructured "Test servers" section (#1836) onto
v2/main — the "Serving the modern protocol era" and "Showcase configs"
headings, the config table, and the per-config subsections — replacing
the eight long paragraphs v2/main still carried.
Folds in v2/main's #1846 correction: main's copy still claims
Mcp-Param-* mirroring is skipped in the browser, which stopped being
true when the Inspector took over building the mirrored headers itself.
The blockquote callout keeps main's shape with the corrected wording.
Scoped to that section only — the whole file is deliberately NOT copied
from main, so #1866's repo-status callout and release-section edits (and
every other v2/main-only line) are untouched.
Shrinks the first v2/main -> main milestone merge from a whole-section
conflict to a single 3-line hunk.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: point the #1846 reference at the issue, not a nonexistent PR URL
#1846 is the issue; the fix shipped as PR #1847. The callout's link used
/pull/1846, which 404s. Per Copilot review.
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>
* docs: correct post-swap branch model, PR policy, and stale gate/tree entries
Part of the go-live phase-8 docs hygiene: everything that silently inverted
when `main` became v2.
Branch/release model — documented canonically in AGENTS.md and corrected
everywhere it was restated:
- AGENTS.md: replace the one-line "Base Branches" bullet with a table giving
each branch its role (v2/main = develop, main = release, v1/main =
maintenance), whether PRs target it, and which npm dist-tag it publishes.
- README.md: the "Repo status" callout had it backwards, claiming `main` was
the legacy v1 implementation. Also note that a v2 release is cut from `main`
after the milestone merge, not from v2/main.
- CONTRIBUTORS.md: the version table listed v2's base branch as `main`.
PR policy:
- Every PR must reference an issue, from anyone, no exceptions.
- PRs are opened by repo maintainers only — org write access is not
authorization. Everyone else files a detailed issue.
- Branch names start with the target version segment (v2/…, v1/…).
- Issues and PRs carry exactly one of `v1`/`v2` at creation; default `v2`.
- Screenshots proving UI/TUI changes go in the gitignored pr-screenshots/.
Corrections to stale content found while in there:
- The TUI coverage bullet still described the Ink/App.tsx/hooks exclusion as
interim pending #1501, which closed 2026-06-29; the gate now covers all of
src/** with only tui-servers.ts excluded.
- The completion checklist said `npm run validate` was the gate and covered
"e2e tests", contradicting the mandatory-pre-push-gate section (npm run ci).
- v1 PR base was given as `main`; it is `v1/main`.
- Project tree omitted core/client, docs/, and scripts/ (README: core/client,
docs/). core/client is under the coverage gate and imported by both App.tsx
trees.
Also adds the launcher and `npm run test:scripts` to the "run the tests" list
and a tiers overview, since neither was mentioned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: require a milestone on every new issue, defaulting to the current one
Adds the rule alongside the version-label rule, since they fail the same way:
an unmilestoned issue drops out of release planning as silently as an
unlabeled one drops out of version filtering.
- Set it at create time (`gh issue create --milestone <title> ...`).
- If the user didn't specify one, default to the current milestone — the open
milestone with the nearest due date — rather than leaving it blank pending a
decision. Includes the `gh api .../milestones` one-liner that identifies it.
- Milestones are release buckets, so pick by when the work ships, not by size;
sub-issues normally inherit their parent's milestone.
Also widens the section's opening callout: an issue is not "created" until it
is labeled, milestoned, AND boarded with a Status — four distinct steps, not
three.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: add .github/copilot-instructions.md and require mirroring into it
GitHub Copilot reads .github/copilot-instructions.md when reviewing a PR. The
repo had no such file, so Copilot reviewed without any of the conventions that
actually govern this codebase — the ban on `any` and on unjustified double
casts, the Mantine-first styling rules, the `.withProps()` extraction rule, the
lib/utils split, test placement, and the per-file >=90% four-dimension gate.
The new file is a review-focused distillation of AGENTS.md: the rules a
reviewer would cite against a diff, plus a short "what to prioritize" list that
puts correctness and security first (this backend spawns processes and proxies
outbound requests). Deliberately omitted: board recipes and IDs, milestone and
branch mechanics, release procedure, and the project tree — no reviewer cites
those, and copying them would double the maintenance surface.
AGENTS.md's "Keep documentation files up to date" section now requires
mirroring review-relevant changes into it in the same PR, states that AGENTS.md
stays the source of truth, and spells out what counts as review-relevant. There
is no generation step and nothing detects drift, so the rule is the only thing
keeping the two in sync.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: address Copilot review — v1 policy wording, stale V2 Go Live, typo
- Align the v1 maintenance policy on "security and bug fixes only" in
AGENTS.md and CONTRIBUTORS.md; both said "security fixes only" while the
README callout, the branch table, and the board description said
"security and bug fixes".
- Drop the stale "or V2 Go Live" instruction from the work-begins step —
that column no longer exists on board #28 (verified via
`gh project field-list 28`), so the option id it implied would be
rejected. Restore the removed-column id list the same edit dropped, now
including V2 Go Live, and re-date the table to today's verification.
- Fix "contains the the new version" typo.
- Strip a stray trailing blank line at the end of AGENTS.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: restore `inspector/` as the Project Structure tree root
The tree is a filesystem layout — every child is a real directory
(`clients/`, `core/`, `test-servers/`). Labeling its root `v2/main/` named
a branch instead, which reads as a path that does not exist in a checkout
and diverged from the identical tree in README.md, which still says
`inspector/`. The branch model is documented in its own table under
Repository & Project Boards; the tree does not need to restate it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: settle the v1 policy wording as "security fixes only"
Reverses the direction the Copilot round took in 33cc0366. That commit
normalized the six v1-policy sites onto "security and bug fixes" because that
was the majority wording — but majority is the wrong tiebreak here. The policy
is set by what we told contributors: the #1819 backlog close note, posted to
120 closed PRs, says "v1 will receive security fixes only", and the five PRs
retained for 1.0.5 are all security fixes.
So the two sites that already said "security fixes only" were the correct ones,
and the four that said otherwise are now aligned to them:
- AGENTS.md: the `v1` label description, the branch-role table, and the v1
board line.
- README.md: the "Repo status" callout.
CONTRIBUTORS.md and AGENTS.md's Project Status bullet already read correctly
and are unchanged.
This is the substance of #1813, which is why that issue's PR is now reduced to
its PR-template-link half — the wording work landed here to avoid two PRs
editing the same lines in opposite directions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: point the AGENTS.md policy link at CONTRIBUTING.md
This PR rewrites the paragraph holding the only in-repo link to the
contributing policy, so it owns that line. #1884 renames the file
CONTRIBUTORS.md -> CONTRIBUTING.md; having #1884 also edit this line would
guarantee a one-line conflict between the two.
Writing the new filename here instead makes #1884 a pure `git mv` with no
overlapping line, so neither PR conflicts with the other.
Between this merging and #1884 merging, the link points at a filename that
doesn't exist yet. That window is deliberate and bounded — this PR is first in
the merge order (#1821) and #1884 is third.
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>
* fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1846)
A strict modern (2026-07-28) HTTP server (e.g. the GitHub MCP Server)
rejects a tools/call whose argument carries an `x-mcp-header` annotation
when the matching `Mcp-Param-*` request header is missing (-32020
"header mismatch"). The Inspector never sent that header.
The SDK only mirrors inside `client.callTool()` (and skips it entirely in
a browser environment), but the Inspector routes tools/call through
`client.request()` for manual MRTR driving (#1704), so the SDK never
mirrors for us — on any transport. On top of that, the remote (web)
transport dropped per-request headers before they reached the backend's
upstream fetch.
- core/json/xMcpHeader.ts: port the SDK's non-public `buildMcpParamHeaders`
+ value encoding (Base64 sentinel for non-ASCII/whitespace/empty values)
and add `mcpParamHeadersForTool`.
- InspectorClient.attemptToolCall: on a modern connection, build the
headers from the tool's declarations + call arguments and attach them to
the tools/call request options. `Protocol.request` forwards `headers`
to the transport and preserves them across MRTR retry legs, so the
direct StreamableHTTP transport applies them. Fixes CLI/TUI.
- RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest:
forward per-send headers and apply them to the backend's upstream
transport.send. The browser can't set cross-origin headers, but the
Node backend can — where the Inspector's real request is issued. Fixes web.
Verified end-to-end against the live GitHub MCP Server (modern HTTP).
Closes#1846
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd
* review: harden backend header allowlist, fix breadcrumbs, trim comments
Address Copilot review on #1847:
- server.ts: restrict client-supplied per-send headers to the `Mcp-Param-`
prefix (string values only) via `mcpParamHeadersOnly` before applying them
upstream, so the per-send channel can't inject arbitrary headers (e.g.
Authorization). `settings.headers` remains the channel for configured
upstream headers. Covered by new unit tests.
- Fix stale `#1810` breadcrumbs → `#1846` in the two new tests.
- Trim the verbose mirroring comments in inspectorClient/types/remote transport.
- Drop the redundant base64-on-the-wire integration test (encoding is unit-
tested; wire delivery is proven by the verbatim test), remove two unneeded
test casts, and cover the boolean `true` branch.
Not changed (declined with reason on the PR): buildMcpParamHeaders remains a
faithful port of the SDK's helper and does not re-validate values against the
declared JSON Schema type — arg types are enforced by schema validation on
both sides, and matching the SDK's conforming wire output is the module's goal.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd
* review: mirror Mcp-Param-* on the task-augmented tools/call too
Address the maintainer review on #1847:
- `callToolStream` (the "Run as task" path) built its own request options and
never mirrored, so a task-augmented `tools/call` on an annotated tool was
still rejected by a strict modern server with -32020. It now mirrors like the
plain path.
- Extract the two blocks both `tools/call` entry points had duplicated —
`convertStringToolArgs` (the string-arg coercion) and
`applyMirroredParamHeaders` (the SEP-2243 mirroring) — so the paths can't
drift again. That duplication is why the task path was missed.
- Regression test: `callToolStream` against the modern `get_weather` server
asserts the POST carries `Mcp-Param-City` (verified failing without the fix).
- README: the modern-network showcase claimed mirroring is skipped in the
browser so the web client gets -32020, and that CLI/TUI mirror. Both are now
wrong — the Inspector mirrors itself, on every client. AGENTS: note that
xMcpHeader.ts now also builds the wire headers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t
* docs: correct the stale "browser omits Mcp-Param-*" copy
Both statements predate this PR and are now the opposite of true, per the
wire capture on #1847 (proxy-observed upstream headers, web leg included).
- ToolDetailPanel: the mirrored-headers note claimed the browser omits
`Mcp-Param-*`. It now says the web client's headers are applied by the Node
backend that issues the upstream request.
- createRemoteFetch: the header comment claimed an `x-mcp-header`-annotated
tool is "uncallable from the web client against a strict modern server" —
the exact wrong conclusion to reach while debugging a -32020. It now records
that the Inspector builds the headers itself, and that they do NOT travel
this `/api/fetch` proxy path: they ride the transport's per-send `headers`
through `/api/mcp/send`.
Copy only — no test asserted the old note text (ToolDetailPanel.test.tsx
matches the section title only).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd
Closes#1811
Four README links pointed at files that don't exist on `v2/main`, and the launcher README described catalog seeding as one behavior when it is two.
- `docs/mcp-server-configuration.md` — written fresh for v2 (the v1.5 file predates the `--catalog`/`--config` split)
- `docs/launcher-config-consolidation-plan.md` — restored from `v1.5/main`, path drift corrected, v2 notes appended
- `docs/images/tui-screenshot.png` — regenerated against the current TUI
- launcher README seeding wording — split into the two real behaviors (web seeds two sample servers; cli/tui seed empty)
Review across three rounds also corrected several claims verified against source: the `--` separator is inverted between web and cli, a cli target placed after a flag is silently discarded, `--server` is a no-op on web and an unknown-option error on tui, the `MCP_CATALOG_PATH` ad-hoc guard is cli-only, and the conflict matrix diverges on both `--header` and `--transport stdio`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
* fix: wait for the TUI to exit before removing its temp dir (#1801)
`done()` sent SIGTERM and then immediately `rmSync`'d the work dir, which
doubles as the TUI's HOME. Nothing waited for the child, so the Ink process
could still be writing into the dir while rmSync walked it — a file landing
after a directory was read but before it was removed surfaces as ENOTEMPTY
on macOS, failing `smoke:tui` after a successful render.
Removal now happens only once the child's `exit` event fires: SIGTERM, then
SIGKILL after SMOKE_TUI_EXIT_GRACE_MS (default 5s), then clean up regardless
after twice that. The crash-before-render path short-circuits via a
`childExited` flag set by the pre-registered exit handler. cleanup() also
warns instead of throwing, so a leftover temp dir can never fail a passing
smoke.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq
* fix: wait on close, not exit, so spawn failure resolves instantly (review #1814)
A failed spawn emits `error` + `close` and never `exit`, so the previous
`child.once("exit")` wait never fired on that path: done() sent SIGTERM to a
process that never existed and resolved only via the give-up timer — 10s at
the default grace, with a warning blaming a SIGTERM the child never received.
Waiting on `close` covers both normal termination and spawn failure, and
additionally guarantees the stdio pipes are drained.
The short-circuit gate moves with it, `childExited` → `childClosed`: keying it
on `exit` would have skipped the drain on the crash-before-render path, which
is exactly where the quoted output matters. Messages that quote that output are
now thunks rendered in finish(), after the drain, instead of being sliced at
call time. SIGTERM is now sent only to a child that is still running, since on
both the crash and spawn-failure paths there is nothing left to signal.
finish() also gets a `finished` guard so its single-entry invariant is local
rather than inferred from process.exit() being synchronous.
Measured, spawn failure (grace shortened to 500ms): 1.04s + spurious warning
before, 0.02s and silent after. Crash-before-render now prints the child's
stderr detail line that the pre-drain slice could truncate. Happy path 3x,
timeout path, and full `npm run smoke` + `npm run validate` all green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq
* fix: bound the drain, and make the give-up warning true (review #1814 r2)
Three follow-ons from the second review pass:
The give-up warning still said the TUI "did not exit within Xms of SIGTERM",
but after switching the wait to `close` neither clause is reliably true — the
timer's most plausible trigger is now a child that has already exited, and on
the crash and spawn-failure paths no SIGTERM is sent at all. That is the same
defect this PR fixed one round ago (a warning naming a signal never sent), so
the deadline is now re-armed with a message describing what is actually being
awaited.
`close` is bounded by whoever holds the stdio pipes, which can outlive the
direct child: a descendant that inherited them keeps it pending. That is the
one real cost of `exit` → `close`, and it is moot only because the TUI spawns
nothing at boot — a reason that lives entirely outside this file. Once `exit`
has fired the deadline drops to DRAIN_MS, keeping the drain guarantee without
inheriting an unrelated process's lifetime. Measured against a grandchild
holding the pipes for 3s: 0.54s and an accurate warning, instead of 3s.
The quoted output is sliced from the tail rather than the head. The thunk added
last round exists to capture bytes that arrive late, which a head slice then
discards as soon as output exceeds 800 chars — and on the timeout path the last
frame is what diagnoses a stuck render, not the first. Verified: a 2KB stderr
crash now reports its final line. The sibling scripts keep `slice(0, 800)`;
changing them is a deliberate sweep, not a drive-by here.
Also corrected the cleanup() comment — the wait lives in done(), and it is for
`close`, not `exit`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq
* docs: correct the cleanup() and DRAIN_MS comments; tail-slice cleanly (review #1814 r3)
The cleanup() comment claimed its warning "should not fire now that removal
waits for close" — true only of the normal path. Both give-up branches call
finish() without close, and those are precisely the states where something may
still hold the dir, so the warning is expected there. Same defect class as the
two warnings already fixed in this PR (a message asserting what the code no
longer guarantees), relocated into a comment.
The DRAIN_MS comment explained why the cap exists but not what it costs: on
that path the work dir is removed while a descendant holding HOME=work may
still be writing to it — #1801's race, re-entered deliberately. That is only
acceptable because cleanup() warns instead of throwing, so record the two as
load-bearing for each other where the constant is defined.
Tail-slicing an Ink stream can begin mid-CSI-sequence, leading a diagnostic
with an escape-code fragment that mangles the line after it. outputTail() drops
through the first newline when the output was actually truncated, and returns
short output untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
cliffhall
Cliff Hall
Sam Morrow