Copilot review round 18. Three of its findings were the same check-then-act:
`planOAuthRevocation` is asynchronous, so the gap between its reads and the
following `storage.clear()` is still a window — shorter than the network one
closed in round 13, but the same shape. An OAuth completion landing at any of
those awaits saved a grant the clear then destroyed.
`OAuthStorage.takeRevocationSnapshot(serverUrl)` reads what revocation needs
and clears the server in ONE synchronous pass over the in-memory state, with
the persist after the mutation. `clearAndPlanRevocation` replaces
`planOAuthRevocation`, and there is no separate `clear` call left in any
caller — the clear cannot be forgotten or reordered.
The snapshot returns values unparsed, deliberately: schema validation is pure
and belongs after the mutation, since running it inside would reintroduce the
`await` this exists to remove.
Two consequences worth naming. A failure of the take-and-clear now REJECTS
rather than returning an outcome, because the state was not cleared and the
caller's contract is that it was; both clients already have a rejection path.
And cross-process atomicity is explicitly not claimed — nothing in this store
takes a lock, every mutation is a read-modify-write over a loaded snapshot, and
`clear` alone always had that property.
Also from round 18:
- The CLI's budget check ran before considering `plan.outcome`, so a key that
needed no network was reported as budget-exhausted — warning that a grant may
be live when that key held none.
- A disconnect failure after a successful clear was reported as "could not
clear OAuth state" in both clients. It now goes to the disconnect channel,
and the success is still reported.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 12, and the best catch of the review.
`onClearOAuth` was wired as `() => { void handleClearOAuth(); }`, so the
promise `AuthTab` awaits resolved instantly. Everything round 6 through 9 built
on that await was therefore inert in production while still passing its own
tests: the pending line never showed, the repeat lock released immediately, and
a rejection went unhandled instead of reaching the failure line.
Passed directly now. The regression test drives the real App rather than
`AuthTab` in isolation — the wiring is the contract, and only an App-level test
can see it. Verified it fails when the void-ing arrow is put back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
`clear(serverUrl)` deletes EVERY `byIssuer` slot, but revocation read only the
context-free (active-issuer) token. A server that had authorized against
issuers A and B therefore revoked B, then destroyed the local record of A while
A's grant stayed live at its authorization server — the same leak this feature
exists to close, one level down.
- `OAuthStorage.listIssuers(serverUrl)` is the new seam (one implementation:
every backend extends `OAuthStorageBase`). `collectGrants` walks it plus the
ctx-less read, deduping by token, so every grant the clear will delete is
covered.
- Metadata is cached once per server, not per issuer, so a grant bound to an
issuer the cached document does not describe cannot be revoked: sending its
token to that endpoint would hand a credential to a server that never minted
it. That grant is reported as failed — dropped unrevoked — rather than
silently discarded or misdirected.
- `aggregateOutcomes` gives a failure precedence over another grant's success,
so one live grant cannot hide behind another's 200. Exported for its own
test, since `computeOutcome` returns early on the empty case.
Also from that round:
- The fixture's /oauth/revoke now verifies token ownership (RFC 7009 §2.1).
Access tokens record their owning `client_id`; a token belonging to another
client is left alone and still answered 200 (§2.2 — the response must not
tell one client whether another's token exists). New e2e case proves a
second registered client cannot revoke the first's grant.
- The TUI rendered the revocation-failure note in cyan, the informational
tone. It is a partial success — the local clear really happened — so it is
not an `error` status, but the grant may still be live and cyan understated
that. `AuthTab` takes an `oauthMessageTone`; the message says so explicitly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
- RFC 8414 §2 defaults an omitted `revocation_endpoint_auth_methods_supported`
to `client_secret_basic`; it does not inherit the token endpoint's list.
Inheriting made metadata advertising only `client_secret_post` there send
POST credentials to an endpoint that never advertised the method.
- The 5s timeout was inert on the web paths: `createRemoteFetch` re-issues the
call as a POST to `/api/fetch` and drops `init.signal`, and the backend's
outbound fetch gets none either, so a wedged AS could hold the teardown open
indefinitely. A wall-clock race now enforces the deadline; the signal is kept
because it genuinely cancels the direct-fetch paths.
- The settings-modal Clear read `settingsModalTarget`, which comes from the
persisted `servers` list, while edits live in `settingsDraft` until the save
debounce. Toggling the checkbox and clearing immediately used the previous
value. It now merges the draft.
- `--relogin` clears both key spellings, so it now revokes from both rather
than stopping at the first populated one — a stale entry under the other
spelling is a live grant. The normalised key goes first, matching
`findStoredServerState`, and a second key holding the same token is skipped.
- The fixture's /oauth/revoke now ENFORCES RFC 7009 §2.1 client
authentication (both RFC 6749 §2.3.1 forms), so the e2e's claim that the
Inspector authenticates correctly is actually tested; two negative cases
assert a wrong secret and an unidentified client are refused.
- Dropped the `as unknown as` casts from the tests: a typed `InspectorLogger`
double, and a spy on the real storage instead of a spread-and-cast.
- Added CLI-level tests through `runCli` for `--no-revoke`, the per-server
`oauth.revokeOnClear`, and the stderr warning, and TUI tests for the
forwarded opt-out and the rendered failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Clearing the Inspector's OAuth state deleted the local copy of the tokens
and stopped there. The access token — and the refresh token, which is
long-lived by design — stayed valid at the authorization server until they
expired on their own, so a day of connect/disconnect iteration left it
holding grants for sessions that ended hours ago. RFC 7009 §1 describes
exactly that case, and nothing in the repo constructed a revocation
request, so a server author implementing `revocation_endpoint` had no way
to exercise it with the tool that exists to exercise this.
core/auth/revocation.ts builds and sends the request; the three clear paths
call it, so the behavior is written once — web's "Clear OAuth state and
disconnect" (both the active and stored-only paths), the TUI's Auth tab,
and the CLI's --relogin. It runs BEFORE the local clear, because the token,
the client credentials and the discovered `revocation_endpoint` all live in
the store the clear is about to empty. The request names the refresh token
when there is one: §2.1 asks the AS to also invalidate the access tokens
issued under the same grant, so one request covers both halves.
Best-effort by construction. Every path returns a TokenRevocationOutcome
rather than throwing: no advertised endpoint means nothing is sent and that
authorization server behaves exactly as before, and a network error, a
non-2xx, a 5s timeout or an unreadable store is reported and nothing more.
Forgetting the tokens is what the user asked for. `lost_authorization_state`
recovery deliberately skips it — it clears a half-finished flow to retry it,
and an authorization that never completed has no grant to revoke.
Client authentication resolves the preregistered slot first, mirroring
BaseOAuthClientProvider.clientInformation; reading only the dynamic slot
would send no authentication for a server configured with `oauth.clientId`.
The Basic credential is byte-identical to the SDK's applyBasicAuth so this
request and the token request cannot present the same secret differently.
Opt out per server with `oauth.revokeOnClear` (default on, only `false`
written to disk) or per CLI run with --no-revoke. Read at clear time rather
than connect time, so toggling needs no reconnect. Turning it off is a
testing affordance: a client that walks away still holding live tokens is a
case a server author may want to reproduce.
test-servers now advertises `revocation_endpoint` and serves POST
/oauth/revoke, faithful on §2.2 (an unknown token is a success) and §2.1
(revoking a refresh token invalidates the access tokens under the same
grant), with oauth-revocation-http.json / oauth-no-revocation-http.json as
showcase configs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
One conflict, in `clients/tui/__tests__/App.test.tsx`: both sides added an
`opts` capture to the test harness's `FakeClient` for different fields —
`defaultMetadata` on v2/main (#1910), `environment.fetch` here (#2067) — so the
resolution is the union of the two, keeping both rationales and both tests.
Everything else auto-merged. `npm run ci` passes on the result.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses review round 6 on #2099. Both nits were correct.
- The proxy-fetch sentinel used `as unknown as typeof fetch`, which AGENTS.md
forbids without justification — and none was needed here: an
`async () => new Response("")` satisfies the fetch signature directly, so it is
now annotated rather than asserted.
- The mock's comment still told readers to override `h.proxyFetchResult`, the
name from an earlier revision; the hook is `h.proxyFetchState.current`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses review round 5 on #2099 (three suppressed comments, no posted ones).
The `fetch: createProxyFetch()` line in each Node client is the whole of that
client's proxy support — it is what puts the proxy at the BOTTOM of the fetch
stack, where InspectorClient's wrappers compose over it rather than discarding
it. `InspectorClient` sets `fetchFn` unconditionally, so the transport-level
fallback never fires for these clients. Both lines could have been deleted with
every existing test still green.
- CLI: a test that runs a real `tools/list` against a real HTTP test server with
HTTP_PROXY pointed at a real forwarding proxy, asserting the request traversed
it. Behavioral rather than structural on purpose — the defect this replaces was
a runtime dispatch failure no structural assertion would have caught. It clears
all six proxy vars first so an ambient value cannot route it elsewhere.
- TUI: the mock added earlier always returned undefined, so the proxy branch was
never exercised. `createProxyFetch`'s return is now controllable, and two tests
assert the sentinel reaches `environment.fetch` when a proxy is configured and
that the slot stays unset when one is not.
Both were confirmed to fail with their client's wiring line removed.
Also updates the `InspectorClientEnvironment.fetch` doc in core/mcp/types.ts,
which still told callers that Node leaves this undefined and uses the global
fetch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Corporate-proxy users could not connect to any remote MCP server from v2.
Two independent defects, both reported in #2067.
1. The web and TUI bundles inlined undici. It was declared in the root and
`clients/cli` manifests but named in no client's tsup `external` list, and
tsup auto-externalizes only what the *nearest* manifest declares — so
esbuild inlined 1.05MB of it into `clients/{web,tui}/build/`. undici is
CommonJS, so the emitted chunk's `require("assert")` hit esbuild's ESM
`__require` shim and threw `Dynamic require of "assert" is not supported`.
The rejected `import()` surfaced as "the `undici` package is not available",
telling users to install a package that was already present — and could not
have helped, because the specifier had been rewritten to a relative chunk at
build time.
2. `withProxyDispatcher` injected a *userland* undici `EnvHttpProxyAgent` into
Node's *built-in* fetch. That couples two different copies of undici at the
dispatcher handler interface, which is not stable across majors: Node 22
embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8
agent handed a built-in undici 7 handler is rejected with
`fetch failed: invalid onRequestStart method`, and the request never leaves
the process. So fixing the bundling alone would only have converted the
web/TUI failure into the CLI's.
Both halves of the pair now come from userland undici — its `fetch` and its
`EnvHttpProxyAgent` — which keeps the handler inside one copy and works
unchanged from the Node 22.19 engine floor through Node 26. Deleting the
wrapper in favor of Node's native `NODE_USE_ENV_PROXY` was not an option:
measured, it is unsupported on 22.19 (the floor) and only lands mid-22.x.
Because undici's `Response` is a different class from `globalThis.Response`,
the proxied fetch re-wraps each response as a genuine global `Response`,
streaming preserved. Without that, `res instanceof Response` is false for
callers that test it — including the SDK's OAuth error formatter, which would
degrade every message to `Raw body: [object Response]`.
The proxy now sits at the bottom of the fetch stack (`environment.fetch` for
the Node clients, and the `createTransportNode` default for the web backend,
its one caller that supplies no `fetchFn`) rather than wrapping the transport's
fetch. Proxying means *substituting* the fetch, not decorating one, so wrapping
a supplied `fetchFn` would discard it; installing it underneath lets
InspectorClient's own wrappers compose over it. A side effect worth having:
OAuth discovery and token requests go through `environment.fetch` too, and were
never proxied before.
Guarding it:
- `undici` is added to all three tsup `external` lists and removed from
`clients/cli/package.json`, so it is root-declared only, per the placement
rule in AGENTS.md. The two installs had already drifted (root 8.5.0 vs cli
8.9.0).
- New `npm run verify:bundle-externals` fails if any package a client declares
external was inlined into that client's bundle anyway. It reads the *built
output*, not the config — those two disagreed for four releases — and derives
what to check from each client's own `external` array, so a newly
externalized package is covered without editing the guard. Verified against
the real pre-fix artifact, which it flags.
- The proxy tests now drive a real forwarding proxy rather than a mock. That
matters: defect 2 was a runtime interface mismatch, which no mock can catch
because no mock ever dispatches.
AGENTS.md's must-not-bundle rule (added by #2082) is extended with why undici
slipped past it, and notes the `node -e` trap — probing the bundled chunk with
`node -e` falsely succeeds, because it exposes a global `require` that
satisfies esbuild's guard.
Closes#2067
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
- Widen the three `_meta` annotations the bulk pass missed, all of them
return types rather than identifier declarations: `mergeMeta`,
`aggregateListParams`, and the `tools/call` params object. The second was
unsound in a way callers could see — it advertised
`{ _meta: Record<string, string> }` while the runtime carried nested JSON.
- `normalizeStoredMetadata`: keep an explicit `null` from a legacy pair
instead of folding it into `""` (only a missing `value` defaults now), and
store through `Object.defineProperty` so a legacy `__proto__` key becomes an
own property rather than hitting the prototype setter.
- Drop the unjustified `as unknown as` from the migration test; the legacy
fixture is untyped and cast once where it crosses into the typed parser.
- The TUI settings test asserted only that the app mounted, so it could not
have caught a pair array being forwarded as numeric `_meta` keys. The fake
client now captures its constructor options and the test asserts the nested
object arrives intact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#1910
`_meta` was typed `Record<string, string>` end to end in the Inspector —
on disk as a `{ key, value }[]` pair array, in the Server Settings form as
key/value rows, and in every request verb's per-call metadata parameter.
Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK
models the field as a `z.looseObject`, so the restriction was ours alone.
Widen the whole path to `RequestMetadata` (`Record<string, JsonValue>`):
- Per-server default metadata is a JSON **object** on disk and in memory.
The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an
existing `mcp.json` keeps working; it is never written back, so a save
migrates the entry.
- The Server Settings → Request Metadata section is a JSON editor
(`JsonObjectInput`) instead of key/value rows — rows cannot express an
object value. It keeps the typed text as the display source and emits
only parsed JSON, the same draft split `SchemaJsonField` uses, so the box
stays typeable; invalid text is flagged inline and not applied.
- The CLI stops flattening `--metadata` / `--tool-metadata` through
`metaValueToString`. Those values were already JSON-parsed, so
`trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}`
and now reaches it as an object.
- `servers/show` redaction replaces a sensitive key's whole value rather
than a string, so a structured value cannot leak through its members.
- `progressTokenOf` narrows the reserved `progressToken` key back to
`string | number` before it reaches the SDK; any other JSON still travels
inside `_meta` but wires up no progress callback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 3 (PR #2039): clearing the banner when `selectedServer`
changes is not enough. Start disconnecting A, switch A → B → A, and by
the time the original request rejects both guards match again — the
attempt token is unchanged and the selected server is A once more — so
the stale failure lands on the freshly re-selected A. Bump the token on
every switch, so a round trip invalidates prior disconnects the same way
a one-way switch does.
New test drives exactly that interleaving; it fails without the bump
(the one-way test does not, which is why this needed its own).
Also attaches the before/after TUI screenshots the review asked for —
the review was right that "no UI change" stopped being true once the
banner was added — and corrects the PR description accordingly.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 2 (PR #2039) surfaced no new visible comments but six
suppressed ones. Four were actionable.
`clients/tui/src/App.tsx` — a stale-completion race in the new banner:
start disconnecting server A, switch to B, then let A's disconnect
reject, and A's failure appears under B. Rapid same-server retries had
the same shape, and the stale attempt is the one most likely to reject.
Claim a monotonic attempt token before awaiting and publish only if it
is still current and the server has not changed; `handleConnect` bumps
the same counter so a connect retires an in-flight disconnect. The
rejection is still owned when superseded — just not published. The
regression test drives a deferred rejection across a server switch, and
fails without the guard.
`tsconfig.lint.json` — the root type-aware block matches
`test-servers/src/**/*.tsx` but the parser project omitted that
extension, so such a file would have failed with "not found in any of
the provided project(s)" instead of being linted. Added, and AGENTS.md
now states the invariant: the include must stay a superset of the lint
globs.
`clients/web/server/{server,vite-hono-plugin}.ts` — the `open()`
rejection branch was untested, and duplicated. Both were fixed by
removing the duplication: `openBrowser` now lives beside
`printServerBanner` in `web-server-config.ts`, with one implementation
to test. Two copies of a one-line `.catch` is two chances for one to
drift back to a bare `open(url)`. The dev plugin is not booted in tests
— it returns early under VITEST by design — but it calls the same
tested helper.
`core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close
path, the entire reason the helper exists, had no test. Exported it (as
this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`)
and covered both legs, asserting no unhandledRejection fires.
Also corrects AGENTS.md, which said each client lists "both" tsconfig
projects — true for cli/tui/launcher, but web lists four leaf projects.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review catch (Copilot, PR #2039): folding the rejected disconnect into
`connectError` meant nobody ever saw it. InfoTab renders that value only
when `serverState.status === "error"`, and a failed disconnect leaves the
status "connected" — so the message existed in state and never on screen.
Worse, it fed `connectError ?? inspectorLastError`, where a stale value
would go on masking a later, real connection error.
Give it its own `disconnectError` state and a header banner that renders
regardless of status, cleared on the next connect or disconnect attempt
and on a server switch, so it can neither linger nor be attributed to
the wrong server. The tests now assert the rendered banner rather than
noting that nothing is visible.
Signed-off-by: cliffhall <cliff@futurescale.com>
A floated promise is invisible at review time — the call reads like an
awaited one minus four characters — and the unhandled rejection it
produces surfaces in a different test, in a different file, as a stack
pointing at SDK internals. In #1947 two un-held `callTool` calls made
`npm run ci` unpassable and took a full investigation to attribute; the
fix was two lines. `@typescript-eslint/no-floating-promises` catches the
class outright, so enable it at `error` everywhere.
The rule is type-aware and `tseslint.configs.recommended` is not, so each
scope's config gains a parser project. The four clients name their own
tsconfig projects (both of them — `src` and `__tests__`/`server` live in
different ones). The root scope has no tsconfig at all, so add
`tsconfig.lint.json`: a lint-only project covering `core/**`,
`test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and
gating nothing. It sets `moduleResolution: bundler` deliberately — under
NodeNext `core/`'s extensionless imports fail to resolve, every import
degrades to `any`, and the rule silently stops seeing promises at all.
Measured surface, and the fixes:
- core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono
invokes subscribers with a bare `subscriber()`, so a promise returned
from the listener is floated one layer up; `closeAbortedStream` owns
the rejection where it can actually be owned.
- tui (5) — `handleDisconnect` had no catch of its own and its only
caller is a key handler that cannot await, so a failing disconnect was
a live unhandled rejection; it now surfaces the message the way
`handleConnect` does. The rest are `void` at callers that genuinely
cannot await, over callees that already own their failures.
- web (92) — 83 are Storybook `expect(...)`, which is instrumented and
returns a promise, now awaited (`scrollAreaStoryAssertions` is async
for the same reason). Two tests were asserting against a provider call
they never awaited. `open(url)` in both web servers now warns instead
of escaping a synchronous callback.
- cli, launcher — already clean.
Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s
to ~19s, inside `validate`. Narrowing the loaded projects is the lever if
that needs reducing later; dropping the rule is not.
Closes#1959
Signed-off-by: cliffhall <cliff@futurescale.com>
Extends the previous commit's fix rather than changing it. `waitUntil` now
settles, but the same window is open wherever a key is written without having
just polled: `press()` reaches its `stdin.write` on a turn that may still have
React's passive-effect flush queued (straight after a plain `tick`, or after an
earlier key committed a render), and `mount()`'s absorbing keypress lands one
tick after the mount commit. Both now `await settleInputHandlers()` before
writing, so "frame visible" implies "input handlers armed" at every dispatch
point, not only after a frame wait.
Adds a `test helpers` case that pins the contract: a `setImmediate` sentinel
stands in for the pending effect flush and must have run by the time
`waitUntil` resolves. Verified it fails when the yield is removed, so a future
regression surfaces here instead of as a differently-named flaky OAuth test
under coverage instrumentation.
Signed-off-by: cliffhall <cliff@futurescale.com>
* 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>
* 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>
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
Addresses the @claude review of #1611: the event registry is now keyed by
(client, fn) rather than event name alone, and a new `fireClientEventFor(client,
…)` helper fires a single client's listeners. The "ignores auth lifecycle events
from a non-selected server" test now fires ambient/recovered/oauthError for ONLY
the non-selected client and asserts no message appears — so removing the
`selectedServerRef.current !== serverName` guard would fail the test — then fires
for the selected client to confirm it does act. Coverage of App.tsx is unchanged
(92.23 / 90.20 / 92.85 / 92.68); existing fire-all `fireClientEvent` behavior is
preserved.
Part of #1610.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
Bring clients/tui/src/App.tsx from 79% to ≥90 on all four dimensions
(92.23 / 90.20 / 92.85 / 92.68) by exercising the mid-session / step-up auth
machinery that the existing suite didn't reach:
- runOAuthAuthentication result branches (success / already_authorized /
insufficient_scope / unsupported), driven deterministically via a new opt-in
runRunnerInteractiveOAuth override (defaults to the real runner so the
existing callback-driven OAuth tests are unchanged).
- The per-client auth-lifecycle listeners (authChallengeAmbient / Recovered /
Interactive / oauthError), fired through a new client-event registry on the
FakeClient, including the non-selected-server early-return guards.
- handleAuthRecoveryRequired step-up-confirm and reauth paths, handleConnect's
OAuth-on-401 branches (AuthRecoveryRequired + EmaClientNotConfigured thrown
mid-flow), handleClearOAuth's connected/disconnect path, and the AuthTab
onAuthorizeStepUp handler (EMA satisfied/interactive/failed/throw + standard
non-EMA authorize, cancel).
Part of #1610.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
* refactor(web): update OAuth E2E tests to use new authentication method
- Replaced `runGuidedAuth` with `authenticate` in the InspectorClient OAuth end-to-end tests to support quick authentication with CIMD pre-registration.
- Updated comments to reflect the change in authentication method and its support for HTTP test metadata URLs.
- Ensured that the tests continue to validate the expected authorization URL after the change.
Additionally, imported `ensureCimdClientRegistration` in the OAuthManager to streamline client registration during the authentication process.
* Guided-auth removal, OAuth smoke testing, and TUI/CLI support
* Added CLI OAuth runner flags tests
* Added clarifying comment on port 0 binding
* README notes on TUI oauth callback port, prettier fixes for CLI tests
* CIMD validation uses inline like EMA IdP
* Fixed TUI test failures from merge
* test: bring 5 OAuth files over the 90 per-file coverage gate
The guided-auth-removal work added/touched several files that fell below
the repo's >=90 per-file coverage gate (npm run coverage). CI doesn't run
the gate (it's local-only), so validate + test:integration passed while
these gaps slipped through. Bring them all back over the line:
- core/auth/node/runner-oauth-callback.ts: test bad-URL/non-http-scheme
throws, no-port->80 default, and IPv6 hostname bracketing. Annotate the
provably-dead guards (empty host, out-of-range port, String(err)
fallback, pathname "/" fallback) -- all rejected by new URL() upstream --
with justified v8 ignore comments instead of lowering the gate.
- core/auth/cimd.ts: test the AS-doesn't-support-CIMD early return.
- core/react/useClientSettingsDraft.ts: test the functional-updater path,
the stale-onChange-after-close prev===null guard, and the
timer-fires-after-close value===null guard.
- ClientSettingsForm.tsx: test the CIMD checkbox toggle and CIMD URL clear.
- clients/tui/src/utils/oauthDisplay.ts: test static/dcr registration
kinds, the expired IdP session, and the configured-scope/empty-scope
branches of formatScopes.
npm run coverage now passes (exit 0) across web, cli, tui, launcher.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): cover useSelectableList hook + leaf Ink components (#1501)
Add ink-testing-library and renderer-based tests for useSelectableList,
SelectableItem, and Tabs. Widen the vitest include glob to .test.tsx and
lift these files from the interim React-surface coverage exclusion. The
blanket src/components/**/*.tsx exclude is expanded into an explicit
per-file list so remaining components can be lifted incrementally.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): add ink-scroll-view passthrough mock for renderer tests (#1501)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): add ink-form passthrough mock for modal renderer tests (#1501)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): cover all Ink tab + modal components via renderer tests (#1501)
Add ink-testing-library tests for the 12 Ink components (InfoTab, AuthTab,
ResourcesTab, PromptsTab, ToolsTab, NotificationsTab, HistoryTab,
RequestsTab, and the Tool/Resource/Prompt test modals + DetailsModal),
mounting them through the ink-scroll-view / ink-form passthrough doubles
and driving keypresses via stdin. Modal components render position:absolute
(empty frame under the test renderer) so their tests assert on behavior —
injected InspectorClient fakes and onClose — while the passthrough
ScrollView still mounts the inner JSX for coverage.
Lift these 12 components from the interim React-surface exclusion in
vitest.config.ts; only App.tsx remains excluded. All files clear the
uniform 90/90/90/90 per-file gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): add App.tsx renderer-test harness + foundation tests (#1501)
Mock the full @inspector/core surface (InspectorClient, the 7 managed-state
classes, the 8 react hooks, transport + auth) behind a controllable harness,
plus the ink-scroll-view/ink-form passthroughs, so App renders under
ink-testing-library and its keyboard state machine can be driven via stdin.
Concurrent ink mounts share raw-mode stdin, so each render is unmounted in
afterEach. Foundation covers server-list nav, tab accelerators, connect/
disconnect, OAuth-tab gating, and connected-status rendering (~50% baseline).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): cover App.tsx via mocked-core renderer tests; lift final exclusion (#1501)
Extend App.test.tsx to 67 tests driving the keyboard state machine, tab
content, modals, OAuth flows, and connect/disconnect across the controllable
@inspector/core mock. App.tsx now clears the 90/90/90/90 per-file gate
(94.6/90.6/94.5/96.6).
Remove the last React-surface exclusion (src/App.tsx) from
clients/tui/vitest.config.ts. The whole clients/tui/src tree is now under
the uniform per-file coverage gate with no React-surface exclusions,
completing the interim-scope lift from #1484.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): raise stdout max-listeners in App test to silence warning (#1501)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): raise App.tsx branch coverage >90 and de-flake coverage runs (#1501)
Add targeted App.tsx tests for the OAuth handlers' non-Error throw arms
(quick onCallback, guided start/advance, run-to-completion) and a re-entrant
quick-auth guard, lifting App.tsx branch coverage 90.6% -> 91.48% for a
safer margin above the 90 gate.
Harden the App renderer tests against the heavier v8 coverage instrumentation,
where React+ink's multi-macrotask render scheduling races fixed ticks:
- press() now settles two ticks per key so focus/tab changes flush before the
next key is routed (ink re-subscribes useInput on re-render);
- expectFrame()/waitUntil() poll for frame content and async flow state
(callbackServer opts) instead of asserting after a single fixed tick;
- ResourcesTab's content-clear test polls for fetched content likewise.
Plain 'npm test' (the CI path) was already stable; this makes the local
coverage gate stable too (13/13 clean runs vs ~2/8 flaky before).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): de-flake component renderer tests under slow CI (#1501)
CI flaked on AuthTab's 'renders quick auth results' under the plain test run:
its oauthFlowState is populated by a post-mount useEffect reading the
inspectorClient, and a single-macrotask tick can race that effect -> setState
-> re-render chain on a slow/loaded CI runner.
Make each component test file's tick() flush several macrotask cycles so such
chains settle deterministically before assertions (no test relies on precise
tick timing — no fake timers / debounce). App.test.tsx already polls via
expectFrame/waitUntil and passed CI, so it's unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(tui): address review — shared setupFile for max-listeners, consume-once ink-form global (#1501)
Review follow-ups on #1538 (both non-blocking cleanups):
- Hoist process.std{out,in}.setMaxListeners into a shared vitest setupFile
(__tests__/setup.ts) wired via setupFiles, so every renderer test file
inherits it and the MaxListenersExceededWarning is silenced across the
whole suite — not just App.test.tsx. Remove the per-file call.
- Make the ink-form double consume (clear) __INK_FORM_SUBMIT_VALUE__ on
submit, eliminating the latent cross-test coupling on the shared global
regardless of how the modal test files are organized.
No production code changed; suite + coverage gate stay green (App.tsx
94.8/91.5/94.5/96.6), and the warning no longer prints.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cliffhall
olaservo
Bob Dickinson