From Copilot's fourteenth pass of #1962. `hasReadableLockShape` and the
diagnostic it drives both promise "lockfileVersion 2+", but the check only
looked for a `packages` table with a root entry. A file declaring v1 while
carrying such a table was accepted, produced an empty version map, and could
make a real skew read as aligned once only one other holder remained — the
same fail-open the shape check was added to close, through the half of the
contract that wasn't enforced.
The declared version is now verified: a finite number, 2 or greater. Absent,
non-numeric, and declared-v1-with-packages cases are all in the table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's thirteenth pass of #1962. This PR added `verify:dep-lockstep`
to the vouch loop in `verify-format-coverage.mjs` without a test for it. The
existing tests exercise `rootReachesScript` in isolation and the opposite
direction (dep-lockstep vouching for format-coverage), so a typo in a sibling's
name here would have left `test:scripts` green while that guard silently
stopped being enforced — the same "a gate that stops gating" failure the
vouch cycle exists to prevent, one level up.
Three cases, reusing the fixture-repo approach: dropping either sibling from
`validate` exits 1 naming that sibling, and with both wired no sibling is
reported missing. The last asserts on the *reason* rather than the exit
status, since the fixture has no client manifests to harvest globs from and
so fails later for an unrelated reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's twelfth pass of #1962. A `/// <reference types="x" />` pulls in
a package's declarations exactly like an import does, but TypeScript reports
it in `preProcessFile`'s `typeReferenceDirectives`, not `importedFiles` — and
only the latter was read. A shared source using one could therefore load the
root's copy of a package alongside a client's without it ever entering the
candidate set, so its skew would pass. Same silent-miss direction as the
earlier syntax gaps, through a channel the parser switch didn't cover because
I only read one of its two outputs.
`typeReferencePackageNames` maps a directive to both candidates, since the
name is the *type* name and not the package: `node` resolves to `@types/node`,
while a package shipping its own declarations resolves to itself. Returning
both over-approximates, the safe direction — whichever isn't installed drops
out downstream. Scoped names mangle `@scope/pkg` → `@types/scope__pkg`,
TypeScript's convention. `path` references name a file, not a package, and are
ignored.
The real candidate set is unchanged at 17: no shared source uses a triple-slash
reference today. As with the other syntax fixes, this closes the channel
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
From Copilot's eleventh pass of #1962, and the most consequential kind of bug
for a gate: it failed *open*.
`topLevelLockVersions` returns an empty map for anything without a v2+
`packages` table — a lockfileVersion 1 file, or a malformed one. `main()` fed
that straight into the comparison, so such an install contributed no holders
at all, and a genuine skew among the remaining installs was reported as
aligned. The guard would have said OK to exactly what it exists to catch.
`hasReadableLockShape` now gates every discovered lockfile before any
comparison, requiring a `packages` object carrying the `""` root entry npm
always writes — "has a packages key" is not enough. An unreadable one is named
and exits 1, telling you to regenerate it. A JSON parse failure likewise
reports the file rather than dying on a bare SyntaxError with no path in it.
The empty-map behavior of the pure helper is kept and its test now says why it
is safe: the shape check rejects those inputs before they can reach
`findSkew`.
Three regression cases: a v1 lockfile in a fixture that IS skewed (so failing
open would show as a pass), a `packages` table with no root entry, and the
unit table for the predicate. Fixtures now write the `""` root entry, matching
real npm output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's tenth pass of #1962. The suite covered only pure helpers, so a
regression in source enumeration, install discovery, lockfile loading, the
sibling-guard vouch, or the nonzero exit on real skew would have left it
green — and the negative cases had only ever been run by hand, once per review
round.
Five end-to-end cases build a throwaway repo and run the guard as a
subprocess: aligned installs exit 0; a skewed one exits 1 naming the package
and every holder while leaving the agreeing package unreported; a package
imported only by `test-servers/src` still fails (so dropping the second shared
tree can't pass); a shared source matching no tracked file fails; and a
`validate` chain that stopped running `verify:format-coverage` fails the vouch
even when the versions are fine.
The fixture must be realpath'd. On macOS `tmpdir()` is `/var/…`, a symlink to
`/private/var/…`, and the guard runs `main()` only when `import.meta.url` (the
resolved path) matches `process.argv[1]` — so launching it via the unresolved
path loads the module and does nothing, exiting 0 silently. That is how all
five first "passed" as no-ops, and the comment records it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's ninth pass of #1962. The one-line descriptions in AGENTS.md and
the README claimed "no install-crossing dependency resolves to two different
versions", which is stronger than the gate: candidates cover only packages the
shared sources name directly, and #1965 records a live counterexample
(`@modelcontextprotocol/sdk`, skewed across two install roots while the guard
stays green).
The boundary was already documented at length on the guard and in the AGENTS.md
rule; the summaries just hadn't been narrowed to match, which is how a doc ends
up asserting a guarantee nobody implemented. Both now say "directly imported"
and point at #1965.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's eighth pass of #1962. The no-op guard tested only
`files.length === 0`, which stopped meaning anything once
`SHARED_SOURCE_FILES` was added: `vitest.shared.mts` alone keeps the total
nonzero, so a moved or renamed `core/` would leave the guard deriving
candidates from a near-empty set and passing on skew it exists to catch.
`sourcesWithNoFiles` now requires every configured dir and every named file to
have contributed, and names the ones that didn't. Directory matching is on a
path separator, so a prefix sibling (`core-internal/`) can't vouch for a
renamed `core/`.
Also drops two test comments left stale by the move to `ts.preProcessFile` —
they still described the deleted TRIVIA regex.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
The guard resolves typescript from `clients/web`. The realistic way for that
to fail is a root install run with INSPECTOR_SKIP_CLIENT_INSTALL=1, which
leaves `clients/web/node_modules` empty — and a bare MODULE_NOT_FOUND stack
gives no hint that the fix is `npm install` at the root. Throw with that
guidance and the original error as `cause`.
Deliberately not a silent skip: an unrun guard guards nothing, which is the
failure mode the sibling-vouch ring exists to prevent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's sixth pass of #1962.
Take the TypeScript-parser suggestion, which was raised across three rounds
and was right. The regex scan got both directions wrong and every widening
traded one failure for the other: it *missed* valid syntax (`import x =
require(…)`, an import-attributes argument, a comment between tokens — each a
silent miss, so the package never entered the candidate set and its skew
passed), and it *invented* names from prose, since `// adapted from "react"`
is indistinguishable from an import to a pattern that cannot tell code from a
comment. The earlier prose test only passed because `cwd omitted` isn't a
valid package name.
`ts.preProcessFile` is the cheap answer I'd missed while arguing against the
full AST: TypeScript's lightweight pre-parse scanner, no parse tree and no
type checking, purpose-built to return module specifiers. It handles every
import form, trivia, strings, and regex literals, and never reads a comment as
code. typescript resolves from `clients/web`, which already carries it, via
the `createRequire` base `smoke-web-browser.mjs` established — a bare import
would resolve relative to `scripts/`, not the cwd.
The derived set drops 18 → 17 real names: every genuine package is retained
and the one prose artifact is gone. Regression tests now use *real* package
names in comments and strings, the case the old test couldn't cover.
Also records the KNOWN BOUNDARY the review's other finding identified.
Candidates cover what the shared sources name directly, so a package reaching
the program only through another package's `.d.ts` is invisible —
`@modelcontextprotocol/sdk` is the live example, skewed 1.29.0 vs 1.30.0 and
present in web's program from both installs, never written in first-party
code. Closing it properly changes what the guard measures, so it is tracked
as #1965, with both candidate derivations measured there: a lockfile closure
is unusable (155 packages, 25 skewed, nearly all irrelevant tooling, and it
misses the SDK anyway), while `tsc --listFilesOnly` is correct and small
(15 for clients/web).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's fifth pass of #1962. `TRIVIA` covered block comments but not
`//`, despite the contract right above it saying a comment is legal wherever
whitespace is — so `import(// lazy\n"pkg")`, `require(// lazy\n"pkg")`, and
`from // reason\n"pkg"` were stepped over, and such a package would never
enter the candidate set. Same silent-miss direction as the block-comment gap.
Both comment syntaxes are now covered. The line-comment branch runs to
end-of-line only; the newline itself is matched by TRIVIA's `\s` branch.
Regression test added alongside the block-comment one.
The derived set is unchanged at 18 real names — widening trivia does not
admit prose, since a specifier must still follow immediately and
`packageNameOf` rejects anything not shaped like one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Both suppressed comments from Copilot's fourth pass of #1962.
TypeScript allows a comment anywhere whitespace is legal, so
`import(/* webpackIgnore: true */ "pkg")` and `from /* why */ "pkg"` are
valid imports the patterns stepped over. That is the dangerous direction: a
missed specifier never enters the candidate set, so the package's skew passes
the guard silently, against the over-approximation the module claims. A
`TRIVIA` fragment now stands in for `\s*` at every inter-token position, with
regression cases for the commented static, dynamic, side-effect, and
`require` forms.
I did not move to the TypeScript parser. This is a root `scripts/` `.mjs`
tool with no bundler and no TS dependency; pulling in a compiler API to read
import specifiers is a lot of machinery for a scan that only needs to
over-approximate, and each missed form is a one-line pattern with a test
beside it. The derived set is unchanged at 18 real names, so nothing here
widens what is checked today.
The README's command row also still described the candidate scan as `core/`
and `test-servers/src` only, omitting the `vitest.shared.mts` the guard now
includes; it also never mentioned that an allowlisted package is tolerated
only within a major. Both stated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
All four suppressed comments from Copilot's third pass of #1962. Each was
correct.
The candidate boundary omitted `vitest.shared.mts` — root-owned, imported by
every client's vitest config, and already treated as shared non-client source
by `verify:typecheck-coverage`. It imports only Node built-ins today, which
is exactly why the omission would have gone unnoticed until a third-party
import appeared there, resolved from the root, and skewed. Shared *files* are
now a first-class list beside the shared dirs.
The allowlist was a blanket exemption by name. Each entry's rationale
establishes that a patch/minor difference is benign, which is no evidence
that a React 18-vs-19 or Hono 4-vs-5 split would be — that is a different
type surface. Being listed now tolerates skew only *within a major version*;
a cross-major skew fails like anything else. Deriving the major from the
lockfile version keeps this automatic, with no per-package range to maintain
and bump. A version that can't be parsed is treated as "cannot prove same
major" and fails rather than passes.
`verify-format-coverage.mjs`'s comment overstated the vouching graph: the
three guards do not each check the others. This one checks both siblings;
each sibling checks only this one. Described as it actually works.
The AGENTS.md rule said to bump a shared dependency in "root + all four
clients", but launcher declares no zod and `findSkew` ignores installs where
a package is absent — so the rule as written asked for spurious dependencies.
Reworded to every install that *declares* it, and mirrored in
`.github/copilot-instructions.md`.
The failure output follows: it no longer advises adding a package that is
already allowlisted, saying "MAJOR skew" instead, and it names all three
shared sources rather than the two dirs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
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
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
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
* 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
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
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>
* docs: refresh V2 board Status option IDs in AGENTS.md (#1823)
`V2 Go Live` (`b3a6966e`) was added to the Status field via the web UI on
2026-07-27 for the go-live phases (#1804) and was not in the table; the
`SDK V2 + New Spec` row it replaced in position (`1bbb6f57`) had already
been removed, so following the table failed with "option Id does not
belong to the field". Record the current five options, note that adding
the column via the web UI left the other four IDs untouched (evidence the
UI path is safe, unlike the GraphQL full-replace hazard documented below
it), and list every removed column's dead ID in one place.
Also add the missing recipe for moving a card that is already on the board:
the existing recipes only cover `item-add` for a new item, leaving no
copy-paste path for looking an existing card's item id up by issue number.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq
* docs: close the ID-rotation pointer over the new recipe (review #1825)
The hazard note's maintenance pointer lists the hardcoded option ids that
must be updated when they rotate, but the recipe added in the previous
commit hardcodes a third one (159c8a02) that the pointer did not name —
reproducing the staleness class this PR exists to fix. Add it.
Also fold in two adjacent fixes: the "In review" casing left behind two
lines from the "In progress" → "In Progress" fix (labels are matched
exactly when re-fetching ids by name), and a note that --limit must stay
above the board's item count, since exceeding it truncates silently and
surfaces as an opaque node-resolution error from item-edit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq
* docs: move the --limit caveat inside the recipe fence (review #1825)
The section tells the reader to copy the recipes verbatim, so guidance that
lives only in the prose above the fence is lost by anyone who grabs the block
— which is the whole point of the block. The two recipes above it already
carry their context as in-block comments; match that.
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>
* docs: add CONTRIBUTORS.md issues-only policy (#1517)
Document the contribution policy: we accept issues, not pull requests.
Maintainers handle design and implementation through a prompt-driven
workflow. Contributors who have already built a change locally should
share the prompt they used rather than a diff.
Cross-reference the policy from AGENTS.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: add PR template redirecting external authors to issues-only policy (#1517)
GitHub auto-populates pull_request_template.md into every new PR body,
so it steers would-be external PR authors to CONTRIBUTORS.md before they
submit a diff.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: drop version-board details from CONTRIBUTORS.md (#1517)
The policy takes effect after v2/main merges into main, leaving a single
line of development. Tell contributors to open a well-formed issue rather
than routing them to per-version project boards.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: add version-board/label table to CONTRIBUTORS.md (closes#1595)
Completes requirement #2 of #1517: a table mapping each active version to
its base branch, project board, and version label, plus a "Label by
version" note mirroring AGENTS.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
* docs: drop v1.5 from CONTRIBUTORS.md, invite deeper contribution (#1517)
Review feedback on PR #1604:
- Remove the v1.5 row/bullet from the version table — v1.5 was an internal
intermediate branch that external contributors never saw, so surfacing it
in an outward-facing doc is noise. The table is now v1 / v2 only.
- Add a "Want to work on the Inspector with us?" section pointing at the MCP
Contributor Discord (#inspector-dev) and the community calls, framing the
issues-only rule as being about unsolicited patches rather than a closed
door, with supervised work as the path toward maintainership.
- Link that section from "Why this policy exists", and add the same Discord
pointer to the PR template.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
* docs: drop project-board links from CONTRIBUTORS.md (#1517)
The org project boards aren't visible to anyone outside the organization, so
linking them in an outward-facing doc points contributors at a 404. The
version table is now base branch + label only, and the PR template names the
version labels instead of "the appropriate version board".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
* docs: v1 takes security fixes only (#1517)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
* docs: use post-runbook branch names (v1/main, main) (#1517)
This doc will be read after the branch runbook completes, when v1 lives on
v1/main and v2 is the default main. Update the version table, the label-by-
version line, and the PR template's label hint + CONTRIBUTORS.md links
accordingly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(auth): offer explicit re-authorize when callback state is lost (#1808)
A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state
made the SDK throw `AuthorizationServerMismatchError` and the Inspector
dead-ended, surfacing the raw SDK text with no path forward.
The SDK folds two very different failures into that one error class, so
`core/auth/issuerBinding.ts` classifies them structurally: a recoverable
"lost authorization state" (the SDK puts prose, not an issuer URL, in the
`recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only
the first gets a recovery affordance.
Web: the callback failure now raises a dedicated banner ("Authorization
state was lost" / "Authorize again") whose action clears the stale OAuth
state for that server and starts a fresh authorization. A genuine
mismatch stays a red, non-dismissible security notification with both
issuers named and no one-click retry.
Node runners (CLI/TUI): the same classification rewrites the callback
rejection into the shared actionable copy instead of the SDK text.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
* fix(auth): detect the SDK mismatch error via isInstance (#1808)
The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares
that brand in a `static {}` block, so it lives on the constructor and is
never reachable from an instance. The predicate always returned false, so
`findIssuerBindingFailure()` never matched and the #1808 recovery banner
was inert in production — the callback dead-ended exactly as before.
Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is
cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand
set), with a `name` fallback for a serialization boundary. Correct the
module doc, which cited cross-bundle safety as the reason to avoid
`instanceof` — the very thing the SDK's branded `hasInstance` provides.
Every fixture now constructs a real `AuthorizationServerMismatchError`
instead of a hand-rolled object with an own `mcpBrand`, which is why a
green `npm run ci` hid this. Adds a test pinning the brand's placement so
the feature cannot silently go inert again.
Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove
the duplicated banner title default, resolve the banner title/action label
where the banner is raised, keep the clear-failure notification open, and
bound remote-supplied issuers in user-facing copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(core): start OAuth on a modern/auto-era connect (#1805)
The SDK's `server/discover` negotiation probe runs before anything else on
`protocolEra: "auto" | "modern"`, and its classifier reports whatever the
transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved
to `data.cause`. That buried both connect-time auth signals: the remote path's
`AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the
authorization URL) and a direct transport's `AuthChallengeError`. An
OAuth-protected server that authorized fine on the legacy era instead produced a
dead-end "Version negotiation probe failed" — no redirect, and the connection
status flipped to `error` despite `isConnectAuthRecoveryError` existing to
prevent exactly that.
- `findNestedAuthError()` walks `cause` / `data.cause` for either typed error.
Not gated on the SDK's error code or message, so it survives rewording.
- `connect()` unwraps the rejection at the earliest point, ahead of
`withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the
outer status guard, and every client's connect error handling — so web, CLI,
and TUI are all fixed without a client-side change.
- On the direct path, enable `interceptAuthChallenges` for the probing eras even
with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw
`SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a
JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely,
no status and not even a cause for the walk to find.
Verified against a live dual-era server that answers 401 on both `initialize`
and `server/discover`: `auto` and `modern` now reject with
`AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the
fix is load-bearing — reverting either fails the new integration tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om
* docs(core): mark the era-probe challenge interception as a removable workaround
Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for
an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as
#1807). Make its lifespan explicit so whoever picks up #1807 knows this block
is the thing to delete, rather than having to re-derive it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
* fix(core): resolve connect() when auth recovery is satisfied silently
Review findings on #1809:
- connect() built its handshake promise once, outside the runConnect closure
withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a
complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg
re-awaited the already-rejected promise and rejected a connect() whose client
was in fact connected. Short-circuit the retry when already connected, and
skip the post-connect block the nested connect already ran (a duplicate
`connect` event re-triggers every list-state manager's refresh).
- Document the accepted 403 side effect of enabling interceptAuthChallenges
with no stored tokens, and why every `{ pin }` counts as probing.
- Replace the private-method double cast in the era-probe test with an
observable assertion on the transport options, and split the non-auth
failure case per era instead of a message alternation that passed either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
* fix(core): key the connect retry leg off attempt, bound nested recoveries
Second-pass review findings on #1809:
- The satisfied-recovery short-circuit keyed off live connection status, so an
onclose landing between the nested connect() and the retry leg fell through
to the rejected handshake promise and reported the stale 401 as the reason
the session died. withDirectAuthRecovery now passes `attempt` to its
operation, so connect() identifies the retry leg structurally and reports the
teardown when the recovered session is already gone.
- Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a
satisfied outcome recovers via a nested connect() with a fresh counter, so a
server that keeps rejecting freshly refreshed tokens recursed without bound.
Counted across the nesting boundary, capped at 3.
- Strengthen the regression test (server info populated by the nested connect,
no duplicate statusChange) and add the two missing cases: a failed reconnect
still rejects, and the recursion bound terminates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
* test(core): cover the recovery teardown branch; fix a duplicated case
Third-pass review findings on #1809, both about tests added in the previous
commit:
- The retry leg's teardown error had no coverage, so a regression to the old
status fall-through would have stayed green. Covered deterministically: the
nested connect dispatches `connect` after setting "connected", so closing the
live transport from that listener flips the status before the retry leg runs.
Asserts the error identity, which is the point of the change.
- "still rejects when the reconnect underneath the recovery fails" used a
challenging transport for every connect, so it walked the same bounded chain
as the recursion-bound test rather than covering a non-auth reconnect
failure, and its comment claimed an "error" status the auth path never
reaches. Now fails the reconnect with a plain error and asserts both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
---------
Co-authored-by: rinormaloku <rinormaloku37@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
cliffhall
Cliff Hall
Sam Morrow