An ad-hoc --server-url / command launch always negotiated the legacy era,
because protocolEra was only settable per server in mcp.json. A wrapper
that mints a short-lived bearer token therefore had to write that token to
disk to exercise a server under auto or modern.
--protocol-era <legacy|auto|modern> now feeds settings.protocolEra in all
three clients through a shared parseProtocolEra coercer:
- CLI/TUI overlay it in loadServerEntries alongside --header, onto an
ad-hoc target and onto a --catalog/--config entry for that run.
- Web stamps it on the in-memory ad-hoc entry, which the backend lifts
into settings like headers; as with --header it is rejected alongside
--catalog/--config, but unlike --header it applies to stdio too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SZ4rrvH6xaCfrst75gJh7x
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds a Skills section to Server Settings with Maximum Number of Skills and
Maximum Catalog Size fields, defaulting to SKILL_MAX_CATALOG_SKILLS (256) and
SKILL_MAX_CATALOG_BYTES (64 MiB). The values persist as skillCatalogMaxSkills /
skillCatalogMaxBytes on the server's mcp.json entry (omitted at the default),
are validated as positive integers by the /api/servers routes, and are read by
verifySkills through getServerSettings(), so the CLI's --verify and the TUI
Skills pane both honor them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P6i1HBSdSS9kL4npZ2dDSe
Signed-off-by: cliffhall <cliff@futurescale.com>
Every variable that changes runtime behavior now lives on one page,
docs/environment-variables.md: which client reads it, its default, and
its effect. It covers three variables that were undocumented
(MCP_LOG_FILE, MCP_INSPECTOR_LOG_DIR, LOG_LEVEL), the secret-store
variables previously only in the Docker guide, and the Node TLS
variables that answer #1936 (a self-signed https://localhost server).
The README docs table, the migration guide, docker.md and the four
client READMEs link to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRJ64MnDTg1TpzCTEV9Xva
Signed-off-by: cliffhall <cliff@futurescale.com>
connectionTimeout defaulted to 0, which InspectorClient.connect() read as
"no bound at all" — so out of the box nothing the Inspector owns bounded a
connect attempt, and the only thing ending a hung handshake was the SDK's
60 s per-request timeout on initialize, reporting "Request timed out".
- Add DEFAULT_CONNECTION_TIMEOUT_MS (30000) to core/mcp/types.ts. An absent
value resolves to it on the client, on disk read, in the web form draft,
and in the CLI's header-only settings shell. An explicit 0 keeps its
documented meaning (no timeout; --connect-timeout 0) and is now persisted
so the opt-out round-trips; the default is the omit-sentinel, as for
taskTtl.
- Key the post-timeout transport teardown to the timer actually firing, not
to connectionTimeout > 0 — otherwise a non-zero default would tear down
every failed connect, including a recoverable 401 whose transport the
auth recovery holds open.
- The timeout message names the bound and where to raise it: "Connection
timed out after N ms. To accommodate a slower server, you may increase
the timeout value in Server Settings."
- The CLI's UNREACHABLE classifier now matches that message.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4h8Xa6Ed9zwtv5ojX35MT
Signed-off-by: cliffhall <cliff@futurescale.com>
`local:gate` ran every client's unit suite bare inside `validate` and
again instrumented inside `coverage` — ~80s of a ~370s run re-executing
exactly the files the coverage pass runs a few minutes later. CI pays
nothing for that duplication because its two jobs run in parallel; the
gate runs serially, so it paid all of it.
The gate's first stage is now `local:validate`: the same `validate:guards`
and `validate:core`, then each client's new `check` script (`format:check`
+ `lint` + `typecheck` + `build`) instead of its `validate`, which is now
literally `check && test`. `npm run validate` and both CI jobs are
unchanged; `local:validate` sits in the `local:` namespace so the
workflow guard keeps it out of CI by construction.
The "strict superset" claim in AGENTS.md is reworded to what is now
true — every CI *check*, not every CI *invocation* — with the reasoning:
`@vitest/coverage-v8` reads V8's precise-coverage profiler and rewrites
no source, so the instrumented pass runs byte-identical code and is only
slower, the stricter direction for the failure class this repo sees.
Measured on the same worktree: 370s -> 262s wall (EXIT=0 both), the
four removed legs summing to 78.5s in the baseline run's own profile.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings, all real.
- No catalog-level bound. The per-skill caps bound what ONE entry can
cost; nothing bounded how many entries there are, and SEP-2640 puts no
ceiling on a catalog — every entry costs at least one resources/read,
so a large listing made `--verify` run indefinitely and transfer
unboundedly. Adds SKILL_MAX_CATALOG_SKILLS / SKILL_MAX_CATALOG_BYTES.
Entries past the budget are still reported, with their static findings
and an `incomplete` reason, rather than dropped or failed: they were
not checked, which is neither a pass nor a verdict against the server.
- `responseBytes` charged `text.length`, which counts UTF-16 code units.
That undercharges non-ASCII by up to 3x, so a decoy block of emoji kept
the counter under 16 MiB while the wire carried far more. Adds
`utf8Length`, an allocation-free UTF-8 byte count — deliberately not
TextEncoder, which would copy a payload the server chose the size of.
- `checkSkillNameCollisions` was O(N^2) in both work and output for a
group of N: every entry filtered all N URIs and embedded the other
N-1. Duplicate names are legal and a server controls N. Now names a
bounded sample and counts the rest.
- `--verify` help documented exit 7 but not exit 8.
The three-tier bounding (per skill, per skill on the wire, per run) is
now a table in clients/cli/README.md, since the run bound is this tool's
limit rather than the spec's and should not read as a conformance rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
**Verification is a tri-state now, because two states could not be right.**
Round 12 was correct that `ok: true` for a manifest whose unread 513th file
may be tampered with is a false pass. Round 15 is correct that forcing `ok:
false` calls a server nonconformant for exceeding limits SEP-2640 states as
SHOULD NOT, with hosts free to support more — contradicting this module's own
rule that a warning never fails a report. Both hold, so:
verified -> exit 0 everything checked, everything passed
failed -> exit 7 a MUST was broken
incomplete -> exit 8 nothing checked was wrong; the walk was cut short
`ok` keeps its narrow meaning, `allSkillsVerified` is stricter than
`every(r => r.ok)` since it selects the exit code, and `anySkillFailed`
separates 7 from 8. A job that tolerates oversized catalogs can allow 8 and
still fail on 7.
**`manifestKey` still used `JSON.stringify` on the entry** — the same crash I
fixed in the TUI in round 12, one file over.
**Directory children were navigable outside the skill root.** A server could
return `skill://other-skill/...` and clicking it left the tree, with "Up"
only comparing equality against the root. Containment is checked on the
normalized URI now, and an outside child renders as text rather than a link.
Worth recording from the tests: `..` cannot escape the authority, so
`skill://a/nested/../x.md` resolves back inside and must stay navigable.
**The TUI matched report rows by raw URI**, which missed a row recorded under
an equivalent spelling while `extraReportFiles` suppressed it as covered.
Normalized, like the membership test beside it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 10 raised no new inline comments; three suppressed findings, two fixed
and one declined with a reason.
**`Object.is` held `0` and `-0` distinct**, so a listing carrying JSON `0`
against a served YAML `-0` produced a mismatch — reported as "the listing
says 0 but the served SKILL.md says 0", a false finding with an
unintelligible explanation. JSON does not distinguish them, so neither may
this: `===` for finite numbers, `Object.is` kept only for the non-finite
cases it was introduced for.
**Browsing a supporting file erased an observed failure.** The frontmatter
check ran off whatever the viewer was showing, so opening a second file made
`showingSkillMd` false and dropped the finding and its error count, with the
skill unchanged. The entry's text is skill-scoped now, written by whichever
read produces it first — the on-selection preview or a verification — and
invalidated only with `manifestKey`. That also subsumes the earlier
same-fetch requirement rather than sitting beside it.
**Declined: the second stderr line on exit 7.** `--verify` writes its summary
and then the ordinary `ErrorEnvelope` that EVERY non-zero exit writes, which
is the documented contract and exactly what `--strict` does. Suppressing it
would make this one command's failure output unparseable by a caller
branching on `.code`. What was wrong is the README, which implied stderr
carries only the summary; it now states both lines and why.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2248.
Completes SEP-2640 support across all three clients, and closes the one
obligation #2234 deliberately left open.
- `checkSkillFrontmatterMatch` compares a served SKILL.md's own YAML
frontmatter against the entry the listing advertised, field by field. No
digest can cover this: a digest is taken over the bytes the server served,
so it proves the file was not altered in transit and says nothing about
whether the listing described it honestly. Needs a real YAML parser; `yaml`
was already a root dependency, so this adds no package — but it does newly
put it on core/'s import graph, hence the three bundler `external` lists.
- `resources/directory/read`: result schemas defined against the normative
text, `InspectorClient.readResourceDirectory`, a Directory section on the
Skills screen, and `--method resources/directory/read` in the CLI. The
client refuses the call locally when the server did not declare
`directoryRead`, which is the SEP's MUST NOT. A child the directory lists
but the entry does not is marked `not listed` rather than merged into the
manifest — the SEP calls a directory result a live observation and forbids
treating it as extending the manifest.
- CLI: `skills/list`, `skills/get` and `resources/directory/read`, plus
`--verify` — one NDJSON report per skill, a summary on stderr, exit 7 on a
violation. Reads are sequential; a conforming manifest may declare 512
entries.
- TUI: a Skills pane, shown only when the server declares the extension. Each
row carries its conformance verdict as a glyph as well as a colour, since
the pane is read over ssh and through script(1). Enter verifies.
- `verifySkills` re-throws `AuthRecoveryRequiredError` rather than recording
it per file: it says the session's authorization expired, so absorbing it
would report N identical read failures and swallow the error the TUI and
the web commands key off to reauthorize.
Two open questions settled, both in code comments where they will be found:
`skills/get` carries no caching attributes because SEP-2640 leaves the
question open in as many words, so requiring them would fail a conforming
server; and there is no `PagedSkillsState` because every consumer of this
list is a whole-catalog verdict — computed over page one of three, "this
server's skills conform" is not merely partial but wrong.
The fixture grows two skills, both for checks that were otherwise
undemonstrable: `lying-listing` (listing and file disagree, digest still
verifies) and `stale-manifest` (serves a file its manifest does not declare).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds `docs/cli-smoke-testing.md` — a copyable connect → list → call →
assert workflow for testing an arbitrary MCP server from a shell or a CI
job, plus index rows in the root README and a pointer from the CLI
README.
The gap this fills is a user-facing one. `clients/cli/README.md` is a
flag reference and `scripts/smoke-cli.mjs` is an internal end-to-end
check of the launcher → CLI path; neither tells someone how to compose
those flags into a check they run on every commit against their own
server.
Covers, in order: connecting over stdio/HTTP/SSE (including the `--`
separator, which splits the opposite way under `--cli`); `--format json`
and its `{result[, appInfo]}` envelope; asserting a tool exists with
`jq -e`; one representative `tools/call` and the `--tool-arg` vs
`--tool-args-json` coercion difference; the exit-code map and how to
branch on it; keeping OAuth non-interactive with `--stored-auth-only`
and an isolated `MCP_STORAGE_DIR`; and negative assertions — refusals,
a secret-shape scan of captured output, and `--strict` schema
portability. Ends with a complete script and a GitHub Actions job.
Every command and output shape in the guide was verified against the
bundled composable test servers over both stdio and HTTP rather than
transcribed from the reference, which corrected three claims that would
otherwise have shipped wrong:
- `--tool-arg zip=012` does not send `12`. `012` is not valid JSON, so
it is sent as the literal string. The real trap is `zip=10001`, which
does coerce to a number and is then rejected by a string schema.
- `if ! cmd; then status=$?` yields `0`, because `!` inverts the
pipeline's status — every failure class would look identical. The
guide uses `|| status=$?` and says why.
- The sample script does not report a failure class on its own; that is
the opt-in `case` block from §5.
The §8 script was run end to end against a live HTTP test server, and
exits 0/3/4/5/6 were each reproduced against a real server.
Closes#1886
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KsKC3HniVF9dCZtDVbYp5B
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 14. Two code fixes and a documentation sweep.
- `buildRevocationRequest` ran outside `revokeToken`'s best-effort try.
`encodeURIComponent` throws on a lone UTF-16 surrogate — valid JSON, so it
can reach there from a persisted client id or secret — and every caller has
already cleared its local state by then, so a rejection instead of a `failed`
outcome would break the documented guarantee. Construction is inside the try
now, and the catch reports `params.endpoint` rather than a `url` that may not
exist on that path.
- The fixture's Basic decode turned a malformed percent escape into an Express
500, reporting a bad credential as a server fault rather than as the
`invalid_client` 401 the endpoint means. Decoding failure is failed
authentication.
- Ten places still said revocation happens *before* the clear, which the
reorder in the previous commit made false: the settings-form description, the
`--no-revoke` help text, both READMEs, `docs/mcp-server-configuration.md`,
three JSDoc blocks, and the PR body. Swept, and the ordering is stated where
it is load-bearing rather than asserted in passing. Also merged the two
adjacent JSDoc blocks on `InspectorClient.clearOAuthTokens`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Clearing the Inspector's OAuth state deleted the local copy of the tokens
and stopped there. The access token — and the refresh token, which is
long-lived by design — stayed valid at the authorization server until they
expired on their own, so a day of connect/disconnect iteration left it
holding grants for sessions that ended hours ago. RFC 7009 §1 describes
exactly that case, and nothing in the repo constructed a revocation
request, so a server author implementing `revocation_endpoint` had no way
to exercise it with the tool that exists to exercise this.
core/auth/revocation.ts builds and sends the request; the three clear paths
call it, so the behavior is written once — web's "Clear OAuth state and
disconnect" (both the active and stored-only paths), the TUI's Auth tab,
and the CLI's --relogin. It runs BEFORE the local clear, because the token,
the client credentials and the discovered `revocation_endpoint` all live in
the store the clear is about to empty. The request names the refresh token
when there is one: §2.1 asks the AS to also invalidate the access tokens
issued under the same grant, so one request covers both halves.
Best-effort by construction. Every path returns a TokenRevocationOutcome
rather than throwing: no advertised endpoint means nothing is sent and that
authorization server behaves exactly as before, and a network error, a
non-2xx, a 5s timeout or an unreadable store is reported and nothing more.
Forgetting the tokens is what the user asked for. `lost_authorization_state`
recovery deliberately skips it — it clears a half-finished flow to retry it,
and an authorization that never completed has no grant to revoke.
Client authentication resolves the preregistered slot first, mirroring
BaseOAuthClientProvider.clientInformation; reading only the dynamic slot
would send no authentication for a server configured with `oauth.clientId`.
The Basic credential is byte-identical to the SDK's applyBasicAuth so this
request and the token request cannot present the same secret differently.
Opt out per server with `oauth.revokeOnClear` (default on, only `false`
written to disk) or per CLI run with --no-revoke. Read at clear time rather
than connect time, so toggling needs no reconnect. Turning it off is a
testing affordance: a client that walks away still holding live tokens is a
case a server author may want to reproduce.
test-servers now advertises `revocation_endpoint` and serves POST
/oauth/revoke, faithful on §2.2 (an unknown token is a success) and §2.1
(revoking a refresh token invalidates the access tokens under the same
grant), with oauth-revocation-http.json / oauth-no-revocation-http.json as
showcase configs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
The root `ci` script was never what runs in GitHub CI, and the name worked
against us three ways: it collides with the built-in `npm ci` (which does
something else entirely and is one keystroke away), it invites the
cross-engine passes into a workflow by looking like the thing a workflow
should call, and it blurs "GitHub CI" and "the local gate" in every doc
that names it.
- Rename `ci` → `local:gate` and `ci:storybook` → `local:storybook`, with
no back-compat alias: `npm run ci` now fails with npm's missing-script
error, which prints the available scripts.
- Add `scripts/lib/workflow-gate.mjs` + its `test:scripts` suite, which
runs over the repo's real `.github/workflows/**` and fails on an
invocation of a `local:*` script, of a non-Chromium engine pass (derived
from SUPPORTED_BROWSERS), or of `smoke:web:engine`, or on `SMOKE_BROWSER`
set to anything but a literal `chromium`. It does not forbid
`npm run smoke`, `smoke:web:chromium` or `smoke:tui` — those belong in CI
and are there today. Comments are not scanned, so the workflow can still
explain itself. This is the durable half; the rest is prose.
- Document the two-tier model once, in the root README ("Two tiers: GitHub
CI and the local gate"), and point the other docs at it rather than
restating the split.
- Sweep the ~40 stale references across README, AGENTS.md,
.github/copilot-instructions.md, the client READMEs, the roadmap doc and
the script headers. AGENTS.md and copilot-instructions.md change
together, per the mirror rule.
No gate changes what it actually runs.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 8 (its one suppressed finding; no new inline comments).
The `boolean-schema` suggestion for a bare `true` said the replacement
"narrows the schema deliberately". That holds in an ordinary property
position and inverts under a negating parent: `{"not": true}` rejects
every value, so swapping in `{"type": "object", …}` leaves the parent
rejecting only objects and every non-object then passes — the enclosing
contract is widened. Under `if` it redirects which branch applies rather
than tightening anything.
- Parent-aware advice under `not`: suggest `{}`, which is the exact
object form of `true` and is already exempt from `untyped-schema` in
that position, with the message saying why a concrete type would be
wrong there.
- Everywhere else, and in the `untyped-schema` suggestion too, describe
the replacement as a deliberate CHANGE of contract rather than a
narrowing. A suggestion has no business claiming an effect it cannot
guarantee across the positions it can print from.
Tests pin the `not`-specific advice and assert no bare-`true` suggestion
in any of the three positions contains "narrow". The CLI README's sample
report is re-captured from a real run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 7.
- Add `contentSchema` to the traversal. It holds the schema the decoded
string content must satisfy, so a bare `true` or a remote `$ref` under
it is a real finding all three clients were walking past. Also admit
the three content keywords to `CONSTRAINING_KEYWORDS` on the same
documented grounds as `format` — annotation-only by vocabulary, but
plainly an attempt to constrain, so reporting such a schema as
accepting anything would be noise. The comment now names that
category rather than singling `format` out.
- Make `handleError` async and await the envelope write. Round 6 made
the `--strict` report await its write and stopped one line short: the
envelope emitted for the very same throw still raced `process.exit()`,
so the machine-readable line a CI caller parses could be lost on a
pipe. The launcher's comment claiming the envelope was "well inside
the pipe buffer" was wrong reasoning and is replaced — fitting in the
buffer says nothing about whether the write was performed. Its call
site now awaits; the bin's `.catch(handleError)` needed no change.
- The CLI README called its sample block the "complete output" when the
error envelope follows it on every non-zero exit. Now scoped to "the
complete report", with the envelope named and linked.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 6.
- Share one traversal between `walk` and `declaresAnyId`. The `$id` guard
added last round descended through EVERY object value, including
instance data — `examples: [{"$id": "x"}]`, a `default`, a `const` —
where `$id` is a payload key declaring no embedded resource. Reading
one there suppressed `remote-ref` for the whole document, turning last
round's false-positive fix into a blanket false negative. Both passes
now go through `forEachSubschema`, driven by the same keyword tables,
so they cannot drift about what counts as a schema position.
- Await the stderr writes. `writeSchemaLintReport` used a bare
`process.stderr.write`, but both CLI exit paths call `process.exit()`
as soon as the work returns, which discards anything still buffered on
a piped stderr — truncating or losing the multi-block `--strict`
report on exactly the redirected-output runs a CI caller uses. Added
`awaitableError` beside the existing `awaitableLog` and awaited it.
The test's fake defers its callback so a synchronous one cannot make
the assertion vacuous; two existing stderr stubs that never invoked
their callbacks were fixed in the same pass.
- Show the CLI README's sample report in full. It printed one error
block immediately followed by the summary, a shape the formatter
cannot produce — every finding gets a block. Captured from a real run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4.
- Add `SCHEMA_UNPORTABLE` to `codeForExit`. `CliExitCodeError` lets a
caller omit the envelope, so without the case an envelope-less throw
reported the generic `error` — the same silent mislabelling the rename
was meant to prevent, one layer down. Covered via the envelope-less
construction that exposed it.
- Keep a local `type` (and the preset's `required`) alongside the remote
`$ref` in the showcase config. The TUI's `schemaToForm` dispatches on
`type`, so a `$ref`-only property fell through to a string field that
`add`'s numeric handler then rejected — making the README's "every
showcase tool is runnable" claim false there. The `remote-ref` finding
is unaffected, since that rule reads `$ref` independently.
- Restrict `type-union` to well-formed unions: non-empty, unique, and
all recognized JSON Schema type names. Its message asserts the
construct is *legal* JSON Schema, which is false for `[]`, `[3]` or
`["bananas"]`, and the `anyOf` it suggested for those would be invalid
too. Malformed schemas stay the SDK parser's business.
- Drop `if`/`then`/`else` from the presence-only allowlist. They only
constrain as a pair, so `{"if": {"const": 1}}` accepts every value and
was evading `untyped-schema`; the pairing is now checked explicitly.
- Distinguish the web sidebar severities by SHAPE, not just colour — a
circle for an error, a triangle for a warning — and name the severity
in the tooltip. Colour alone left a colour-blind reader unable to tell
them apart.
- Fix the stale "across 2 tools" count in the CLI README's hint sample.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
- Rename the `--strict` failure from "invalid" to "unportable"
(`EXIT_CODES.SCHEMA_UNPORTABLE`, envelope `schema_unportable`, and the
message text). The module argues at length that these schemas ARE
valid JSON Schema and are merely refused by some clients, so a
machine-readable code of `schema_invalid` told automated callers the
opposite. Done now, while nothing depends on it.
- Add a fixture-path integration case. Every other test built a
`ServerConfig` by hand, so `resolveConfig`'s `rawToolSchemas`
forwarding was uncovered and `unportable-schemas-http.json` was loaded
by nothing — the shipped fixture could have lost its overrides with
the suite still green. It now goes through loadConfig/resolveConfig
and asserts the exact per-tool verdict the README claims.
- Refresh the CLI README's sample output, which still showed the
pre-round-2 wording and fixture, and so dropped the contract warning
that rewording existed to add. Captured from a real run rather than
written by hand, plus a paragraph making the suggestion pattern
explicit.
- Drop the self-referential "[Try it](#unportable-tool-schemas)" link in
the root README, which pointed back at its own section.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2.
- Remove `non-object-root` entirely. It could never fire: the SDK types
`inputSchema` with `type: literal("object")`, so a tool with any other
root fails `ListToolsResultSchema` and `salvageListItems` drops it
before any client sees it. Verified against a live server advertising
`inputSchema: {type: "array"}` — `listAllTools()` returns []. A rule
that cannot fire is worse than none, because the docs then claim a
check the tool does not perform; the condition is already reported
through the malformed-items surface. Pinned by an integration test so
a change in that behavior is a signal rather than a silent gap.
- Fix the bare-`false` suggestion. `properties: {a: false}` forbids the
property; deleting the entry *permits* it with any value under the
default `additionalProperties`, so the advice inverted the schema it
replaced. Now recommends `{"not": {}}` and says why deletion differs.
- Exempt `not`-parented schemas from `untyped-schema`, so the `{"not":
{}}` above does not trip this module's own rule — under negation an
always-accepting subschema means always-reject, making "accepts any
value" the exact opposite of what it does. Scoped to that position,
not inherited by its subtree.
- Say plainly, in the `true` and untyped suggestions, that they narrow
the schema deliberately rather than implying an equivalent rewrite.
`true` has no portable object-form equivalent — `{}` is the very shape
this lint flags.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#1005
A tool schema can be perfectly legal JSON Schema and still be rejected by
the client the server is meant to run against — Go's `jsonschema` emits a
bare `true` for `interface{}`, which the SDK accepts and Claude Code
refuses with an opaque "Invalid input". The Inspector passed it through
silently.
Adds `core/json/schemaLint.ts`, a portability lint over a tool's
inputSchema/outputSchema, and surfaces one shared verdict everywhere:
the CLI's `--strict` report (exit 6 on an error-severity finding), the
TUI's tool list marker + detail pane, and the web Tools sidebar flag +
detail section.
Deliberately not a JSON Schema validator: a census of 617 public servers
found zero that fail the SDK's own parse, so a conformance check would
report nothing on real servers. Each rule is instead a construct that is
legal JSON Schema and known to be refused or mishandled by a shipping
client — a bare boolean in a schema position, an array-form `type`, a
remote `$ref`, a schema with no validation keyword, a non-object root.
`test-servers/configs/unportable-schemas-http.json` exercises every rule,
via a new `rawToolSchemas` override — the Zod-built presets cannot emit
any of these shapes, which is the same reason a real server only hits
this when its schemas come from another generator.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
The web client and the TUI both read `savedSettings.metadata` and pass it
as `InspectorClientOptions.defaultMetadata`. The CLI never did, and
`InspectorClient` only reads the option rather than falling back to
`serverSettings.metadata` — so a per-server `metadata` entry was silently
inert for `--cli` runs.
Pass it beside the existing `roots:`, guarded on a non-empty map the same
way web and the TUI guard theirs, so `{}` still means "no defaults".
Precedence is unchanged: call-time keys override defaults in `mergeMeta`,
so `--metadata` stays per-invocation and wins on a key collision.
Covered by three catalog-driven tests asserting against the test server's
recorded wire requests. `createTestConfig` is retyped `StoredMCPServer` —
the accurate on-disk shape — since a fixture that cannot express the
per-server keys cannot cover the settings that reach the client.
Closes#2093
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
One conflict, in `clients/tui/__tests__/App.test.tsx`: both sides added an
`opts` capture to the test harness's `FakeClient` for different fields —
`defaultMetadata` on v2/main (#1910), `environment.fetch` here (#2067) — so the
resolution is the union of the two, keeping both rationales and both tests.
Everything else auto-merged. `npm run ci` passes on the result.
Signed-off-by: cliffhall <cliff@futurescale.com>
Corporate-proxy users could not connect to any remote MCP server from v2.
Two independent defects, both reported in #2067.
1. The web and TUI bundles inlined undici. It was declared in the root and
`clients/cli` manifests but named in no client's tsup `external` list, and
tsup auto-externalizes only what the *nearest* manifest declares — so
esbuild inlined 1.05MB of it into `clients/{web,tui}/build/`. undici is
CommonJS, so the emitted chunk's `require("assert")` hit esbuild's ESM
`__require` shim and threw `Dynamic require of "assert" is not supported`.
The rejected `import()` surfaced as "the `undici` package is not available",
telling users to install a package that was already present — and could not
have helped, because the specifier had been rewritten to a relative chunk at
build time.
2. `withProxyDispatcher` injected a *userland* undici `EnvHttpProxyAgent` into
Node's *built-in* fetch. That couples two different copies of undici at the
dispatcher handler interface, which is not stable across majors: Node 22
embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8
agent handed a built-in undici 7 handler is rejected with
`fetch failed: invalid onRequestStart method`, and the request never leaves
the process. So fixing the bundling alone would only have converted the
web/TUI failure into the CLI's.
Both halves of the pair now come from userland undici — its `fetch` and its
`EnvHttpProxyAgent` — which keeps the handler inside one copy and works
unchanged from the Node 22.19 engine floor through Node 26. Deleting the
wrapper in favor of Node's native `NODE_USE_ENV_PROXY` was not an option:
measured, it is unsupported on 22.19 (the floor) and only lands mid-22.x.
Because undici's `Response` is a different class from `globalThis.Response`,
the proxied fetch re-wraps each response as a genuine global `Response`,
streaming preserved. Without that, `res instanceof Response` is false for
callers that test it — including the SDK's OAuth error formatter, which would
degrade every message to `Raw body: [object Response]`.
The proxy now sits at the bottom of the fetch stack (`environment.fetch` for
the Node clients, and the `createTransportNode` default for the web backend,
its one caller that supplies no `fetchFn`) rather than wrapping the transport's
fetch. Proxying means *substituting* the fetch, not decorating one, so wrapping
a supplied `fetchFn` would discard it; installing it underneath lets
InspectorClient's own wrappers compose over it. A side effect worth having:
OAuth discovery and token requests go through `environment.fetch` too, and were
never proxied before.
Guarding it:
- `undici` is added to all three tsup `external` lists and removed from
`clients/cli/package.json`, so it is root-declared only, per the placement
rule in AGENTS.md. The two installs had already drifted (root 8.5.0 vs cli
8.9.0).
- New `npm run verify:bundle-externals` fails if any package a client declares
external was inlined into that client's bundle anyway. It reads the *built
output*, not the config — those two disagreed for four releases — and derives
what to check from each client's own `external` array, so a newly
externalized package is covered without editing the guard. Verified against
the real pre-fix artifact, which it flags.
- The proxy tests now drive a real forwarding proxy rather than a mock. That
matters: defect 2 was a runtime interface mismatch, which no mock can catch
because no mock ever dispatches.
AGENTS.md's must-not-bundle rule (added by #2082) is extended with why undici
slipped past it, and notes the `node -e` trap — probing the bundled chunk with
`node -e` falsely succeeds, because it exposes a global `require` that
satisfies esbuild's guard.
Closes#2067
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#1910
`_meta` was typed `Record<string, string>` end to end in the Inspector —
on disk as a `{ key, value }[]` pair array, in the Server Settings form as
key/value rows, and in every request verb's per-call metadata parameter.
Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK
models the field as a `z.looseObject`, so the restriction was ours alone.
Widen the whole path to `RequestMetadata` (`Record<string, JsonValue>`):
- Per-server default metadata is a JSON **object** on disk and in memory.
The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an
existing `mcp.json` keeps working; it is never written back, so a save
migrates the entry.
- The Server Settings → Request Metadata section is a JSON editor
(`JsonObjectInput`) instead of key/value rows — rows cannot express an
object value. It keeps the typed text as the display source and emits
only parsed JSON, the same draft split `SchemaJsonField` uses, so the box
stays typeable; invalid text is flagged inline and not applied.
- The CLI stops flattening `--metadata` / `--tool-metadata` through
`metaValueToString`. Those values were already JSON-parsed, so
`trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}`
and now reaches it as an object.
- `servers/show` redaction replaces a sensitive key's whole value rather
than a string, so a structured value cannot leak through its members.
- `progressTokenOf` narrows the reserved `progressToken` key back to
`string | number` before it reaches the SDK; any other JSON still travels
inside `_meta` but wires up no progress callback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#1822
Adds docs/v1-to-v2-migration.md — the map for users arriving from v1:
- CLI flag mapping (every v1 flag survives; lists what v2 adds)
- `--config` vs `--catalog` semantics with before/after examples
- Node engine bump (>=22.7.5 → >=22.19.0)
- what no longer ships (the three sub-packages; v2 is one tarball)
- the three behavior changes most likely to bite: exit code 5 on a
failing tool call, target-must-come-first under --cli, and the
reversed `--` separator
- env-var mapping (MCP_PROXY_AUTH_TOKEN → MCP_INSPECTOR_API_TOKEN,
MCP_PROXY_FULL_ADDRESS removed, SERVER_PORT repurposed), the v1 UI
config settings, web query params, and Docker
Linked from the root README (banner + docs list), the CLI README, and
docs/mcp-server-configuration.md; AGENTS.md's docs/ tree entry updated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
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(core): register peer-request handlers before connect (#1797)
The roots/sampling/elicitation capabilities are advertised on the SDK
`Client` at construction, so from the moment `connect()` sends
`notifications/initialized` the server may issue any of those requests.
The handlers, however, were registered after the handshake resolved and
after `fetchServerInfo()` — leaving a window in which the SDK answered
`-32601 Method not found`.
`server-filesystem` asks for `roots/list` the instant it is initialized
(that is how it learns its allowed directories) and lost the race, so it
silently fell back to its CLI-argument directories instead of the roots
configured in the Inspector. `server-everything` asks later and did not.
Extract the four peer-request handler blocks into
`registerPeerRequestHandlers()` and call it before `client.connect()`.
None of them depend on server capabilities — only on constructor-set
state. The notification handlers that do gate on `this.capabilities` stay
in `connect()`, after `initialize`.
Regression test drives the race deterministically with a fake transport
that delivers `roots/list` synchronously from inside the `send()` of
`notifications/initialized`; it fails on the pre-fix ordering.
Closes#1797
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-1 fixes — pre-connect roots/list_changed, wider timing test (#1797)
Finding 1: `notifications/roots/list_changed` was the one handler in the
post-handshake block that gates on nothing but constructor state, so it
sat in the same race — a server emitting it right after `initialized` had
it silently dropped (no wire error; the symptom is a missed UI refresh).
Moved to a new `registerPeerNotificationHandlers()` called next to
`registerPeerRequestHandlers()` before `connect()`.
Finding 2: the timing test asserted one of the four moved handler blocks.
It now injects `tasks/list` and the `roots/list_changed` notification in
the same burst as `roots/list`, so moving any of the block back after
`connect()` fails the test. Sampling/elicitation stay out — they park a
pending request awaiting user input and have no reply to assert on.
Dropped the unused `sessionId`/`setProtocolVersion` transport fields.
Verified the added assertions are load-bearing: the notification
assertion fails against the previous commit, the `roots/list` one against
`origin/v2/main`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-2 fixes — correct the roots/list_changed premise (#1797)
The round-1 doc comment claimed a server may emit
`notifications/roots/list_changed` at initialize. It can't, normally:
`RootsListChangedNotificationSchema` is a member of
`ClientNotificationSchema` and absent from `ServerNotificationSchema` —
the client sends it (setRoots does, at :4251). The inbound handler is
defensive coverage for a non-conformant server, and dispatches
`rootsChange` with our own already-known roots. Reworded the method doc
and the test's header so neither states the false premise; the move
itself stands (it gates on no server capability, so it belongs with the
request handlers).
Also from review:
- Dropped the vacuous `notificationRejected` flag and its inverted
comment. The SDK does not throw for an unregistered notification, so
the try/catch could never observe one — confirmed by bisect, where the
pre-fix failure was always `rootsChanges`, never the flag. Removing the
catch also stops it swallowing a genuine throw from the injected
requests.
- Narrowed `replies` to responses (`"result" in message || "error" in
message`) so it can't also collect client-originated requests.
- Annotated the unreachable `if (!this.client)` guard with a justified
`v8 ignore` per the AGENTS.md dead-guard policy.
Re-verified both regression directions: the `roots/list` assertion fails
against `origin/v2/main`, the `rootsChange` one against `55e25b7c`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-3 fix — drop stale comments inside the roots handler (#1797)
The four inline comments moved verbatim with the handler contradicted the
method doc reworded one round earlier: they said "server's roots" (they
are ours — it is a ClientNotification) and "we'll just dispatch an empty
array", which is not what `this.roots || []` does when roots are
configured. The new timing test asserts exactly that non-empty dispatch.
Replaced with one accurate line. Comment-only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(cli): advertise roots so roots/set is actually serviceable (#1797)
Round-4 review found the same -32601 symptom on a second path: the CLI
constructed its InspectorClient with no `roots` option, so
`capabilities.roots` was never advertised and no `roots/list` handler was
registered — yet `--method roots/set` calls `setRoots()`, which announces
`notifications/roots/list_changed` to the server. A server taking up that
invitation got "Method not found", after the CLI reported success.
The review's suggested fix — register `roots/list` unconditionally — is
not implementable: the SDK asserts the client capability inside
`setRequestHandler` ("Client does not support roots capability"), so it
throws during connect for any client built without the option, and
`registerCapabilities` refuses to run after connect. A client that omits
`roots` therefore cannot legally serve `roots/list` at all.
Fixed at the layer that can fix it: `cli.ts` seeds `roots: []`, matching
web (`App.tsx` always passes it). The core registration stays gated on
the constructor value, now with the SDK constraint documented so the next
reader doesn't retry the unconditional version.
`setRoots()`'s doc claimed it "will enable roots capability if it wasn't
already enabled" — it cannot, for the reasons above. Corrected, and
dropped the dead `if (this.roots === undefined) this.roots = []` the very
next line overwrote.
Tests: a CLI test drives `list_roots` over an HTTP test server (verified
load-bearing — without the seed it fails on the -32601 error text), and
the core timing test gains a case pinning that roots set after connect
are served with the current values, not the constructor's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(cli,tui): advertise the roots configured in mcp.json (#1797)
Round-5 review: seeding `roots: []` fixed the -32601 but not the outcome.
`server-filesystem` discards an empty roots answer (`validatedRootDirs
.length > 0`) and keeps its CLI-argument directories — which is the
symptom this PR set out to fix. The user's roots were already in hand two
lines away, on `serverSettings.roots` (lifted from mcp.json by
`mcpConfigToServerEntries`). Now passed through `cleanRoots`, the same
shared helper web uses, so both clients answer `roots/list` byte-identically
for the same config.
This matters most on the CLI because it is one-shot: `--method roots/set`
sets roots on a connection torn down a few statements later, so the config
file is the only durable way to give a run its roots.
Same gap in the TUI (`App.tsx` built its options with no `roots` key at
all), fixed the same way. The review suggested a follow-up issue for it;
folding it in here instead, since it is the identical line and splitting
it would leave a known-wrong client in tree.
Also from review: hoisted `waiters` up with the other fields, and
`injectRequest` now rejects after 1s naming the method and id rather than
hanging to the vitest timeout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-6 fixes — document roots in the CLI README, harden cleanRoots (#1797)
- clients/cli/README.md enumerated the settings lifted from a catalog/config
file without roots. Roots are the least discoverable member of that list —
there is no flag, and `--method roots/set` dies with the connection — so
the file is the only durable way to supply them. Said so.
- cleanRoots() trusted its input's shape. `Root[]` is a compile-time type over
hand-editable mcp.json, and as of this PR all three clients feed it straight
from disk, so `"roots": [{"name":"Work"}]` or `"roots": "file:///work"` threw
at connect. Non-array bails to []; an entry without a string uri is dropped
with a warning. Hardened in the one shared place rather than three call
sites. Review suggested a follow-up issue; folded in for the same reason as
the TUI fix last round.
- Test comment said the CLI passes a `roots: []` seed — true one commit ago.
- injectRequest clears its timeout on reply (an armed timer outliving a test is
the #1760 teardown class), and setRoots' doc drops the "despite what this
comment used to claim" archaeology.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-7 fix — cleanRoots also throws on a non-string name (#1797)
The round-6 hardening guarded `uri` and left `name` throwing one line
later: `r.name?.trim()` guards null/undefined, not "is a string", so
`"roots": [{"uri":"file:///a","name":42}]` still died at connect — the
same crash class, one field over, reachable from all three clients and
the web settings-save path. Reproduced against the shipped function body
before fixing.
A bad `name` now costs the name, not the root (it is optional, and the
uri is still usable), with a warning. The doc no longer overstates what
is validated.
Also from review: the CLI test comment said "`cli.ts` now seeds
`roots: []`" — true only of that ad-hoc test's path; and the
registration comment's `(:588)` line reference is now a durable name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): normalize roots in setRoots() too (#1797)
Round-8 review: this PR made cleanRoots the single normalizer for roots at
connect and at save, but setRoots — the third way roots enter the client —
did a bare `this.roots = [...roots]`. Web cleans at its call site; the CLI
does not, so `--method roots/set --roots-json '[{"name":"Work"}]'` stored a
root with no uri, advertised it in the roots/list reply, and echoed it back
as success. cleanRoots is idempotent, so calling it inside setRoots covers
the CLI without disturbing web's pre-clean, and all three entry points now
agree.
Review filed this as an optional follow-up; folding it in for the same
reason as the TUI and cleanRoots items — it is smaller than the issue
describing it.
Test asserts the malformed entry is dropped and the valid one survives;
verified load-bearing against the bare-spread version.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-9 fixes — normalize roots in the constructor too (#1797)
The round-8 commit claimed "all three entry points now agree"; review
pointed out the constructor is a fourth, and the only one still raw. All
three clients clean at their call site, so nothing was broken — but the
invariant was enforced by convention rather than by the class, at the very
entry point this whole thread started from. `this.roots` now runs through
cleanRoots, with a ternary so the `undefined`-vs-`[]` distinction that
gates capabilities.roots survives.
Also from review:
- The CLI roots/set test provoked a cleanRoots warning without suppressing
it (AGENTS.md). Suppressed, and the spy now asserts the drop was
reported rather than silent.
- `enables roots when previously undefined …` in coverage-backfill was
named after behaviour round 5 established does not exist. Renamed, with
a comment on what that client actually can and cannot do.
- setRoots dispatches a copy to rootsChange, as getRoots() already did, so
a listener can't push into the list we advertise.
- run-method.test.ts's connectStdio now passes `roots: []` like cli.ts, so
the helper matches the client shape production builds.
Verified load-bearing: the constructor test fails against the raw
assignment. validate / coverage / build-gate / smoke / storybook all green
(run as separate stages — full `npm run ci` kept being killed mid-coverage
by the environment).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-10 fixes — copy at both rootsChange dispatches (#1797)
Round-9 claimed setRoots' copy closed the "a listener can't push into the
list we advertise" hole. It closed one of two sites: my reply conflated
the notification handler's *dispatch* with the roots/list *request*
handler — the latter is serialized to the wire immediately and is fine,
the former hands listeners the internal array. Both copy now.
Also from review: dropped the `({}) as never` in the new constructor test
for the real factory the sibling tests use (this test never connects, so
it is never invoked), and the needless `async` on it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-11 doc fixes; merge v2/main (#1797)
- cleanRoots' opening sentence still framed it as the settings-form
normalizer, contradicting the paragraph below it that says every client
feeds it from disk. It has four callers now; says so.
- Marked the two runtime guards `Keep:` with the reason. The `Root[]`
parameter narrows both to `never`, so "the compiler says this can't
happen" is the argument someone will use to delete the checks rounds 6-7
installed. The reason now lives where the deletion would happen.
- Noted the constructor-normalization case in the timing test's header, so
it is discoverable from a file otherwise about the initialized race.
Also merged origin/v2/main (#1796, loopback binding), which had advanced
by one commit and touched the smoke scripts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): clear queued peer requests when connect() fails (#1797)
Round-12 review found a second-order consequence of the handler move.
Before it, registration happened after every await in connect(), so the
moment a peer request could be queued the connect could no longer fail —
queue and failure path never overlapped. Registering before the handshake
puts both the handshake and the post-connect `setLoggingLevel` inside that
window.
The catch sets status to error and drops the transport but never cleared
pendingSamples/pendingElicitations; only disconnect() does, and connect()
reaches it solely on the connect-timeout path (not the default). So a
server that elicits at `initialized` and then fails the connect left a
live pending-request modal on a dead connection — web derives it from the
queue lengths with no status gate — and the elicitation's cancel() never
fired, hanging any awaitUrlElicitation waiter.
Extracted disconnect()'s clear-and-cancel into clearPendingPeerRequests()
and called it from the catch, dispatching the change events so the UI
drops the modal. CLI/TUI are unaffected either way (sample/elicit off).
Test note: the first version of this test passed with and without the fix
— its `requestedSchema` omitted `properties`, so the SDK rejected the
inbound params and nothing was ever queued. Fixed the fixture; it now
fails without the change with a queued ElicitationCreateMessage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): clear the peer-request queue on a mid-session crash too (#1797)
Round-13 review, two items.
The test asserted only `getPendingElicitations()`/`getPendingSamples()`,
which read the arrays directly — but the arrays are not what removes the
modal. `usePendingClientRequests` tracks its own state off the
pendingSamplesChange/pendingElicitationsChange events, so a regression that
cleared without dispatching would strand a live modal on a dead connection
and stay green. Both tests now assert the events fired with an empty
payload; verified by deleting just the dispatches, which fails them.
The connect-failure fix left the other way a connection ends uncovered:
a mid-session crash goes through the transport's `onclose`, which sets
status and fires `disconnect` but never touched the queues. Same two
hazards, and arguably more reachable. `clearAndAnnouncePendingPeerRequests`
wraps the clear plus its events, guarded on a non-empty queue so the paths
that overlap (a connect failure whose dropCachedTransport also fires
onclose) announce once; called from both. New test queues an elicitation,
fires onclose, and asserts the queue and the event — fails without the
change.
Review flagged the crash path as a follow-up; folding it in, as with the
earlier out-of-scope items — it is the same seam and three lines.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-14 fixes — make both teardown paths agree (#1797)
- The crash path's comment claimed "cleared before the `disconnect` event
so a consumer sees an empty queue" while disconnect() cleared *after*
its own dispatch — an invariant stated as a property and held at one of
two sites, the same shape as the round-10 finding. Moved disconnect()'s
clear above its status/disconnect block so both paths agree; its change
events stay batched with the other teardown dispatches. Full gate green,
so nothing depended on the old ordering.
- clearPendingPeerRequests' doc still named the `connect()` failure path as
a caller; that path now goes through clearAndAnnouncePendingPeerRequests.
Names the two real callers.
- The mid-session test waited on `await Promise.resolve()` — the
microtask-counting pattern this file replaced with injectRequest two
rounds ago, working today only because the enqueue happens exactly one
tick deep. Waits on the client's own `newPendingElicitation` instead.
- The file header still called the constructor case "the last case"; it is
third of five, and the two teardown cases went undescribed. Rewritten.
- Restored the missing blank line between two tests, and noted why the
sampling-event assertion holds for a queue that never held anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-15 fixes — scope the parity claim to the arrays (#1797)
Round-14 made the two teardown paths agree on when the queue is cleared,
and the comment then claimed parity flatly. It holds for the arrays but
not the events: the crash path announces before `disconnect`, while
disconnect() batches its change events with the rest of its teardown, so
they land just after. Since the events are what drives the modal, the
distinction matters to the next reader. Scoped the comment rather than
un-batching — un-batching would trade a documented ordering for a real
behaviour change with no gain.
Same rule as round 14, one level down: check the other site, then check
the other axis.
Also gave the new `newPendingElicitation` wait the timeout that
`injectRequest` documents, so a regression that stops the enqueue fails at
the wait instead of hanging to the vitest timeout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* test: pin disconnect()'s clear-before-announce ordering (#1797)
Round-14 moved clearPendingPeerRequests() above disconnect()'s status
block so a `disconnect` consumer sees an empty queue on both teardown
paths, and round 15 documented it — but nothing enforced it. The existing
`disconnect` listeners only count events, and the two teardown tests cover
the crash and connect-failure paths, neither of which goes through
disconnect(). So the one behavioural change in the stack was untested in
both directions, and the tidy that undoes it (regrouping the clear with
the batched change events) would have stayed green.
Test records the queue length from inside a `disconnect` listener; the
fixture's close() never fires onclose, so it exercises disconnect()'s own
ordering rather than the crash path. Verified against the pre-round-14
position: `expected 1 to be +0`.
Folded the newPendingElicitation wait into a shared helper now that two
tests use it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* test: pin disconnect()'s announce as well as its clear (#1797)
Round-16 pinned the array half of disconnect()'s teardown and left the
events unpinned — and nothing else in the suite covers them, so deleting
disconnect()'s two change-event dispatches would have left a live modal
after a user-initiated disconnect with the suite green. That is the path
most likely to have a modal open when it runs. Asserted after the await
rather than inside the disconnect listener, since disconnect() batches the
events deliberately; verified by deleting the dispatches (`expected
undefined to be +0`).
Header rewritten by category rather than position: it has now gone stale
twice in three rounds because it counted tests ("the last two") and
enumerated paths that a new test falsified. It names the three teardown
paths and what distinguishes them instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-18 doc fixes — scope the three-path claim, re-attribute cancel() (#1797)
The header I rewrote last round claimed all three teardown paths clear
before announcing. True only if "announcing" means the `disconnect` event:
the connect-failure path emits none at all, and both it and the crash path
clear after their `statusChange`. Scoped to what's true. Same shape as
rounds 10/14/15 — a parity asserted in prose that holds on one of the axes
it could mean — which is now the fourth time, in a sentence I wrote.
ElicitationCreateMessage.cancel()'s doc still credited `disconnect()`
teardown; this PR made clearPendingPeerRequests() its only caller, serving
three paths. Re-attributed, and promoted the no-`onRemove` note to its own
paragraph with the consequence spelled out — it quietly became a
three-caller invariant during this PR, and "don't splice mid-iteration"
reads like a style note until you know it would skip every other entry and
leave those promises unsettled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): gate roots/list on what was advertised, not on this.roots (#1797)
Round-19 review found a latent wedge, reproduced before fixing: a client
constructed without `roots` that later calls setRoots() has this.roots
defined, so registerPeerRequestHandlers tried to register the handler on
every subsequent connect() — and the SDK throws "Client does not support
roots capability" from setRequestHandler when the capability was never
advertised. That throw lands before the handshake, so the client can never
reconnect. Worse than the -32601 this PR started from: the connection is
gone rather than answering one method badly.
Not a regression (the same gate lived after the handshake before) and
unreachable from the three shipped clients, which all pass roots. But
core/ is a library surface, and the failure is unrecoverable.
Gate now reads a construction-time `rootsCapabilityAdvertised` — the thing
the SDK actually asserts on — so this.roots stays free to change. setRoots'
doc says what does happen on such a client: the roots are stored and
readable, but no server can ask for them.
Also scoped the header's claim that the connect-failure path emits no
`disconnect`: true of connect()'s own catch, but on a real transport
dropCachedTransport's close() fires onclose first, which does dispatch one
— making the catch's own call the auth-recovery backstop.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-20 — derive the roots gate from the advertisement itself (#1797)
Round-19's fix restored the invariant but not the derivation:
rootsCapabilityAdvertised was computed from `options.roots` alongside the
capability block rather than from it, so the two encodings of "did we
advertise roots" could drift. Adding any term to the advertisement
condition would leave the flag true with `capabilities.roots` absent —
the same unrecoverable wedge, reintroduced silently and in the throwing
direction.
Now read off the built capability object after it is assembled, so
divergence is structurally impossible rather than merely absent. Verified
the reconnect test still fails against the old `this.roots` gate.
Also: named the capability-lifetime category in the test header (the new
test was none of the three it listed), moved that test below the teardown
group so the group stays contiguous, and rewrapped the over-long comment
line.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): gate elicitation/create on what was advertised too (#1797)
Round-21 review asked round-20's question of the capability next door, and
there the two derivations already disagreed — no future edit needed.
`elicit` is typed `boolean | { form?, url? }`, so `{}` or
`{ form: false, url: false }` is valid, enables no mode, and advertises no
`capabilities.elicitation` — while the registration gated on `this.elicit`
being truthy. Reproduced before fixing:
CONNECT ERROR: Client does not support elicitation capability
(required for elicitation/create)
Since #1797 moved registration before the handshake, that throw means the
client cannot connect at all. Same wedge as the roots one, one capability
over, and reachable through a documented option value rather than a future
edit.
Gated on a new elicitationCapabilityAdvertised, derived from the built
capability object exactly as the roots flag now is. Test constructs with
`{ form: false, url: false }` and asserts connect() resolves; verified
load-bearing.
Also made the four construction-time capability inputs (sample, elicit,
receiverTasks, advertisedExtensions) `readonly`. The round-19 bug was a
mutator making a construction-time fact stale, so a future setter for any
of these should be a compile error rather than a review catch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): derive tasks.requests and the remaining gates from the advertisement (#1797)
capabilities.tasks.requests declares which server→client requests we accept
as tasks, but was built from `receiverTasks` alone — so
`{ receiverTasks: true, elicit: false }` told the server it could send a
task-augmented elicitation/create while advertising no elicitation
capability and (since the last commit) registering no handler. A server
taking the invitation gets -32601: the advertise-then-refuse shape this PR
opened with, and the last place it survived. Now built from
capabilities.sampling / capabilities.elicitation, both decided just above,
with the key omitted entirely if neither applies.
Also made the remaining three registration gates read the built capability
object like the two from the last two rounds — sampling, tasks/*, and the
URL-elicitation completion handler. None could diverge today, but a mixed
convention means the next reader has to re-derive which gates are safe, and
adding one condition to an advertisement block is exactly how elicitation
broke. Five gates, one rule now.
Test asserts a receiverTasks+elicit:false client advertises tasks.requests
with sampling and without elicitation; verified load-bearing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-23 — pin the omitted-requests arm, drop the count again (#1797)
The `requests` key is omitted when neither sampling nor elicitation is
advertised, but every `receiverTasks: true` construction in the tree left
one of them enabled, so that arm was unexercised — and it would not have
shown as a coverage failure, since one uncovered branch in a ~4900-line
file sits well inside the per-file gate. Second client in the same test
asserts `tasks` survives (list/cancel are still serviceable) while
`requests` is absent rather than empty; verified against `requests:
taskRequests`.
The header regained a numeral four commits after round 17 rewrote it by
category to stop exactly that, and it was already stale. Dropped, and named
the third category — advertise-only-what-you-serve, which is about the
capability object rather than a registration gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): settle queued samples on teardown instead of discarding them (#1797)
Round-24 review. Round 13 recorded the samples-vs-elicitations asymmetry
as deliberate on two grounds: nothing internal awaits a sample, and the
transport is gone by then. The first still holds; the second is false on
one path, and it is a path this PR created. `connect()`'s catch drops the
transport only when `!transportHasAuthProvider`, so on the HTTP+OAuth path
— web's ordinary authenticated case — it is retained and the caller
reconnects over it. A sampling request queued during the handshake was
then discarded with the connection still live: no response frame ever
written, and the server waits forever for a reply to a request we
accepted. Accept-then-silence, which is worse for the peer than the
-32601 this PR opened with.
SamplingCreateMessage.cancel() mirrors the elicitation one — settles
(rejects, what the UI's decline path already sends and the SDK turns into
an error response) without calling onRemove, since the caller iterates the
queue and clears it. clearPendingPeerRequests loops it like the
elicitations.
Also corrected two comments that assumed the catch always tore the
transport down, and noted that the retention is gated on
transportHasAuthProvider alone — independently of
isConnectAuthRecoveryError, which gates only the status hold.
Test asserts settlement rather than a wire frame, since on the disconnect()
path the transport is closed before the settle; a later respond() throwing
"already resolved or rejected" is the proof. Verified load-bearing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): mark receiver-task payload promises handled (#1797)
Round-25 review. clearPendingPeerRequests iterates every entry in
pendingSamples, and two shapes live there: the plain server-request one,
whose reject reaches the SDK handler, and the task-augmented one, whose
reject reaches the receiver task's payloadPromise. That promise only gets
a consumer when the server polls tasks/result, so a task sitting in
input_required has no handler attached — and the settle added last commit
surfaced as an unhandled rejection.
Not hypothetical: the repo already worked around it test-side, with a
helper that reaches into private state to attach no-op catches, whose
comment describes this exact race. Fixed at the source instead — one
`void payloadPromise.catch(() => {})` at creation, which also covers the
pre-existing cancelReceiverTask path. Removed the now-redundant helper and
its two call sites. Verified: dropping the source line makes that suite
report two unhandled rejections.
Corrected SamplingCreateMessage.cancel()'s doc — its "no response frame is
ever written" rationale is true of the plain shape only; a task-augmented
sample already answered the wire with a CreateTaskResult, so settling it is
about not leaving the task in input_required limbo.
Nits: generalized the enqueue wait helper to both events so the sampling
wait gets the timeout the elicitation one has; moved the sampling teardown
test back into the contiguous teardown group; named the settle property in
the header; fixed the fixture doc that still claimed to record replies.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-26 — drop the comment that outlived its call (#1797)
The suppressReceiverPayloadRejections call went last commit; its two-line
comment stayed, describing a pre-attach that no longer happens and
crediting the test for handling a rejection the source now handles.
Removed. Also reworded the surviving .catch() in inspectorClient.test.ts:
it is still needed, but as a value capture for the assertion, not for
unhandled-rejection suppression.
Rewrapped the teardown paragraph in the test header — my round-25 edit
left a 104-char line, and two hand-fixes left it ragged, so it is reflowed
in one pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-27 — skip the roots/list_changed the SDK would refuse (#1797)
Review reported setRoots() emitting roots/list_changed on a client that
never advertised roots, inviting a re-fetch we'd answer -32601. Probed it:
the premise is wrong. The SDK asserts the notification capability too —
`notification()` rejects with "Client does not support roots list changed
notifications" and nothing reaches the wire. No spec violation and no
invitation; what actually happened is that setRoots provoked that
rejection and logged it as "Failed to send", implying a transient failure
where the truth is structural.
Kept the early return on that corrected basis: it stops us provoking a
rejection we know will come, and warns accurately instead. Doc and comment
say the SDK is the enforcer rather than claiming this guard is what keeps
the wire clean.
The test pins the end state (no invitation for roots we can't serve) and
passes either way — recorded as such rather than presented as a regression
guard, since it isn't one.
Also moved the advertise-category test below the two gate cases, matching
the order the header introduces them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-28 — gate the notification on the predicate the SDK asserts (#1797)
The roots/list_changed guard read `capabilities.roots !== undefined`, but
the SDK asserts `capabilities.roots.listChanged` for the notification —
wider than the flag. Identical today (roots are only ever advertised as
`{ listChanged: true }`), but a conditional advertisement would un-gate it
silently, which is the round-21 shape again. Split into
rootsListChangedCapabilityAdvertised, mirroring
urlElicitationCapabilityAdvertised, which already reads its assertion's
sub-field; rootsCapabilityAdvertised keeps the roots/list registration,
where presence is what setRequestHandler checks.
Also moved the new test out of the gate pair it had split — it is a
runtime-emission case, not a registration-gate one — and extended the
header's converse-category paragraph to cover notifications, which indexes
it without inventing a category. Reflowed the setRoots doc paragraph in one
pass (111-char line from the last commit, plus a ragged orphan from an
earlier edit).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-29 — chase the corrected premise into clients/cli (#1797)
Round 27 corrected "setRoots announces roots/list_changed to a server that
then gets -32601" — the SDK refuses the notification from a client that
never declared it, so nothing reached the wire. That reword happened in
core/, but the same premise had been written into the two comments
justifying the CLI's roots seed, and they still asserted it. Both now state
the real pre-fix failure: a server asking for roots on its own got -32601,
and roots/set could not announce at all.
Also fixed the warn message under the round-28 gate: it named the roots
capability while the gate is on roots.listChanged — precisely the pair that
split was made to distinguish, so a client advertising `{}` would have been
told it advertised nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): reject in-flight raw-wire requests on a mid-session crash (#1797)
rejectPendingRawWireRequests had one caller — disconnect() — so the crash
path settled the inbound peer-request queue this PR added and left the
outbound raw-wire map alone. That map holds the modern tasks/* frames the
SDK's era gate refuses to route, so the SDK's chained _onclose doesn't know
about them: a Tasks-tab poll in flight when the server died waited out its
own 30s timeout and reported a timeout for what was a crash, on a
connection that had already dispatched `disconnect`.
Pre-existing — both the map and the crash path predate this PR. Fixing it
here because the invariant it violates is one this PR wrote down: "every
path that ends a connection has to clear that queue, announce it, and
settle each entry", in clearAndAnnouncePendingPeerRequests' doc and the
timing test's header. One channel was true of one path out of three.
Test starts a raw-wire request and fires onclose; without the call it
hangs to the vitest timeout, which is the symptom.
Checked the two siblings and left both: taskInputAbortControllers is
already unblocked via the elicitation cancel the crash path performs, and
activeToolCallAbortController rides an ordinary client.request the SDK
settles itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-31 test fixes — justify the cast, drop the no-op tick (#1797)
- The new test's `as unknown as` had no justification, which AGENTS.md
explicitly prohibits. It is unavoidable — rawWireRequest is private and
every public route is gated on isTasksExtensionNegotiated(), which needs
a modern handshake the fixture doesn't perform — so that reasoning is now
written down instead of left to be re-derived.
- `await Promise.resolve()` was a no-op: rawWireRequest registers its entry
in the promise executor, synchronously, before any suspension. Dropped,
which also makes the test pin that ordering rather than paper over it —
and stops the third recurrence of the microtask-counting pattern this
file's own helper doc argues against.
- Moved the test into the teardown group and extended the header's teardown
paragraph to cover the outbound direction, which it framed entirely as
inbound.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-32 — separate the outbound teardown claim from the inbound one (#1797)
My round-31 header edit spliced the outbound raw-wire passage into the
middle of the inbound claim, so "There are three: a failed connect(), a
mid-session transport close, and an explicit disconnect()" ended up reading
as scoping the outbound settle — which is two by design, since nothing
populates the map before the handshake. Round 31 established that scoping
so the code wouldn't imply an impossible state; the prose then un-scoped
it. Split into its own paragraph with its own scope clause, and reflowed
both in one pass rather than nudging breaks.
Also raced the raw-wire assertion, for the reason the file's own wait
helper documents: without the teardown rejection the test waited out the
request timeout and reported a bare vitest timeout. It now fails with
"raw-wire request not settled by teardown" — verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-33 — extract the timeout race, mark the failure-path promise handled (#1797)
Three copies of "reject after 1s so a regression fails where it happened"
had accumulated, each re-explaining itself. Extracted `withTimeout`, used
by the enqueue wait and the raw-wire assertion; injectRequest keeps its own
since its timeout also deletes the waiter entry.
On the failing path the raw-wire test left the production 30s timer armed,
so a regressed run got the intended failure plus a stray unhandled
rejection up to 30s later — the armed-timer class the file's own comment
names, on the failure path. `settled` is marked handled before the race.
Verified: with the fix removed the run fails with "raw-wire request not
settled by teardown" and no unhandled rejection.
Recorded why disconnect() drains the SDK's in-flight requests but rejects
the raw-wire map outright: the user asked to disconnect, and a Tasks poll
is not worth delaying that for. Pre-existing and deliberate, now written
down where the asymmetry is visible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-34 — drop a no-op guard and the false mechanism it asserted (#1797)
Round 33 claimed the raw-wire test could leave `settled` rejecting
unhandled on a regression run, and round 33's fix encoded that as
`void settled.catch(() => {})` with a comment explaining it. Both were
wrong: `Promise.race` attaches a rejection handler to every input, so
`withTimeout(settled, …)` on the next line already marks it handled.
Verified with a standalone script — no unhandledRejection fires, and the
pre-fix version was equally safe.
Removed the line and the comment. The comment mattered more than the line:
it told the next reader that `Promise.race` does not mark its losers
handled, which is false and would cost them elsewhere.
Kept the true half by setting `timeout: 50` on that test's client, so a
regression leaves no 30s timer armed and fails with `Raw request
"tasks/get" timed out after 50 ms` — more self-describing than the raced
generic message. Verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-35 — the drain comment described a trade-off nobody made (#1797)
Last round's comment said raw-wire requests "get no share of the drain
above: the user asked to disconnect, and a Tasks poll is not worth delaying
that for" — framing a decision. Verified it isn't one: `safeDisconnectTimeout`
defaults to 0 and the only caller anywhere that passes a value is a single
test, so nothing is drained in production; and the drain polls the SDK's
`_responseHandlers`, which never holds raw-wire ids, so those were never
eligible regardless.
Reworded to what is true. The framing came from my own round-33 reply,
which described the drain as running on every disconnect — a comment
faithfully encoding a half-read mechanism, which is the same failure as the
`Promise.race` one it followed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-36 — the race comment outlived the mechanism its own commit replaced (#1797)
`timeout: 50` made the request settle in ~50ms, so the 1s race can never
win and a regression fails on the assertion (`timed out after 50 ms` vs the
expected /Connection closed/) — which the commit message said, while the
comment beside it still credited the race. Reworded: the short timeout is
what fails fast and names itself, the race is the backstop for a promise
that never settles at all. Race kept; it covers the case where the
request's own timer is broken too.
Third comment in three rounds encoding a mechanism the same commit changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-37 — scope the timeout comment, align the bypass guards (#1797)
`timeout: 50` is the client-wide request timeout, not a bound on the
raw-wire request; it is inert in that test only because the test issues no
SDK request after connect. Noted, so someone adding one later knows it
would inherit a 50ms budget rather than reading the comment and looking
elsewhere.
The two installReceiverTaskResponseBypass guards read `this.receiverTasks`
while the five gates around them read the advertisement. Installing the
bypass mutates the Protocol's _requestHandlers map, so it is registration —
same as the setRequestHandler it follows. Equivalent today; aligned so the
convention is uniform rather than something the next reader has to
re-derive per site. The genuinely-runtime `receiverTasks` reads are left.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-38 — one predicate for the whole bypass mechanism (#1797)
Round 37 moved the bypass *install* guard to the advertisement and left the
three branches it controls on the raw option, splitting one immutable fact
across two sources. The dead direction is the harmful one: if the tasks
advertisement ever acquires a condition — the exact edit that produced the
elicitation wedge — then advertised goes false while receiverTasks stays
true, the bypass is not installed, and the handler still returns
`{ task }` into the SDK's validating wrapper, which rejects it. That is the
breakage the bypass exists to prevent, and before round 37 it could not
happen because install and branch read the same field.
All three now read tasksCapabilityAdvertised. `receiverTasks` is left where
it belongs: assigned once, and driving the advertisement.
Also noted that the wrapper's own check is redundant with its install guard
and kept deliberately, since it must agree with the handler branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): don't carry receiver tasks into the next session (#1797)
Round-39 review found the last member of the enumeration rounds 12-14 and
31 built: receiverTaskRecords was cleared only by disconnect(). The crash
path doesn't clear it, the connect() failure catch doesn't, and connect()
never reset it — so a record created in one session survived into the next
connect() on the same instance, and `tasks/list` is answered from that map.
A new server would be told about a task it never created.
Reachable in web (the only client with receiverTasks): a task-augmented
sampling/createMessage arriving in the handshake window creates a record;
if that connect then fails with a 401, App.tsx retries connect() on the
same instance.
Cleared at the top of connect() rather than only on the crash path — the
auth-recovery retry means ending a session isn't the only way a new one
begins, and starting clean covers every route in. disconnect() keeps its
clear via the same extracted helper.
Pre-existing — the crash path never cleared these — but the handshake
window this PR opened is what lets a receiver task exist before connect
resolves, and this was the one member left outside the invariant the PR
documented. Test verified load-bearing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): reset all session-scoped state on connect, not just receiver tasks (#1797)
Round-40 review: receiverTaskRecords was one of five collections cleared
only by disconnect(). Two of the other four have real symptoms on the same
crash-then-reconnect path:
- subscribedResources — the modern subscribeToResource early-returns on a
uri already in the set, so after a reconnect the user's Subscribe click
silently sends nothing to the new server while the UI keeps listing it.
- cancelledTaskIds — a leftover id mislabels a *new* task sharing it as
cancelled rather than failed, and server task ids are often short.
taskInputAbortControllers leaked un-aborted controllers.
modernLogLevel had the opposite asymmetry: disconnect() cleared it so it
couldn't leak (#1629), but nothing restored it, so a reconnect after an
explicit disconnect silently lost the user's configured level while a
reconnect after a crash kept a mid-session override. It is now recomputed
from serverSettings the way the constructor does, so both routes agree.
clearReceiverTasks became resetSessionState, called from the same place.
Tests cover the two symptomatic siblings and now assert receiver tasks
through the `tasks/list` handler — what a server actually sees, and the
symptom itself — which also removed the unjustified `as unknown as` the
previous commit added. The remaining cast is justified in place, since
cancelledTaskIds has no public reader.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(web): re-seed the Logs control when the client re-seeds its own level (#1797)
Round-41 review caught a consequence of the last commit. `resetSessionState`
restores the client's modern log level from the server setting at connect;
`resetSessionScopedUiState` still blanked the web control to null on every
disconnect. On the one path that reconnects the *same* instance —
auth recovery — the control then read Off while every modern request carried
`_meta` logLevel "debug", the default. A tool whose job is showing what you
send should not disagree with itself about what it sent.
The control now re-seeds from the active server's settings exactly as
setupClientForServer does, instead of blanking.
Also from review:
- Added the tests the restore never had, covering both halves: a mid-session
override does not survive a reconnect, and a disconnect no longer drops the
configured level (the #1629 half, and the actual behaviour change).
- resetSessionState's doc claimed "every piece of state scoped to one
connection", which invites assuming membership rather than checking it.
Now states the rule: state a new session would misread; what only needs
settling on the way out stays with the teardown paths.
- Header: session-scoping paragraph moved below the outbound one to match
test order, and it now names all five members rather than three.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* refactor(core): one derivation for the modern log level (#1797)
Round-42 review: the configured level was computed independently in four
places — twice in InspectorClient (constructor, resetSessionState) and twice
in App.tsx (the seed and the re-seed). The client and the Logs control
agreed only because four sites happened to compute the same expression, and
any drift reproduces exactly the symptom the previous commit fixed: the
control reading one level while every modern request stamps another.
Extracted resolveModernLogLevel into core/mcp/types.ts beside
DEFAULT_MODERN_LOG_LEVEL; all four call sites use it, with the web mapping
undefined→null at its own boundary (display, not a second derivation).
Also from review:
- disconnect()'s comment still credited itself with preventing the #1629
leak, which resetSessionState now prevents. It says what the line does
now — read "not opted in" while disconnected — and notes the web control
deliberately shows the configured level in that window.
- resetSessionState's doc claimed an un-aborted taskInputAbortControllers
entry "leaks a paused poll loop". Both registration sites release in a
finally, so nothing leaks permanently; the abort closes the window
between a crash and the unwind. Softened, and it now has the test it
lacked — the only member of the five without one.
- Noted in App.tsx that the re-seed depends on activeServerIdRef still
holding the outgoing id (it syncs in a passive effect), so an eager clear
would silently drop it to the default.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): reset the subscription stream state with the set it derives from (#1797)
Round-43 review: resetSessionState cleared subscribedResources but left
modernStreamState, so a crash-then-reconnect on the same instance held an
empty set alongside `{ active: true }` — a combination the rest of the file
treats as impossible, since onModernSubscriptionClosed and
onModernReconnectFailed both compute `active` from `subscribedResources.size
> 0`, and the comment at :4712 asserts a terminal drop clears both. No
symptom today only because the UI mirror resets itself independently, in
another file — which is the arrangement this PR has spent its rounds
removing. Clearing it here also announces the reset; that set's every other
mutation dispatches.
Also from review:
- The web re-seed treated "no server found" as the default level rather
than Off, because resolveModernLogLevel returns the default for absent
*settings*. Reachable via a second disconnect after a crash, and via
removing the active server. Now distinguishes the two.
- Converged the two injectRequest copies: withTimeout takes an optional
onTimeout, so both drop their waiter on the timeout path. The newer copy
had lost exactly the cleanup that was round 33's reason for leaving the
original alone.
Test seeds the stream state a live subscription would have left; verified
load-bearing (the earlier version passed either way, since the fixture
never activates a stream).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(web,core): correct the re-seed branch; share the subscription-stream reset (#1797)
Round-44 review. My round-43 fix traded one wrong branch for another: it
read `?.settings`, which is undefined both when no server matched *and*
when the server has no settings node — and the second is the common case
(a hand-written mcp.json entry never opened in Server Settings). Those
servers showed Debug while connected and Off after disconnect, which is
exactly the client/UI disagreement rounds 41-42 removed, with no
modernLogLevelChange event to reconcile it. Now branches on the server.
Worth noting no test would have caught it: App.tsx is a documented
exclusion from the coverage gate.
Also from review:
- resetSessionState cleared subscribedResources without announcing it,
two lines below a comment arguing that every other mutation of that set
dispatches. And it took two statements of a six-statement cluster
disconnect() clears together — leaving modernReconnectAttempts (a new
session's first drop would back off as if it were the old one's nth),
modernListenGeneration (the bump that makes an in-flight re-listen bail),
and a stale modernSubscription. One resetSubscriptionStream helper now
serves both, announcing both axes; disconnect() keeps only the live
stream's best-effort close. That also removed the file's one assignment
to modernStreamState that bypassed its setter.
- The stream-state test pinned the value, not the dispatch — the half the
UI actually tracks. Now asserts both; verified against a bare field
assignment.
- onModernSubscriptionClosed's comment claimed the disconnect reset always
clears the set; on a crash it doesn't until the next connect. Says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-45 — reattach the orphaned doc, order the announcements (#1797)
Extracting resetSubscriptionStream last round inserted it *and its doc*
between resetSessionState's doc and resetSessionState — so two doc blocks
stacked, the new helper took the nearest, and the 23-line block explaining
each member's symptom attached to nothing. Dead text where it sat, and the
most load-bearing comment in the batch. Moved, not reworded.
That is round 32's finding in production code, from the same cause:
inserting anything between a doc block and its method is a move that needs
checking, prose or code.
Also from review:
- resetSubscriptionStream announced the cleared set before updating the
stream state derived from it, so a listener in between saw an empty set
with an `active` stream — the combination the helper exists to prevent,
exposed by its own dispatches. Both fields move first, then both are
announced. The onModernSubscriptionClosed comment now says that window
is never observable, which is newly true.
- The test header called the log level "cleared"; it is re-seeded, which is
the #1629 half of the test twenty lines below. Says "reset", with the
distinction spelled out, and the paragraph reflowed in one pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* fix(core): close the listen stream the connect-path reset drops (#1797)
Round-46 review: resetSubscriptionStream's doc claimed disconnect() is the
only caller with a stream worth closing, and the helper did a bare
`modernSubscription = null`. Not guaranteed on the connect path — an
`onerror` without an `onclose` leaves the transport up, connect() reuses
it, and the reset then drops the last reference to a stream still open on
the server. Nothing can close it afterwards: the `closed` handler bails on
the bumped generation, and `closed` never resolves for a live stream.
Before the extraction the surviving reference was eventually closed by
refreshModernSubscription as `previous`, so this was newly unclosable.
The helper now closes it itself, best-effort, after the dispatches so the
round-45 ordering is unaffected; disconnect() drops its two hand-rolled
lines.
Also from review:
- The session test pinned the subscription set's value but not its
dispatch — the half the UI tracks, and the gap round 43 closed for the
stream axis one line away. Both now asserted; verified load-bearing.
- resetSessionState's doc said disconnect() "clears all of this", which is
true by duplication for two of the five members; it now says a sixth has
to be added in both places. And "the teardown paths" handle the in-flight
tool call only on the disconnect route — on a crash it is callTool's own
finally. Named.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-47 — fix the doc's count, catch the closed-promise chain (#1797)
- resetSessionState's doc said "two members through the same helpers, the
other two hand-rolled" for a body with five. It is two and three — and
the member the enumeration dropped is the one that is *paired* rather
than duplicated: modernLogLevel is re-derived here and blanked in
disconnect(), deliberately. Both corrected.
- `subscription.closed.then(...)` had no rejection handler, unlike every
other promise around that stream. Newly reachable: before the last
commit a connect-path reset dropped the reference with `closed`
permanently pending, so the chain never settled at all; closing the
stream is what makes it settle. Added, with the Node-terminates reasoning
the file already applies to payloadPromise.
- The listen-stream test exercised transport reuse without asserting it —
the half that makes "still open on the server" true. Counts factory calls
now, so the premise fails loudly if a future change recreates the
transport rather than silently ceasing to apply.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-48 — scope the catch, guard the close, index the test (#1797)
- The .catch comment claimed the connect-path close made a *rejecting*
`closed` reachable. It doesn't: closing resolves it. What that close newly
reaches is the handler running at all, where the reference used to be
dropped with `closed` pending forever. And chaining after `.then` also
swallowed handler throws — silently abandoning a re-listen on the path
whose job is recovering a dropped stream. Now the second argument to
`.then`, scoped to the rejection, with the comment saying which half is
which.
- `closing?.close()` reaches into third-party code from `disconnect()`'s
straight-line teardown, none of which is inside a `try` — a synchronous
throw would skip the raw-wire rejects, the task aborts, the receiver-task
clear and nine dispatches, silently, since most callers catch a
disconnect(). Best-effort now against both failure modes.
- Header indexes the listen-stream test: same category, but a release
obligation rather than a misread one.
- Its comment described the generation guard, which the stub can't exercise;
marked as production behaviour.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TrCvpunxToe7qnppk6zwr
* review: round-49 — pin the best-effort close, unorphan the doc (#1797)
Round 48 made `resetSubscriptionStream`'s close best-effort against both a
rejected promise and a synchronous throw, but nothing exercised either arm:
both `modernSubscription` stubs in the tree resolve. On the `disconnect()`
path an escaping failure would silently skip the raw-wire rejects, the paused
task-input aborts, the tool-call abort and `clearReceiverTasks()` — the
findings rounds 13 and 30 closed, re-opened at once with a green suite. Two
table-driven cases now seed a `close()` that throws and one that rejects,
and assert teardown continued past it.
Also: the `.then` comment gave opposite verdicts on one mechanism (an
unhandled rejection is named unacceptable, then chosen for a handler throw) —
reworded to say the handler cannot throw today and why the shape is
defensive; and `refreshModernSubscription`'s doc block, orphaned two methods
up since before this branch, moved onto the method it describes.
Closes#1797
* review: round-50 — sweep the queues start-clean, unify the close (#1797)
`resetSessionState`'s doc excluded the peer-request queues and the raw-wire
map because "the teardown paths handle them" — true of the paths that run.
An `onerror` without an `onclose` runs none of them: it flips status to
"error" and leaves `baseTransport` cached, so a `connect()` on the same
instance reuses a live transport. That is the route the round-46 stream close
was built on, and it strands these two the same way — the web pending-request
modal is derived from the queue length with no status gate, so it outlives
the session, and answering it writes a response for the old request id onto
the new connection. Both are now swept start-clean beside the reset (both
helpers are idempotent, so the routes that already ran them are unaffected),
the end-clean clears stay put for the `disconnect`-event consumer, and both
docs say which is which.
Also folds the three subscription `close()` sites onto one
`closeSubscriptionBestEffort` helper, so all three absorb a synchronous throw
as well as a rejection — at `refreshModernSubscription`'s first site the
reference is already dropped, so a throw there both leaks a live stream and
aborts the refresh before its replacement is opened.
Closes#1797
* review: round-51 — pin the sweep's ordering, name the category (#1797)
The connect-start sweep must sit after `resetSessionState()`, and the two
calls read as independent. Cancelling a task-augmented peer request settles
it synchronously into the record callback, which ends in `upsertReceiverTask`
— a no-op only because `clearReceiverTasks()` just emptied the map. Hoisted,
it would emit a `notifications/tasks/status` for the outgoing session's task
onto the transport the connect is about to reuse. Stated at the call and
pinned by a test (which fails on the hoist).
Adding that third caller also made five docs stale, all describing the old
set by count or as "teardown" — the class round 23 diagnosed. All five now
name the category: every route out, plus the top of `connect()` as a backstop
for the one route in that settles nothing.
And the site where round 50's fold actually changed behaviour —
`refreshModernSubscription`'s first close, where the reference is already
dropped and a `.catch()` never caught a synchronous throw — is now tested
from the public surface (a re-list over a poisoned stream), table-driven over
both arms. Verified to fail on the pre-fold form.
Closes#1797
* review: round-52 — say what the sweep prevents, scope the close doc (#1797)
The sweep's comment named as its harm the exact write the sweep performs: on
this route the SDK's transport is still live, so cancelling a queued peer
request emits a response for the old id immediately — which is right, and is
the settle-don't-discard rule, at the best available moment. As written it
read as an argument against emitting the frame, inviting the clear-without-
settling regression rounds 24–25 removed. It now names the real hazard: a
*user*-authored answer landing arbitrarily later, past the re-handshake.
`closeSubscriptionBestEffort`'s "at each site" held at two of three — the
superseded-generation discard stored nothing and has nothing following it,
so its escaping failure would only reject an already-superseded
`subscribeToResource`. Scoped, still wrapped.
Also moves the `disconnect()`-side close pair next to its `connect()`-side
twin, and states which axis the file's ordering follows, since route and
category both describe it and cut differently.
Closes#1797
* review: round-53 — give each close site its real harm, pin it (#1797)
The doc grouped `resetSubscriptionStream`'s close with the "takes the
caller's remaining work with it" sites. It can't: the call is `void`-ed onto
an `async` helper, so even a synchronous `close()` throw becomes a dropped
rejection — `disconnect()`'s teardown runs regardless. The harm there is an
*unhandled* rejection, which this PR already established (the payloadPromise
fix) is the process-fatal class in Node — so the one site with the severe
failure mode was documented as having the mild one. Each site now carries its
own consequence, and the local comment no longer gives two reasons that
cancel each other.
The test had the same gap: both its assertions hold with the helper's
try/catch deleted. It now also asserts no unhandled rejection fired, which
does fail without the guard (verified on both arms); the teardown assertions
stay, since they still catch unwrapping the call to a bare `close()`.
Also names the two back-references the round-52 move left positional, and
softens the "on the wire until dropCachedTransport()" claim — that drop is
conditional and never runs on stdio.
Closes#1797
* review: round-54 — the mildest site isn't mild from every caller (#1797)
`refreshModernSubscription` has three callers, and the superseded-generation
discard's failure reaches all of them. From `unsubscribeFromResource` — which
deliberately keeps the removal when a re-listen fails — it surfaces "Failed
to unsubscribe" for an unsubscribe that actually stuck, which is worse than
the consequence the doc assigned to the site it called mildest. The re-listen
bullet had the mirror gap: on the reconnect caller the escape is absorbed
into the backoff run, not a set left with no stream. Both now name their
callers, and the generation-bump claim is softened to what it proves (a newer
refresh *started*).
Test: the `unhandledRejection` listener is process-wide, so the assertion now
filters to this fixture's own reason rather than failing on a rejection some
other test left pending; and it is registered inside the `try` whose `finally`
removes it, so a throw in the setup can't leak it into the rest of the file.
Re-verified the filtered form still fails on both arms with the helper's
try/catch removed.
Closes#1797
* fix(core): don't roll back a subscribe a newer refresh superseded (#1797)
`subscribeToResource`'s rollback assumes what its comment says — that the
server filter is unchanged — which is exactly what supersession falsifies.
The newer refresh built its filter from the set *including* this URI, so on
success the server honors it; rolling back leaves the set missing a URI the
live stream carries (the UI showing it unsubscribed while its
resources/updated keep arriving) and, if it was the only one, an empty set
with an active stream — the combination `resetSubscriptionStream` exists to
prevent, reached from the other end.
Reachable without any close() failure: a rejecting `listen()` propagates the
same way, which a user subscribing while the reconnect timer re-lists — i.e.
exactly when the server is flaky — can produce. The rollback is now gated on
the generation not having advanced past this call's own bump; the error still
reaches the caller either way. Test drives two overlapping subscribes with
the first `listen()` rejecting, and fails without the gate.
The doc bullet that called this the mild site is corrected with it: both
user-initiated callers report a failure that did not happen, in mirrored
ways. Also gives the two close-failure arms distinct reasons so the
unhandled-rejection filter can exclude the sibling arm as well as a foreign
rejection.
Closes#1797
* fix(core): reconcile the stream badge when a re-listen this call owned fails
The supersession gate left the both-fail case stuck: two overlapping
subscribes whose listens both reject leave the superseded URI in the set (its
rollback correctly skipped) while the state keeps the optimistic
"connecting" — no stream, no reconnect armed (neither onModernSubscriptionClosed
nor onModernReconnectFailed ran), so the badge never changes and no
resources/updated ever arrive. That is the caveat this file already wrote
down for the neighbouring case — the bump proves a newer refresh *started* —
applied to the gate itself.
Fixed in the branch that knows the answer: the call that still owns the
re-listen reconciles the state instead of only handling an emptied set,
reusing the "ended" badge onModernSubscriptionClosed already reports for
"stream gone, URIs still subscribed". unsubscribeFromResource gets the same
treatment — it rolls nothing back by design, but its failed re-listen left
the badge reading Listening over no stream.
Also: the gate's comment named one of the two things that bump the
generation (resetSubscriptionStream also does, from disconnect and the
start-clean reset) and cross-referenced the wrong invariant; both corrected,
and `<=` tightened to `===`, which is what the monotonic synchronous bump
actually guarantees.
Closes#1797
* fix(core): order the subscription writes, retry instead of stranding
Two sites emptied the subscription set and announced it before the stream
state caught up — the last-URI rollback in subscribeToResource's catch, and
(pre-existing) the ordinary last-URI unsubscribe, which dispatches a
round-trip before the re-listen sets INACTIVE. Both exposed an empty set with
an active stream: the pair resetSubscriptionStream orders its own writes to
prevent, and which three comments in this branch now cite. State before the
announce at both, asserted from the consumer's side.
And the reconcile added last round settled for an honest but dead badge: it
was the only route to "stream gone, URIs live" that had made no attempt at
all, over subscriptions the server may never have honored, recoverable only
by an Unsubscribe/Subscribe toggle (a fresh Subscribe early-returns). It now
hands them to the reconnect machinery, which already fits — base delay after
a user-initiated refresh, bails on a terminal status or an emptied set, and
lands on the same "ended" badge past the cap. That also removes the
provably-dead null check the reconcile carried.
Closes#1797
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes#1795.
Enforce localhost-only binding in code (the "never 0.0.0.0" rule was only a spec note): refuse an all-interfaces HOST — across every spelling the OS binds as the wildcard (inet_aton decimal/octal/hex, IPv6, IPv4-mapped, zone-scoped, IDNA/Unicode) — unless DANGEROUSLY_BIND_ALL_INTERFACES=true (Docker's sanctioned opt-in). Fixes the reported stdio-connect 403 by expanding the default loopback origin allow-list to all three interchangeable forms.
Hardening surfaced across an extended review: closes an IDNA/fullwidth wildcard bypass, a blank-ALLOWED_ORIGINS fail-open, and the CLI/TUI OAuth-callback wildcard bind (loopback-only now, RFC 8252); canonicalizes the origin allow-list the way a browser does; derives frame-ancestors from the real embedder list (IPv6-literal exclusion verified in headless Chromium); strictPort on all three bind points; and preserves the CLI exit-code map + JSON envelope through the mcp-inspector --cli launcher path. Shared host normalization lives in core/node/hostUrl.ts. Docs + tests + smokes throughout.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
* feat(cli): catalog listing, auth flags, and OAuth browser UX
Add servers/list and servers/show, --relogin / --stored-auth-only, browser
OAuth navigation with OSC 8 links, and extract method handlers under
clients/cli/src/handlers. Tracks #1781.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): address #1782 review — relogin clear, stored-auth-only, docs
Clear both raw and normalized OAuth keys on --relogin; check the shared
store before bailing under --stored-auth-only mid-session; restore the
tests README link target; document servers/show redaction limits; keep
mcpi groundwork with TODO(#1432) markers.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): gate OAuth browser open and harden #1782 review follow-ups
Arm auto-open only for CLI-owned interactive OAuth, honor MCP_AUTO_OPEN_ENABLED,
move --relogin clear after validation, skip keychain on servers/list, and cover
secret redaction after rehydrate.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): silence disarmed OAuth URLs and round-3 review nits
Suppress authorize URL print when navigation is disarmed or
--stored-auth-only; document MCP_AUTO_OPEN_ENABLED; redact Cookie/auth
headers; keep servers/show unit tests off the OS keychain.
Co-authored-by: Cursor <cursoragent@cursor.com>
* test(cli): cover withArmedAutoOpen for the ≥90 branch gate
CI failed on cliOAuth.ts branch coverage after the auto-open arming
helper landed; exercise the armed path through runCliInteractiveOAuth.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): mid-RPC unauthorized recovery and clear round-4 nits
Mirror connect's UnauthorizedError path in withCliAuthRecoveryRetry, let
MCP_AUTO_OPEN_ENABLED=true force open off-TTY, mock open-url in the
in-process runner, reject --relogin on catalog methods, restore the
__tests__ README inventory, and narrow OAuth helpers to structural client types.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): reconnect after mid-RPC OAuth and finish round-5 nits
Reconnect before retrying after unauthorized recovery, guard non-OAuth
configs, decouple forceAutoOpen, move open-url mock to vitest setupFiles,
and reject --relogin with stored-auth short-circuits.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): fail fast on non-TTY interactive OAuth and round-6 nits
Skip interactive OAuth without a TTY unless MCP_AUTO_OPEN_ENABLED=true;
document --stored-auth-only for CI; redact settings.metadata; reject
--relogin for stdio; clarify AuthRecoveryRequired reconnect comments.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): admit interactive OAuth when stdin is a TTY (round-7)
Gate on stdin||stderr so `2>&1 | tee` still works; fold confirm/auto-open/
TTY into CliOAuthConnectOptions; rename authRequiredFailure; document
MCP_AUTO_OPEN_ENABLED=true as non-TTY admit.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): fail fast on step-up without stdin (round-8)
Decline [y/N] on readline EOF/close, require a stdin TTY for the default
step-up confirmer (force-open admit is not enough), document stderr-only
browser auto-open vs stdin||stderr flow admit, and drop the alias wrapper.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): allow piped step-up answers; bound silent pipes (round-9)
Drop the hard stdin-TTY gate that broke `echo y | …`. Keep EOF decline and
add a short non-TTY timeout so an open pipe that never writes cannot hang.
Clarify isTTY as admit-only; extract the readline mock factory.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): keep partial piped step-up answers (round-10)
Capture readline `line` before close so `printf y` without a newline is
not treated as decline; timeout is auth_required with a distinct message;
document the 5s bound and post-confirm callback wait; pin the TTY no-timeout arm.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): tighten step-up timeout messaging and TTY test (round-11)
Make the TTY no-timeout assertion use a settled flag (the Promise.race
check was vacuous), say "timed out" instead of "declined" on the pipe
bound, and document that piped answers need a newline or stdin close.
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
- Refresh failure now falls back to injecting a still-present stored access
token instead of hard-exiting 3, so a transient auth-server hiccup doesn't
regress a previously-working invocation (review #1). Only a failure with no
access token to fall back on surfaces as exit 3.
- Persist rotated tokens via the shared writeStoreFile (0o600 + mkdir -p)
instead of a raw writeFile, so the secrets file keeps owner-only perms
identical to the web backend (review #2).
- Missing clientInformation now throws the distinct no_client_information
envelope code rather than reusing no_stored_token (review #5a).
- Docs: --wait-for-auth no longer claims parity with --use-stored-auth (it
injects the freshly-landed token without refreshing); noted the
always-refresh rotation/concurrency caveat (review #3, #4).
- Tests: discovery-returns-undefined metadata path, no_client_information
code, and an e2e refresh-failure → access-token fallback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
Closes#1665
`--use-stored-auth` previously injected the stored access token blindly, so an
expired token failed even with a valid refresh_token on disk. Now, when a
`refresh_token` is stored, the CLI runs the OAuth refresh grant and injects the
fresh access token, persisting the rotation.
- New exported `refreshStoredAuthToken(serverUrl, statePath, deps?)`: reuses the
SDK's `refreshAuthorization` (not a hand-rolled token request) with the stored
`clientInformation` + `serverMetadata` (discovered via core
`getAuthorizationServerUrl` + SDK `discoverAuthorizationServerMetadata` when
the metadata wasn't persisted). Rotated tokens are written back through
`serializeOAuthPersistBlob` under the same `{servers,idpSessions}` key, so web
and CLI stay consistent. Missing refresh_token/clientInformation or a failed
grant throw `CliExitCodeError(AUTH_REQUIRED)` with a clear message + envelope
code (`no_stored_token` / `refresh_failed`).
- `--use-stored-auth` prefers refresh when a refresh_token is present (covers
both absent and expired access tokens — the stored blob has no expiry, so the
refresh token is the durable credential); falls back to injecting the stored
access token otherwise (unchanged).
- `deps` are injectable so the refresh/discovery are unit-tested without a live
token endpoint; an e2e test drives the real SDK path against a mock token
endpoint and asserts the refreshed token is injected + the rotation persisted.
- CLI README updated (removed the "blind injection" caveat).
CLI coverage gate green (cli.ts 100/92/100/100).
Part of the #1579 decomposition (Wave 4, CLI lane).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
Closes#1666#1575 shipped `--print-handoff` with an interim deep-link shape (hardcoded
`transport: "http"`, marked `TODO(#1576)`). Now that #1576 owns the canonical
`?serverUrl&transport&autoConnect` format, `--print-handoff` emits exactly it:
- New `deepLinkTransport(serverUrl, transport)` helper derives `http`|`sse`
from the resolved server — explicit `--transport` wins, else auto-detect from
the URL path (`/sse` → sse), else default `http`. Unlike the core resolver it
defaults instead of throwing (the handoff is best-effort; the web parser
defaults an unknown transport to http too).
- `buildHandoff` takes the transport, emits the normalized serverUrl +
derived transport in the query, and keeps `autoConnect=<token>` (which #1576
adopted as the canonical CSRF gate, so the interim guess was already correct).
- Removed the `TODO(#1576)` marker; updated the doc comment + CLI README.
Tests: handoff emits `transport=http` for `/mcp`, `transport=sse` for `/sse`
and for explicit `--transport sse`; `deepLinkTransport` unit tests cover the
explicit/auto/default/stdio/unparseable branches. CLI coverage gate green
(cli.ts 100/92.8/100/100).
Part of the #1579 decomposition (Wave 4).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* feat(cli): programmatic ergonomics — initialize, tool-args-json, connect-timeout, env-var fixes
Adds the #1573 programmatic-ergonomics surface to the CLI, re-implemented
informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts:
- `--method initialize` — connect-only probe emitting
`{serverInfo, protocolVersion, capabilities, instructions}`.
- `--tool-args-json '{"k":"v"}'` — tool args passed verbatim as JSON (no
key=value coercion); mutually exclusive with `--tool-arg`, with clear
errors for combined use, malformed JSON, and non-object values.
- `--connect-timeout <ms>` — default 15000 for ad-hoc `--server-url`/target
runs, file-level timeout otherwise; `0` disables. Backed by the exported
`withConnectTimeout()` helper.
- MCP_CATALOG_PATH is ignored when an ad-hoc target is given, so a shell that
exports it can still run one-off ad-hoc invocations.
- MCP_STORAGE_DIR is honored by core `getStateFilePath` (`<dir>/oauth.json`),
below the per-file MCP_INSPECTOR_OAUTH_STATE_PATH override.
Tests: new programmatic-ergonomics.test.ts (CLI, in-process runCli + direct
withConnectTimeout unit tests) and getStateFilePath resolution cases in the
web storage integration suite. Per-file coverage gate stays ≥90.
README: documents the new flags and env-var semantics.
Closes#1573
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* feat(cli): --app-info probe + --format json envelope
Adds the #1574 app-probing and machine-readable output surface, re-implemented
informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts.
- `--method tools/call --tool-name <t> --app-info` — prints one JSON line
describing the tool's app posture (hasApp, resourceUri, csp, permissions,
domain, …) WITHOUT invoking the tool; exit 0 when it has an app, 2 (no_app)
when it doesn't. A missing tool exits 5 (tool_not_found) — distinct from the
no-app exit so a typo isn't mistaken for "no app".
- `--method tools/list --app-info` — one NDJSON line per tool over a single
connection (`| jq -c 'select(.hasApp)'`).
- `--format json` — a single stdout envelope (`{result}` plus `{appInfo}` as a
sibling key for App tools), no banner, so any method pipes into jq.
- A tools/call returning isError:true now exits 5 (tool_is_error) after
printing its payload, so && chains stop on a failed call.
Implementation: `collectAppInfo()` (core `extractAppInfo` + a tolerated
resource read that reports `resourceError`), an NDJSON loop in tools/list, and
a single `emitResult()` that owns format + exit-code handling. `runMethod` now
returns a `MethodOutcome` discriminated union so the NDJSON path can signal
"already emitted".
Tests: app-info.test.ts (exit-0/2/5 + NDJSON + no-invoke), format-json.test.ts
(envelope shapes + bad --format), emit-result.test.ts (direct unit coverage of
the isError→exit-5 and resourceError branches via exported emitResult/
collectAppInfo). Updated the tools.test "nonexistent tool" case to the new
exit-5 tool_not_found contract. Per-file coverage ≥90 on all four dimensions
(cli.ts 100% stmts/funcs/lines). README documents the flags, NDJSON shape, and
exit semantics.
Closes#1574
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* feat(cli): stored-auth suite — --use-stored-auth, --wait-for-auth, --list-stored-auth, --print-handoff
Adds the #1575 web→CLI OAuth handoff surface, re-implemented informed by PR
#1510 and adapted to v2/main.
- `--use-stored-auth` — read the shared oauth.json and inject
`Authorization: Bearer <token>` for --server-url; exit 3 (no_stored_token),
listing stored keys, when none matches.
- `--wait-for-auth <sec>` — poll oauth.json (500ms) until a token for
--server-url appears, then proceed; timeout → exit 3 (auth_wait_timeout).
- `--list-stored-auth` — print `{oauthStatePath, storedServerUrls}` and exit
(short-circuit, no connect).
- `--print-handoff` — emit `{serverUrl, deepLink, portForwardCmd,
oauthStatePath, apiToken, note}` for --server-url and exit.
Adaptations vs. the reference:
- State-path resolution reuses core `getStateFilePath()` (the
MCP_INSPECTOR_OAUTH_STATE_PATH → <MCP_STORAGE_DIR>/oauth.json → default
precedence added in #1573) instead of a CLI-local resolver.
- The state file is read through the shared isomorphic
`parseOAuthPersistBlob`, so both the plain `{servers,idpSessions}` layout the
web backend writes and the legacy `{state,version}` envelope are accepted.
- `parseArgs` now returns a ParseResult union so the short-circuit modes return
without a server connection.
Blind injection: the token is passed as-is; a stale one surfaces as HTTP 401 →
exit 3. Documented in the README along with all four flags, the state-path
precedence, and a follow-up note that --print-handoff's deepLink shape is
interim pending #1576's canonical handoff format.
Tests: stored-auth.test.ts drives injection, --header coexistence, missing
--server-url, no-token exit-3, URL-normalisation matching, MCP_STORAGE_DIR
resolution, listing, handoff (+note), wait-for-auth polling/timeout, and
normalizeServerUrl units — all against a real in-process HTTP test server.
Per-file coverage ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines).
Closes#1575
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* docs(core): scope MCP_STORAGE_DIR comment in getStateFilePath to the OAuth backend
Address review of PR #1651: the doc comment claimed MCP_STORAGE_DIR was
honoured "across the app, so setting it once points every persist backend at
the same directory", but the branch only relocates oauth.json. Narrow the
comment to state it is OAuth-backend-only (client.json/mcp.json still resolve
from getDefaultStorageDir → HOME) and record why the resolution lives here
rather than in getDefaultStorageDir (the default-path constant is evaluated at
module load, so a runtime-set MCP_STORAGE_DIR — as the CLI stored-auth /
--wait-for-auth flows set it — must be read at call time). Comment-only; no
behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* review(cli): per-tool-tolerant app-info listing + drop dead helper (PR #1653)
Address review of PR #1653:
- collectAppInfo now never throws: a malformed `_meta.ui.resourceUri`
(extractAppInfo throws) folds into `{hasApp:false, resourceError}` just like a
resources/read failure. This makes `tools/list --app-info` per-tool tolerant —
one bad tool no longer aborts the whole NDJSON listing with a generic exit-1.
Guarding inside collectAppInfo (vs. a loop-local try/catch) keeps the branch
unit-testable; added an emit-result.test.ts case for the malformed-URI path.
- Documented that `tools/list --app-info` always emits NDJSON regardless of
`--format` (the per-tool list shape is fixed; --format json only reshapes the
single-result paths) — in both the code comment and the README.
- Removed the now-unused `expectJsonError` helper (its only caller moved to the
exit-5 tool_not_found contract).
Coverage stays ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
* review(cli): stored-auth follow-ups — TODO(#1576), wait-for-auth guard test, docs (PR #1655)
Address review of PR #1655:
- Added an explicit TODO(#1576) on buildHandoff's deepLink: `transport:"http"`
is hardcoded even for SSE, and `autoConnect=<token>` is not one of the three
token sources the web app reads (per CLAUDE.md) — both to be reconciled with
#1576's canonical handoff format.
- Added a test for `--wait-for-auth` without `--server-url`, covering the
previously-untested `--wait-for-auth` arm of the shared missing-server-url
guard's message ternary.
- Documented that `--list-stored-auth` / `--print-handoff` are mutually
exclusive short-circuit modes (list takes precedence) and that a stored
refresh_token is not yet used (noted as a follow-up in code + README).
No behavior change; coverage stays ≥90 on all four dimensions (cli.ts 100%
stmts/funcs/lines).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- transport.ts: update the SSE-branch comment to say the default path uses
fetchWithOptionalAuthIntercept (proxy-aware baseFetch + optional auth-challenge
intercept), not the bare proxy-aware baseFetch.
- clients/cli/README.md: fix the §5 mid-session validation link to use the
../../specification/ prefix like the sibling links in the same sentence
(previously resolved to a nonexistent clients/cli/ path).
Both flagged by the @claude review of PR #1601; comment/doc-only, no runtime change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
undici 8.5.0 requires Node >= 22.19.0 (its declared engines), so CI's
Node 20.x could not load undici's CacheStorage (missing
webidl.util.markAsUncloneable) and the proxy integration tests failed.
Bump the CI setup-node to 22.x to match the repo's Node floor.
Reconcile the declared floor with undici's real requirement: bump root
engines.node to >=22.19.0 and correct the CLI README note (undici 8.5.0
needs >=22.19.0, not >=22.7.5). Also document proxy support in the web
README and clarify that a caller-supplied eventSourceInit.fetch bypasses
the proxy wrapper by design.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
Wrap the Node transport's fetch with an undici EnvHttpProxyAgent dispatcher
when a standard proxy env var is set, so the CLI and web backend can reach
remote MCP servers through corporate proxies with zero new flags. undici is
imported lazily (only when a proxy var is present) and added as a dependency
at the repo root and in the CLI client; a missing undici raises an actionable
error. Documents the behavior and dependency in the CLI README.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
Give the CLI a stable machine-readable failure surface:
- EXIT_CODES map (0 ok / 1 usage / 2 no-app / 3 auth-required /
4 unreachable / 5 tool-error)
- ErrorEnvelope interface + CliExitCodeError for pre-classified exits
- pure classifyError() (status/cause/pattern heuristics) and
formatErrorOutput()/handleError() that emit one JSON line on stderr
- in-process cli-runner mirrors the binary via formatErrorOutput so tests
observe the real exit code and envelope
- document the exit-code map + envelope shape in clients/cli/README.md
Wave 1 foundation of the PR #1510 decomposition; shapes kept clean for
reuse by --app-info (exit 2), stored-auth (exit 3), and --format json.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
* refactor(web): update OAuth E2E tests to use new authentication method
- Replaced `runGuidedAuth` with `authenticate` in the InspectorClient OAuth end-to-end tests to support quick authentication with CIMD pre-registration.
- Updated comments to reflect the change in authentication method and its support for HTTP test metadata URLs.
- Ensured that the tests continue to validate the expected authorization URL after the change.
Additionally, imported `ensureCimdClientRegistration` in the OAuthManager to streamline client registration during the authentication process.
* Guided-auth removal, OAuth smoke testing, and TUI/CLI support
* Added CLI OAuth runner flags tests
* Added clarifying comment on port 0 binding
* README notes on TUI oauth callback port, prettier fixes for CLI tests
* CIMD validation uses inline like EMA IdP
* Fixed TUI test failures from merge
* test: bring 5 OAuth files over the 90 per-file coverage gate
The guided-auth-removal work added/touched several files that fell below
the repo's >=90 per-file coverage gate (npm run coverage). CI doesn't run
the gate (it's local-only), so validate + test:integration passed while
these gaps slipped through. Bring them all back over the line:
- core/auth/node/runner-oauth-callback.ts: test bad-URL/non-http-scheme
throws, no-port->80 default, and IPv6 hostname bracketing. Annotate the
provably-dead guards (empty host, out-of-range port, String(err)
fallback, pathname "/" fallback) -- all rejected by new URL() upstream --
with justified v8 ignore comments instead of lowering the gate.
- core/auth/cimd.ts: test the AS-doesn't-support-CIMD early return.
- core/react/useClientSettingsDraft.ts: test the functional-updater path,
the stale-onChange-after-close prev===null guard, and the
timer-fires-after-close value===null guard.
- ClientSettingsForm.tsx: test the CIMD checkbox toggle and CIMD URL clear.
- clients/tui/src/utils/oauthDisplay.ts: test static/dcr registration
kinds, the expired IdP session, and the configured-scope/empty-scope
branches of formatScopes.
npm run coverage now passes (exit 0) across web, cli, tui, launcher.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(launcher): self-contained validate; split smokes into top-level step (#1504)
Previously `validate:launcher` built cli/tui/web and ran the cross-client
smokes, so the launcher's "validation" depended on every other client
being built — and the launcher had no lint/format/coverage of its own.
- clients/launcher: add a web-style `validate` (format:check && lint &&
build && test:coverage) with a Node-only eslint flat config, prettier,
and a per-file coverage gate (parse-launcher-argv.ts covered;
src/index.ts excluded as binary bootstrap). 100% on the gated file.
- Root `validate:launcher` now just delegates: `cd clients/launcher &&
npm run validate` — no dependency on the other clients.
- New top-level `smoke` step (smoke:launcher + smoke:cli + smoke:tui +
smoke:web) with NO build commands; `validate` runs it last. Each
client's own validation builds its bundle first, so nothing builds
twice (cli/tui via test:coverage, launcher via its validate;
smoke:web builds dist on demand).
- clients/tui test:coverage now builds the TUI bundle (mirroring cli) so
the build-free smoke has it available.
- CI: replace the standalone cli/tui/launcher build step with a
"Validate launcher" step. Docs updated (AGENTS.md, launcher README).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(cli,tui,root): every client self-validates; smoke separate from validate
Extends the launcher self-validate work to the CLI and TUI, and makes the
top-level `validate` nothing but the per-client validations — smokes are
now a separate top-level target.
- clients/cli, clients/tui: add a web-style `validate` with a Node flat
eslint config + prettier. CLI lint/format pass as-is. TUI registers
eslint-plugin-react-hooks for the classic rules only (rules-of-hooks +
exhaustive-deps, which the source already references); the stricter
react-hooks@7 rules are not enforced on the interim component surface
(#1501).
- Avoid double builds:
- CLI's e2e spawns the built binary, so its `test:coverage` builds first;
`validate` folds that in (`format:check && lint && test:coverage`, no
separate `build`).
- TUI tests don't spawn a bundle, so `validate` keeps an explicit
`build` (for the smoke) and `test:coverage` is pure.
- Root: `validate` = validate:web && validate:cli && validate:tui &&
validate:launcher (each delegates to the client's own validate). `smoke`
is no longer part of `validate` — it's a separate top-level target run
after a build/validate, with no build commands of its own.
- CI: per-client "Validate X" steps + a single "Run cross-client smokes"
(npm run smoke). Docs updated (AGENTS.md, cli/tui READMEs).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(root): drop redundant test aggregator scripts
Now that every client self-validates and the top-level `validate` chains
the per-client validations, the root `test` / `test:web` / `test:cli` /
`test:tui` / `test:coverage` aggregators are redundant — run
`npm run validate` (all) or `cd clients/<name> && npm run validate` (one).
`test:storybook` stays (it's the one check not part of validate). CI was
already using each client's own scripts, so it's unaffected. Docs updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(web): silence happy-dom infra noise in unit runs
Passing unit runs printed dozens of alarming-but-expected lines that made
green runs look broken — all from happy-dom performing real I/O with no
server behind it:
- `<iframe src="/sandbox.html">` (MCP Apps sandbox) made happy-dom fetch
and later unload that URL, emitting ~40 `DOMException [NetworkError/
AbortError]` + `AsyncTaskManager destroyed` lines. Disable child-frame
navigation in the unit project (the tests only assert on the iframe
element/attributes, not its document), removing the noise with no
coverage loss.
- App-level renders read `GET /api/config` via useSandboxUrl /
useServerListWritable, 404-ing ~76x. Add a benign default `fetch` (empty
200) in the unit setup; tests that exercise fetch still override it.
Negative tests that assert on errors already suppress their own console
output (e.g. jsonUtils/schemaUtils); CLI, TUI, and the web integration
suite were already clean. Unit suite is now noise-free; 2424 web tests
pass and the coverage gate is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(scripts): make validate fast (test), split coverage into its own target
Each client's `validate` now runs `test` instead of `test:coverage`, so
the everyday pre-push check skips v8 instrumentation (and, for web, the
slower integration project). The per-file coverage gate moves to a
dedicated top-level `coverage` script:
coverage = coverage:web && coverage:cli && coverage:tui && coverage:launcher
coverage:<name> = cd clients/<name> && npm run test:coverage
`validate` still builds each client's bundle (cli via its test `pretest`,
the others via an explicit `build`), so `npm run validate && npm run
smoke` works unchanged. CI now runs `validate` (fast checks + builds),
then `coverage` (the gate), then `smoke` — collapsing the old per-client
steps. Docs updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: drop the coverage gate from CI; keep web integration as its own step
CI no longer runs `npm run coverage` — the per-file coverage gate is now a
local/manual check (`npm run coverage` before pushing). To avoid silently
dropping the web integration suite (it only ran via test:coverage, and
`validate` runs the unit project only), CI gains a standalone
`clients/web` `test:integration` step that exercises the real
stdio/HTTP/OAuth/storage paths without coverage instrumentation.
CI is now: validate (fast) → web integration → smoke → storybook. Docs
updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(smoke): give smoke:launcher its own script like the others
smoke:launcher was the odd one out — an inline chain of three
`node clients/launcher/build/index.js ... --help` commands while smoke:cli
/ smoke:tui / smoke:web each run a `scripts/smoke-*.mjs`. Extract it to
`scripts/smoke-launcher.mjs`, which runs the same `--help` / `--cli --help`
/ `--tui --help` dispatch checks but now also asserts each prints that
mode's usage banner (proving the launcher resolved the right client build)
and verifies the required builds exist up front with a clear error. Prints
a `smoke:launcher OK — ...` summary in the same style as the rest. Docs
updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore: reorder package.json scripts for readability
Move frequently-run scripts toward the top and rarely-run-by-hand ones
(pretest hooks, lint/format internals, etc.) toward the end, consistently
across the root and client manifests. Pure reordering — no script names or
values changed, and no other manifest content is touched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: drop references to removed root inspector / test:storybook scripts
The reordering dropped the root `inspector` and `test:storybook`
aggregators (intentional — `inspector` is redundant with `web`, and
storybook is run from clients/web in CI). Update AGENTS.md so it no longer
points at `npm run inspector`, and run `test:storybook` from `clients/web/`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore: address PR #1505 review (format glob gap, fetch-baseline comment)
- TUI format/format:check now also cover the client-root source files
(index.ts, tui.tsx, dev.ts) that live outside src/ — `eslint .` already
lints them, so they were linted but not format-checked. (cli/launcher
have no source outside src/; their root .ts are single-quoted configs
intentionally left out of prettier, matching web.)
- Reword the setup.ts default-fetch comment: it overstated auto-restore
(the unit project sets no restoreMocks/unstubGlobals), and now also
flags that the permissive 200 default means a test asserting on a fetch
FAILURE must install its own rejecting stub.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore: remove redundant root install:clients script
`install:clients` ran the same `node scripts/install-clients.mjs` as
`postinstall`, which fires automatically on `npm install` — so the manual
alias was redundant. Drop it and the doc references (AGENTS.md, spec).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(ci): skip smoke:tui in CI (Ink needs a real TTY)
This PR made CI run the full `npm run smoke`, which surfaced that
smoke:tui fails in headless CI: the Ink TUI throws "Raw mode is not
supported" on mount (no TTY) and exits before rendering its first frame.
The old CI never ran smoke:tui, so this was latent.
smoke-tui.mjs now self-skips when `process.env.CI` is set — the boot/
render check is inherently local-only (a TUI needs a terminal). The TUI is
still built and unit-tested in CI; `npm run smoke` stays the single
command everywhere (full locally, tui-skipped in CI). Verified
`CI=true npm run smoke` is green (launcher/cli OK, tui SKIPPED, web OK).
Docs + CI comment updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(cli,tui): add in-process source coverage gates (#1484)
CLI and TUI source had no coverage gate: CLI tests spawned the built
binary out-of-process (coverage never saw clients/cli/src) and the TUI
had only two smoke tests. Regressions there were invisible to CI.
CLI:
- Rewrite __tests__/helpers/cli-runner.ts to invoke runCli() in-process
(capture process.std*.write, map thrown errors to an exit code),
preserving the CliResult shape so all existing tests run unchanged and
clients/cli/src is now measured under v8 instrumentation.
- cli.ts: program.exitOverride() so a commander usage error throws
(catchable in-process) instead of tearing down the worker; --help /
--version still exit cleanly. Drop the dead, never-imported main().
- Add a thin out-of-process e2e.test.ts that spawns the built binary for
the shebang / process.exit paths the in-process runner can't reach.
- Add @vitest/coverage-v8, a per-file gate (90/85/80/50) excluding only
src/index.ts (binary bootstrap), and pool: 'forks' for capture
isolation. cli.ts lands at ~98% lines / 100% functions.
TUI:
- Add @vitest/coverage-v8 and an interim per-file gate over the
non-React logic (logger.ts, components/tabsConfig.ts, utils/*); the
Ink components, App.tsx, and hooks/ are excluded with a documented
follow-up (#1501). Add tests covering the included modules (100%).
Wiring + docs:
- CI runs each client's test:coverage; root test:cli / test:tui (used by
validate) and root test:coverage now enforce the gates.
- gitignore clients/**/coverage; update AGENTS.md and client READMEs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(cli): clarify in-process runner footguns from PR #1502 review
Review notes on the in-process CLI test runner (no behavior change):
- Warn in runCli's docblock that --help/--version must NOT go through the
in-process runner: exitOverride only re-throws non-zero exits, so exitCode
0 falls through to process.exit(0) and would kill the vitest worker. Those
belong in the out-of-process e2e.test.ts layer.
- Document that a timeout orphans the uncancellable invokeCli promise (its
stdio subprocess stays alive; later writes hit the restored stdout) — a
known, acceptable trade-off on the hang-only failure path.
- Fix a stale reference to the removed main() (errors now route through
index.ts).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(cli): fix remaining stale main() reference in vitest exclusion comment
PR #1502 re-review caught a second stale `main()` mention (the first was
fixed in 52c23e41). src/index.ts inlines the bootstrap as
runCli(process.argv).then(...).catch(handleError) — there is no main().
Comment-only; no behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore: compose root test:coverage from per-client scripts
Replace the brittle inline cd-chain with a composition of the existing
test:cli / test:tui scripts plus a new matching test:web. Behavior is
unchanged (web → cli → tui coverage gates) but there's a single source of
truth for each client's coverage invocation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(cli): lift config-file headers/timeouts/OAuth into serverSettings (#1482)
The CLI resolved file-sourced servers via resolveServerConfigs(), which
returns bare MCPServerConfig and dropped disk-level headers, timeouts, and
OAuth — CLI connections from a --catalog/--config file silently ignored
them. The TUI already lifted these via mcpConfigToServerEntries().
Extract the TUI's catalog/config -> per-server-settings resolution into a
shared core module (core/mcp/node/servers.ts: loadServerEntries +
selectServerEntry) and route both CLI and TUI through it, so the two share
one file->settings path. --header still overrides the file's headers for
the run while preserving the file's timeouts/OAuth.
- core: add loadServerEntries/selectServerEntry/headersToServerSettings
- tui: tui-servers.ts re-exports the shared core helpers (no logic dup)
- cli: resolve via loadServerEntries + selectServerEntry; drop the
CLI-only conflict check and bare-config path
- tests: core servers.test.ts (settings lifting, header merge/override,
stdio overrides, seed/error/conflict, selection); CLI integration tests
asserting a config header is sent over HTTP and --header overrides it
- docs: CLI README note on file settings being applied
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(core): address #1500 review — dedup overrides, drop dead wrapper, clearer error, spec refresh
- Remove the now-unused resolveLaunchServerConfigs() wrapper (both CLI and
TUI route through loadServerEntries now); migrate its default-catalog
launch tests to compose withDefaultCatalogPath + resolveServerConfigs.
- Deduplicate applyStdioOverrides against config.ts applyOverrides: export
the latter (now trims cwd) and reuse it in servers.ts.
- selectServerEntry's unknown-name error is source-agnostic ("not found"
not "not found in config file") since entries may be a single ad-hoc
default, where "config file" would mislead.
- Refresh specification/ docs (v2_cli_tui_launcher.md,
v2_catalog_launch_config.md): CLI/TUI resolution now via
loadServerEntries/selectServerEntry; mark gap G1 resolved (#1482);
withDefaultConfigPath → withDefaultCatalogPath.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(cli): extend smoke:cli to cover #1482 lift→serverSettings
Adds end-to-end smoke coverage for the config-file headers/timeouts/OAuth
lift introduced in #1482:
- unknown --server now errors source-agnostically ("not found")
- multi-server catalog selection via --server (and the omit-it error)
- --header merge does not break the stdio connect path
- HTTP transport: a config-file `headers` object is lifted onto the wire,
and a CLI --header overrides it (verified against an in-process test
server's recorded requests via a non-blocking spawn)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(cli,tui): normalize --catalog / --config with web; update --help (#1347)
Bring the CLI and TUI in line with the web catalog vs read-only session
model introduced in #1481/#1483, and close the outstanding --help item
from #1347.
Core (core/mcp/node/config.ts):
- Add `catalogPath` to ServerConfigOptions (writable catalog slot).
- `resolveServerSource()` picks the active source: `--catalog` (writable,
seed-if-missing) wins over `--config` (read-only, error-if-missing).
- Writable sources seed an empty `{"mcpServers":{}}` on first run (matches
the web backend) instead of erroring; read-only `--config` still errors.
- `serverSourceConflict()` shared conflict matrix (mirrors run-web):
--catalog/--config mutually exclusive; neither combines with an ad-hoc
target.
- `readServerListFile()` public loader for runners (TUI) needing the raw map.
- Rename `withDefaultConfigPath` → `withDefaultCatalogPath`; the no-flag
default now fills the writable catalog slot, not --config.
CLI / TUI:
- Add `--catalog <path>` (+ MCP_CATALOG_PATH) and re-document `--config` as a
read-only session in --help.
- Enforce the shared conflict matrix before resolution.
Full writable persistence/CRUD threading remains deferred to #1482 / #1432
(CLI/TUI are read consumers today; the split surfaces as seed- vs
error-on-missing).
Tests: core resolution matrix (source/conflict/seed/read-only), CLI catalog
+ conflict integration tests, TUI loadTuiServers unit tests. READMEs updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(launcher): add smoke:cli + smoke:tui e2e launcher smokes (#1347)
The launcher smokes only checked `--help` for --cli/--tui; nothing
exercised the launcher → CLI/TUI → core path on the built artifact (only
smoke:web did, for web). Add two end-to-end smokes wired into
validate:launcher between smoke:launcher and smoke:web:
- smoke:cli (scripts/smoke-cli.mjs): runs `--cli` against the bundled
stdio test server via a temp --catalog and asserts tools/list returns
the tools, plus the #1347 resolution paths — default-catalog
seed-on-missing, read-only --config error-without-seed, and the
--catalog/--config conflict.
- smoke:tui (scripts/smoke-tui.mjs): launches `--tui --catalog <temp>`
and asserts the Ink app renders its first frame ("MCP Servers") within
a timeout, then SIGTERMs it (shallow boot/render check).
Both build test-servers/build on demand if missing. Docs updated
(AGENTS.md validate description, launcher README).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor: address review on #1499 (comment accuracy, CLI/TUI flag parity)
Review feedback from @claude on PR #1499 (all non-blocking):
- Align CLI catalogPath resolution with the TUI: `options.catalog?.trim()
|| process.env.MCP_CATALOG_PATH` so an explicit empty `--catalog ""`
falls back to MCP_CATALOG_PATH identically in both front-ends (CLI used
`??`, which only guards undefined/null).
- Clarify the `serverSourceConflict` comment: it mirrors only the
source-selection portion of web's matrix and deliberately omits web's
`--header` rejection, since CLI/TUI merge --header into per-server
settings.
- Note the deliberate --header broadcast semantics in loadTuiServers.
- Share an EMPTY_CATALOG_CONTENT constant between seedEmptyCatalog and
readServerListContent instead of two separate literals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(cli): trim --config to match --catalog and the TUI (#1499 review)
Cosmetic uniformity follow-up from the re-review: configPath now uses
`options.config?.trim() || undefined`, matching the catalog slot above it
and the TUI. Behavior is unchanged (resolveServerSource/serverSourceConflict
already trim downstream).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ports the CLI, TUI, and launcher from v1.5 into v2 behind a single
`mcp-inspector` entrypoint (launcher → web/cli/tui). Includes the
`mcp-inspector --web --dev` launcher fix (Vite alias resolution + prod-server
double-start), the root `postinstall` install cascade with simplified CI, and
spec/AGENTS docs (no-workspaces rationale, Development setup).
Closes#1246.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cliff Hall
Bob Dickinson