Author	SHA1	Message	Date
Cliff Hall	7784553674	fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808) (#1810) ... * fix(auth): offer explicit re-authorize when callback state is lost (#1808) A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state made the SDK throw `AuthorizationServerMismatchError` and the Inspector dead-ended, surfacing the raw SDK text with no path forward. The SDK folds two very different failures into that one error class, so `core/auth/issuerBinding.ts` classifies them structurally: a recoverable "lost authorization state" (the SDK puts prose, not an issuer URL, in the `recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only the first gets a recovery affordance. Web: the callback failure now raises a dedicated banner ("Authorization state was lost" / "Authorize again") whose action clears the stale OAuth state for that server and starts a fresh authorization. A genuine mismatch stays a red, non-dismissible security notification with both issuers named and no one-click retry. Node runners (CLI/TUI): the same classification rewrites the callback rejection into the shared actionable copy instead of the SDK text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(auth): detect the SDK mismatch error via isInstance (#1808) The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares that brand in a `static {}` block, so it lives on the constructor and is never reachable from an instance. The predicate always returned false, so `findIssuerBindingFailure()` never matched and the #1808 recovery banner was inert in production — the callback dead-ended exactly as before. Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand set), with a `name` fallback for a serialization boundary. Correct the module doc, which cited cross-bundle safety as the reason to avoid `instanceof` — the very thing the SDK's branded `hasInstance` provides. Every fixture now constructs a real `AuthorizationServerMismatchError` instead of a hand-rolled object with an own `mcpBrand`, which is why a green `npm run ci` hid this. Adds a test pinning the brand's placement so the feature cannot silently go inert again. Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove the duplicated banner title default, resolve the banner title/action label where the banner is raised, keep the clear-failure notification open, and bound remote-supplied issuers in user-facing copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 18:08:31 -04:00
Cliff Hall	01df9d81bb	fix(core): start OAuth on a modern/auto-era connect (#1805) (#1809) ... * fix(core): start OAuth on a modern/auto-era connect (#1805) The SDK's `server/discover` negotiation probe runs before anything else on `protocolEra: "auto" | "modern"`, and its classifier reports whatever the transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved to `data.cause`. That buried both connect-time auth signals: the remote path's `AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the authorization URL) and a direct transport's `AuthChallengeError`. An OAuth-protected server that authorized fine on the legacy era instead produced a dead-end "Version negotiation probe failed" — no redirect, and the connection status flipped to `error` despite `isConnectAuthRecoveryError` existing to prevent exactly that. - `findNestedAuthError()` walks `cause` / `data.cause` for either typed error. Not gated on the SDK's error code or message, so it survives rewording. - `connect()` unwraps the rejection at the earliest point, ahead of `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the outer status guard, and every client's connect error handling — so web, CLI, and TUI are all fixed without a client-side change. - On the direct path, enable `interceptAuthChallenges` for the probing eras even with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely, no status and not even a cause for the walk to find. Verified against a live dual-era server that answers 401 on both `initialize` and `server/discover`: `auto` and `modern` now reject with `AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the fix is load-bearing — reverting either fails the new integration tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om * docs(core): mark the era-probe challenge interception as a removable workaround Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as #1807). Make its lifespan explicit so whoever picks up #1807 knows this block is the thing to delete, rather than having to re-derive it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): resolve connect() when auth recovery is satisfied silently Review findings on #1809: - connect() built its handshake promise once, outside the runConnect closure withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg re-awaited the already-rejected promise and rejected a connect() whose client was in fact connected. Short-circuit the retry when already connected, and skip the post-connect block the nested connect already ran (a duplicate `connect` event re-triggers every list-state manager's refresh). - Document the accepted 403 side effect of enabling interceptAuthChallenges with no stored tokens, and why every `{ pin }` counts as probing. - Replace the private-method double cast in the era-probe test with an observable assertion on the transport options, and split the non-auth failure case per era instead of a message alternation that passed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): key the connect retry leg off attempt, bound nested recoveries Second-pass review findings on #1809: - The satisfied-recovery short-circuit keyed off live connection status, so an onclose landing between the nested connect() and the retry leg fell through to the rejected handshake promise and reported the stale 401 as the reason the session died. withDirectAuthRecovery now passes `attempt` to its operation, so connect() identifies the retry leg structurally and reports the teardown when the recovered session is already gone. - Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a satisfied outcome recovers via a nested connect() with a fresh counter, so a server that keeps rejecting freshly refreshed tokens recursed without bound. Counted across the nesting boundary, capped at 3. - Strengthen the regression test (server info populated by the nested connect, no duplicate statusChange) and add the two missing cases: a failed reconnect still rejects, and the recursion bound terminates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * test(core): cover the recovery teardown branch; fix a duplicated case Third-pass review findings on #1809, both about tests added in the previous commit: - The retry leg's teardown error had no coverage, so a regression to the old status fall-through would have stayed green. Covered deterministically: the nested connect dispatches `connect` after setting "connected", so closing the live transport from that listener flips the status before the retry leg runs. Asserts the error identity, which is the point of the change. - "still rejects when the reconnect underneath the recovery fails" used a challenging transport for every connect, so it walked the same bounded chain as the recursion-bound test rather than covering a non-auth reconnect failure, and its comment claimed an "error" status the auth path never reaches. Now fails the reconnect with a plain error and asserts both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: rinormaloku <rinormaloku37@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 15:06:01 -04:00
Cliff Hall	d9e4f444a2	test(cli,tui): typecheck the __tests__ dirs (#1791) (#1799) ... * test(cli,tui): typecheck the __tests__ dirs (#1791) The cli/tui `typecheck` scripts only checked `src` (the src-only tsconfig excludes `**/*.test.*`), leaving the `__tests__` dirs — which do the most `as` casting — with no tsc pass. Add a `tsconfig.test.json` per client that includes `__tests__` with the test-exclusions dropped and the test-only path aliases (`@modelcontextprotocol/inspector-test-server`, the `@inspector/core/*` deep paths, express/vitest) resolving what vitest resolves, then fold `tsc -p tsconfig.test.json` into each `typecheck`. Fixes the 76 errors it surfaced: - cli: a wrong `@modelcontextprotocol/inspector-core/...` module specifier (should be `@inspector/core/...`; only a type import so vitest never caught it), `r.headers` optional-access guards, and partial OAuth mocks replaced with typed `makeFakeCliOAuthClient()` / `makeFakeServerSettings()` factories instead of `as` casts. Adds `@types/express` (devDep) so the transitively aliased test-server source typechecks, mirroring clients/web. - tui: rest-param signatures on the App OAuth spies (fixes the spread-arg and narrow-return errors), a widened callback param (`iss?`), a `makeFakeServer- Settings()` factory for the tuiOAuth tests, required OAuth-metadata/client fields in AuthTab, and a loosened `entry()` message param for the deliberately-malformed wire messages HistoryTab's defensive branches render. Gate coverage only; no runtime behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(cli,tui): address review — real spy signatures, config typecheck, nits Responds to the @claude review of #1799: - App.test.tsx: type the OAuth spies against the real InspectorClient method signatures (`vi.fn<InspectorClient["…"]>()`) and forward `Parameters<…>` tuples from the FakeClient wrappers, so `toHaveBeenCalledWith(...)` on the six OAuth-lifecycle spies stays argument-checked under the new gate rather than accepting anything via `unknown[]`. `getOAuthState` regains its wide `OAuthConnectionState | undefined` return. - Typecheck the client config files too: add `vitest.config.ts` / `tsup.config.ts` (and tui `dev.ts`) to each src `tsconfig.json` include — they were the one first-party surface still getting no tsc pass. - Trim the redundant `src/**/*` from each `tsconfig.test.json` include (tsc pulls in the src the tests import; the src-only config already checks it). - Rename the tui settings helper to `oauth-test-fakes.ts` to match cli's, and soften both factories' docstrings to stop naming a loader they don't mirror field-for-field. - Fix a stale `@modelcontextprotocol/inspector-core/...` specifier in a createRemoteTransport JSDoc example (the same wrong package name the gate caught in the cli tests, surviving in a comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-2 review — correct claims, tighten types, typecheck launcher Responds to the round-2 @claude review of #1799: - Correct the toHaveBeenCalledWith claim: vitest doesn't type-check `toHaveBeenCalledWith(...)` arguments against the mock's signature, so the App.test.tsx comment no longer claims it does — the typed spies buy checked implementations/return payloads (the stale-literal fix), not arg-checking. - authenticate spy now uses the real `InspectorClient["authenticate"]` signature (returns a `URL`), so it no longer diverges from production; the one mockResolvedValue is updated to a URL. - makeFakeCliOAuthClient members are typed against the real CliOAuthClient signatures (not bare vi.fn()), so a wrong-signature override is rejected. - HistoryTab: revert the `entry()` `message: unknown` widening (it un-checked the ~15 well-formed fixtures) and instead scope a named `MALFORMED` cast to the two intentionally-malformed messages. - Typecheck the launcher `__tests__` too (the same gate hole in the one client the PR hadn't touched): new clients/launcher/tsconfig.test.json + a typecheck script folded into validate. Completes the "every client's __tests__ is typechecked" invariant. - Doc the new test-typecheck + config-file typecheck in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-3 nits — launcher config typecheck, callTool typed, docs Responds to the round-3 @claude review of #1799 (all non-blocking): - launcher: add `vitest.config.ts` to tsconfig.test.json's include (it was in neither project — the build config's rootDir: ./src rejects it). Now typechecked. - App.test.tsx: type `callTool` against `InspectorClient["callTool"]` (+ the Parameters<…> wrapper) so its `mockResolvedValue` payloads are checked, and give `clearOAuthTokens` an `async () => {}` impl so the mock actually returns the `Promise<void>` its type promises. - AGENTS.md: correct the per-client alias-set description (cli's is widest, tui's is core+react only, launcher's has no paths) and note launcher's config file lives in its test project, not the src project. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-4 — durable typecheck-coverage guard + launcher/doc nits Responds to the round-4 @claude review of #1799 (all non-blocking): - Add `verify:typecheck-coverage` (scripts/verify-typecheck-coverage.mjs), the typecheck-coverage analog of verify:format-coverage and the durable guard for the invariant this PR establishes: for each Node client it runs the tsconfig projects its `typecheck` names with `tsc --listFiles`, unions them, and fails on any tracked .ts/.tsx that lands in no project. This closes the class the review kept surfacing by hand (launcher/__tests__ in round 3, launcher/vitest.config.ts in round 4) — a new top-level file can no longer go untypechecked silently. Wired into `npm run ci` and the GitHub workflow. - launcher: reorder `validate` to run `typecheck` before `build` (matches tui and the documented order), and make `typecheck` self-contained — it now runs both `tsconfig.json` and `tsconfig.test.json`, so running it standalone means the same thing it does in cli/tui. - makeFakeCliOAuthClient: note on the comment that the typing covers the defaults, not `overrides` (a bare vi.fn() override is Mock<(...args)=>any>). - Docs: README table + AGENTS.md updated for the new guard and the corrected launcher typecheck/validate ordering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-5 — wire-check the guard, --listFilesOnly, into validate Responds to the round-5 @claude review of #1799: - Finding 1 (the pre-merge one): the guard now ports verify:format-coverage's reachability checks — it asserts each client's `typecheck` is reachable from its own `validate` and that the root chain runs each client's `validate`, so it can't stay green while measuring a typecheck nothing invokes. Verified: stripping `npm run typecheck` from launcher's validate now fails the guard. - Nit 3: switch `tsc --listFiles` → `tsc --listFilesOnly` (identical file list, ~4× faster: whole guard 23s → ~4s). Now cheap enough for the inner loop, so it moves into `validate` (right after verify:format-coverage) instead of a standalone ci step — matching its sibling. Removed the separate ci-chain entry and GitHub workflow step (validate covers it). - Nit 4: `npx --no-install tsc` so a missing local tsc fails fast instead of reaching for the registry (matches verify-build-gate.mjs). - Nit 5: corrected projectFiles' JSDoc to describe the filter that's actually there (drops repo-external + node_modules paths; core/ + test-servers/ stay but are harmless). - Finding 2: fixed the 2-space misindent on the new root package.json line (prettier --write). Docs (README/AGENTS) updated for the validate placement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): address round-6 guard nits — dedupe wiring, robust diagnostics Responds to the round-6 @claude review of #1799 (all guard-ergonomics nits): - Nit 2: extract the shared reachability logic (`reachableScripts`, `rootRunsClientValidate`) into scripts/lib/npm-scripts.mjs and consume it from BOTH verify-format-coverage.mjs and verify-typecheck-coverage.mjs, so the "is this gate actually run?" wiring can't drift between them (same rationale as scripts/lib/prod-web-server.mjs). - Nit 1: typecheckProjects now harvests `-p`/`--project` from every script reachable from `typecheck` (not the single string), so a delegating `typecheck` (`npm run typecheck:src && …`) is handled instead of failing with a false "names no -p project" diagnosis. Also accepts `--project`. - Nit 3: on a tsc config error, echo the `error TS…` diagnostic (scanning both streams — tsc prints config errors to stdout) so a broken tsconfig is self-explaining rather than surfacing only as a misleading "file in no project" report. - Nit 4: refreshed the CI workflow's stale "Validate" step name/comment (it now runs the coverage guards + validate:core + per-client typecheck). - Nit 5: reworded the `.d.ts` carve-out comment to read as pre-emptive (there are none under cli/tui/launcher today). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-7 — guard .mts/.cts too; drop dead export Responds to the round-7 @claude review of #1799: - Finding 1: verify-typecheck-coverage now requires a tsc pass for tracked `.mts`/`.cts` (not just `.ts`/`.tsx`), matching verify-format-coverage's extension set — `.mts` is already the shared-config idiom here, so a client `vitest.config.ts` → `.mts` rename can no longer drop out of both the tsconfig include and the guard's required set. No-op on today's tree (no client `.mts`/`.cts`); confirmed a tracked `.mts` with a type error is now caught. Ambient `.d.{ts,mts,cts}` stay excluded. - Nit 2: drop the unused `export` on `rootReachedCommands` (internal to the lib; nothing imports it, and scripts/ isn't eslint-gated to catch dead exports). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-8 — single casts in HistoryTab, uniform wrappers, doc scope Responds to the round-8 @claude review of #1799: - Finding 1: reduce the `MALFORMED` helper and `entry()`'s return cast from `as unknown as` to a single `as` (both compile clean — the malformed shapes are comparable to the JSONRPCMessage union), and delete the false "only option" sentence. Fixes the AGENTS.md avoid-double-casts violation. - Nit 3: write all seven FakeClient OAuth wrappers as `(...a: Parameters<InspectorClient["…"]>)` (the three zero-arg ones were hand-written `()` forwarders), so the block comment's "forward the same Parameters<…> tuple" is literally true and a future added parameter can't be silently dropped. - Nit 2: update the five places that still described the guard as `.ts`/`.tsx` only to `.ts`/`.tsx`/`.mts`/`.cts` (script header + trackedSourceFiles JSDoc, AGENTS.md ×2, README.md). Nits 4 (js config files out of the typecheck guard by construction) and the footnoted callTool/ancestor-tsc items left as agreed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-9 — drop entry()'s harmful cast; guards vouch for each other Responds to the round-9 @claude review of #1799: - Finding 1: remove `entry()`'s `as MessageEntry` return cast in HistoryTab.test.tsx. It was unnecessary (the spread of Partial<MessageEntry> over complete defaults is directly assignable) and harmful — it silenced type errors in the helper's OWN default fields (a `direction`/`method` typo now surfaces TS2820/TS2353). MALFORMED's single cast stays (load-bearing). This leaves MALFORMED as the file's only cast, matching what the comments describe. - Nit 2: add `rootReachesScript` to scripts/lib/npm-scripts.mjs and have each coverage guard assert its SIBLING is still wired into the root `validate`. A guard can't detect being unrun itself, but the two now vouch for each other, so dropping either from `validate` is caught by the other (only deleting both slips through). Verified: removing verify:typecheck-coverage from validate now fails verify:format-coverage, and vice-versa. Nits 3 (git-index visibility) and 4 (pre-existing as-never/as-unknown casts in tests — out of scope, none added by this PR) left as agreed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-10 — reject non-checking typecheck passes; pre/post hooks; docs Responds to the round-10 @claude review of #1799: - Finding 1: verify-typecheck-coverage now rejects a `-p <project>` whose own command carries `--noCheck` or `--listFilesOnly` — such a pass lists files without type-checking them, which previously satisfied both the wiring and file-coverage halves while checking nothing. (Splits each script on `&&`/`||`/`;` so a flag on one command can't taint another.) Verified a `typecheck` using `tsc … --noCheck` now fails with a clear message; `--noCheck` is a live idiom here (cli's test-servers:build), so this is a real copy-paste hazard. - Nit 2: reachableScripts (shared lib) now also follows npm's implicit `pre<name>`/`post<name>` lifecycle hooks, so moving a gate into e.g. `prevalidate` is recognized instead of producing a false "not reachable" failure (cli already uses a `pretest` hook). Verified. - Nit 3: AGENTS.md "Test placement" now states the Node clients (cli/tui/ launcher) keep all tests in `__tests__/`, enforced by verify:typecheck-coverage (a co-located src test lands in no tsconfig project). - Nit 4: noted on the factory comments that `Partial<…>` overrides admit an explicit `undefined` for a required field (exactOptionalPropertyTypes off). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * fix: round-11 — launcher build excludes tests; guard catches all noCheck vectors Responds to the round-11 @claude review of #1799: - Finding 1 (shipping bug): clients/launcher/tsconfig.json now excludes `**/*.test.{ts,tsx}` (matching cli/tui). launcher's `build` is an EMITTING tsc over src/**, and clients/launcher/build is in the root `files` allowlist, so a co-located `src/**/*.test.ts` would have been compiled into the published tarball. Excluding it also makes such a test land in no tsconfig project, so verify:typecheck-coverage catches it — making the AGENTS.md placement rule literally true for all three clients. - Finding 2: verify-typecheck-coverage now catches both remaining ways a typecheck could check nothing: (a) case-insensitive `--noCheck`/`--nocheck`/ `--listFilesOnly` on the command, and (b) `noCheck` set in the tsconfig itself (detected via `tsc --showConfig`, which surfaces it from the config or its extends chain). Verified both fail the guard. - Nits 3 & 4: restructured into an explicit gate-integrity phase (wiring + neutered + config-noCheck + no-projects) that reports under its own accurate header and exits BEFORE the file-coverage pass — so an inert gate no longer prints file-remediation advice, nor buries the real cause under an "in no tsconfig project" line for every file that gate covered. The one CIMD OAuth E2E integration test that flaked in the coverage run passes 32/32 in isolation and on re-run; unrelated to these script/config changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * fix: round-12 — exclude .mts/.cts tests from build; drop false integrity line Responds to the round-12 @claude review of #1799: - Finding 1 (tarball, again): the round-11 test exclude only covered `.ts`/`.tsx`, so a co-located `src/*.test.mts` still compiled into launcher's shipped `build/`. All three clients now exclude `**/*.test.*` (one glob, covering .ts/.tsx/.mts/.cts) — which is also exactly what AGENTS.md already claims. Verified a `src/probe.test.mts` is no longer emitted and now fails the guard. - Finding 2: the gate-integrity phase no longer prints a false "names no `-p <project>`" line when config-`noCheck` disables projects — the condition now keys off `projects.length` (harvested), not `checking.length` (post- filter), so a client whose projects were all config-disabled shows only the accurate per-project lines. - Nit 3: recorded the boundary on the phase comment — it does not detect shell-level failure suppression (`… || true`, `; exit 0`). - Nit 5: the file-coverage remediation now leads with "for a co-located test, move it to `__tests__/`" (adding it to the src `include` would reintroduce the build emit) before the general add-to-include advice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-13 — implicit-tsconfig + derived clients; diagnostics Responds to the round-13 @claude review of #1799: - Finding 1: verify-typecheck-coverage now understands a `tsc` command with no `-p`/`-b` project flag — it resolves the implicit `./tsconfig.json` (tsc's own default), so the idiomatic `tsc --noEmit` form counts as coverage instead of producing false "in no tsconfig project" failures. Also harvests `-b`/`--build` project paths. Verified `tsc --noEmit && tsc --noEmit -p tsconfig.test.json` now reports OK. - Finding 2: CLIENTS is no longer hardcoded — it's derived from the `cd clients/<x> && npm run validate` entries in the root `validate` chain (minus an explicit web exclusion), so a new Node client is auto-required, matching the repo-wide reach of its sibling verify-format-coverage. Added a guard-would-check-nothing bail if the derivation yields an empty set. Verified a new `clients/newclient` is picked up and its uncovered file flagged. - Nit 3: the phase-2 remediation now leads with the general add-to-`include` advice and only appends the "move it to __tests__/" note when a failing path is actually a test file (was misdirecting plain files). - Nit 4: AGENTS.md no longer says the test config includes "only" __tests__ (launcher's also includes its vitest.config.ts). (Two different web integration tests each flaked once under parallel load during the coverage runs — both pass in isolation and on a clean re-run; timing- sensitive and unrelated to these script/doc-only changes.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-14 — enumerate clients from disk (fail-closed) Responds to the round-14 @claude review of #1799: - Finding 1: replace the root-chain `CLIENTS` derivation (round 13) with a disk enumeration — every `clients/<name>` whose package.json has a `typecheck` script. The chain derivation was fail-open: a root-chain edit that kept running a client but didn't match the strict regex silently dropped it from the required set (112→53 files, still green). Disk is fail-closed — a client can't be silently dropped, a new client with a typecheck is auto-required, and web (no typecheck script; builds via `tsc -b`) is excluded on its own, so the explicit exclusion is gone. The existing `rootRunsClientValidate` wiring check still verifies the root chain runs each — now LOUD when it doesn't. Verified: `cd ./clients/tui && …` now hard-fails the integrity phase instead of silently shrinking coverage; `cd clients/tui && npm run build && npm run validate` stays gated. Removed the now-unused `clientsRunByRootValidate` from the shared lib. - Nits 2 & 3: documented the two unreachable-today limitations on typecheckProjects — a solution-style `tsc -b` (empty `files` + `references`) would under-report, and the implicit-tsconfig fallback assumes no file operands. - Nit 4: refreshed the file header to mention `-b`/`--build` + the implicit `./tsconfig.json` (only the JSDoc had been updated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-15 — enroll-or-exempt enumeration; normalize `cd ./` Responds to the round-15 @claude review of #1799: - Finding 1: the disk enumeration keyed on the presence of a script named `typecheck`, so renaming it silently dropped the client (112→53, still green) — contradicting the docstring. Now every `clients/*` with a readable package.json must EITHER declare a `typecheck` (→ enrolled) OR be in an explicit EXEMPT map (`clients/web`, reason: `tsc -b`); anything else is a hard gate-integrity failure. This is fail-closed on the rename axis too, and web's skip is a stated decision rather than an accident of which scripts it declares — so web gaining a `typecheck` no longer auto-enrolls it into the solution-`-b` false failures. Verified: renaming tui's `typecheck` now hard-fails; a `tsc -b` on web stays exempt. - Nit 3: `rootRunsClientValidate` normalizes a leading `./`, so `cd ./clients/tui && npm run validate` (which genuinely runs it) is no longer a false "no longer runs" alarm. - Nit 2: README/AGENTS/script-header prose no longer names a fixed cli/tui/launcher scope — they describe the disk-discovered "every clients/* with a typecheck script (web exempt)" rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-16 — anchor wiring match; close enumeration axes Responds to the round-16 @claude review of #1799: - Finding 1 (only false-PASS): rootRunsClientValidate matched the client dir as a bare substring, so a prefix-sibling (`clients/tui` vs `clients/tui-next`) satisfied the wiring check for the shorter name — dropping a client from the root chain went green if a sibling was present. Now an anchored regex (escaped dir + optional leading `./` + a trailing boundary). Verified the sibling case now hard-fails. - Nit 2: a `clients/*` dir holding tracked TS but no readable package.json is now a gate-integrity failure (was treated as "not a client"), so the "fail-closed on every axis" claim holds. Verified. - Nit 3: readdirSync is wrapped, so a missing `clients/` dir yields the intended "found no clients/* dir" message rather than a raw ENOENT stack. - Nit 4: EXEMPT's reason is now surfaced in the success line ("clients/web exempt: typechecks via tsc -b …"), so the exemption is visible at runtime, not only in source. (Also moved `isRequiredSource` above nodeClients — nit 2's new tracked-TS check calls trackedSourceFiles at module-init, which referenced it before init.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-17 — share quote-aware tokenizer between guards Responds to the round-17 @claude review of #1799: - Finding 1: the two coverage guards parsed shell strings differently — verify-format-coverage had a quote-aware tokenize(); verify-typecheck-coverage used a raw regex. So a quoted `-p "tsconfig.test.json"` (a form tsc accepts) made the typecheck guard report every file that project covers as uncovered, and a quoted `cd "clients/tui"` in the root chain tripped a false wiring failure. Moved tokenize() (now single+double quote) into the shared lib, imported it into both guards, rewrote typecheckProjects to walk tokens (`-p`/`--project`/`-b`/`--build` → next token), and strip quotes before the `cd` match in rootRunsClientValidate. Verified both quoted forms now pass. - Nit 2: a stale EXEMPT key (a dir that no longer exists) was asserted in the success line unchecked. nodeClients now fails if an EXEMPT key isn't a real `clients/*` directory. Verified. - Nit 3 (build/dist filter belt-and-braces) left as a recorded boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-18 — match tsc by basename; strip quotes on both halves Responds to the round-18 @claude review of #1799: - Finding 1 (regression from round 17): `tokens.includes("tsc")` didn't match a path-invoked binary (`node_modules/.bin/tsc`), which the previous `\btsc\b` regex did — so such a `typecheck` was falsely reported as naming no project. Now matches on the token basename (`isTsc`), keeping round-17's quoting fix while restoring the tolerance. Verified a `node_modules/.bin/tsc` typecheck reports OK. - Nit 2: `rootRunsClientValidate` stripped quotes for the `cd` match but tested `npm run validate` against the unstripped string, so `npm run "validate"` false-failed. Both halves now test the stripped string. - Nits 3 & 4: documented on typecheckProjects — the contrived `--noCheck false` (checking on) is still treated as disabling, and the `&&`/`||`/`;` split runs before tokenizing (a quoted operator inside an arg would split mid-token, unreachable for project paths). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-19 — typecheck test-servers' unimported bin; --prefix wiring form Responds to the round-19 @claude review of #1799: - Finding 1: `test-servers/src/server-composable.ts` (a `#!/usr/bin/env node` bin entry nothing imports) got no tsc pass — the barrel alias covers the other 13 test-servers/src files transitively, but not this one, and its own build runs `tsc --noCheck`, so its errors were emitted into `test-servers/build` unchecked. Added it explicitly to `clients/cli/tsconfig.test.json`'s include (zero source changes; verified a bad line in it now fails cli's typecheck). Softened the AGENTS.md clause that implied the whole test-server source is transitively covered. - Nit 2: `rootRunsClientValidate` now also recognizes the `npm --prefix <dir> run validate` invocation form (was `cd`-only). - Nit 3 (stderr spy drops the completion-callback overload): left as-is — latent, not live (no exercised path awaits a stderr drain), and adding typed overload params would reintroduce the write-overload complexity the round-1 vi.spyOn refactor removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-20 — anchor validate tail; guard shared-source class Responds to the round-20 @claude review of #1799: - Finding 1 (false PASS): the wiring matcher's tail `/\brun validate\b/` also matched `run validate:fast` (`:` is a word boundary) — a DIFFERENT script that may skip typecheck. So the root chain could run a client's `validate:fast` while the guard kept measuring its `validate` (unrun), green. Anchored the tail to a boundary (`/\brun validate(?=$|[\s&;])/`). Verified `validate:fast` now hard-fails. - Finding 2: durably close the test-servers/src class instead of point-fixing. Added a SHARED_ROOTS check — every tracked TS under `test-servers/src` + the root `vitest.shared.mts` (the `format:check:shared`/`lint:shared` surface) must land in the GLOBAL union of client projects. So a NEW unimported bin entry is caught (not just server-composable.ts, named in round 19). Verified a fresh `server-orphan-bin.ts` now fails. Zero source changes — the union already covers 15/15. - Nit 4: the integrity message and rootRunsClientValidate docstring now mention the `npm --prefix` form too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-21 — deny-by-default non-client coverage Responds to the round-21 @claude review of #1799 (findings 1 & 2, one fix): - Replaced the `SHARED_ROOTS` allowlist with a deny-by-default non-client set: all tracked TS minus `clients/*` (per-client pass) minus a reasoned `core/` exemption (covered by web's `tsc -b`). This closes BOTH round-21 findings at once: - Finding 1 (stale allowlist fail-open): a rename of `test-servers/src` no longer silently drops it — its files reappear in the required "other" set and hard-fail (verified: `git mv test-servers/src test-servers/source` now fails instead of going 127→113 green). The subtractive `core/` exemption is fail-closed on staleness for the same reason. - Finding 2 (allowlist vs deny-by-default): a new top-level TS location is now auto-required (verified `tools/probe.ts` is flagged), matching the repo-wide reach of the sibling verify-format-coverage. - Nit 3: added the shared-source clause to the README table row (AGENTS got it last round). Header comment updated to describe the non-client coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-22 — narrow core/ exemption to what web's tsc -b covers Responds to the round-22 @claude review of #1799: - Finding 1: the `core/` exemption was whole-tree, but web's `tsc -b` only covers `core/**/*.ts` MINUS co-located `*.test.ts(x)` (its app project's include). So a `core/*.tsx`/`*.mts`/`*.cts` or a co-located `core` test got no tsc pass from anything. Narrowed the exemption to exactly that shape (`isExemptCoreFile`): a core `.ts` that isn't a co-located test. So a core `.tsx`/`.mts`/co-located test is now required in some project. Zero-fix today (0 such files); verified `core/mcp/probe-widget.tsx` and a co-located core `.test.ts` are now flagged. - Nit 2: `EXEMPT_ROOTS` is now a Map (dir → reason) and its reason is surfaced in the success line alongside the client exemption. - Nit 3: the non-client miss message is now the general "in no client's tsconfig project"; the `test-servers` remediation clause is conditional on an actual `test-servers/` path (tracked via `nonClientMisses`, not string-sniffing the composed message). - Nit 4: success line says "(N clients + non-client)". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-23 — core exemption mirrors both web projects Responds to the round-23 @claude review of #1799: - Finding 1: round 22's `isExemptCoreFile` mirrored only web's app project, but web's `tsc -b` also builds `tsconfig.test.json`, which includes `core/**/__tests__/**/*.{ts,tsx}`. So a `core/**/__tests__/*.test.ts` (a live layout — `core/mcp/__tests__/fakeInspectorClient.ts` already sits there) was flagged though web checks it. Now mirrors the UNION: under `__tests__/` → exempt `.ts`/`.tsx`; otherwise `.ts` minus co-located `*.test.ts(x)`. Verified a core `__tests__` test now passes while a core `.tsx` outside `__tests__` stays required. - Finding 2: `EXEMPT_ROOTS` was a Map whose keys nothing read (round 22 replaced its filter with hardcoded `isExemptCoreFile`) — a phantom registry. Collapsed to a single `CORE_EXEMPT_REASON` const used in the OK line. - Nit 3: README/AGENTS scope descriptions updated (core is no longer wholesale excluded; the non-client set is deny-by-default). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-24 — enroll clients/web via tsconfig references Responds to the round-24 @claude review of #1799: - Finding 1: `clients/web` was whole-tree EXEMPT, but web's `tsc -b` only reaches what its four referenced projects include — a file at web's package root (e.g. `playwright.config.ts`) got no tsc pass and passed EVERY gate, fully silently. This was the last whole-tree exemption wider than the gate it defers to. Now web is ENROLLED: a client with no `typecheck` script but a `tsconfig.json` `references` array is measured through those reference projects (`tsc -p <ref> --listFilesOnly`), and is wired iff its `validate` runs `tsc -b` (`validateRunsTscBuild`). EXEMPT is now the empty escape hatch. Verified: a web-root `playwright.config.ts` with a type error is now flagged, and dropping `tsc -b` from web's build hard-fails the integrity phase. Zero-fix on the tree — the union of web's 4 projects covers all 625 tracked web files. - Nit 2: assert `core/` exists (the OK line unconditionally states its exemption) — parity with the EXEMPT stale-key check. - Nit 3: a `core/` miss now gets core-specific remediation (widen web's `tsconfig.app.json` include, not a cli/tui project). - Nit 4: README/AGENTS scope descriptions corrected — web is enrolled (not exempt), and the exact core-exempt shape (app `*.ts` minus co-located tests + test `__tests__/**/*.{ts,tsx}`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-25 — drop redundant core exemption; guard tsc -b --noCheck Responds to the round-25 @claude review of #1799: - Finding 1: enrolling web (round 24) made the hand-maintained `core/` exemption both redundant and fail-open — web's measured `tsc -b` projects already cover core, and the mirror could go stale (narrowing web's include let an unimported core error pass). Deleted `isExemptCoreFile`, `CORE_EXEMPT_REASON`, the core presence assertion, and the exemptNote core clause. `core/` is now required like everything else and covered by measurement (127→894 files; a `core/*.tsx` web doesn't reach is now caught by measurement, not a hand-model). Kept the core-specific remediation clause. - Finding 2: the reference (`tsc -b`) path skipped the non-inertness check — a `tsc -b --noCheck` build gated nothing while the guard stayed green. Turned `validateRunsTscBuild` into `tscBuildStatus` (ok/neutered/none); a neutered `tsc -b` now hard-fails, matching the typecheck-script path. Factored the disabling-flag predicate (`isDisablingFlag`) so both paths share it. - Nit 3: `clientTsconfigReferences` now strips block comments too (every other tsconfig in the repo uses `/* */`). - Nit 5: README/AGENTS wiring clause covers web's `tsc -b` (not just the `typecheck` script), and core is described as measured, not exempt. Nit 4 resolves itself: EXEMPT is empty and the exemptNote/ternary now collapse cleanly. Nit 6 (tsc -b bound to a specific tsconfig) left as recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-26 — expand solution configs wherever harvested Responds to the round-26 @claude review of #1799: - Finding 1: the reference (`tsc -b`) path was selected only by "no typecheck script", so adding `typecheck: "tsc -b"` to web (the natural uniformity move) flipped it to the typecheck-script path, where the harvested solution config lists nothing → 665 false failures. Fixed at the measurement layer: projectFiles now expands a solution config (`{"files":[], "references":[…]}` — lists nothing under --listFilesOnly) to its references and measures those recursively, so a `tsc -b` project is measured no matter which path harvests it. Generalized the references reader (tsconfigReferences(path)). Verified web with `typecheck: "tsc -b"` now passes. Dropped the stale "web … is out of scope" JSDoc clause. - Nit 2: a `core/**/__tests__/**` miss now points at `tsconfig.test.json` (its actual owner), not `tsconfig.app.json`. - Nit 4: AGENTS.md's one-line summary now mentions the non-client (`core/`, `test-servers/src`) coverage, matching the detailed bullet. Nit 3 (EXEMPT empty, dormant stale-key check) left as the intended escape hatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-27 — non-inertness follows references on both paths Responds to the round-27 @claude review of #1799: - Finding 1: round 26 made COVERAGE follow a solution config's references, but the non-inertness (`noCheck`) check on the typecheck-script path still only inspected the harvested solution (empty compilerOptions) — so a `noCheck` in a REFERENCED project was invisible there (caught only on the reference path). Factored a shared `resolveLeafProjects` (a solution → its real leaf projects, cached rawProjectFiles) used by BOTH coverage (`projectFiles`) and a shared `checkingLeaves` disable-check, so a `noCheck` in a referenced project is now caught on either path. Verified: `typecheck: "tsc -b"` + `noCheck` in tsconfig.app.json now hard-fails (was green). Caching also cut the guard from ~20s to ~11s. - Nit 2: directory-form references (`{ "path": "./packages/a" }`) now resolve to `<dir>/tsconfig.json`, so a nested-solution dir reference is expanded. - Nit 3: reworded the empty-listFiles comment (it's "solution config OR errored config", both of which the reference expansion handles). - Nit 4: script header notes a `tsc -b` solution is reduced to its references on either enrollment path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-28 — dedup shared leaf in checkingLeaves Responds to the round-28 @claude review of #1799 (which found no new gate hole): - Finding 1: `checkingLeaves` dedup'd the `checking` array but ran projectDisablesChecking + integrity.push on every visit, so a leaf reachable from two harvested projects produced a duplicate `noCheck` line (and a redundant `tsc --showConfig`). Now tracks visited leaves in a Set and skips an already-seen leaf entirely. Verified a duplicate harvested project yields one line, not two. - Nit 3 (record-completeness): noted in AGENTS that the one tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by `@typescript-eslint/ban-ts-comment` across lint:core/lint:shared/each client. Nit 2 (the ~11s perf claim isn't reproducible on a CI-class runner) and nit 4 (hybrid-solution / composite boundaries, both tsc-rejected → unreachable) left as recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(scripts): unit-test the typecheck-coverage guard's parsers Responds to the round-29 @claude review of #1799 (no new gate hole; the finding is that the guard itself has no tests): - Made the guard's pure helpers importable: exported `typecheckProjects`, `tscBuildStatus`, `tsconfigReferences`, `clientTsconfigReferences`, `resolveLeafProjects`, `isTsc`, `isDisablingFlag`, `isRequiredSource`, and moved the module-init + phase execution behind an exported `main()` run only when the file is executed directly (importing it for tests no longer runs the guard). Behavior when run directly is unchanged (894 files). - Added table-driven tests (node's built-in `node --test`; the root has no vitest harness by design) for the shared lib (`tokenize`, `reachableScripts` incl. pre/post hooks, `rootRunsClientValidate` across cd/--prefix/quoted/ prefix-sibling/`validate:fast`, `rootReachesScript`) and the guard's parsers (`isRequiredSource`, `isTsc`, `isDisablingFlag`, `typecheckProjects`, `tscBuildStatus`) — one case per rule a review round found (r7/r10/r13/r15/ r16/r17/r18/r19/r20/r25). 12 tests. Wired `test:scripts` into `validate`. - Nit 2: a `scripts/*.ts` miss now gets a scripts-specific remediation (keep it `.mjs`, or give `scripts/` its own tsconfig project). - Documented the guard's test suite in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(scripts): make test:scripts self-guarding; fill parser test gaps Responds to the round-30 @claude review of #1799 (mutation testing killed 5/5; findings are about the new test gate + suite completeness): - Finding 1: `test:scripts` lacked the two properties the guard enforces for every tsc pass. verify-typecheck-coverage now asserts, in phase 1, that `test:scripts` is reachable from the root `validate` (via the already-imported rootReachesScript) AND that its tracked `scripts/**/*.{test,spec}.*` file set is non-empty and every file is matched by the command's glob (a new exported `globToRegExp`). So dropping it from validate, or renaming a test to `*.spec.mjs`/`*.test.mts` (which node --test silently skips), now hard-fails. Verified both. - Nit 2: extracted a pure `parseTsconfigReferences(raw)` from tsconfigReferences and table-tested the JSONC tolerance (block/line comments, trailing comma, no references, malformed, no path) — the r17/r26 rules had no cover. - Nit 3: added `-b`/`--build`/bare-`tsc -b` and explicit `--project` cases to typecheckProjects's test so its title matches its table. - Nit 4: rewrote the rootRunsClientValidate test to put `cmd` in a script reached from validate, and added the reachability cases (an orphan cd-validate must NOT count; validate:tui indirection must) — the r5 property. - Nit 5: added test:scripts to AGENTS's validate ordering + the README table. 15 guard tests now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-31 — glob harvest follows delegation; brace/? globs Round-31 review findings, both false failures in the new `test:scripts` self-guard: 1. The glob half read `rootPkg.scripts["test:scripts"]` literally while the wiring half two lines above used `reachableScripts`, so a delegating `test:scripts` (`npm run test:scripts:lib && npm run test:scripts:guard`) — which genuinely runs all 15 tests — reported every tracked test file as "not matched by the glob", advising a rename the files already satisfy. Now harvested across every script reachable FROM `test:scripts`, the same fix round 6 applied to the `typecheck` harvest; `pretest:scripts` counts for free. 2. `globToRegExp` escaped `{`/`}` as literals and left `?` as a regex quantifier, both divergent from `node --test`'s glob. `*.{test,spec}.mjs` is the natural widening here (the guard's own file scan already treats a `.spec.` file as a test) and false-failed; `?` silently meant "optional previous char" and threw outright when segment-leading. Adds `?` → `[^/]` and a brace-alternation pass, with an unbalanced brace falling back to a literal. Nit 3: `resolveLeafProjects`'s two path-resolution rules (the r26 directory-form reference, and refs resolving against the referring config's dir) extracted as `projectConfigFile`/`refToProject` so they are one-row tables like the other parsers, rather than untestable behind a `tsc` spawn. Nit 4: AGENTS.md + README document that the guard now enforces `test:scripts` on three axes (wired, non-empty, glob-covered). Verified: probes C and D exit 0; probe B (rename to `*.spec.mjs`) still caught; `test:scripts` 18/18; `verify:typecheck-coverage` OK 894 files; `verify:format-coverage` OK 919; `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-32 — measure globs with node's own matcher Round-32 review; both findings point at deleting code. 1. `globToRegExp` hand-modeled `node --test`'s glob and was still one form short — character classes (`*[.]test.mjs`, `*.test.[mc]js`) hit the escape set and became literals, so the guard false-failed on files node genuinely runs. Replaced the whole translator (and its brace balance pre-scan) with node's `path.matchesGlob` — 22.5.0+, and the repo floor is >=22.19.0. Same *measure, don't model* move round 25 made when it deleted `isExemptCoreFile` in favor of measuring web's projects; being exact for every form node accepts is the point, since node's matcher is the thing being modeled. 2. The round-31 glob harvest was inline in `main()`, so mutation-reverting it to the literal single-script read left the suite green — the commit's headline fix was its one untested change. Extracted as `testScriptGlobs`, with rows for the direct form, the delegating form (both child globs, not `npm`/`run`/`<name>`), a `pretest:scripts` hook, and an unreachable script contributing nothing. Nit 3: `resolveLeafProjects`' JSDoc moved back onto it (the extraction left it stranded above `projectConfigFile`, which had two stacked doc blocks). Nit 4a: a `test:scripts` naming no path/glob harvested nothing, and `.some()` over an empty list blamed every test file with unfollowable advice. Now one explicit message naming the real problem. Verified: probes for the direct, delegating, brace, `[.]` and `[mc]` forms all exit 0 (the last two were false failures before); probe B (rename to `*.spec.mjs`) still caught; bare `node --test` reports the new single message; `test:scripts` 18/18; `verify:typecheck-coverage` OK 894; `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-33 — scope the glob harvest to `node --test` segments 1. False pass. `testScriptGlobs` kept every non-flag token from every reachable script, so a glob belonging to a NEIGHBOURING command was attributed to the test runner — and since `.some()` needs only one glob to match, a broader one suppressed a real miss rather than adding an inert pattern. A `pretest:scripts` of `prettier --check "scripts/**/*.mjs"` (reachable via the pre-hook round 31 added) was enough: the probe-B rename to `*.spec.mjs` then passed while `node --test` silently ran 12 of 18 tests. Now scoped per command segment to the ones invoking `--test`, exactly as `typecheckProjects` gates on `tokens.some(isTsc)`. The JSDoc paragraph claiming a surplus token can't suppress a miss is corrected — it was false. 2. The gate-integrity block is extracted as `testScriptProblems(scripts, testFiles)`, pure and outside `main()`. The nit-4a branch added last round was the third consecutive commit whose newest branch sat outside the test seam and mutation-survived; extracting the block ends the pattern rather than pinning one more condition from the outside. Verified. The finding-1 probe (prettier pre-hook + `*.spec.mjs` rename) now exits 1 where it exited 0; the hook alone still exits 0. Mutation testing over the whole helper — empty-harvest branch to `if (false)`, segment gate removed, wiring axis removed, non-empty axis removed, segment split removed, `reachableScripts` to `Object.keys`, `matchesTestGlob` to equality — kills 7/7, including the branch that survived the last three rounds. `test:scripts` 21/21 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-34 — harvest only positional args; normalize `./` Both findings are the axis round 33 fixed, one level in. 1. False pass. The segment gate stopped a neighbouring COMMAND's glob, but within a kept segment the filter was still "any non-flag token", so a glob-valued flag's VALUE was harvested as a positional arg. `--test-coverage-include "scripts/**/*.mjs"` is broader than the test glob and matches a renamed `*.spec.mjs`, so it vouched for the very file the runner skips — and it isn't hypothetical: `scripts/` sits outside the ≥90 coverage gate, so that flag is the obvious next thing added here. Tokens preceded by a value-taking `--test-*` flag are now dropped. 2. False failure. `./scripts/**/*.test.mjs` — which `node --test` accepts, and the common npm-script idiom — matched nothing, because `git ls-files` emits no `./`. Both files were then blamed for a rename that never happened, the unfollowable-remediation shape fixed for the bare-runner case last round. A leading `./` is normalized off, as `rootRunsClientValidate` already does. Nit 4: the gate-integrity footer's typecheck-wiring advice is now conditioned on at least one issue being about the typecheck wiring — a `test:scripts` failure carries its own per-line remediation, and appending `--noCheck`/`tsc -b` advice under a renamed-test-file message sends the reader after the wrong thing. Verified. The coverage-include probe with the rename now exits 1 where it exited 0, and exits 0 without the rename; the `./` probe exits 0 where it exited 1; a `test:scripts`-only failure no longer prints the typecheck footer. Mutation sweep 9/9 killed, including both new rules. `test:scripts` 22/22 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-35 — fourth axis: flags that narrow the run 1. False pass on a new axis. The three axes ask whether the tests are run, whether there are any, and whether the runner is told about each — none asks what it then DOES with them. `--test-shard 1/2` keeps every glob intact and executes 6 of 22 tests, exiting 0, with the guard green; `--test-only` runs 2, as does a non-matching `--test-name-pattern`. This is `--noCheck` for the test runner, one domain over. Adds `NARROWING_TEST_FLAGS` and a fourth axis in `testScriptProblems`. Three of those flags were already in `VALUE_TAKING_TEST_FLAGS` — the guard knew them well enough to drop their values but not that they shrink the run. Both `--flag value` and `--flag=value` report as the bare flag name. 2. The footer conditioning added last round was itself inside `main()` — the pattern the same commit's comment documented, recurring in the commit that documented it. Extracted as `integrityAdvice(problems, nonTypecheck)`. Doing that fixes nit 3 in the same move: the old condition keyed off `testScriptIssues` specifically, so it covered one of three non-typecheck categories. A sibling-vouch failure ("the root validate no longer runs verify:format-coverage") and client-enrollment problems still got `--noCheck`/`tsc -b` advice about a typecheck pass they have nothing to do with. All three sets are now excluded. Verified. `--test-shard 1/2`, `--test-only` and `--test-name-pattern zzz` each now exit 1 naming the flag, while node runs 6/2/2 of 22. A sibling-vouch failure no longer prints the typecheck footer. Mutation sweep 12/12 killed, including all three new rules and the branch this commit replaces. `test:scripts` 24/24 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-36 — discover test files by content, not name 1. False pass on the axis underneath the other four. The required test-file set was built with the same name pattern the `test:scripts` glob uses, so a rename escaping BOTH was invisible: the file left `testFiles` entirely, so nothing asked whether the runner was told about it, and the survivor satisfied the non-empty axis. A dot→hyphen slip (`npm-scripts.test.mjs` → `npm-scripts-test.mjs`) dropped 6 of 24 tests with every gate green — and the file that stops running is the one pinning the r16 prefix-sibling and r20 tail anchors, the series' two other false passes. Round 30 fixed "renamed to a form the glob misses" by broadening the name pattern; a fifth name pattern only moves the boundary again. `node --test` decides what a test is by whether it registers tests, so `isScriptsTestFile` reads that instead — the same measure-don't-model move as r25 and r32. It also covers what no name pattern can: a NEW test file added under a name the glob misses, never run and never noticed. Nit 2: `enrollmentProblems` no longer suppresses the footer. "declares no `typecheck` script …" IS about the typecheck pass and the footer's first clause is its canonical fix; excluding it lost the advice for the r15 rename scenario. The criterion is "is this about the typecheck pass at all", not "does it carry per-line advice" — several typecheck problems carry their own and still want the footer. JSDoc corrected accordingly. Nit 3: the complementary-shard boundary (`1/2` + `2/2` across two reachable scripts genuinely runs everything, and is flagged anyway) recorded on the helper, alongside the guard's other documented boundaries. Nit 4: the three `--test-coverage-{lines,branches,functions}` thresholds added to VALUE_TAKING_TEST_FLAGS. Their values are numeric and so inert rather than suppressing, but model completeness is cheaper than re-deriving that later. Verified. The hyphen rename now exits 1 naming the file; the `.spec.mjs` rename is still caught; an enrollment failure prints the footer again; the guard is unchanged at 894 files OK (exactly the 2 tracked test files reference `node:test`, none of the other 12 `scripts/` files do). Mutation sweep 14/14. `test:scripts` 25/25 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-37 — register-not-mention; guard the discovery read Both findings were introduced by round 36's content predicate. 1. False failure. The predicate matched a bare `node:test` mention, so a shared helper importing the runner's `mock` API — registering no tests — was enrolled as a required test file and told to rename itself to `*.test.mjs`, which would only make node run an empty file. `scripts/lib/` is already the repo's shared-helper location, so this is the plausible next file there. Tightened to node's actual criterion: an import/require OF `node:test` AND a test registration. 2. Regression. `readFileSync` was passed as an argument, so it ran for every tracked `scripts/` file before the predicate's extension gate could reject it — a file in the index but not the worktree (`rm` without `git rm`) took the guard down with a raw ENOENT stack, aborting phase 1 before any real finding printed. Discovery did no reads at all before round 36. Extension is now gated first and the read wrapped: an unreadable tracked file warns and is skipped, the round-6 precedent where a failed `tsc` echoes its diagnostic instead of dying. Verified: the mock-helper probe exits 0; the tracked-but-deleted probe exits 0 with a one-line skip warning instead of a stack; the dot→hyphen rename is still caught. Mutation sweep over the predicate 3/3 (import form, registration, extension). `test:scripts` 25/25 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-37 nit 3 — one definition of the JS-extension check Finding 2's fix hoisted the extension gate into `main()` (it must run before the read), leaving a second copy of the regex inside the predicate that reads as protection but can never fire there. Extracted as `isJsFile` and used in both positions, so the two can't drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-27 12:05:35 -04:00
Cliff Hall	c2af547cc8	fix(core): register peer-request handlers before connect (#1797) (#1798) ... * 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>	2026-07-27 11:32:23 -04:00
Cliff Hall	cea22d86b8	feat(web): enforce localhost-only binding; expand loopback origin allow-list (#1796) ... 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)	2026-07-26 22:15:10 -04:00
Bob Dickinson	33e88eac9f	CLI: catalog listing, auth flags, and OAuth browser UX (#1782) ... * 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>	2026-07-26 16:55:25 -04:00
Cliff Hall	da37692485	refactor(web): sweep AGENTS.md TypeScript + Mantine rule violations (#1771) ... Codebase-wide sweep bringing the web client into strict compliance with the AGENTS.md TypeScript and Mantine/React conventions: ~170 inline 2+-static-prop elements extracted to named .withProps() constants across ~77 files, the AGENTS.md rule wording tightened to match (static-prop boundary + Accordion/Transition/data-*/Box exemptions), a real ScreenStage Box->Flex layout regression fixed (direction: column) with a Storybook regression guard, and an App.tsx double-cast tightened. Closes #1762.	2026-07-25 11:38:49 -04:00
Cliff Hall	8aba3122c5	refactor(web): consolidate the three GET /api/config hooks into one (#1770) ... * refactor(web): consolidate the three GET /api/config hooks into one (#1643) useSandboxUrl, useServerListWritable, and useInspectorVersion each fetched the same static /api/config payload separately and read one field from it — three requests on load for one payload. Replace them with a single useInitialConfig hook that fetches once and exposes { version, sandboxUrl, writable, loading } with the same tolerant parsing/defaults (writable defaults true; version/sandboxUrl undefined when absent; unmount-safe). - Add core/react/useInitialConfig.ts; wire App.tsx to it. - Remove the three former single-field hooks and their tests. - Fold the fetch-branch matrix into one useInitialConfig test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): address review nits on useInitialConfig tests (#1643) - "keeps writable true for any non-false value" now sends real falsy-but- not-false values a nonconforming backend could actually emit (null / 0 / "no") instead of `writable: undefined`, which was indistinguishable from an absent key and duplicated the omitted-field test. - Soften the post-unmount comment to say the test exercises the isCancelled() guard branches for coverage rather than implying it can detect their removal (React 18 dropped the setState-after-unmount warning). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs(web): fix contradicting unmount-guard comment; it.each writable cases (#1643) Second-review follow-ups: - core/react/useInitialConfig.ts: correct the isCancelled() rationale — React 18 no longer warns on setState-after-unmount; the guard exists to stop a stale payload (after unmount or a baseUrl/authToken re-run) overwriting current state. The old wording contradicted the corrected test comment. - useInitialConfig.test.tsx: convert the writable-non-false loop to it.each so each value (null / 0 / "no") reports independently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): use vitest %j specifier in it.each title; tidy hook docblock (#1643) Third-review follow-ups (both title/comment-only): - useInitialConfig.test.tsx: `%p` is a Jest specifier vitest doesn't support, so all three cases rendered the literal `%p`. Switch to `%j` — titles now render null / 0 / "no" distinctly (verified: `writable=null`, `writable=0`, `writable="no"`). - useInitialConfig.ts: extend the class docblock to say a response is dropped after unmount OR a re-fetch, matching the corrected inline guard comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-24 22:12:19 -04:00
Cliff Hall	373f346e5f	chore: close quality-gate coverage gaps (#1689) (#1766) ... Closes #1689. Typecheck cli/tui src, add a core/ format+lint gate, and gate core/json + core/client coverage. See PR #1766 for the full review history.	2026-07-24 17:20:12 -04:00
Cliff Hall	40aaf47705	test: deterministic remote-server teardown to kill #1667 afterEach hang (#1763) ... * test: deterministic remote-server teardown to kill #1667 afterEach hang The remote integration tests close their Hono HTTP server in afterEach with `server.close(cb)`, which waits for all keep-alive sockets to go idle before firing the callback. undici's global-fetch connection pool holds sockets open, and under the full parallel/instrumented `npm run coverage` load they may not go idle before the 30s hook timeout — intermittently failing the dead-transport tests in remote-auth-branches.test.ts. The crash-on-startup / dead-session tests are the worst case: they never disconnect, so the session and its dead subprocess transport linger when the server closes. Force the sockets closed with `server.closeAllConnections()` alongside close() so the callback fires deterministically regardless of parallel load, in both remote-auth-branches.test.ts and its sibling connect-crash.test.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * server: unref the abandoned-session sweep timer The transport-onclose path arms a 30s setTimeout to GC a session whose client never opens the events stream. That sweep is best-effort cleanup, not work the process must stay alive for — unref it so a pending timer can't keep the event loop (or a test worker) alive for 30s after everything else has finished. Also narrow closeAllConnections() behind an `in` guard in the two remote teardown helpers: ServerType is Server | Http2Server and the method is http1-only, so the direct call failed tsc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: extract shared closeHarnessServer teardown helper Address review feedback on #1763: the deterministic close()+closeAllConnections teardown (and its multi-line rationale) was duplicated across remote-auth-branches.test.ts and connect-crash.test.ts. Extract it to a shared closeHarnessServer(server) helper so the logic and its comment live in one place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-24 12:20:54 -04:00
Cliff Hall	99ef731746	Refactor / justify codebase-wide 'as unknown as' casts (#1690) (#1756) ... * core: refactor serverList.ts object↔Record casts into typed helper (#1690) Remove the four pre-existing `as unknown as Record<string, unknown>` / `as unknown as MCPServerConfig` casts in core/mcp/serverList.ts: - stripInspectorFields: rewrite as clone + `delete` over INSPECTOR_FIELD_KEYS. Every Inspector-extension key is optional on StoredMCPServer, so the result is still (a subtype of) MCPServerConfig with no cast. - oauth-strip in extractSecretsFromStored: `oauth` is optional, so a plain `delete stripped.oauth` is legal — drop the cast. - mcpConfigToServerEntries: route the one genuinely-needed widening through a single documented `toRecord(value: object)` helper. Taking `object` makes the single `as Record<string, unknown>` legal (no TS2352), so the double cast disappears from the call site. Net: zero `as unknown as` in serverList.ts; the one remaining structural cast lives inside the audited toRecord helper. Behavior-neutral — existing coverage (79 serverList tests) unchanged; full `npm run ci` passes. Closes #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * core+web+test-servers: eliminate/justify remaining `as unknown as` casts (#1690) Codebase-wide follow-on to the serverList refactor: audit every `as unknown as` double cast outside tests and either remove it or ensure it carries a justifying comment (the review bar is "no *unjustified* double cast"). Removed (widened via a shared helper or narrowed to a single cast): - Promote the serverList `toRecord(value: object)` widening helper to core/json/jsonUtils.ts and reuse it at core/mcp/node/config.ts (normalize loop), core/mcp/remote/node/server.ts (pino level dispatch), and clients/web/src/App.tsx (window global lookup). - inspectorClient.rawWireRequest: type the frame as `JSONRPCRequest` and narrow only `params` with a single structural cast (the SDK's `_meta`-typed params is what blocked direct assignment). - toolOutputValidation: SDK `outputSchema` → `JsonSchemaType` is a single cast. - test-server-fixtures: the optional `task` handle is reachable with a single cast (target field is optional). Justified in place (genuinely unavoidable — private SDK internals, partial interface stubs, third-party/nominal bridges): the inspectorClient private-map accesses, resourceContext + tokenAuthProvider partial `OAuthClientProvider` stubs, the ext-apps peer bridge, modern/legacy `_requestHandlers` bypasses, and the happy-dom `matchMedia` test mock. Behavior-neutral. Full `npm run ci` passes (validate, ≥90 coverage gate, smoke, Storybook). Refs #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * address @claude review nits: consistent cast justifications + drop reflow noise (#1690) - test-servers/src/modern-tasks.ts: add an inline justification above the `_requestHandlers` double cast, mirroring the sibling in composable-test-server.ts (was leaning on the function JSDoc only). - Annotate the partial-`AppBridge` mock double casts in the story fixtures (AppsScreen ×2, AppRenderer, InspectorView) so no non-test-file double cast is unjustified. - inspectorClient.ts: revert unrelated Prettier reflow hunks (protocolVersion, task-content array, timeout-reject wrap, reconnect guard) so the diff is the single rawWireRequest cast change only. Behavior-neutral (comment/format-only). `npm run ci` green apart from a known flaky redaction test whose assertion collided with a random request id. Refs #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * actually revert the reflow noise this time (#1690) The reflow reverts in cc187ea were silently undone by a `prettier --write` I ran on the same file right after, so the four Prettier-only hunks from 6a3afa4a were still in the diff (caught in re-review). Revert them for real now — and without re-running Prettier on the file: - core/mcp/inspectorClient.ts: restore the protocolVersion line, task-content array, timeout-reject call, and reconnect guard to their origin/v2/main form, so the file's diff is only the single rawWireRequest cast change. - test-servers/src/modern-tasks.ts: restore the ModernTaskStatus union to its one-line form (the leading-`|` reflow was also Prettier churn), leaving only the new `_requestHandlers` justification comment. Formatting-only; `npm run validate` green. Refs #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-24 09:34:32 -04:00
Cliff Hall	e2c75da5ce	Extensions: Connection Info both eras + io.modelcontextprotocol/ui advertisement (Phase 3 of #1633) (#1744) ... * Extensions: Connection Info both eras + io.modelcontextprotocol/ui advertisement (Phase 3 of #1633) Completes the Extensions surfacing and advertises the MCP Apps UI extension. core/mcp/extensions.ts: - Add UI_EXTENSION_KEY ("io.modelcontextprotocol/ui") and MCP_APP_MIME_TYPE ("text/html;profile=mcp-app") — mirrors of ext-apps' EXTENSION_ID / RESOURCE_MIME_TYPE (hardcoded: the constants live on the package's /server subpath or an extensionless re-export that doesn't resolve under NodeNext). - Registry entries gain an optional `advertisement` value; the builder stamps it (default {}). Add the `ui` entry, default-advertised with `{ mimeTypes: ["text/html;profile=mcp-app"] }` — the Inspector always renders App tools. No ext-apps cast needed: the extensions value type is `object`. ConnectionInfoContent: - Source server extensions from the negotiated `capabilities.extensions` (getServerCapabilities, era-transparent) instead of the modern-only discoverResult, so a LEGACY server advertising extensions renders them too. - Add a two-column "Server Extensions" / "Advertised Extensions" section (the Inspector's own advertised map), mirroring the server/client capability columns. Drop the now-redundant Extensions row from the modern Discovery section (Supported versions stays). Tests: extensions.test.ts covers the ui entry + advertisement value and the per-extension enable/disable; ConnectionInfoContent.test.tsx covers legacy server extensions (no discovery), the advertised-extensions column, and the em-dash empty states. npm run ci green (the one InspectorView localStorage failure is a known flake, passes on re-run). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: address Phase 3 review (mime drift-guard, story, exact em-dashes) - extensions-mimetype.test.ts (integration/node): assert MCP_APP_MIME_TYPE equals ext-apps' real RESOURCE_MIME_TYPE (imported) and that the client advertises it — a genuine drift guard, replacing the tautological literal check in the unit test (which now just asserts the type). - ConnectionInfoContent Modern story: carry the tasks extension on initializeResult.capabilities too, so the Server Extensions row demonstrates it (the row now reads negotiated server capabilities, not discoverResult). - Tighten the em-dash assertions from ">= N" to exact counts (3/3/2) now that the two extension sections em-dash by default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: drift-guard UI_EXTENSION_KEY against ext-apps EXTENSION_ID (review) Address the Phase 3 re-review nit: the extension id was the other half of the same drift risk (a server keys its Apps lookup on it). Import ext-apps' EXTENSION_ID from the /server subpath in the node integration test (fine there; the browser build is what avoids /server) and assert UI_EXTENSION_KEY === it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: clone the registry advertisement so builds never alias it (review) Address the Phase 3 re-review's optional defensive nit: buildClientExtensions returned the same object reference as the ADVERTISABLE_EXTENSIONS entry for the ui advertisement, so an in-place mutation of a stamped value (e.g. extensions[ui].mimeTypes) could corrupt the registry for later connections. structuredClone the advertisement on stamp; add a test asserting a mutated build doesn't leak into the next one. Nothing mutates it today — purely defensive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: add #1740 Connection Info proof screenshots End-to-end smoke test in the web client against the legacy advertised-extensions-http.json server: the Connection Info modal shows the new era-transparent Server Extensions / Advertised Extensions section, with Advertised Extensions listing io.modelcontextprotocol/tasks + io.modelcontextprotocol/ui (the new ui advertisement), and only ui after toggling Tasks off and reconnecting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-23 10:11:36 -04:00
Cliff Hall	fe295396af	Extensions: advertised-extensions toggle in Server Settings (Phase 2 of #1633) (#1743) ... * Extensions: advertised-extensions toggle in Server Settings (Phase 2 of #1633) Adds the debugging knob from #1633: users choose which extensions the Inspector advertises to a server, and a server may register different tools in response. Builds on Phase 1's registry + advertisedExtensions option (#1738). Settings + persistence (core): - InspectorServerSettings.advertisedExtensions + StoredMCPServer field, wired through serverList's read/write converters and INSPECTOR_FIELD_KEY_MAP (omit-when-empty, byte-stable round-trip). - validateSettings (remote PUT route): isBooleanRecord guard + carry a non-empty map; unknown/malformed rejected. - App.tsx threads savedSettings.advertisedExtensions into the client options. UI: - ServerSettingsForm renders an "Advertised Extensions" group of checkboxes driven by ADVERTISABLE_EXTENSIONS; a per-server override wins over the registry default. Modal folds a toggle into settings.advertisedExtensions. Test server + proof: - composable-test-server extensionGatedTools: a tool registered disabled and enabled on `initialized` iff the client declared the mapped extension (getClientCapabilities). Legacy stateful leg. Threaded through load-config/resolve-config; advertised-extensions-http.json showcase + README. - Integration test connects legacy with/without the tasks extension advertised and asserts the gated tool appears/disappears in tools/list — the acceptance criterion, proven end to end, not asserted. Tests: serverList round-trip (non-empty/absent/empty), ServerSettingsForm toggle rows, ServerSettingsModal fold, validateSettings accept/reject, and the integration gating test. npm run ci green (per-file ≥90 gate held). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: add #1739 proof screenshots (advertised-extensions toggle) End-to-end smoke test in the web client against the legacy advertised-extensions-http.json test server (port 3220): connecting with the Tasks extension advertised (default) lists echo + get_weather, unchecking the Advertised Extensions → Tasks toggle and reconnecting lists only echo — the server's tool registration changing on the client-declared extension. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: reconverge advertisedExtensions to no-override on default (review) Address Phase 2 review observation #1: toggling an extension back to its registry default now deletes the override key (and drops the whole map to undefined when it empties), so the on-disk `advertisedExtensions` reconverges to the byte-stable "no override" state instead of persisting a redundant `{ key: <default> }`. Matches the omit-when-default policy the other settings use. Only a value that differs from the default is persisted. Adds a modal test asserting the reconvergence (re-checking the sole disabling override drops advertisedExtensions to undefined). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-22 16:23:43 -04:00
Cliff Hall	2f80958d3f	Extensions: first-class advertised-extensions map (Phase 1 of #1633) (#1742) ... * Extensions: first-class advertised-extensions map (Phase 1 of #1633) Collapse the two ad-hoc `capabilities.extensions` spreads in the InspectorClient constructor (EMA, gated on enterpriseManaged; the modern Tasks extension, always-on) into a single builder that is the source of truth for what the client advertises. - core/mcp/extensions.ts: `ADVERTISABLE_EXTENSIONS` registry (id + label + default-on flag), the `EMA_EXTENSION_KEY` constant, and `buildClientExtensions()`. The registry is the shared list the Server Settings toggle UI (Phase 2, #1739) will drive; EMA stays a conditional built-in (auth-mode driven, not a free toggle). - InspectorClientOptions.advertisedExtensions: per-extension overrides (present key wins over the registry default) so a future UI can toggle advertised extensions as a debugging knob. - Behavior preserved: tasks defaults to advertised, so the map is non-empty and rides every modern request envelope; EMA advertised iff enterpriseManaged. The raw modern-task envelope re-stamp is left untouched (it fires only on the raw tasks channel, which requires the extension anyway). Tests: extensions.test.ts covers the registry and every builder branch (defaults, EMA on/off, override enable/disable, unknown key) at 100%. The tasks-era and EMA integration suites still assert the same wire advertisement. `advertisedExtensions` is plumbed but not yet wired to the form (that lands in Phase 2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: address Phase 1 review (EMA-override test + envelope comment) - extensions.test.ts: add an EMA-specific assertion that an override for EMA_EXTENSION_KEY cannot advertise it (auth-mode only) — documents intent and guards against EMA being added to ADVERTISABLE_EXTENSIONS by mistake. - inspectorClient.ts: rewrite the withModernTaskEnvelope tasks-stamp comment so it stays accurate once Phase 2 lets a user disable general tasks advertisement — the force-stamp is load-bearing (the raw tasks/* channel requires the extension), not a redundant re-add. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * TUI: de-flake step-up OAuth frame assertions (raise poll ceiling) The step-up authorize tests intermittently timed out on the final "Step-up authorization succeeded" frame. Root cause is a test-infra timing budget, not product code: waitForFrame/waitUntil polled only 25× (25ms tick ≈ 625ms), and the step-up flow adds an extra async OAuth-runner hop plus React commits that can exceed that under CI load with v8 coverage. Hoist the poll count into a named POLL_TRIES = 100 (~2.5s ceiling). A poll exits the instant its predicate is true, so a higher ceiling never slows a passing assertion — it only widens the margin for the slow path. Verified 5× green (3 plain + 2 under --coverage), including the previously-flaky step-up cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * TUI: prettier-format the wrapped waitForFrame signature The POLL_TRIES change pushed waitForFrame's signature past the print width; Prettier wraps it. (Root has no aggregate format script — format is per-client, so this was missed locally.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-22 15:25:45 -04:00
Cliff Hall	e70a11360c	Tools: x-mcp-header annotation tooling + -32602 unknown-tool rendering (#1632) (#1735) ... * Tools: x-mcp-header annotation tooling + -32602 unknown-tool rendering (#1632) Card 9/10 of the SDK-v2 + New Spec stack. Adds SEP-2243 `x-mcp-header` tooling to the Tools tab and adjusts result rendering for SDK v2's unknown-tool behavior, and bumps the SDK to 2.0.0-beta.5. - **`core/json/xMcpHeader.ts`**: a faithful port of the SDK's (non-public) `scanXMcpHeaderDeclarations` — validates every SEP-2243 constraint (primitive types incl. `number`, RFC 9110 token, case-insensitive uniqueness, statically-reachable `properties`-only chain) and derives the mirrored `Mcp-Param-{Name}` params for a kept tool. - **Excluded tools**: the SDK silently drops tools with invalid annotations from `tools/list`. InspectorClient now re-lists the RAW (unfiltered) list and scans it to recompute the excluded set with reasons — gated to the modern non-stdio era (matching the SDK's gate), recomputed on every aggregate refresh, surfaced via a new `excludedToolsChange` event + `getExcludedTools()` and `useInspectorClient().excludedTools`. The Tools sidebar lists them struck-through under an "Excluded (SEP-2243)" divider with the reason on hover. - **Mirrored args**: ToolDetailPanel shows a "Mirrored request headers" section listing each arg → `Mcp-Param-{Name}` (with the browser-omits note). - **-32602 rendering**: SDK v2 rejects an unknown-tool call with `-32602` instead of an `isError` result, which previously rendered nothing. New ToolCallErrorPanel renders a thrown tool-call error, with a targeted "Unknown Tool" hint for `-32602`. Adds unit + Storybook + integration coverage (a real modern server with an invalid-header tool fixture) for every path. `npm run ci`-equivalent checks green. Closes #1632 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: x-mcp-header modern showcase config + preset (#1632) Register the `invalid_header_tool` fixture as a preset and add `xmcpheader-modern-http.json` — a modern HTTP showcase serving echo, a valid-header `get_weather`, and the invalid-header tool — so the Tools-tab x-mcp-header tooling (mirrored headers, excluded-tools surface, -32602 rendering) can be exercised manually. Documented in the README alongside the other per-SEP showcase configs. Smoke-tested end to end in the web client. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: add #1632 proof screenshots to pr-screenshots End-to-end web-client verification of the x-mcp-header Tools tooling against the modern showcase server: excluded-tools surface + reason, mirrored-headers section, and the -32602 unknown-tool error panel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Respond to review: disambiguate -32602, doc/search nits (#1632) Addresses the review on PR #1735: 1. -32602 "Unknown Tool" mislabel: `-32602` is generic Invalid params, so a known tool called with bad arguments hit the same code and rendered under the definitive "Unknown Tool" heading. Replace `isUnknownToolError` with `classifyToolCallError(code, message)` → "unknown-tool" only when the message names it (not found / unknown tool / …), else "invalid-params" (new "Invalid Parameters" heading + schema hint), else "generic". 2. Double pagination walk: expand the `listAllTools` comment to document that the excluded-tools recompute is a deliberately un-cached second raw walk (doubles list round-trips on modern non-stdio, runs even on a cache-served aggregate) — accepted for a debugging tool, no-op on legacy/stdio. 3. Doc nit: `scanXMcpHeaderDeclarations` no longer claims to visit "$ref targets" — reworded to note `$ref` is never followed (refs resolve into the unreachable `$defs` body). 4. Search nit: the excluded-list filter now matches name AND title, matching the main tools filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tighten unknown-tool matcher to be tool-scoped (#1632 re-review nit) The re-review noted the -32602 disambiguation was symmetric: broad markers ("does not exist", "unrecognized") could inversely mislabel a genuine invalid-params message (e.g. "property 'region' does not exist") as "Unknown Tool". Scope the not-found/does-not-exist family to require the word "tool" in the same clause (matching the SDK's "Tool <name> not found"); keep "unknown tool" / "no such tool" as standalone unambiguous markers. Adds a tool-less-"does not exist" test asserting it classifies as invalid-params. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Add trigger_invalid_params showcase tool + Invalid Parameters screenshot (#1632) Adds a `trigger_invalid_params` spec-error trigger to the modern showcase server: the injector answers its `tools/call` with a real `-32602 Invalid params` JSON-RPC error whose message does NOT name an unknown tool. This lets the "Invalid Parameters" branch of the tool-call error panel (added in the review response) be exercised live, distinct from the unknown-tool `-32602`. - `SPEC_ERROR_TRIGGERS` gains the `trigger_invalid_params` entry (HTTP 200, in-band -32602) + preset registration. - Enable `injectSpecErrors` on `xmcpheader-modern-http.json` and add the tool. - README + pr-screenshots README document it; adds the live "Invalid Parameters" proof screenshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-22 11:42:54 -04:00
Cliff Hall	2259b1a3e1	Tasks: io.modelcontextprotocol/tasks extension on modern servers (era fork) (#1731) ... * Tasks: modern io.modelcontextprotocol/tasks extension (era fork core) Core + test-server implementation of the SEP-2663 tasks extension for modern (2026-07-28) connections, forked from the legacy 2025-11-25 tasks path. SDK v2 removed all tasks support AND era-gates the tasks/* spec methods out of the modern era on both the client (outbound) and server (inbound) sides, and its codec rejects a resultType:"task" result outright. So the Inspector drives the extension itself: task creation rides tools/call (a modern method) with the resultType:"task" frame rewritten at the transport to a CallToolResult carrying the handle in _meta; tasks/get/update/cancel go over a raw-wire request channel that bypasses the era gate (full modern envelope, string ids, consumed by the transport). input_required tasks reuse the existing pending-request modal via fulfilInputRequests, answered with tasks/update. No tasks/list, no tasks/result. Tab + run-as-task gate on the negotiated extension; the managed task store's refresh re-polls known handles on modern (no server list). Test server serves the extension via an Express interceptor (SDK can't) + a shared poll-driven runtime. Full integration coverage of both eras. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: tests + coverage (raw-wire, schemas, era integration) Adds unit coverage for modernTaskSchemas and the raw-wire request channel (transport-null guard, send failure, timeout, error response, teardown), plus end-to-end era integration tests (create/poll/input-required/update/cancel, synchronous passthrough, unknown-id error, modern store re-poll). Fixes the App and constructor-capabilities tests for the always-on tasks extension. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: docs + share test servers per era to de-flake coverage Document the tasks era-fork test configs in README and the new core/test-server files in AGENTS.md. Share one modern + one legacy test server across the era integration block (started lazily, stopped in afterAll) — starting a fresh Express server per test made connection negotiation flaky under the heavy concurrent coverage run. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Restore pr-screenshots deleted inadvertently The logging/subscriptions PR screenshots + README were staged-deleted in the working tree before this branch; restore them from v2/main so this PR doesn't remove other PRs' artifacts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: offer Run-as-task for any tool + PR screenshots On modern (SEP-2663) task creation is server-directed — the client declares the extension once and any tool may return a task — so the Tools screen now offers 'Run as task' for every tool on a modern connection (threaded via a modernTasks prop through InspectorView → ToolsScreen → ToolDetailPanel), rather than gating on the legacy per-tool taskSupport. Verified end-to-end in the web UI (remote proxy): create → poll tasks/get → input_required elicitation → tasks/update → inline result, plus the unchanged legacy tasks/list/result flow. Adds proof screenshots. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: address two follow-ups (task-input note + ordinary-path auto-poll) 1. The pending-request note now distinguishes a modern task round from an MRTR round: a new 'task-input-required' PendingRequestOrigin threads through fulfilInputRequests → the pending-request modal → MrtrOriginNote, which shows 'submitted via a tasks/update request (SEP-2663), not a retry' instead of the MRTR retry copy. 2. The ordinary (non-run-as-task) modern callTool path now auto-polls an unsolicited task handle: attemptToolCall detects the rewritten handle in the result _meta and drives it to termination (get → input_required/update → inline result), dispatching task events so the Tasks tab + toasts track it and the tool result resolves to the task's final result. The poll loop is shared with the streaming path via extracted helpers (submitModernTaskInput / modernTaskTerminalOutcome / modernTaskPollInterval). Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: address review (negotiated envelope version, input-round cap, clarifying comment) + fresh smoke screenshots Review of #1731: 1. withModernTaskEnvelope now stamps the NEGOTIATED protocol version (getProtocolVersion() ?? MODERN_PROTOCOL_VERSION) so the envelope matches the MCP-Protocol-Version header instead of a hardcoded constant. 2. A never-completing input_required task is now bounded by MRTR_MAX_ROUNDS in the shared submitModernTaskInput (threaded through both poll loops), so a non-conformant server can't re-prompt the user forever. New modern_loop_task test-server tool + integration test cover the cap. 3. ToolDetailPanel: comment clarifying that per-tool taskSupport:"forbidden" is deliberately ignored on modern (task creation is server-directed under SEP-2663). (The abort-during-poll observation is addressed in the PR reply: the cancel path uses tasks/cancel and disconnect rejects the in-flight raw request, so the poll stops; matches legacy.) Fresh web-UI smoke screenshots (modern connected, task completed, input_required with the corrected tasks/update note, input completed). Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: fix uncancelable input-required loop + sticky cancel Two bug fixes reported during manual testing of the tasks era-fork (#1631): 1. A modern task paused at `input_required` (e.g. `modern_loop_task`, which never advances) could not be cancelled: the poll loop blocked on the pending-request modal, so `cancelRequestorTask` sent `tasks/cancel` but the modal kept re-prompting. Now each modern poll loop registers a per-task AbortController (`registerTaskInputAbort`), `cancelRequestorTask` aborts it to reject the pending elicitation and unblock the poll, and the embedded input requests are tagged with their owning task id (`RELATED_TASK_META_KEY`) so the pending-request UI's Cancel action cancels the TASK rather than sending a "cancel" answer the server would re-prompt on. 2. On the legacy server, cancelling a task briefly showed CANCELLED then flipped to COMPLETED when the in-flight tool call resolved. Cancellation is a deliberate terminal decision, so `ManagedRequestorTasksState` now remembers cancelled task ids and ignores any later non-cancelled status update. Also: move the compact task id onto its own line in the embedded monitoring sidebar so the status badge / Cancel Task control don't wrap; DRY the duplicated task poll-interval logic into a shared `taskPollInterval` helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Refresh tasks PR screenshots + capture both bug-fix scenarios Recapture the five tasks flow screenshots against the current build and add two demonstrating the post-review fixes: modern_loop_task cancelled from the pending-request modal (uncancelable-loop fix) and a legacy task staying CANCELLED after the in-flight call resolves (sticky-cancel fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address re-review: sticky cancel on legacy refresh + App cancel-routing test Re-review of the cancelable-loop / sticky-cancel fix raised two non-blocking hardening items: 1. The legacy tasks/list refresh() rebuilt the list from the server, honoring only dismissedTaskIds — so a cooperative-cancel server that re-lists a user-cancelled task as completed could un-stick the cancel on a manual Refresh. The refresh path now pins a cancelledTaskIds task to "cancelled" (keeping it visible-as-cancelled, matching the live-merge handlers) rather than dropping it. Covered by a new state test. 2. Added an App.test.tsx case locking in the user-facing half of the fix: Cancel on a task-input-required elicitation calls cancelRequestorTask (not respond). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-21 19:17:04 -04:00
Cliff Hall	320daf2f67	Resources: subscriptions/listen model on modern servers (era fork) (#1630) (#1730) ... Fork resource subscriptions by protocol era: legacy keeps resources/subscribe; modern (2026-07-28) models subscriptions as a filter over one long-lived subscriptions/listen stream with reconnect-by-re-listen (capped backoff), an optimistic Connecting→Listening header badge, and a fix so the browser proxy no longer blocks listen() ~60s. Closes #1630.	2026-07-20 19:33:40 -04:00
Bob Dickinson	a71e6d2012	Auth hardening CI gaps + SDK consolidation: EMA leg 2 / scopes (#1527) (#1725) ... Close #1527 acceptance gaps with automated tests (SEP-2468 iss, SEP-2207 offline_access, SEP-2351 discovery shapes, SEP-2352 AS-migration, EraNegotiationFailed→401 unwrap); lean EMA leg 2 and scope helpers on the v2 SDK; drop deprecated TS baseUrl. Reviewed LGTM.	2026-07-19 22:36:21 -04:00
Cliff Hall	ec185f1053	Logging: per-request log level on modern servers (era fork) (#1629) (#1724) ... * Logging: per-request log level on modern servers (era fork) (#1629) On modern (2026-07-28) servers `logging/setLevel` and the session-scoped level are gone: the client opts into logs per request via the `io.modelcontextprotocol/logLevel` `_meta` key, and the SDK does not attach it automatically, so server logs are silently absent until stamped. Core (`InspectorClient`): - `setModernLogLevel(level | undefined)` / `getModernLogLevel()` store the opt-in level; `mergeMeta` stamps it on every outgoing request, gated on the negotiated modern era so legacy connections are untouched. - Cleared on disconnect so it can't leak into the next connection. Web UI: - `LogControls` forks on `protocolEra`: legacy keeps the `logging/setLevel` selector + Set button; modern shows a per-request opt-in Select (Off + levels, applied immediately) with copy explaining logs arrive on the originating request's stream. - Threaded `protocolEra` / `modernLogLevel` / `onSetModernLogLevel` through LoggingScreen and InspectorView from App. Tests: unit coverage for the modern LogControls/LoggingScreen fork; integration coverage proving the `logLevel` `_meta` is stamped on a real modern server once opted in (and not on legacy). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: logging era-fork showcase configs + proof screenshots (#1629) Add two manual-testing configs and README docs for the logging era fork: - logging-legacy-http.json: plain streamable-HTTP server with `logging: true` and a `send_notification` tool. The Logs tab shows the legacy session-scoped "Set Active Level" selector + Set button; calling the tool streams logs. - logging-modern-http.json: same, with `transport.modern: true`. Connect with Protocol Era = Modern and the Logs tab shows the per-request opt-in control; the client stamps `_meta["io.modelcontextprotocol/logLevel"]` on every request (visible in the Network tab request body). pr-screenshots/ documents the verified behavior end-to-end against both real servers: the legacy control with logs streaming, the modern per-request control, and the modern tools/call request body carrying the stamped logLevel _meta. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: send_notification emits via per-request notifier so modern logs stream (#1629) The `send_notification` tool sent its `notifications/message` through the global `server.server.notification()`. On the modern (2026-07-28) stateless leg that isn't tied to the in-flight request, so the SDK answered the tool call with plain JSON and dropped the log — nothing reached the Inspector's Logs panel. Prefer the per-request `extra.sendNotification` (rebuilt from `ctx.mcpReq.notify`): scoped to the current request, it makes the SDK upgrade the response to SSE and stream the log back on the originating request's stream. Falls back to the global notifier (legacy path / no per-request context). Verified end-to-end: opting in via the Logs-tab "Log Level per Request" control and calling send_notification now streams the entry on the modern server; legacy behavior unchanged (server-notification integration tests green). README documents the modern logging showcase and the caveat that this test server streams whenever a tool emits — the SDK beta strips the per-request envelope from the handler context, so the tool can't gate on the client's logLevel opt-in (the client-side `_meta` stamp #1629 owns is still fully visible in the Network tab). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: send_notification uses SDK request-scoped log() → spec-faithful modern gating (#1629) The previous fix routed `send_notification` through `extra.sendNotification` (`ctx.mcpReq.notify`), which streams on the modern leg but emits unconditionally — the log showed up even when the client hadn't opted in via the per-request `logLevel`. The SDK already provides the correct primitive: `ctx.mcpReq.log(level, data, logger)` (added in `McpServer.buildContext`). It applies the era-correct threshold gating itself — on the modern (2026-07-28) leg it reads the per-request `logLevel` opt-in from the request envelope and drops the message when the client didn't opt in or the level is below the requested severity; on legacy it honors the session level from `logging/setLevel` — and emits through `notify`, so the modern response still upgrades to SSE and the log rides the originating request's stream. Surface it as `extra.log` (`toHandlerExtra` maps `ctx.mcpReq.log`) and have `send_notification` prefer it, falling back to the global notifier for callers without per-request context. Verified end to end: opting in via the Logs-tab "Log Level per Request" control stamps `logLevel` in the request `_meta` and streams the log; setting it back to Off omits the key and the server gates the log (plain JSON response, no notification). Legacy server-notification integration tests still green. (No SDK change needed — the earlier apparent gap was our test server using the wrong low-level API.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * web+core: add "Log Level per Request" as a per-server setting, defaulting to on (#1629) Adds the modern per-request log level as a persisted Server Settings option (Options → "Log Level per Request", below Protocol Era), so a server can opt into logs by default instead of the Logs-tab control starting at Off. - types.ts: `ModernLogLevel` (`"off"` | the 8 levels), `DEFAULT_MODERN_LOG_LEVEL` = "debug" (default-on so a modern connection surfaces server logs out of the box), `MODERN_LOG_LEVELS`, and an `isModernLogLevel` guard. Field added to `InspectorServerSettings` and `StoredMCPServer`. - serverList.ts: round-trip the field (lift on read, omit-on-default on write, drop unknown literals) mirroring `protocolEra`; add to the persisted field allowlist. - remote/node/server.ts: validateSettings accepts/validates it and the normalize path drops a malformed literal. - inspectorClient.ts: the constructor seeds `modernLogLevel` from the setting (`"off"` → not opted in), so a modern connection stamps the level from the first request without UI interaction. mergeMeta still gates on the modern era. - ServerSettingsForm/Modal: a Select (Off + 8 levels) with a modern-era description. App seeds the live Logs-tab control from the setting on connect so the two stay in sync. Tests: form Select (default/onChange/off/reflect), modal handler, serverList round-trips, route validation (accept/reject/drop), and constructor seeding (seeded level stamped from the first request; "off" opts out). The prior modern-era "stamps once set" test is updated for the new default-on behavior. Coverage gate green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Fix CI + review: format ServerSettingsForm, drop stray network export, broaden ignore - Prettier-format ServerSettingsForm.tsx (the format:check failure that broke CI — my pre-push `npm run format` from the repo root was a silent no-op since there is no root format script; formatting is per-client). - Remove `inspector-network-logging-modern-2026-07-19T15-11-27.013Z.json`, a Network-tab export debugging artifact accidentally committed in ca2aa526 (flagged by review). - Broaden the .gitignore debugging-artifacts section with `/inspector-network-*.json` so future network exports are ignored (the existing `/inspector-history-*.json` didn't match the network export naming). Verified with the full `npm run ci` (validate → coverage → smoke → storybook), green across all clients. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: add modern-era log delivery + gating e2e tests (#1629) The modern-era suite covered the client-side `_meta` logLevel stamp but not that a log actually flows back — the legacy suite already has that (`should receive server-initiated notifications via SSE/streamable-http`), so the modern side was the gap. Add two live e2e tests against a modern (`createMcpHandler`) server serving the `send_notification` tool with `logging: true`: - opted-in (server setting = a level): calling the tool delivers a `notifications/message` over the request's SSE stream, with the right level/logger/data. - not opted in (setting "off"): the tool still returns a result, but the server gates the log — no `notifications/message` is received. This exercises the full path end to end: the client seeding + `_meta` opt-in, and the server's `send_notification` → `ctx.mcpReq.log` era-correct gating. Verified with the full `npm run ci` (green across all clients). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * web: hide the modern "Log Level per Request" setting on legacy connections (#1629) The per-request log level is meaningless on a legacy server (which uses logging/setLevel), so hide it in Server Settings when the era is legacy: - protocolEra pinned to "legacy" → hidden (no connection needed), or - protocolEra "auto" that negotiated legacy at connect time → hidden. It stays visible for a pinned "modern" server, and for an "auto" server that is either not yet connected (era unknown) or resolved to modern. Threads the negotiated era down: App passes `negotiatedEra` to ServerSettingsModal only when the settings target is the live-connected server (`connectionStatus === "connected" && settingsModalTargetId === activeServerId`); the modal forwards it to ServerSettingsForm, which gates the control on `configuredEra === "modern" || (configuredEra === "auto" && negotiatedEra !== "legacy")`. Tests: visibility matrix on the form (legacy hidden; auto+legacy hidden; auto+unconnected and auto+modern and modern shown) and a modal hide case; the existing modern-log tests now base on a modern-era settings object. Verified in the browser (legacy hides, switching to modern reveals it reactively) and with the full `npm run ci` (green across all clients). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-19 17:08:22 -04:00
Cliff Hall	b3b04c6939	Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721) (#1723) ... * Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721) Manual/auto refresh of tools/resources/prompts now forces a cache-bypassing round trip on modern servers, delivering the raw-request cacheMode control deferred from #1628. The InspectorClient list/read path uses the SDK's single-page `client.request` verbs (for pagination debugging), which never consult the SDK response cache — so `cacheMode` was inert there. This adds a separate aggregate path that routes through the SDK's cache-aware high-level verbs, where `cacheMode` is honored: - InspectorClient: new `listAllTools/Prompts/Resources/ResourceTemplates( { cacheMode, metadata })` that call `client.listTools()` etc. (all-page aggregate + SEP-2243 x-mcp-header exclusion + cache consult/write). A shared `getCacheableRequestOptions` / `aggregateListParams` back all four. - InspectorClientProtocol: the four `listAll*` methods added. - ManagedListState: delegates page walking to the SDK verb (config `fetchPage` loop → `fetchAll`); `refresh(metadata?, cacheMode?)` threads cacheMode. A `list_changed` auto-refresh bypasses the cache (`cacheMode: "refresh"`). - Managed hooks: user-initiated `refresh()` passes `cacheMode: "refresh"`, so the existing per-tab Refresh buttons become genuinely cache-bypassing — no App wiring change. Connect-time load stays default (`'use'`). The single-page `listTools(cursor)` primitive is unchanged; the paged hooks still surface per-page `nextCursor`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Single-page list pagination: sidebar toggle + server setting (#1721) Delivers the deferred single-page fetch UI: Tools/Resources/Prompts can be loaded one page at a time instead of auto-aggregating every page — a defensive option for servers with very large lists. - New `singlePageLists` server setting (persisted, server-wide), plumbed through ServerConfig/InspectorServerSettings, serverList convert/default, the node + remote settings paths, and the Server Settings form ("Fetch Lists One Page at a Time"). - New `ListPaginationControls` element (a "Single page" switch + "Load next page" button + "N pages loaded" status) rendered in each list sidebar; the toggle edits the setting live (optimistic) and persists it. - The paged state stores (PagedTools/Prompts/ResourcesState) now own loading: they auto-load page 1 on connect in single-page mode and track nextCursor + page count as observable state; the managed stores skip their all-page walk in single-page mode (so nothing is over-fetched) while still lighting the list-changed indicator. `usePaginatedList` selects the display source and pagination controls per mode — pure, no data-loading in React effects. - Refresh is mode-aware: reload page 1 in single-page mode, refetch the whole aggregate otherwise. Sidebars widened 340→360 so the controls don't crowd list entries. - Test server: `test-servers/configs/pagination-http.json` (12 tools/resources/ prompts, page size 4 → 3 pages each) exercises the flow end to end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Add pr-screenshots proving single-page pagination (#1721) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address @claude review: template load, double-click guard, auth recovery (#1721) - Fix: resource templates never loaded on connect in single-page mode. The connect-time all-page skip now applies only to lists with a paged counterpart (new `deferInSinglePage` flag on ManagedListConfig; false for resource templates), so templates still aggregate on connect regardless of the setting. - Guard `PagedTools/Prompts/ResourcesState.loadPage` against a concurrent load (fast double-click on "Load next page") so the same page can't be appended twice; the in-flight load owns the cursor and the second call is a no-op. - Thread ambient auth recovery through the single-page refresh, Load-next-page, and mode-switch loads (they previously bypassed it) — usePaginatedList's onRefresh/onLoadMore now return their promises so App can wrap them. - Drop the misleading "aggregated list" wording from the paged-store docs. Tests added for each; npm run coverage green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Review round 2: revert optimistic toggle when persisting the setting fails (#1721) Non-blocking review follow-up. If updateServerSettings rejects, the optimistic `singlePageListsOverride` is now cleared and the live client setting rolled back (the effect only clears the override when the persisted value changes, which doesn't happen on failure), so the UI and client reflect what's on disk rather than the failed edit. Added an App test for the revert path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Fix single-page Refresh not clearing list-changed indicator + UI polish (#1721) Round-3 review fix (blocking): in single-page mode a `list_changed` lights the list-changed indicator, but the single-page Refresh reloads page 1 of the paged store and bypassed `useManaged*.refresh()`, so the indicator was never cleared. Expose `clearListChanged` from useManagedTools/Prompts/Resources and call it in the single-page branch of onRefreshTools/Prompts/Resources. Tests added. UI polish (user feedback on the sidebar control): - Rename the switch label "Single page" → "Paginated". - Put "Load next page" on the same row as the switch, space-between (switch fully left, button fully right). - Move the "N pages loaded" status to its own line below the row, centered. - Hide the whole control once the list is known to be a single page (single-page mode, page 1 loaded, no nextCursor) — nothing to paginate. pr-screenshots refreshed to the new layout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Rename the singlePageLists setting to paginatedLists for clarity (#1721) `singlePageLists` read backwards from the "Paginated" switch label (it sounds like paginated=false). Rename the server setting to `paginatedLists` with the same value meaning — true = paginated/one-page-at-a-time, false = aggregate-all — and follow through on the related identifiers for consistency: - Setting field `paginatedLists` across ServerConfig / InspectorServerSettings, serverList convert/default, node + remote settings paths, and the form/modal callbacks (onPaginatedListsChange). - `usePaginatedList` / `ListPaginationControls` prop `singlePage` → `paginated`, `onSinglePageChange` → `onPaginatedChange`. - `ManagedListConfig.deferInSinglePage` → `deferWhenPaginated`. - App state (paginatedListsOverride, onTogglePaginatedLists, …) and all tests/fixtures + mode-referring comments. "single-page" wording that denotes the SDK's one-page `client.request` verb (inspectorClient) is intentionally left as-is. Purely a rename — no behavior change; unit + integration + cli + tui + coverage all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Format App.test.tsx (prettier) after the paginatedLists rename (#1721) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-19 01:06:31 -04:00
Cliff Hall	d552564966	Network: modern Mcp-* headers, new error codes, cancellation-as-abort (#1628) (#1722) ... * Network: modern Mcp-* headers, new error codes, cancellation-as-abort (#1628) Upgrade the Network tab for the modern Streamable HTTP transport (SEP-2243 / SEP-2575): - Recognise and highlight the mirrored headers (Mcp-Method, Mcp-Name, Mcp-Param-*, MCP-Protocol-Version), decode =?base64?…?= sentinel values inline, and cross-check each against the request body so a HeaderMismatch is visible before the server rejects it. - Show the new spec error codes distinctly: -32020 HeaderMismatch, -32021 MissingRequiredClientCapability, -32022 UnsupportedProtocolVersion (with the server-advertised `supported` versions), and modern -32601 MethodNotFound on HTTP 404 (distinguished from a legacy bare 404 by its JSON-RPC body). - Reflect cancellation as a connection abort ("Cancelled") rather than a hard error, per SEP §7.5. - Confirm and document browser-path header mirroring: the SDK builds the Mcp-* headers client-side and the /api/fetch proxy forwards them verbatim (new integration test). New pure util `mcpNetworkHeaders.ts` holds the recognition/decode/classify logic; new `McpErrorBadge` element renders the distinct error chips. The optional raw-request controls (cacheMode bypass/refresh, single-page fetch) are split to #1721 — they are cross-cutting core + feature-tab work beyond the Network tab. Also refreshes the AGENTS.md board Status option-ID table (the Building / MCP Apps Extension columns were removed). Closes #1628 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Network smoke-test showcase + fix redaction clobbering JSON-RPC error codes Adds a composable "modern-network" showcase server for exercising the #1628 Network-tab work end-to-end, and fixes a real bug the smoke test surfaced. Bug fix (core): the fetch-tracking body redaction masked ANY field named `code` (targeting the OAuth authorization code), including the numeric JSON-RPC `error.code`. That destroyed the field the Network tab classifies the modern spec errors on, so -32020/-32021/-32022/-32601 never rendered against real traffic. Redaction now only masks STRING values of a sensitive field (secrets are always strings; a numeric code is never one). Test-server showcase (test-servers): `modern-network-http.json` serves a `get_weather` tool with an `x-mcp-header: "City"` annotation plus four `trigger_*` tools. A new `transport.modern.injectSpecErrors` flag installs a middleware on the modern leg that answers the triggers with real HTTP status + JSON-RPC error bodies (400/-32020, 400/-32021, 400/-32022 w/ `supported`, 404/-32601), so the Inspector's error rendering can be driven live. Mirroring finding (verified live, comment corrected in createRemoteFetch.ts): the SDK builds the STANDARD mirrored headers (Mcp-Method / Mcp-Name / MCP-Protocol-Version) unconditionally — they reach the server through the web proxy — but gates `Mcp-Param-*` on a non-browser environment (`detectProbeEnvironment() !== "browser"`), so the web client never emits them. An x-mcp-header tool is therefore uncallable from the web client against a strict modern server (it answers -32020); it works from the Node CLI/TUI. The earlier "mirroring works through the proxy" note was corrected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Network: move the spec-error badge off the crowded compact top row In the narrow monitoring sidebar (embedded layout) the McpErrorBadge shared the top row with the timestamp/method/category/duration/status chips, which truncated the method and category ("PO…", "TRANSP…") and clipped the error name. Render the badge separately from `metaBadges` and place it on the second row, to the right of the URL, so the top row stays readable and the error name shows in full. The full-width layout keeps it in the top-right cluster (where there's room). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Network: keep the spec error name's casing in McpErrorBadge (no uppercase) The four spec/SDK error identifiers are PascalCase (`HeaderMismatch`, `MissingRequiredClientCapability`, `UnsupportedProtocolVersion`, `MethodNotFound`). Mantine's Badge uppercases label text by default, which flattened them to a hard-to-read run-together string ("UNSUPPORTEDPROTOCOLVERSION"). Set `tt="none"` so the badge shows the spec's own casing with word boundaries intact; short badges (POST, status) keep the default uppercase. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Network: in the full-width layout, move the spec-error badge to the second row The full-width entry kept the McpErrorBadge in the top-right cluster, where a long spec name (e.g. "MissingRequiredClientCapability") crowded the top row and truncated the method chips and URL. Move it to the start of the second row (the expand-toggle row) so the top row stays readable; the toggle stays at the end (`space-between` only when the badge is present). The compact sidebar already places it on its second row beside the URL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Protocol: show Replay as the right-side icon in the wide layout too The compact (sidebar) ProtocolEntry already renders Replay as an icon next to the pin/expand icons; the wide layout used a left-aligned "Replay" text button instead. Make the wide layout match — Replay as the ReplayButton icon at the start of the right-side action cluster (Replay, Pin, Expand) — so the two layouts are consistent. Drops the now-unused SubtleButton constant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Move the modern spec-error chip + alert from the Network tab to the Protocol tab The Network tab is focused on the HTTP transaction; the distinct protocol-error taxonomy (-32020/-32021/-32022/-32601) belongs with the protocol messages. So: - ProtocolEntry now renders the McpErrorBadge chip on the top line (right after the direction badge, both layouts) plus an explanatory alert in the expanded detail, classified by JSON-RPC error code via `classifyProtocolSpecError`. - NetworkEntry drops the chip + alert; it keeps the HTTP-level features (status badge, Mcp-* header highlight/sentinel-decode, header/body mismatch marker, cancellation-as-abort). - The SDK delivers -32020/21/22 (HTTP 400) as JSON-RPC error responses, so they reach the Protocol log directly. -32601 (HTTP 404) and other non-enveloped errors are *thrown* — the request stays pending. `enrichProtocolEntries` (App) correlates the Protocol log to the Network log by JSON-RPC id (parsed from the transport fetch's requestBody) and folds the thrown error onto its pending request, so it too appears in Protocol. This is a derived view; the underlying log is not mutated. Verified live against the modern-network showcase: both a -32020 error response and a thrown -32601 render distinctly in the Protocol tab. Follow-up: a "reveal in Network" link on the Protocol alert (cross-tab scroll-to + expand of the correlated HTTP entry). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Protocol: show the spec-error chip only in the wide layout, after the method Refine the placement of the spec-error chip added in the previous commit: - Wide layout: move it from after the direction badge to right after the method chip (reads as "tools/call -32020 HeaderMismatch"). - Compact sidebar row: drop the chip entirely — the two-line row stays uncluttered and relies on its ERROR status badge. The explanatory alert still appears in the expanded detail in both layouts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Protocol: "reveal in Network" link — jump from a spec error to its HTTP entry Adds the cross-tab navigation that was the follow-up to relocating the spec-error rendering. The expanded Protocol alert now shows a "View the HTTP request in the Network tab" link when the entry has a correlated Network request; clicking it: - correlates the Protocol message to its transport fetch by JSON-RPC id (`revealableMessageIds` gates the link; `correlateFetchEntry` finds the target on click), - clears whatever filter would hide the target (network filterText + category, and the embedded monitorSearch), - switches to the Network view in whichever mode is active — the pinned column (`pinMonitor`) when the sidebar is open, or the full-screen header tab (`onActiveTabChange`) when it's closed, - scrolls the matching NetworkEntry into view and force-expands it. The reveal is a one-shot: a new `revealId` threads NetworkScreen → NetworkStreamPanel → NetworkEntry, where the targeted entry scrolls itself into view (rAF, after `useScrollMemory`'s restore) + expands, then clears the signal via `onRevealComplete`. Verified live in both the pinned-sidebar and closed/full-screen layouts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Protocol: only show the "reveal in Network" link when it adds value Gate the link on a new `httpRelevant` flag: show it only when the error is either thrown by the SDK (its real HTTP response lives only in the Network log — the Protocol entry is a synthetic fold) OR tied to the HTTP request headers. A delivered, protocol-only error gets no link, since the raw HTTP entry adds nothing over the alert. - -32020 HeaderMismatch → link (the mirrored headers are the problem) - -32601 MethodNotFound → link (thrown; the 404 response is only in Network) - -32021 MissingRequiredClientCapability → no link (delivered, protocol-only) - -32022 UnsupportedProtocolVersion → no link (supported list is in the alert) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Network: cite SEP-2575 for cancellation-as-abort, not the internal doc's §7.5 The cancellation note referenced "(SEP §7.5)", but §7.5 is a section of the internal v2_new_spec_impact.md doc, not a SEP. The behavior — closing the Streamable HTTP request stream is the cancellation signal; notifications/cancelled is stdio-only — is defined in SEP-2575 (stateless-mcp). Cite SEP-2575 in the user-facing alert and the two related comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Network: fix reveal one-shot canceling its own scroll; review doc/comment nits Address @claude review of #1722: - Reveal one-shot: move onRevealComplete() inside the rAF (after scrollIntoView) so clearing revealId (which flips revealed->false and runs the effect cleanup) can no longer race/cancel the pending scroll. Add a flip-back test. - Comment the intentional double-mount of revealId (pinned monitor + full-screen ScreenStage both match; clear is idempotent). - Doc drift after the Network->Protocol relocation: McpErrorBadge + the mcpNetworkHeaders module header now say the spec-error chip lives in Protocol. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Network: clarify redaction array-of-scalars edge + revealableIds superset Address second @claude review of #1722 (both non-blocking polish notes): - fetchTracking redaction: note that masking assumes a sensitive value is a scalar or object; an array-of-scalars under a sensitive key would slip through (no OAuth/token payload has that shape). - revealableMessageIds: document it's a superset of the linked entries; the spec-error/httpRelevant narrowing is applied downstream at render. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Protocol: gate modern -32601 framing to a genuine 404, not in-band 200 Address third @claude review of #1722. classifyProtocolSpecError recognised -32601 from the code alone, so an ordinary in-band 'method not found' returned on HTTP 200 was dressed up as the modern MethodNotFound taxonomy: the spec badge, a 404-asserting alert, and a 'reveal in Network' link that jumped to a plain 200 (claiming diagnostic value that wasn't there). Gate the modern -32601 treatment on the correlated fetch actually being a 404 (mirroring classifyMcpSpecError). -32020/-32021/-32022 are SEP-reserved and unaffected. The correlated HTTP status is threaded from InspectorView (new correlatedFetchStatusById) through ProtocolScreen/ProtocolListPanel to ProtocolEntry; when unknown, -32601 falls back to code-only recognition (the thrown-404 path always carries a 404). Badge, alert copy, and link now resolve together, so a plain in-band -32601 falls through to ordinary error rendering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Protocol: require a genuine 404 for modern -32601 (drop unknown-status fallback) Address fourth @claude review of #1722. The prior fix only rejected -32601 when the status was *known* and not 404; an unknown status fell back to code-only recognition. But the thrown-404 path always carries a 404 (folded in from a 404 fetch), so the unknown-status branch is only ever reached by an ordinary in-band -32601 with no correlated 404 — most notably a stdio connection (no HTTP at all) — which then wrongly got the modern MethodNotFound badge and 404-asserting copy. Drop the escape hatch: -32601 is the modern taxonomy only on httpStatus === 404. Nothing intended is lost (the genuine case always has its 404). Update the tests (code-only/undefined -32601 now expects null; add a stdio no-correlated-fetch ProtocolEntry case) and mark the MethodNotFound story with correlatedHttpStatus 404. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-18 17:32:05 -04:00
Cliff Hall	9b2c188d11	MRTR: drive input_required manually to keep pending-request UX (#1704) (#1719) ... * MRTR: drive input_required manually to keep pending-request UX (#1704) Follow-up to #1627/#1705. Instead of letting the SDK auto-fulfil the modern (2026-07-28) MRTR retry loop, InspectorClient now drives it itself so the Inspector keeps its pending-request UX, owns the loop, and shows era-accurate semantics. Core (core/mcp/inspectorClient.ts): - Construct the SDK Client with `inputRequired: { autoFulfill: false }` (unconditional; a no-op on legacy, which never returns input_required). - New `requestWithInputRequired()` driver: issues the request with `allowInputRequired` + `withInputRequired(schema)`, and while the result is `input_required`, fulfils each embedded request through the SAME pending sampling/elicitation plumbing, then retries the original call with `inputResponses` + the echoed `requestState` on a fresh id, bounded by MRTR_MAX_ROUNDS. - Route tools/call, prompts/get, and resources/read through the driver. - Embedded roots/list is auto-answered from configured roots (no pending UX); decline/cancel is echoed to the server (not treated as an abort); a tool-call abort mid-round settles the pending request and surfaces ToolCallCancelledError. - Extract shared `enqueuePendingSample`/`enqueuePendingElicitation` helpers used by both the legacy inbound handlers and the driver; add a per-request `origin` ("server-request" | "input-required") to the pending wrappers. Web UI: - New `MrtrOriginNote` element shows an "input_required — your answer returns as a retry" note on a modern MRTR round; legacy panels are unchanged. - Thread `origin` App -> PendingClientRequestModal -> the sampling/elicitation panels (and the inline panels). Test servers: new MRTR presets (multi-round, roots, sampling, loop, edge-case param shaping). Integration tests cover the manual round-trip (asserting the pending UI pauses and the retry carries inputResponses + requestState on a new id), multi-round, roots auto-answer, sampling, decline-echoed, maxRounds bound, abort mid-round, and requestState-only/inputRequests-only param shaping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Review: clarify sampling-reject and between-rounds abort comments (#1704) Address review nits on PR #1719 (comment-only, no behavior change): - enqueuePendingSample docstring: sampling has no decline/cancel action; the panel either sends a result (resolve) or Rejects (reject → fails the call). The shared "declined/cancelled resolves" wording was accurate only for the elicitation case. - callToolWithRetries abort comment: note the between-rounds case — when the abort lands while an MRTR round awaits an embedded pending request there is no in-flight SDK request, so nothing is sent on the wire; the outcome (ToolCallCancelledError, no failed-call record) is identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: add mrtr-showcase config exercising every MRTR preset (#1704) A single modern-era streamable-HTTP server (port 3111) bundling all six MRTR presets — mrtr_confirm, mrtr_two_step, mrtr_sample, mrtr_roots, mrtr_edge, mrtr_loop — for manual testing of the manual-driving pending-request UX across single-round, multi-round, sampling, roots, param-shaping, and maxRounds paths. Document it in the README alongside modern-mrtr-http.json. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-17 23:15:19 -04:00
Cliff Hall	e026051b47	Era-aware connection model & capability gating (#1626) (#1697) ... * Era-aware connection model & capability gating (#1626) Make protocol era (SEP §7.8) a first-class, per-server connection dimension, orthogonal to the transport type, and drive the SDK Client's versionNegotiation from it. Config - Add `ServerProtocolEra` ("legacy" | "auto" | "modern"), `DEFAULT_PROTOCOL_ERA` (legacy, per the SDK's don't-auto-probe guidance), `MODERN_PROTOCOL_VERSION`, and `eraToVersionNegotiation()` in core/mcp/types.ts. - Persist `protocolEra` on `StoredMCPServer` / `InspectorServerSettings`, round-tripping through serverList.ts (omitting the default on disk) and the `/api/servers` read + write validators. InspectorClient / state - Thread `versionNegotiation` through `InspectorClientOptions` (default legacy), replacing the hardcoded pin. After connect, read `getProtocolEra()` / `getDiscoverResult()` / `getNegotiatedProtocolVersion()`, expose them via getters, dispatch `protocolEraChange` / `discoverResultChange`, and clear on disconnect. `useInspectorClient` surfaces `protocolEra` / `discoverResult`. Web UI - ServerSettingsForm: a Protocol Era selector bound to the per-server setting. - ConnectionInfoContent: era-aware — Era badge, Session ("Sessionless" on modern), and a Discovery section (supported versions + extensions) from the `server/discover` result. CLI/TUI also map the mcp.json `protocolEra` into versionNegotiation so the field applies uniformly. Version negotiation stays fully client-side; modern connect works over the existing remote tunnel and disconnect is sessionless (the SDK skips DELETE when there is no session id). Mcp-* header fidelity is left to the History/Network card. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address @claude review: era docstrings, era validation, stdio session label, AGENTS.md rule - Fix stale docstrings/comments in inspectorClient.ts, useInspectorClient.ts, inspectorClientEventTarget.ts, and ConnectionInfoContent.tsx: getProtocolEra() returns "legacy" (not undefined) on a plain legacy connect; it is undefined only when not connected. - Validate protocolEra at the storedFieldsToInspectorSettings read boundary (new shared isProtocolEra guard) so a hand-edited mcp.json read directly by the CLI/TUI drops an unknown era instead of passing it to eraToVersionNegotiation. Keeps the exhaustive switch intact. - ConnectionInfoContent: the Session row is HTTP-only — show "N/A (stdio)" for stdio instead of the misleading "Session-based". - AGENTS.md: add a rule against unjustified `as unknown as` double casts, and drop the one such cast introduced in the serverList test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address re-review: dedupe isProtocolEra guard in remote server Replace the local isProtocolEra in remote/node/server.ts with the shared guard now exported from serverList.ts (already imported here), so the era literal set lives in one place and can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-16 23:39:29 -04:00
Cliff Hall	7fecebed5e	OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1694) ... * OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1625) Re-key the persisted OAuth store to (server, issuer) and add the SEP-837/2350/2352 authorization-hardening UX. The SDK v2 auth() drives the SEP-2352 flow off the provider's ctx.issuer; the Inspector's job is to honor it and round-trip the issuer stamp. SEP-2352 — issuer-bound credentials - ServerOAuthState nests issuer-bound credentials under byIssuer[issuer] = { clientInformation, clientRegistrationKind, tokens }, with an activeIssuer pointer answering the transport's ctx-less per-request bearer read. - Fix a latent bug: OAuthStorageBase re-parsed tokens/client-info through OAuthTokensSchema/OAuthClientInformationSchema, which strip the non-RFC `issuer` field and silently defeat the SDK's discardIfIssuerMismatch. The store re-attaches the stamp from the byIssuer key. - BaseOAuthClientProvider threads ctx.issuer and implements discoveryState()/saveDiscoveryState() (activates the SDK's callback-leg AuthorizationServerMismatchError binding check) and invalidateCredentials(). - Lazy, migration-free upgrade: a legacy pre-1625 blob's top-level clientInformation/tokens are returned as an unkeyed fallback until the first issuer-stamped save promotes them into byIssuer. SEP-837 — application_type - DCR declares application_type: "native" (localhost Inspector). - ClientSettingsForm renders RegistrationRejectedError detail and reflects DCR's deprecated-in-favor-of-CIMD (SEP-991) status. SEP-2350 — step-up scope union - Per-server onInsufficientScope (reauthorize | throw) setting wired ServerSettingsForm -> StoredMCPServer.oauth -> StreamableHTTPClientTransport. - StepUpAuthModal visualizes the prior-union-challenged scope set, tagging each scope `new` vs `already granted`. mcpAuth simplification - Delete the hand-rolled authorizeWithoutRefresh (mishandled issuer stamping); forward forceReauthorization/skipIssuerMetadataValidation to native SDK auth() so both the initial and step-up paths get SEP-2352 stamping. Vocabulary + docs - OAuthStep gains cimd_fetch / issuer_comparison / scope_step_up (recorded on a step-up redirect); the Network tab labels auth requests by OAuth phase. - v2_auth_hardening.md gains an as-built status section. Deferred to the era-aware card: the SdkError(EraNegotiationFailed) -> UnauthorizedError unwrap and finishAuth(URLSearchParams) form (only fire under non-legacy negotiation). Closes #1625 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address @claude review: issuer-stamp source gating + clear/activeIssuer + phase over-match - SEP-2352: stamp the issuer only when the credential actually came from the byIssuer slot (`fromSlot`), not merely when a slot exists. A partially-populated slot alongside a surviving legacy field would otherwise stamp a legacy (possibly cross-AS) credential with the current issuer, defeating discardIfIssuerMismatch. - Issuer-scoped clears no longer promote the cleared issuer to `activeIssuer` (updateIssuerSlot gains a `setActive` flag; save paths keep the promotion). - oauthNetworkPhase matches register/token/authorize as the final path segment (trailing slash tolerated) so `/token/refresh` or `/api/register/foo` no longer over-match. - Tests for all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-16 16:52:53 -04:00
cliffhall	f4fb4b50c9	Forward RFC 9207 iss to the v2 SDK (fixes OAuth + EMA leg 1) ... SDK v2 added RFC 9207 §2.4 authorization-response issuer validation (`validateAuthorizationResponseIssuer` / `IssuerMismatchError`) as a mix-up attack defense. It is strict: when AS metadata advertises `authorization_response_iss_parameter_supported: true` and no `iss` reaches `exchangeAuthorization`/`auth()`, the exchange throws. v1.29.0 had no `iss` parameter and no RFC 9207 validation at all, so dropping the callback `iss` was harmless. The migration swapped in the stricter SDK without feeding it the new input, breaking the authorization-code exchange against any RFC 9207 authorization server (Okta/xaa.dev advertise it): Issuer mismatch in authorization response (RFC 9207): expected "https://idp.xaa.dev", received undefined Found by smoke-testing EMA against live xaa.dev staging; not caught by CI because the mock IdP never advertised the parameter, so the SDK took its lenient path (a missing `iss` is only fatal when advertised). `iss` was dropped in three places, with the plumbing half-built — `oauthManager`/`inspectorClient` already accepted `iss`, but nothing supplied or forwarded it: - `parseOAuthCallbackParams` never extracted `iss` from the callback query - `mcpAuth` never forwarded `options.iss` to SDK `auth()` — its own comment ("forwarded on v2 upgrade; ignored by v1 SDK auth()") predicted this exact gap, so standard OAuth was broken identically, not just EMA - EMA leg 1 (`completeIdpOidcAuthorization`) never passed `iss` to `exchangeAuthorization` Thread `iss` end to end: callback query -> CallbackParams -> resumeAfterOAuth -> completeOAuthFlow -> {mcpAuth | EMA leg 1} -> SDK. The Node callback server and runner carry it for CLI/TUI. Tests: `ema-mock-servers.ts` now advertises `authorization_response_iss_parameter_supported: true` so the mock is at least as strict as real IdPs — with it absent the SDK silently takes the lenient path and a dropped `iss` goes unnoticed. Flipping it made three existing tests reproduce the failure. Adds regression coverage for forwarding and for rejecting a mismatched `iss` before any network call. Verified end to end against live xaa.dev staging: EMA legs 1-3 (IdP SSO -> ID-JAG mint -> resource token) now complete and the MCP session initializes. web 3099, cli 212, tui 274 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjMemXZohSR2EndBQYif2b	2026-07-15 20:16:43 -04:00
cliffhall	dbb8c40245	Address review: justify/refine SDK-gap casts (as unknown as) ... Per human review on #1688: for each `as unknown as` in the PR, either refactor or justify with a comment naming the SDK gap (so it can inform SDK-side type improvements). - Sampling/elicit receiver-task returns: route through a typed `CreateTaskResult` so the object shape IS checked; the remaining cast to CreateMessageResult / ElicitResult is the honest SDK gap (the 2-arg setRequestHandler overload types the handler return as the spec result only, not the task-augmented variant). - `_requestHandlers` / `_progressHandlers` private-field casts: comments now name the exact missing public API (opt a handler out of result validation; keep a progress subscription alive across a resolved request). - test-server `_requestHandlers` cast: same SDK gap on the server side, noted. - serverList object->Record casts (pre-existing): documented as plain structural widening (no index signature -> TS requires the `unknown` step), not an SDK workaround. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-15 14:47:49 -04:00
cliffhall	8a7a38f800	Fix MCP App load regression: translate ext-apps v1 setNotificationHandler API to v2 ... An MCP App (e.g. NightRider's drum-pad) failed to load with "'[object Object]' is not a spec notification method; pass schemas as the second argument to setNotificationHandler()". `@modelcontextprotocol/ext-apps` still peers on SDK v1 and, during the AppBridge connect handshake, subscribes to tools/resources/prompts list_changed with the v1 schema-first API `setNotificationHandler(NotificationSchema, handler)`. SDK v2 requires a method STRING and throws on a schema object — the migration's compat shim covered the types but not this runtime API difference. Fix in the app-renderer client Proxy (`getAppRendererClient`): translate a schema-first `setNotificationHandler` call to v2's method-string form by reading the method literal off the schema's `shape.method.value`; native string-first calls pass through, and an unrecognized first arg falls through to the SDK's clear error rather than being swallowed. Only `setNotificationHandler` needed this — AppBridge's `request(req, schema, …)` calls work as-is (the v1 SDK schemas expose `~standard`, so v2 accepts them). Remove when ext-apps#702 ships a v2 peer. Added an integration test covering the schema-first translation end-to-end (handler fires on a real tools/list_changed), plus the string-passthrough and unrecognized-schema branches. Coverage ≥90 holds; typecheck + App tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-15 12:44:20 -04:00
cliffhall	f9f79fff54	Address @claude review: format core/, typed output-validation error, document progress heuristic ... - Run Prettier on the 60 core/ files this PR touched — the deterministic import rewrite had collapsed multi-line imports and left throws unwrapped; core/ is in no format gate so it slipped past CI. Now Prettier-clean. - Default-path output-schema violation now throws ProtocolError(ProtocolErrorCode.InvalidParams, …) instead of a bare Error, matching the prior v1 contract (SDK threw a typed McpError with a code) so downstream `instanceof`/`.code` branching keeps working. - Document the "assumes serial construction" invariant on pollTaskToolCall's progress-subscription revival (the `_progressHandlers` new-key heuristic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-15 11:08:54 -04:00
cliffhall	19a6df7ad3	SDK v2 migration (behavior-neutral, versionNegotiation: 'legacy') (#1624) ... Migrate all three clients + core + test-servers from @modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior identical against legacy (2025-11-25) servers. Foundation for the SDK V2 + New Spec stack. Packages - Split @modelcontextprotocol/sdk -> @modelcontextprotocol/client (Client, transports, OAuth, error taxonomy, protocol types) + core (Zod schemas); stdio -> client/stdio, Ajv -> client/validators/ajv. Test-servers -> @modelcontextprotocol/server (+ server-legacy/sse). Deterministic import rewrite (schemas -> core, everything else -> client); the codemod misroutes client code to /server so it was not used for path routing. - new Client(..., { versionNegotiation: { mode: 'legacy' } }) — nothing 2026-era goes on the wire. Runtime (core/mcp/inspectorClient.ts) - Error taxonomy: McpError -> ProtocolError, ErrorCode -> ProtocolErrorCode; RequestTimeout -> SdkError/SdkErrorCode. - Handlers take method strings; task methods (tasks/*, notifications/tasks/*) use the 3-arg custom form (excluded from v2's spec-method set). - Legacy tasks kept working (per project requirement): requestor tasks re- implemented over raw client.request + explicit deprecated schemas; experimental.tasks.callToolStream replaced by a local pollTaskToolCall async generator (with _progressHandlers revival so task-execution progress still reaches the caller). Receiver (server-initiated) tasks: the v2 Client validates a sampling/elicit handler result and rejects the legacy { task } response with -32602 — installReceiverTaskResponseBypass swaps the wrapped entry in the private _requestHandlers map for the task-augmented branch so { task } reaches the wire (fixes server-initiated sampling-via-task). - Pagination: list verbs use raw client.request({method}, List*ResultSchema) for single-page behavior (v2's client.listX() auto-aggregate all pages). Web / auth / apps - Rename local JsonSchemaType -> InspectorFormSchema + add toFormSchema() structural narrowing at the SDK-schema -> form boundary. - ext-apps still peers on SDK v1: one compat boundary with documented casts (remove when ext-apps#702 lands a v2 peer). - OAuth provider saveClientInformation reconciled with v2's OAuthClientInformationContext; InvalidClientMetadataError re-homed as a local class (server-legacy/auth is not browser-safe). - vitest.shared.mts dedupes @modelcontextprotocol/client+core to one copy so vi.mock intercepts the SUT's import. Accepted v2 behavior changes (per #1624 / spec §9.2): unknown-tool call rejects with -32602; cancellation is stream-abort (no guaranteed notifications/cancelled frame); output-schema strictness restored via the Inspector's own Ajv check on the default path. npm run ci green: validate (web 3087 / cli 212 / tui 274 / launcher 5), coverage (>=90 per-file, all clients), smoke (launcher/cli/web), storybook (410). Closes #1624 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-15 10:44:24 -04:00
Cliff Hall	a029e42a19	cli: Wave 3 — programmatic review path (rollup of #1573–#1575) (#1655) ... * 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>	2026-07-11 22:14:23 -04:00
cliffhall	a26cbac609	review: harden version read + fix MonitoringScreen docstring ... Address PR #1641 review feedback: - Add readInspectorVersionSafe() (non-throwing) in core/node/version.ts and use it in the web backend, so a failure to resolve the root manifest hides the (cosmetic) version badge instead of crashing the backend at startup. Covered by tests for both the resolved and unresolvable paths. - Update MonitoringScreen's docstrings: it now wraps every supplied screen in a ScreenStage (only the active tab mounts, cross-fading) rather than rendering just the active tab's node. The third fetch-consolidation suggestion (one useInitialConfig instead of three GET /api/config hooks) is tracked as a follow-up in #1643. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw	2026-07-10 22:40:34 -04:00
cliffhall	650e5bd020	feat(web): show the Inspector version in the lower-right corner (#1639) ... Display the single-source version (root package.json) in the web UI's lower-right corner, in grey — matching the CLI and TUI. The browser can't read the filesystem, so the version flows backend → browser via GET /api/config: - Add `version?: string` to InitialConfigPayload; the web backend (webServerConfigToInitialPayload) reads it once via the shared readInspectorVersion() and tags it onto the payload. - Add a `useInspectorVersion` hook mirroring useSandboxUrl/useServerListWritable. - Add a presentational `VersionBadge` element rendered fixed lower-right, grey, non-interactive; styling lives in a `versionBadge` Text theme variant (no inline styles, --inspector-* tokens). - Wire it into App.tsx. Verified live: /api/config serves `version: "2.0.0"` and the badge renders `v2.0.0` fixed at the lower-right in grey. Tests: useInspectorVersion (full branch coverage), VersionBadge (unit + Storybook), web-server-config payload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw	2026-07-10 19:07:31 -04:00
cliffhall	7815548300	refactor(version): single-source the version from the root package.json ... The Inspector ships as one package with one version, but four client package.jsons each carried their own `version` and were read at runtime (cli.ts, tui App.tsx), and core hardcoded a third, stale value (0.20.0) that the TUI and web actually reported to servers. Consolidate onto the root package.json as the single source of truth. - Add core/node/version.ts: shared, Node-only readInspectorVersion() that walks up to the root package.json (always shipped in the tarball). 100% covered by a new integration test. - CLI and TUI read the version via the shared reader instead of their own manifest; TUI now also passes a real clientIdentity (was falling through to the stale core default). - Drop the `version` field from all four client package.jsons; the root keeps the only version. TUI header name/description become local display constants. - core default client identity: replace the stale 0.20.0 hardcode with a neutral 0.0.0 placeholder (real clients pass clientIdentity; web will source its version from GET /api/config, see #1639). - Stop shipping clients/cli/package.json and clients/tui/package.json (no longer read at runtime) — supersedes the earlier "ship them" fix; update pack:verify and the root README packaging invariants accordingly. - Add core/node/** to the web coverage include so the new module is gated. - Add the shared-code architecture diagram to the root README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw	2026-07-10 17:31:21 -04:00
cliffhall	32a3610737	Merge remote-tracking branch 'origin/v2/main' into 1579-wave-1 ... # Conflicts: # clients/cli/package-lock.json # clients/cli/package.json # clients/web/src/App.tsx # clients/web/src/lib/environmentFactory.test.ts # clients/web/src/lib/environmentFactory.ts # clients/web/src/lib/remoteOAuthStorage.test.ts # clients/web/src/lib/remoteOAuthStorage.ts # clients/web/src/test/core/auth/connection-state.test.ts # clients/web/src/test/core/auth/providers.test.ts # clients/web/src/test/core/auth/storage-browser.test.ts # clients/web/src/test/core/auth/storage-remote.test.ts # clients/web/src/test/core/mcp/oauthManager.test.ts # clients/web/src/test/integration/auth/node/storage.test.ts # clients/web/src/test/integration/storage/adapters.test.ts # clients/web/src/utils/clearServerOAuthState.test.ts # clients/web/src/utils/clearServerOAuthState.ts # core/auth/connection-state.ts # core/auth/oauth-storage.ts # core/auth/providers.ts # core/auth/storage.ts # core/auth/store.ts # core/storage/adapters/remote-storage.ts	2026-07-09 09:41:08 -04:00
cliffhall	1e6a2825d4	fix(auth): keep oauth-persist browser-safe (move Node file backend out) ... The refactor routed the browser's RemoteOAuthStorage through `oauth-persist.ts`, which value-imported the Node-only `store-io.js` (`atomically` -> `stubborn-fs` -> `node:process.getuid`, plus `node:fs/promises`). That dragged Node built-ins into the browser bundle and the web app crashed to a blank page at runtime — a regression from v2/main, where browser OAuth storage used a Zustand JSON adapter with no Node imports. Unit/integration tests run in node/happy-dom and `smoke:web` only checks the served HTML, so none of them execute the app in a real browser and the break slipped through. Fix, following the existing `store-id.ts` precedent: - Split the pure JSON (de)serializers into Node-free `core/storage/store-serialize.ts`; `store-io.ts` re-exports them so existing importers are unaffected. - Move the Node-only `createFileOAuthPersistBackend` into `core/auth/node/oauth-persist-file.ts`; update `storage-node.ts` and the storage adapters test to import from there. - `oauth-persist.ts` now imports only the Node-free serializers, so the browser bundle no longer pulls in `node:fs`/`atomically`. Tests: - oauth-persist-browser-safe.test.ts: source-level guard that `oauth-persist.ts` never imports store-io/node:/atomically. - Direct unit tests for the isomorphic remote + session backends (restoring oauth-persist.ts coverage after the file backend moved out). - store.test.ts: OAuthMemoryStore unit coverage incl. replace() fallbacks. - storage.test.ts: clearAllOAuthClientState null-snapshot (no file) path. Verified end-to-end in a real browser (Playwright): DCR OAuth flow completes, tokens persist via the remote store, and after a full page reload the server reconnects with no re-authentication. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-08 16:20:31 -04:00
cliffhall	54d5b18b1e	fix(auth): drop redundant empty-object guard in remote OAuth read() ... parseOAuthPersistBlob already returns null for {} (the server's missing-file response) and for a literal null body, so the Object.keys(store).length === 0 guard was redundant — and would throw a TypeError on a null body before parseOAuthPersistBlob could handle it. Pass the parsed res.json() straight through. Addresses Claude review item #1 on #1612. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-08 13:54:14 -04:00
cliffhall	edc7dbb1c1	Merge remote-tracking branch 'origin/v2/main' into v2/1549-oauth-storage-no-zustand ... # Conflicts: # core/auth/store.ts	2026-07-08 13:44:12 -04:00
cliffhall	ba48e06ffd	fix(auth): guard OAuth-storage async paths against unhandled rejections ... Review follow-ups on the RemoteOAuthStorage web migration: - useEmaIdpLoginState.logout(): add a .catch to the now-async clearEmaIdpSession call so a storage failure no longer surfaces as an unhandled rejection; leave loginState unchanged on failure (the session was not actually cleared). Add a test asserting no unhandled rejection. - createOAuthStore: attach a no-op catch to the eager persistLoadPromise so a hydration read that fails before any load() awaits it does not become an unhandled rejection; real awaiters still observe the rejection via the shared reference. - storage-remote.test: replace `as unknown as typeof fetch` double-casts with the typed vi.fn<typeof fetch>() pattern the same PR established. - remoteOAuthStorage: correct the doc comment to describe the cache-of-one accurately instead of implying a keyed map. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-08 10:59:40 -04:00
Bob Dickinson	af174666d6	fix(auth): address Claude review on OAuth persist serialization ... Serialize persist() in OAuthStorageBase so overlapping remote POSTs cannot reorder and clobber state. Regenerate launcher package-lock.json to drop stale zustand entries. Let parseOAuthPersistBlob accept parsed objects from res.json() without a redundant JSON round-trip.	2026-07-05 18:38:13 -07:00
Bob Dickinson	3c65bdb4c6	Removed Zustand storage, cleanup up auth storage IO interfaces, implementations, and usage.	2026-07-04 21:56:45 -07:00
cliffhall	c026d4d84a	docs: address review nits on the v2/main merge ... - 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	2026-07-04 15:42:22 -04:00
Bob Dickinson	512f45d357	fix(auth): reject load() when persist read fails	2026-07-03 14:50:04 -07:00
cliffhall	cfbdfdf2bd	Merge remote-tracking branch 'origin/v2/main' into 1579-wave-1 ... # Conflicts: # AGENTS.md # clients/cli/README.md # clients/web/src/App.test.tsx # clients/web/src/App.tsx # clients/web/src/test/core/auth/utils.test.ts # core/mcp/node/transport.ts # test-servers/src/composable-test-server.ts	2026-07-03 17:39:07 -04:00
Bob Dickinson	a24091484c	fix(auth): await OAuth storage load() to fix remote OAuth races	2026-07-03 14:18:00 -07:00
cliffhall	14404bc405	refactor: remove unsafe non-null assertions / double-cast in auth paths ... Address the TypeScript-safety review items on the mid-session auth work: - oauthManager: replace the unguarded `this.oauthConfig.storage!` assertions on the EMA re-mint/persist paths with a `requireStorage()` helper that throws a clear error; in `getOAuthState` reuse the already-narrowed local `storage`. - cli: `parseRunnerOAuthCallbackUrl` always returns a config, so make `callMethod`'s `callbackUrlConfig` (and the always-passed `cliAuthOverrides`) required instead of asserting them non-null at three call sites. - test-servers/resolve-config: drop the `as unknown as T` double cast; collect into a base-shaped array and do a single honest downcast at the return. No behavior change; validate + coverage green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZyeWManYTvcXxjcrhxSQc	2026-07-03 13:16:05 -04:00
cliffhall	f4b6095719	test: close ≥90% per-file coverage gaps for mid-session auth ... The mid-session auth work passed `validate`/CI but failed the local-only `npm run test:coverage` per-file gate (≥90% lines/statements/functions/ branches) on 12 files. Add targeted unit/integration tests so every changed file clears the gate; annotate provably-dead defensive branches with justified `/* v8 ignore */` comments rather than relaxing the gate. Files brought to ≥90 on all four dimensions: - core/auth: mcpAuth, oauthUx, utils, challenge, node/runner-interactive-oauth - core/mcp: oauthManager, inspectorClient, node/authChallengeFetch, remote/remoteClientTransport, remote/node/{remote-session,server} - clients/web/src/utils: oauthResume, browserTabVisibility `npm run test:coverage` now passes (3575 tests, 0 threshold failures); `npm run validate` green across all four clients. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZyeWManYTvcXxjcrhxSQc	2026-07-03 12:54:37 -04:00
Bob Dickinson	a066873e37	Fixes for remaining issues	2026-07-02 18:36:44 -07:00
Bob Dickinson	52023ff1c3	fix(auth): clear OAuth callback timer and reorder EMA scope persist ... Clear the loopback callback timeout in runner-interactive-oauth finally so already_authorized and error paths do not hang the CLI/TUI. Check EMA step-up satisfaction against minted token scope before persisting the requested union, matching the standard OAuth path. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-07-02 17:55:34 -07:00
Bob Dickinson	562bdd00f1	All of the mid-session and step-up auth work	2026-07-02 16:35:31 -07:00
cliffhall	775b98a019	docs(core): clarify redaction scope + add nested request-body test ... Address round-1 review feedback on fetchTracking redaction: - Cross-reference REDACTED_VALUE / REDACTED_HEADER_VALUE so a reader understands why two identical "[REDACTED]" constants coexist. - Document that redactBody deliberately scopes to form-encoded + JSON (multipart/binary bodies pass through; OAuth never uses multipart). - Add a tracker-path test asserting a nested sensitive key in a JSON request body is redacted in the recorded copy while the live outbound body stays byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX	2026-07-01 22:34:40 -04:00