| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
775107776d |
fix: address Copilot review round 1 on #2248
Five findings, all real. **Critical — `verifySkills` selected `contents[0]`.** These bytes are hashed against the file's advertised digest, so accepting a block the server labelled something else verifies one file's content against another file's digest, and can report that as `verified`. A false pass is worse than a missing check. `contentsFor(result, uri)` now selects by NORMALIZED identity, which keeps the canonicalized-echo case that motivated the original code while rejecting an unrelated block, and reports a read error when nothing answers for the URI. This is what `onReadSkillFile` already did; the two paths hash bytes against digests and must not disagree about which bytes. **`skills/get` was dispatched without the extension gate** that `skills/list` had. Hoisted into `assertSkillsSupported` so the two cannot drift — declaring the extension commits a server to both. Without it, an undeclared server's -32601 is indistinguishable to a script from the -32602 a declared server returns for a URI it does not serve. **The TUI could strand a user on a hidden Skills tab.** The tab left the bar when the gate went false but `activeTab` did not, so the pane kept rendering for a server that never declared the extension. Reset to `info`, following the Auth precedent — additionally gated on `connected`, because this gate reads a server declaration and would otherwise fire during a reconnect. **The TUI keyed a verdict by URI**, so a refresh replacing the manifest under the same URI left hashes computed for the previous snapshot describing the new one. Keyed on the entry now, as the web screen already was. **A failed "Load more" discarded the pages on screen and the retry cursor.** Both are preserved now, and the success and failure paths share one staleness-guarded `commit` helper so they cannot drift on which results they may write — which is how they came to disagree. Two nits: the six-skill counts. The docs sentence was more wrong than reported (five awkward skills of six, three outright violations) and is rewritten rather than renumbered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
9a8488681e |
feat: Skills extension phase 3 — CLI, TUI, directory reads, frontmatter check
Closes #2248. Completes SEP-2640 support across all three clients, and closes the one obligation #2234 deliberately left open. - `checkSkillFrontmatterMatch` compares a served SKILL.md's own YAML frontmatter against the entry the listing advertised, field by field. No digest can cover this: a digest is taken over the bytes the server served, so it proves the file was not altered in transit and says nothing about whether the listing described it honestly. Needs a real YAML parser; `yaml` was already a root dependency, so this adds no package — but it does newly put it on core/'s import graph, hence the three bundler `external` lists. - `resources/directory/read`: result schemas defined against the normative text, `InspectorClient.readResourceDirectory`, a Directory section on the Skills screen, and `--method resources/directory/read` in the CLI. The client refuses the call locally when the server did not declare `directoryRead`, which is the SEP's MUST NOT. A child the directory lists but the entry does not is marked `not listed` rather than merged into the manifest — the SEP calls a directory result a live observation and forbids treating it as extending the manifest. - CLI: `skills/list`, `skills/get` and `resources/directory/read`, plus `--verify` — one NDJSON report per skill, a summary on stderr, exit 7 on a violation. Reads are sequential; a conforming manifest may declare 512 entries. - TUI: a Skills pane, shown only when the server declares the extension. Each row carries its conformance verdict as a glyph as well as a colour, since the pane is read over ssh and through script(1). Enter verifies. - `verifySkills` re-throws `AuthRecoveryRequiredError` rather than recording it per file: it says the session's authorization expired, so absorbing it would report N identical read failures and swallow the error the TUI and the web commands key off to reauthorize. Two open questions settled, both in code comments where they will be found: `skills/get` carries no caching attributes because SEP-2640 leaves the question open in as many words, so requiring them would fail a conforming server; and there is no `PagedSkillsState` because every consumer of this list is a whole-catalog verdict — computed over page one of three, "this server's skills conform" is not merely partial but wrong. The fixture grows two skills, both for checks that were otherwise undemonstrable: `lying-listing` (listing and file disagree, digest still verifies) and `stale-manifest` (serves a file its manifest does not declare). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
a3bb679f1e |
fix: strip ANSI from TUI test frames so FORCE_COLOR cannot break assertions (#2207)
Ink writes styling as escape sequences inside the styled run, so an
accelerator underline splits the word it decorates: `<Text underline>I</Text>nfo`
reaches the frame buffer with escapes between `I` and `nfo`, and a plain
`toContain("Info")` fails against a component that is rendering correctly.
Chalk only emits color when it detects a TTY, so CI stays green and the six
failures land only on a developer whose shell exports FORCE_COLOR.
Add `__tests__/helpers/renderTui.tsx` — ink-testing-library's `render` with
`lastFrame()` and `frames` ANSI-stripped — and point all 16 renderer test
files at it. `frames` is a getter reading through to the live array, since
Ink appends to it as the component re-renders; the raw bytes stay reachable
as `stdout.lastFrame()`.
Pinning FORCE_COLOR=0 would turn the six green while hiding the class: any
future assertion on styled text has the same problem, and the suite would
stop covering the styled path at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MFp2LVH7UJST5i8bDBJghw
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
463227c947 |
fix(auth): make snapshot-and-clear one atomic storage step (#2144)
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> |
||
|
cliffhall
|
c0bcdc7e4a |
fix(tui): forward the clear promise from App, so AuthTab can actually await it (#2144)
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>
|
||
|
cliffhall
|
781aa69d19 |
fix(auth): revoke every issuer-bound grant, not just the active one (#2144)
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> |
||
|
cliffhall
|
7f27d7e9c7 |
fix(auth): address Copilot review round 1 on the RFC 7009 revocation (#2144)
- 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> |
||
|
cliffhall
|
65fd2b9e35 |
feat(auth): revoke OAuth tokens at the authorization server on clear (#2144)
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> |
||
|
cliffhall
|
531cf84e60 |
Merge branch 'v2/main' into v2/fix/2067-proxy-undici
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> |
||
|
cliffhall
|
21cf1bd6c3 |
test(tui): drop an unjustified double cast; fix a stale mock comment
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> |
||
|
cliffhall
|
91e08067dc |
test(proxy): cover the CLI and TUI bottom-of-stack wiring
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> |
||
|
cliffhall
|
d90a955164 |
fix(proxy): route through undici's own fetch and keep undici external
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> |
||
|
cliffhall
|
0f628a8174 |
fix: address Copilot review on the _meta widening
- 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>
|
||
|
cliffhall
|
b33532c024 |
feat: allow complex JSON values in request _meta
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> |
||
|
Cliff Hall
|
405d0f4d0d |
Merge pull request #2039 from modelcontextprotocol/v2/chore/1959-no-floating-promises
chore(lint): enable @typescript-eslint/no-floating-promises in all five ESLint scopes |
||
|
cliffhall
|
85e3215534 |
fix(tui): retire the disconnect token on a server switch, not just clear
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> |
||
|
cliffhall
|
d830213796 |
fix: address round-2 review — stale disconnect, lint-project gap, coverage
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> |
||
|
cliffhall
|
5d8094357d |
fix(tui): surface a disconnect failure independently of connection status
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> |
||
|
cliffhall
|
519ecf49ee |
chore(lint): enable no-floating-promises in all five ESLint scopes
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> |
||
|
cliffhall
|
ae0e9d53a3 |
test(tui): settle input handlers at the keypress sites too, and pin the contract
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> |
||
olaservo
|
9c6e4aeb4e |
fix: settle ink input-handler re-subscription after frame waits in TUI App tests
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Cliff Hall
|
d9e4f444a2 |
test(cli,tui): typecheck the __tests__ dirs (#1791) (#1799)
* test(cli,tui): typecheck the __tests__ dirs (#1791) The cli/tui `typecheck` scripts only checked `src` (the src-only tsconfig excludes `**/*.test.*`), leaving the `__tests__` dirs — which do the most `as` casting — with no tsc pass. Add a `tsconfig.test.json` per client that includes `__tests__` with the test-exclusions dropped and the test-only path aliases (`@modelcontextprotocol/inspector-test-server`, the `@inspector/core/*` deep paths, express/vitest) resolving what vitest resolves, then fold `tsc -p tsconfig.test.json` into each `typecheck`. Fixes the 76 errors it surfaced: - cli: a wrong `@modelcontextprotocol/inspector-core/...` module specifier (should be `@inspector/core/...`; only a type import so vitest never caught it), `r.headers` optional-access guards, and partial OAuth mocks replaced with typed `makeFakeCliOAuthClient()` / `makeFakeServerSettings()` factories instead of `as` casts. Adds `@types/express` (devDep) so the transitively aliased test-server source typechecks, mirroring clients/web. - tui: rest-param signatures on the App OAuth spies (fixes the spread-arg and narrow-return errors), a widened callback param (`iss?`), a `makeFakeServer- Settings()` factory for the tuiOAuth tests, required OAuth-metadata/client fields in AuthTab, and a loosened `entry()` message param for the deliberately-malformed wire messages HistoryTab's defensive branches render. Gate coverage only; no runtime behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(cli,tui): address review — real spy signatures, config typecheck, nits Responds to the @claude review of #1799: - App.test.tsx: type the OAuth spies against the real InspectorClient method signatures (`vi.fn<InspectorClient["…"]>()`) and forward `Parameters<…>` tuples from the FakeClient wrappers, so `toHaveBeenCalledWith(...)` on the six OAuth-lifecycle spies stays argument-checked under the new gate rather than accepting anything via `unknown[]`. `getOAuthState` regains its wide `OAuthConnectionState | undefined` return. - Typecheck the client config files too: add `vitest.config.ts` / `tsup.config.ts` (and tui `dev.ts`) to each src `tsconfig.json` include — they were the one first-party surface still getting no tsc pass. - Trim the redundant `src/**/*` from each `tsconfig.test.json` include (tsc pulls in the src the tests import; the src-only config already checks it). - Rename the tui settings helper to `oauth-test-fakes.ts` to match cli's, and soften both factories' docstrings to stop naming a loader they don't mirror field-for-field. - Fix a stale `@modelcontextprotocol/inspector-core/...` specifier in a createRemoteTransport JSDoc example (the same wrong package name the gate caught in the cli tests, surviving in a comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-2 review — correct claims, tighten types, typecheck launcher Responds to the round-2 @claude review of #1799: - Correct the toHaveBeenCalledWith claim: vitest doesn't type-check `toHaveBeenCalledWith(...)` arguments against the mock's signature, so the App.test.tsx comment no longer claims it does — the typed spies buy checked implementations/return payloads (the stale-literal fix), not arg-checking. - authenticate spy now uses the real `InspectorClient["authenticate"]` signature (returns a `URL`), so it no longer diverges from production; the one mockResolvedValue is updated to a URL. - makeFakeCliOAuthClient members are typed against the real CliOAuthClient signatures (not bare vi.fn()), so a wrong-signature override is rejected. - HistoryTab: revert the `entry()` `message: unknown` widening (it un-checked the ~15 well-formed fixtures) and instead scope a named `MALFORMED` cast to the two intentionally-malformed messages. - Typecheck the launcher `__tests__` too (the same gate hole in the one client the PR hadn't touched): new clients/launcher/tsconfig.test.json + a typecheck script folded into validate. Completes the "every client's __tests__ is typechecked" invariant. - Doc the new test-typecheck + config-file typecheck in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-3 nits — launcher config typecheck, callTool typed, docs Responds to the round-3 @claude review of #1799 (all non-blocking): - launcher: add `vitest.config.ts` to tsconfig.test.json's include (it was in neither project — the build config's rootDir: ./src rejects it). Now typechecked. - App.test.tsx: type `callTool` against `InspectorClient["callTool"]` (+ the Parameters<…> wrapper) so its `mockResolvedValue` payloads are checked, and give `clearOAuthTokens` an `async () => {}` impl so the mock actually returns the `Promise<void>` its type promises. - AGENTS.md: correct the per-client alias-set description (cli's is widest, tui's is core+react only, launcher's has no paths) and note launcher's config file lives in its test project, not the src project. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-4 — durable typecheck-coverage guard + launcher/doc nits Responds to the round-4 @claude review of #1799 (all non-blocking): - Add `verify:typecheck-coverage` (scripts/verify-typecheck-coverage.mjs), the typecheck-coverage analog of verify:format-coverage and the durable guard for the invariant this PR establishes: for each Node client it runs the tsconfig projects its `typecheck` names with `tsc --listFiles`, unions them, and fails on any tracked .ts/.tsx that lands in no project. This closes the class the review kept surfacing by hand (launcher/__tests__ in round 3, launcher/vitest.config.ts in round 4) — a new top-level file can no longer go untypechecked silently. Wired into `npm run ci` and the GitHub workflow. - launcher: reorder `validate` to run `typecheck` before `build` (matches tui and the documented order), and make `typecheck` self-contained — it now runs both `tsconfig.json` and `tsconfig.test.json`, so running it standalone means the same thing it does in cli/tui. - makeFakeCliOAuthClient: note on the comment that the typing covers the defaults, not `overrides` (a bare vi.fn() override is Mock<(...args)=>any>). - Docs: README table + AGENTS.md updated for the new guard and the corrected launcher typecheck/validate ordering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: address round-5 — wire-check the guard, --listFilesOnly, into validate Responds to the round-5 @claude review of #1799: - Finding 1 (the pre-merge one): the guard now ports verify:format-coverage's reachability checks — it asserts each client's `typecheck` is reachable from its own `validate` and that the root chain runs each client's `validate`, so it can't stay green while measuring a typecheck nothing invokes. Verified: stripping `npm run typecheck` from launcher's validate now fails the guard. - Nit 3: switch `tsc --listFiles` → `tsc --listFilesOnly` (identical file list, ~4× faster: whole guard 23s → ~4s). Now cheap enough for the inner loop, so it moves into `validate` (right after verify:format-coverage) instead of a standalone ci step — matching its sibling. Removed the separate ci-chain entry and GitHub workflow step (validate covers it). - Nit 4: `npx --no-install tsc` so a missing local tsc fails fast instead of reaching for the registry (matches verify-build-gate.mjs). - Nit 5: corrected projectFiles' JSDoc to describe the filter that's actually there (drops repo-external + node_modules paths; core/ + test-servers/ stay but are harmless). - Finding 2: fixed the 2-space misindent on the new root package.json line (prettier --write). Docs (README/AGENTS) updated for the validate placement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): address round-6 guard nits — dedupe wiring, robust diagnostics Responds to the round-6 @claude review of #1799 (all guard-ergonomics nits): - Nit 2: extract the shared reachability logic (`reachableScripts`, `rootRunsClientValidate`) into scripts/lib/npm-scripts.mjs and consume it from BOTH verify-format-coverage.mjs and verify-typecheck-coverage.mjs, so the "is this gate actually run?" wiring can't drift between them (same rationale as scripts/lib/prod-web-server.mjs). - Nit 1: typecheckProjects now harvests `-p`/`--project` from every script reachable from `typecheck` (not the single string), so a delegating `typecheck` (`npm run typecheck:src && …`) is handled instead of failing with a false "names no -p project" diagnosis. Also accepts `--project`. - Nit 3: on a tsc config error, echo the `error TS…` diagnostic (scanning both streams — tsc prints config errors to stdout) so a broken tsconfig is self-explaining rather than surfacing only as a misleading "file in no project" report. - Nit 4: refreshed the CI workflow's stale "Validate" step name/comment (it now runs the coverage guards + validate:core + per-client typecheck). - Nit 5: reworded the `.d.ts` carve-out comment to read as pre-emptive (there are none under cli/tui/launcher today). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-7 — guard .mts/.cts too; drop dead export Responds to the round-7 @claude review of #1799: - Finding 1: verify-typecheck-coverage now requires a tsc pass for tracked `.mts`/`.cts` (not just `.ts`/`.tsx`), matching verify-format-coverage's extension set — `.mts` is already the shared-config idiom here, so a client `vitest.config.ts` → `.mts` rename can no longer drop out of both the tsconfig include and the guard's required set. No-op on today's tree (no client `.mts`/`.cts`); confirmed a tracked `.mts` with a type error is now caught. Ambient `.d.{ts,mts,cts}` stay excluded. - Nit 2: drop the unused `export` on `rootReachedCommands` (internal to the lib; nothing imports it, and scripts/ isn't eslint-gated to catch dead exports). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-8 — single casts in HistoryTab, uniform wrappers, doc scope Responds to the round-8 @claude review of #1799: - Finding 1: reduce the `MALFORMED` helper and `entry()`'s return cast from `as unknown as` to a single `as` (both compile clean — the malformed shapes are comparable to the JSONRPCMessage union), and delete the false "only option" sentence. Fixes the AGENTS.md avoid-double-casts violation. - Nit 3: write all seven FakeClient OAuth wrappers as `(...a: Parameters<InspectorClient["…"]>)` (the three zero-arg ones were hand-written `()` forwarders), so the block comment's "forward the same Parameters<…> tuple" is literally true and a future added parameter can't be silently dropped. - Nit 2: update the five places that still described the guard as `.ts`/`.tsx` only to `.ts`/`.tsx`/`.mts`/`.cts` (script header + trackedSourceFiles JSDoc, AGENTS.md ×2, README.md). Nits 4 (js config files out of the typecheck guard by construction) and the footnoted callTool/ancestor-tsc items left as agreed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-9 — drop entry()'s harmful cast; guards vouch for each other Responds to the round-9 @claude review of #1799: - Finding 1: remove `entry()`'s `as MessageEntry` return cast in HistoryTab.test.tsx. It was unnecessary (the spread of Partial<MessageEntry> over complete defaults is directly assignable) and harmful — it silenced type errors in the helper's OWN default fields (a `direction`/`method` typo now surfaces TS2820/TS2353). MALFORMED's single cast stays (load-bearing). This leaves MALFORMED as the file's only cast, matching what the comments describe. - Nit 2: add `rootReachesScript` to scripts/lib/npm-scripts.mjs and have each coverage guard assert its SIBLING is still wired into the root `validate`. A guard can't detect being unrun itself, but the two now vouch for each other, so dropping either from `validate` is caught by the other (only deleting both slips through). Verified: removing verify:typecheck-coverage from validate now fails verify:format-coverage, and vice-versa. Nits 3 (git-index visibility) and 4 (pre-existing as-never/as-unknown casts in tests — out of scope, none added by this PR) left as agreed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-10 — reject non-checking typecheck passes; pre/post hooks; docs Responds to the round-10 @claude review of #1799: - Finding 1: verify-typecheck-coverage now rejects a `-p <project>` whose own command carries `--noCheck` or `--listFilesOnly` — such a pass lists files without type-checking them, which previously satisfied both the wiring and file-coverage halves while checking nothing. (Splits each script on `&&`/`||`/`;` so a flag on one command can't taint another.) Verified a `typecheck` using `tsc … --noCheck` now fails with a clear message; `--noCheck` is a live idiom here (cli's test-servers:build), so this is a real copy-paste hazard. - Nit 2: reachableScripts (shared lib) now also follows npm's implicit `pre<name>`/`post<name>` lifecycle hooks, so moving a gate into e.g. `prevalidate` is recognized instead of producing a false "not reachable" failure (cli already uses a `pretest` hook). Verified. - Nit 3: AGENTS.md "Test placement" now states the Node clients (cli/tui/ launcher) keep all tests in `__tests__/`, enforced by verify:typecheck-coverage (a co-located src test lands in no tsconfig project). - Nit 4: noted on the factory comments that `Partial<…>` overrides admit an explicit `undefined` for a required field (exactOptionalPropertyTypes off). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * fix: round-11 — launcher build excludes tests; guard catches all noCheck vectors Responds to the round-11 @claude review of #1799: - Finding 1 (shipping bug): clients/launcher/tsconfig.json now excludes `**/*.test.{ts,tsx}` (matching cli/tui). launcher's `build` is an EMITTING tsc over src/**, and clients/launcher/build is in the root `files` allowlist, so a co-located `src/**/*.test.ts` would have been compiled into the published tarball. Excluding it also makes such a test land in no tsconfig project, so verify:typecheck-coverage catches it — making the AGENTS.md placement rule literally true for all three clients. - Finding 2: verify-typecheck-coverage now catches both remaining ways a typecheck could check nothing: (a) case-insensitive `--noCheck`/`--nocheck`/ `--listFilesOnly` on the command, and (b) `noCheck` set in the tsconfig itself (detected via `tsc --showConfig`, which surfaces it from the config or its extends chain). Verified both fail the guard. - Nits 3 & 4: restructured into an explicit gate-integrity phase (wiring + neutered + config-noCheck + no-projects) that reports under its own accurate header and exits BEFORE the file-coverage pass — so an inert gate no longer prints file-remediation advice, nor buries the real cause under an "in no tsconfig project" line for every file that gate covered. The one CIMD OAuth E2E integration test that flaked in the coverage run passes 32/32 in isolation and on re-run; unrelated to these script/config changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * fix: round-12 — exclude .mts/.cts tests from build; drop false integrity line Responds to the round-12 @claude review of #1799: - Finding 1 (tarball, again): the round-11 test exclude only covered `.ts`/`.tsx`, so a co-located `src/*.test.mts` still compiled into launcher's shipped `build/`. All three clients now exclude `**/*.test.*` (one glob, covering .ts/.tsx/.mts/.cts) — which is also exactly what AGENTS.md already claims. Verified a `src/probe.test.mts` is no longer emitted and now fails the guard. - Finding 2: the gate-integrity phase no longer prints a false "names no `-p <project>`" line when config-`noCheck` disables projects — the condition now keys off `projects.length` (harvested), not `checking.length` (post- filter), so a client whose projects were all config-disabled shows only the accurate per-project lines. - Nit 3: recorded the boundary on the phase comment — it does not detect shell-level failure suppression (`… || true`, `; exit 0`). - Nit 5: the file-coverage remediation now leads with "for a co-located test, move it to `__tests__/`" (adding it to the src `include` would reintroduce the build emit) before the general add-to-include advice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-13 — implicit-tsconfig + derived clients; diagnostics Responds to the round-13 @claude review of #1799: - Finding 1: verify-typecheck-coverage now understands a `tsc` command with no `-p`/`-b` project flag — it resolves the implicit `./tsconfig.json` (tsc's own default), so the idiomatic `tsc --noEmit` form counts as coverage instead of producing false "in no tsconfig project" failures. Also harvests `-b`/`--build` project paths. Verified `tsc --noEmit && tsc --noEmit -p tsconfig.test.json` now reports OK. - Finding 2: CLIENTS is no longer hardcoded — it's derived from the `cd clients/<x> && npm run validate` entries in the root `validate` chain (minus an explicit web exclusion), so a new Node client is auto-required, matching the repo-wide reach of its sibling verify-format-coverage. Added a guard-would-check-nothing bail if the derivation yields an empty set. Verified a new `clients/newclient` is picked up and its uncovered file flagged. - Nit 3: the phase-2 remediation now leads with the general add-to-`include` advice and only appends the "move it to __tests__/" note when a failing path is actually a test file (was misdirecting plain files). - Nit 4: AGENTS.md no longer says the test config includes "only" __tests__ (launcher's also includes its vitest.config.ts). (Two different web integration tests each flaked once under parallel load during the coverage runs — both pass in isolation and on a clean re-run; timing- sensitive and unrelated to these script/doc-only changes.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-14 — enumerate clients from disk (fail-closed) Responds to the round-14 @claude review of #1799: - Finding 1: replace the root-chain `CLIENTS` derivation (round 13) with a disk enumeration — every `clients/<name>` whose package.json has a `typecheck` script. The chain derivation was fail-open: a root-chain edit that kept running a client but didn't match the strict regex silently dropped it from the required set (112→53 files, still green). Disk is fail-closed — a client can't be silently dropped, a new client with a typecheck is auto-required, and web (no typecheck script; builds via `tsc -b`) is excluded on its own, so the explicit exclusion is gone. The existing `rootRunsClientValidate` wiring check still verifies the root chain runs each — now LOUD when it doesn't. Verified: `cd ./clients/tui && …` now hard-fails the integrity phase instead of silently shrinking coverage; `cd clients/tui && npm run build && npm run validate` stays gated. Removed the now-unused `clientsRunByRootValidate` from the shared lib. - Nits 2 & 3: documented the two unreachable-today limitations on typecheckProjects — a solution-style `tsc -b` (empty `files` + `references`) would under-report, and the implicit-tsconfig fallback assumes no file operands. - Nit 4: refreshed the file header to mention `-b`/`--build` + the implicit `./tsconfig.json` (only the JSDoc had been updated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-15 — enroll-or-exempt enumeration; normalize `cd ./` Responds to the round-15 @claude review of #1799: - Finding 1: the disk enumeration keyed on the presence of a script named `typecheck`, so renaming it silently dropped the client (112→53, still green) — contradicting the docstring. Now every `clients/*` with a readable package.json must EITHER declare a `typecheck` (→ enrolled) OR be in an explicit EXEMPT map (`clients/web`, reason: `tsc -b`); anything else is a hard gate-integrity failure. This is fail-closed on the rename axis too, and web's skip is a stated decision rather than an accident of which scripts it declares — so web gaining a `typecheck` no longer auto-enrolls it into the solution-`-b` false failures. Verified: renaming tui's `typecheck` now hard-fails; a `tsc -b` on web stays exempt. - Nit 3: `rootRunsClientValidate` normalizes a leading `./`, so `cd ./clients/tui && npm run validate` (which genuinely runs it) is no longer a false "no longer runs" alarm. - Nit 2: README/AGENTS/script-header prose no longer names a fixed cli/tui/launcher scope — they describe the disk-discovered "every clients/* with a typecheck script (web exempt)" rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-16 — anchor wiring match; close enumeration axes Responds to the round-16 @claude review of #1799: - Finding 1 (only false-PASS): rootRunsClientValidate matched the client dir as a bare substring, so a prefix-sibling (`clients/tui` vs `clients/tui-next`) satisfied the wiring check for the shorter name — dropping a client from the root chain went green if a sibling was present. Now an anchored regex (escaped dir + optional leading `./` + a trailing boundary). Verified the sibling case now hard-fails. - Nit 2: a `clients/*` dir holding tracked TS but no readable package.json is now a gate-integrity failure (was treated as "not a client"), so the "fail-closed on every axis" claim holds. Verified. - Nit 3: readdirSync is wrapped, so a missing `clients/` dir yields the intended "found no clients/* dir" message rather than a raw ENOENT stack. - Nit 4: EXEMPT's reason is now surfaced in the success line ("clients/web exempt: typechecks via tsc -b …"), so the exemption is visible at runtime, not only in source. (Also moved `isRequiredSource` above nodeClients — nit 2's new tracked-TS check calls trackedSourceFiles at module-init, which referenced it before init.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-17 — share quote-aware tokenizer between guards Responds to the round-17 @claude review of #1799: - Finding 1: the two coverage guards parsed shell strings differently — verify-format-coverage had a quote-aware tokenize(); verify-typecheck-coverage used a raw regex. So a quoted `-p "tsconfig.test.json"` (a form tsc accepts) made the typecheck guard report every file that project covers as uncovered, and a quoted `cd "clients/tui"` in the root chain tripped a false wiring failure. Moved tokenize() (now single+double quote) into the shared lib, imported it into both guards, rewrote typecheckProjects to walk tokens (`-p`/`--project`/`-b`/`--build` → next token), and strip quotes before the `cd` match in rootRunsClientValidate. Verified both quoted forms now pass. - Nit 2: a stale EXEMPT key (a dir that no longer exists) was asserted in the success line unchecked. nodeClients now fails if an EXEMPT key isn't a real `clients/*` directory. Verified. - Nit 3 (build/dist filter belt-and-braces) left as a recorded boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-18 — match tsc by basename; strip quotes on both halves Responds to the round-18 @claude review of #1799: - Finding 1 (regression from round 17): `tokens.includes("tsc")` didn't match a path-invoked binary (`node_modules/.bin/tsc`), which the previous `\btsc\b` regex did — so such a `typecheck` was falsely reported as naming no project. Now matches on the token basename (`isTsc`), keeping round-17's quoting fix while restoring the tolerance. Verified a `node_modules/.bin/tsc` typecheck reports OK. - Nit 2: `rootRunsClientValidate` stripped quotes for the `cd` match but tested `npm run validate` against the unstripped string, so `npm run "validate"` false-failed. Both halves now test the stripped string. - Nits 3 & 4: documented on typecheckProjects — the contrived `--noCheck false` (checking on) is still treated as disabling, and the `&&`/`||`/`;` split runs before tokenizing (a quoted operator inside an arg would split mid-token, unreachable for project paths). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: round-19 — typecheck test-servers' unimported bin; --prefix wiring form Responds to the round-19 @claude review of #1799: - Finding 1: `test-servers/src/server-composable.ts` (a `#!/usr/bin/env node` bin entry nothing imports) got no tsc pass — the barrel alias covers the other 13 test-servers/src files transitively, but not this one, and its own build runs `tsc --noCheck`, so its errors were emitted into `test-servers/build` unchecked. Added it explicitly to `clients/cli/tsconfig.test.json`'s include (zero source changes; verified a bad line in it now fails cli's typecheck). Softened the AGENTS.md clause that implied the whole test-server source is transitively covered. - Nit 2: `rootRunsClientValidate` now also recognizes the `npm --prefix <dir> run validate` invocation form (was `cd`-only). - Nit 3 (stderr spy drops the completion-callback overload): left as-is — latent, not live (no exercised path awaits a stderr drain), and adding typed overload params would reintroduce the write-overload complexity the round-1 vi.spyOn refactor removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-20 — anchor validate tail; guard shared-source class Responds to the round-20 @claude review of #1799: - Finding 1 (false PASS): the wiring matcher's tail `/\brun validate\b/` also matched `run validate:fast` (`:` is a word boundary) — a DIFFERENT script that may skip typecheck. So the root chain could run a client's `validate:fast` while the guard kept measuring its `validate` (unrun), green. Anchored the tail to a boundary (`/\brun validate(?=$|[\s&;])/`). Verified `validate:fast` now hard-fails. - Finding 2: durably close the test-servers/src class instead of point-fixing. Added a SHARED_ROOTS check — every tracked TS under `test-servers/src` + the root `vitest.shared.mts` (the `format:check:shared`/`lint:shared` surface) must land in the GLOBAL union of client projects. So a NEW unimported bin entry is caught (not just server-composable.ts, named in round 19). Verified a fresh `server-orphan-bin.ts` now fails. Zero source changes — the union already covers 15/15. - Nit 4: the integrity message and rootRunsClientValidate docstring now mention the `npm --prefix` form too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-21 — deny-by-default non-client coverage Responds to the round-21 @claude review of #1799 (findings 1 & 2, one fix): - Replaced the `SHARED_ROOTS` allowlist with a deny-by-default non-client set: all tracked TS minus `clients/*` (per-client pass) minus a reasoned `core/` exemption (covered by web's `tsc -b`). This closes BOTH round-21 findings at once: - Finding 1 (stale allowlist fail-open): a rename of `test-servers/src` no longer silently drops it — its files reappear in the required "other" set and hard-fail (verified: `git mv test-servers/src test-servers/source` now fails instead of going 127→113 green). The subtractive `core/` exemption is fail-closed on staleness for the same reason. - Finding 2 (allowlist vs deny-by-default): a new top-level TS location is now auto-required (verified `tools/probe.ts` is flagged), matching the repo-wide reach of the sibling verify-format-coverage. - Nit 3: added the shared-source clause to the README table row (AGENTS got it last round). Header comment updated to describe the non-client coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-22 — narrow core/ exemption to what web's tsc -b covers Responds to the round-22 @claude review of #1799: - Finding 1: the `core/` exemption was whole-tree, but web's `tsc -b` only covers `core/**/*.ts` MINUS co-located `*.test.ts(x)` (its app project's include). So a `core/*.tsx`/`*.mts`/`*.cts` or a co-located `core` test got no tsc pass from anything. Narrowed the exemption to exactly that shape (`isExemptCoreFile`): a core `.ts` that isn't a co-located test. So a core `.tsx`/`.mts`/co-located test is now required in some project. Zero-fix today (0 such files); verified `core/mcp/probe-widget.tsx` and a co-located core `.test.ts` are now flagged. - Nit 2: `EXEMPT_ROOTS` is now a Map (dir → reason) and its reason is surfaced in the success line alongside the client exemption. - Nit 3: the non-client miss message is now the general "in no client's tsconfig project"; the `test-servers` remediation clause is conditional on an actual `test-servers/` path (tracked via `nonClientMisses`, not string-sniffing the composed message). - Nit 4: success line says "(N clients + non-client)". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-23 — core exemption mirrors both web projects Responds to the round-23 @claude review of #1799: - Finding 1: round 22's `isExemptCoreFile` mirrored only web's app project, but web's `tsc -b` also builds `tsconfig.test.json`, which includes `core/**/__tests__/**/*.{ts,tsx}`. So a `core/**/__tests__/*.test.ts` (a live layout — `core/mcp/__tests__/fakeInspectorClient.ts` already sits there) was flagged though web checks it. Now mirrors the UNION: under `__tests__/` → exempt `.ts`/`.tsx`; otherwise `.ts` minus co-located `*.test.ts(x)`. Verified a core `__tests__` test now passes while a core `.tsx` outside `__tests__` stays required. - Finding 2: `EXEMPT_ROOTS` was a Map whose keys nothing read (round 22 replaced its filter with hardcoded `isExemptCoreFile`) — a phantom registry. Collapsed to a single `CORE_EXEMPT_REASON` const used in the OK line. - Nit 3: README/AGENTS scope descriptions updated (core is no longer wholesale excluded; the non-client set is deny-by-default). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-24 — enroll clients/web via tsconfig references Responds to the round-24 @claude review of #1799: - Finding 1: `clients/web` was whole-tree EXEMPT, but web's `tsc -b` only reaches what its four referenced projects include — a file at web's package root (e.g. `playwright.config.ts`) got no tsc pass and passed EVERY gate, fully silently. This was the last whole-tree exemption wider than the gate it defers to. Now web is ENROLLED: a client with no `typecheck` script but a `tsconfig.json` `references` array is measured through those reference projects (`tsc -p <ref> --listFilesOnly`), and is wired iff its `validate` runs `tsc -b` (`validateRunsTscBuild`). EXEMPT is now the empty escape hatch. Verified: a web-root `playwright.config.ts` with a type error is now flagged, and dropping `tsc -b` from web's build hard-fails the integrity phase. Zero-fix on the tree — the union of web's 4 projects covers all 625 tracked web files. - Nit 2: assert `core/` exists (the OK line unconditionally states its exemption) — parity with the EXEMPT stale-key check. - Nit 3: a `core/` miss now gets core-specific remediation (widen web's `tsconfig.app.json` include, not a cli/tui project). - Nit 4: README/AGENTS scope descriptions corrected — web is enrolled (not exempt), and the exact core-exempt shape (app `*.ts` minus co-located tests + test `__tests__/**/*.{ts,tsx}`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-25 — drop redundant core exemption; guard tsc -b --noCheck Responds to the round-25 @claude review of #1799: - Finding 1: enrolling web (round 24) made the hand-maintained `core/` exemption both redundant and fail-open — web's measured `tsc -b` projects already cover core, and the mirror could go stale (narrowing web's include let an unimported core error pass). Deleted `isExemptCoreFile`, `CORE_EXEMPT_REASON`, the core presence assertion, and the exemptNote core clause. `core/` is now required like everything else and covered by measurement (127→894 files; a `core/*.tsx` web doesn't reach is now caught by measurement, not a hand-model). Kept the core-specific remediation clause. - Finding 2: the reference (`tsc -b`) path skipped the non-inertness check — a `tsc -b --noCheck` build gated nothing while the guard stayed green. Turned `validateRunsTscBuild` into `tscBuildStatus` (ok/neutered/none); a neutered `tsc -b` now hard-fails, matching the typecheck-script path. Factored the disabling-flag predicate (`isDisablingFlag`) so both paths share it. - Nit 3: `clientTsconfigReferences` now strips block comments too (every other tsconfig in the repo uses `/* */`). - Nit 5: README/AGENTS wiring clause covers web's `tsc -b` (not just the `typecheck` script), and core is described as measured, not exempt. Nit 4 resolves itself: EXEMPT is empty and the exemptNote/ternary now collapse cleanly. Nit 6 (tsc -b bound to a specific tsconfig) left as recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-26 — expand solution configs wherever harvested Responds to the round-26 @claude review of #1799: - Finding 1: the reference (`tsc -b`) path was selected only by "no typecheck script", so adding `typecheck: "tsc -b"` to web (the natural uniformity move) flipped it to the typecheck-script path, where the harvested solution config lists nothing → 665 false failures. Fixed at the measurement layer: projectFiles now expands a solution config (`{"files":[], "references":[…]}` — lists nothing under --listFilesOnly) to its references and measures those recursively, so a `tsc -b` project is measured no matter which path harvests it. Generalized the references reader (tsconfigReferences(path)). Verified web with `typecheck: "tsc -b"` now passes. Dropped the stale "web … is out of scope" JSDoc clause. - Nit 2: a `core/**/__tests__/**` miss now points at `tsconfig.test.json` (its actual owner), not `tsconfig.app.json`. - Nit 4: AGENTS.md's one-line summary now mentions the non-client (`core/`, `test-servers/src`) coverage, matching the detailed bullet. Nit 3 (EXEMPT empty, dormant stale-key check) left as the intended escape hatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-27 — non-inertness follows references on both paths Responds to the round-27 @claude review of #1799: - Finding 1: round 26 made COVERAGE follow a solution config's references, but the non-inertness (`noCheck`) check on the typecheck-script path still only inspected the harvested solution (empty compilerOptions) — so a `noCheck` in a REFERENCED project was invisible there (caught only on the reference path). Factored a shared `resolveLeafProjects` (a solution → its real leaf projects, cached rawProjectFiles) used by BOTH coverage (`projectFiles`) and a shared `checkingLeaves` disable-check, so a `noCheck` in a referenced project is now caught on either path. Verified: `typecheck: "tsc -b"` + `noCheck` in tsconfig.app.json now hard-fails (was green). Caching also cut the guard from ~20s to ~11s. - Nit 2: directory-form references (`{ "path": "./packages/a" }`) now resolve to `<dir>/tsconfig.json`, so a nested-solution dir reference is expanded. - Nit 3: reworded the empty-listFiles comment (it's "solution config OR errored config", both of which the reference expansion handles). - Nit 4: script header notes a `tsc -b` solution is reduced to its references on either enrollment path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-28 — dedup shared leaf in checkingLeaves Responds to the round-28 @claude review of #1799 (which found no new gate hole): - Finding 1: `checkingLeaves` dedup'd the `checking` array but ran projectDisablesChecking + integrity.push on every visit, so a leaf reachable from two harvested projects produced a duplicate `noCheck` line (and a redundant `tsc --showConfig`). Now tracks visited leaves in a Set and skips an already-seen leaf entirely. Verified a duplicate harvested project yields one line, not two. - Nit 3 (record-completeness): noted in AGENTS that the one tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by `@typescript-eslint/ban-ts-comment` across lint:core/lint:shared/each client. Nit 2 (the ~11s perf claim isn't reproducible on a CI-class runner) and nit 4 (hybrid-solution / composite boundaries, both tsc-rejected → unreachable) left as recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(scripts): unit-test the typecheck-coverage guard's parsers Responds to the round-29 @claude review of #1799 (no new gate hole; the finding is that the guard itself has no tests): - Made the guard's pure helpers importable: exported `typecheckProjects`, `tscBuildStatus`, `tsconfigReferences`, `clientTsconfigReferences`, `resolveLeafProjects`, `isTsc`, `isDisablingFlag`, `isRequiredSource`, and moved the module-init + phase execution behind an exported `main()` run only when the file is executed directly (importing it for tests no longer runs the guard). Behavior when run directly is unchanged (894 files). - Added table-driven tests (node's built-in `node --test`; the root has no vitest harness by design) for the shared lib (`tokenize`, `reachableScripts` incl. pre/post hooks, `rootRunsClientValidate` across cd/--prefix/quoted/ prefix-sibling/`validate:fast`, `rootReachesScript`) and the guard's parsers (`isRequiredSource`, `isTsc`, `isDisablingFlag`, `typecheckProjects`, `tscBuildStatus`) — one case per rule a review round found (r7/r10/r13/r15/ r16/r17/r18/r19/r20/r25). 12 tests. Wired `test:scripts` into `validate`. - Nit 2: a `scripts/*.ts` miss now gets a scripts-specific remediation (keep it `.mjs`, or give `scripts/` its own tsconfig project). - Documented the guard's test suite in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(scripts): make test:scripts self-guarding; fill parser test gaps Responds to the round-30 @claude review of #1799 (mutation testing killed 5/5; findings are about the new test gate + suite completeness): - Finding 1: `test:scripts` lacked the two properties the guard enforces for every tsc pass. verify-typecheck-coverage now asserts, in phase 1, that `test:scripts` is reachable from the root `validate` (via the already-imported rootReachesScript) AND that its tracked `scripts/**/*.{test,spec}.*` file set is non-empty and every file is matched by the command's glob (a new exported `globToRegExp`). So dropping it from validate, or renaming a test to `*.spec.mjs`/`*.test.mts` (which node --test silently skips), now hard-fails. Verified both. - Nit 2: extracted a pure `parseTsconfigReferences(raw)` from tsconfigReferences and table-tested the JSONC tolerance (block/line comments, trailing comma, no references, malformed, no path) — the r17/r26 rules had no cover. - Nit 3: added `-b`/`--build`/bare-`tsc -b` and explicit `--project` cases to typecheckProjects's test so its title matches its table. - Nit 4: rewrote the rootRunsClientValidate test to put `cmd` in a script reached from validate, and added the reachability cases (an orphan cd-validate must NOT count; validate:tui indirection must) — the r5 property. - Nit 5: added test:scripts to AGENTS's validate ordering + the README table. 15 guard tests now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * chore(scripts): round-31 — glob harvest follows delegation; brace/? globs Round-31 review findings, both false failures in the new `test:scripts` self-guard: 1. The glob half read `rootPkg.scripts["test:scripts"]` literally while the wiring half two lines above used `reachableScripts`, so a delegating `test:scripts` (`npm run test:scripts:lib && npm run test:scripts:guard`) — which genuinely runs all 15 tests — reported every tracked test file as "not matched by the glob", advising a rename the files already satisfy. Now harvested across every script reachable FROM `test:scripts`, the same fix round 6 applied to the `typecheck` harvest; `pretest:scripts` counts for free. 2. `globToRegExp` escaped `{`/`}` as literals and left `?` as a regex quantifier, both divergent from `node --test`'s glob. `*.{test,spec}.mjs` is the natural widening here (the guard's own file scan already treats a `.spec.` file as a test) and false-failed; `?` silently meant "optional previous char" and threw outright when segment-leading. Adds `?` → `[^/]` and a brace-alternation pass, with an unbalanced brace falling back to a literal. Nit 3: `resolveLeafProjects`'s two path-resolution rules (the r26 directory-form reference, and refs resolving against the referring config's dir) extracted as `projectConfigFile`/`refToProject` so they are one-row tables like the other parsers, rather than untestable behind a `tsc` spawn. Nit 4: AGENTS.md + README document that the guard now enforces `test:scripts` on three axes (wired, non-empty, glob-covered). Verified: probes C and D exit 0; probe B (rename to `*.spec.mjs`) still caught; `test:scripts` 18/18; `verify:typecheck-coverage` OK 894 files; `verify:format-coverage` OK 919; `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-32 — measure globs with node's own matcher Round-32 review; both findings point at deleting code. 1. `globToRegExp` hand-modeled `node --test`'s glob and was still one form short — character classes (`*[.]test.mjs`, `*.test.[mc]js`) hit the escape set and became literals, so the guard false-failed on files node genuinely runs. Replaced the whole translator (and its brace balance pre-scan) with node's `path.matchesGlob` — 22.5.0+, and the repo floor is >=22.19.0. Same *measure, don't model* move round 25 made when it deleted `isExemptCoreFile` in favor of measuring web's projects; being exact for every form node accepts is the point, since node's matcher is the thing being modeled. 2. The round-31 glob harvest was inline in `main()`, so mutation-reverting it to the literal single-script read left the suite green — the commit's headline fix was its one untested change. Extracted as `testScriptGlobs`, with rows for the direct form, the delegating form (both child globs, not `npm`/`run`/`<name>`), a `pretest:scripts` hook, and an unreachable script contributing nothing. Nit 3: `resolveLeafProjects`' JSDoc moved back onto it (the extraction left it stranded above `projectConfigFile`, which had two stacked doc blocks). Nit 4a: a `test:scripts` naming no path/glob harvested nothing, and `.some()` over an empty list blamed every test file with unfollowable advice. Now one explicit message naming the real problem. Verified: probes for the direct, delegating, brace, `[.]` and `[mc]` forms all exit 0 (the last two were false failures before); probe B (rename to `*.spec.mjs`) still caught; bare `node --test` reports the new single message; `test:scripts` 18/18; `verify:typecheck-coverage` OK 894; `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-33 — scope the glob harvest to `node --test` segments 1. False pass. `testScriptGlobs` kept every non-flag token from every reachable script, so a glob belonging to a NEIGHBOURING command was attributed to the test runner — and since `.some()` needs only one glob to match, a broader one suppressed a real miss rather than adding an inert pattern. A `pretest:scripts` of `prettier --check "scripts/**/*.mjs"` (reachable via the pre-hook round 31 added) was enough: the probe-B rename to `*.spec.mjs` then passed while `node --test` silently ran 12 of 18 tests. Now scoped per command segment to the ones invoking `--test`, exactly as `typecheckProjects` gates on `tokens.some(isTsc)`. The JSDoc paragraph claiming a surplus token can't suppress a miss is corrected — it was false. 2. The gate-integrity block is extracted as `testScriptProblems(scripts, testFiles)`, pure and outside `main()`. The nit-4a branch added last round was the third consecutive commit whose newest branch sat outside the test seam and mutation-survived; extracting the block ends the pattern rather than pinning one more condition from the outside. Verified. The finding-1 probe (prettier pre-hook + `*.spec.mjs` rename) now exits 1 where it exited 0; the hook alone still exits 0. Mutation testing over the whole helper — empty-harvest branch to `if (false)`, segment gate removed, wiring axis removed, non-empty axis removed, segment split removed, `reachableScripts` to `Object.keys`, `matchesTestGlob` to equality — kills 7/7, including the branch that survived the last three rounds. `test:scripts` 21/21 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-34 — harvest only positional args; normalize `./` Both findings are the axis round 33 fixed, one level in. 1. False pass. The segment gate stopped a neighbouring COMMAND's glob, but within a kept segment the filter was still "any non-flag token", so a glob-valued flag's VALUE was harvested as a positional arg. `--test-coverage-include "scripts/**/*.mjs"` is broader than the test glob and matches a renamed `*.spec.mjs`, so it vouched for the very file the runner skips — and it isn't hypothetical: `scripts/` sits outside the ≥90 coverage gate, so that flag is the obvious next thing added here. Tokens preceded by a value-taking `--test-*` flag are now dropped. 2. False failure. `./scripts/**/*.test.mjs` — which `node --test` accepts, and the common npm-script idiom — matched nothing, because `git ls-files` emits no `./`. Both files were then blamed for a rename that never happened, the unfollowable-remediation shape fixed for the bare-runner case last round. A leading `./` is normalized off, as `rootRunsClientValidate` already does. Nit 4: the gate-integrity footer's typecheck-wiring advice is now conditioned on at least one issue being about the typecheck wiring — a `test:scripts` failure carries its own per-line remediation, and appending `--noCheck`/`tsc -b` advice under a renamed-test-file message sends the reader after the wrong thing. Verified. The coverage-include probe with the rename now exits 1 where it exited 0, and exits 0 without the rename; the `./` probe exits 0 where it exited 1; a `test:scripts`-only failure no longer prints the typecheck footer. Mutation sweep 9/9 killed, including both new rules. `test:scripts` 22/22 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-35 — fourth axis: flags that narrow the run 1. False pass on a new axis. The three axes ask whether the tests are run, whether there are any, and whether the runner is told about each — none asks what it then DOES with them. `--test-shard 1/2` keeps every glob intact and executes 6 of 22 tests, exiting 0, with the guard green; `--test-only` runs 2, as does a non-matching `--test-name-pattern`. This is `--noCheck` for the test runner, one domain over. Adds `NARROWING_TEST_FLAGS` and a fourth axis in `testScriptProblems`. Three of those flags were already in `VALUE_TAKING_TEST_FLAGS` — the guard knew them well enough to drop their values but not that they shrink the run. Both `--flag value` and `--flag=value` report as the bare flag name. 2. The footer conditioning added last round was itself inside `main()` — the pattern the same commit's comment documented, recurring in the commit that documented it. Extracted as `integrityAdvice(problems, nonTypecheck)`. Doing that fixes nit 3 in the same move: the old condition keyed off `testScriptIssues` specifically, so it covered one of three non-typecheck categories. A sibling-vouch failure ("the root validate no longer runs verify:format-coverage") and client-enrollment problems still got `--noCheck`/`tsc -b` advice about a typecheck pass they have nothing to do with. All three sets are now excluded. Verified. `--test-shard 1/2`, `--test-only` and `--test-name-pattern zzz` each now exit 1 naming the flag, while node runs 6/2/2 of 22. A sibling-vouch failure no longer prints the typecheck footer. Mutation sweep 12/12 killed, including all three new rules and the branch this commit replaces. `test:scripts` 24/24 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-36 — discover test files by content, not name 1. False pass on the axis underneath the other four. The required test-file set was built with the same name pattern the `test:scripts` glob uses, so a rename escaping BOTH was invisible: the file left `testFiles` entirely, so nothing asked whether the runner was told about it, and the survivor satisfied the non-empty axis. A dot→hyphen slip (`npm-scripts.test.mjs` → `npm-scripts-test.mjs`) dropped 6 of 24 tests with every gate green — and the file that stops running is the one pinning the r16 prefix-sibling and r20 tail anchors, the series' two other false passes. Round 30 fixed "renamed to a form the glob misses" by broadening the name pattern; a fifth name pattern only moves the boundary again. `node --test` decides what a test is by whether it registers tests, so `isScriptsTestFile` reads that instead — the same measure-don't-model move as r25 and r32. It also covers what no name pattern can: a NEW test file added under a name the glob misses, never run and never noticed. Nit 2: `enrollmentProblems` no longer suppresses the footer. "declares no `typecheck` script …" IS about the typecheck pass and the footer's first clause is its canonical fix; excluding it lost the advice for the r15 rename scenario. The criterion is "is this about the typecheck pass at all", not "does it carry per-line advice" — several typecheck problems carry their own and still want the footer. JSDoc corrected accordingly. Nit 3: the complementary-shard boundary (`1/2` + `2/2` across two reachable scripts genuinely runs everything, and is flagged anyway) recorded on the helper, alongside the guard's other documented boundaries. Nit 4: the three `--test-coverage-{lines,branches,functions}` thresholds added to VALUE_TAKING_TEST_FLAGS. Their values are numeric and so inert rather than suppressing, but model completeness is cheaper than re-deriving that later. Verified. The hyphen rename now exits 1 naming the file; the `.spec.mjs` rename is still caught; an enrollment failure prints the footer again; the guard is unchanged at 894 files OK (exactly the 2 tracked test files reference `node:test`, none of the other 12 `scripts/` files do). Mutation sweep 14/14. `test:scripts` 25/25 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-37 — register-not-mention; guard the discovery read Both findings were introduced by round 36's content predicate. 1. False failure. The predicate matched a bare `node:test` mention, so a shared helper importing the runner's `mock` API — registering no tests — was enrolled as a required test file and told to rename itself to `*.test.mjs`, which would only make node run an empty file. `scripts/lib/` is already the repo's shared-helper location, so this is the plausible next file there. Tightened to node's actual criterion: an import/require OF `node:test` AND a test registration. 2. Regression. `readFileSync` was passed as an argument, so it ran for every tracked `scripts/` file before the predicate's extension gate could reject it — a file in the index but not the worktree (`rm` without `git rm`) took the guard down with a raw ENOENT stack, aborting phase 1 before any real finding printed. Discovery did no reads at all before round 36. Extension is now gated first and the read wrapped: an unreadable tracked file warns and is skipped, the round-6 precedent where a failed `tsc` echoes its diagnostic instead of dying. Verified: the mock-helper probe exits 0; the tracked-but-deleted probe exits 0 with a one-line skip warning instead of a stack; the dot→hyphen rename is still caught. Mutation sweep over the predicate 3/3 (import form, registration, extension). `test:scripts` 25/25 · `verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 · `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw * chore(scripts): round-37 nit 3 — one definition of the JS-extension check Finding 2's fix hoisted the extension gate into `main()` (it must run before the read), leaving a second copy of the regex inside the predicate that reads as protection but can never fire there. Extracted as `isJsFile` and used in both positions, so the two can't drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vgJ6ZYj9VLhfSjgua48zw --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
Bob Dickinson
|
d009fcca09 |
Fixed tab naming to correspond to the web client. (#1780) | ||
|
Cliff Hall
|
2f80958d3f |
Extensions: first-class advertised-extensions map (Phase 1 of #1633) (#1742)
* Extensions: first-class advertised-extensions map (Phase 1 of #1633) Collapse the two ad-hoc `capabilities.extensions` spreads in the InspectorClient constructor (EMA, gated on enterpriseManaged; the modern Tasks extension, always-on) into a single builder that is the source of truth for what the client advertises. - core/mcp/extensions.ts: `ADVERTISABLE_EXTENSIONS` registry (id + label + default-on flag), the `EMA_EXTENSION_KEY` constant, and `buildClientExtensions()`. The registry is the shared list the Server Settings toggle UI (Phase 2, #1739) will drive; EMA stays a conditional built-in (auth-mode driven, not a free toggle). - InspectorClientOptions.advertisedExtensions: per-extension overrides (present key wins over the registry default) so a future UI can toggle advertised extensions as a debugging knob. - Behavior preserved: tasks defaults to advertised, so the map is non-empty and rides every modern request envelope; EMA advertised iff enterpriseManaged. The raw modern-task envelope re-stamp is left untouched (it fires only on the raw tasks channel, which requires the extension anyway). Tests: extensions.test.ts covers the registry and every builder branch (defaults, EMA on/off, override enable/disable, unknown key) at 100%. The tasks-era and EMA integration suites still assert the same wire advertisement. `advertisedExtensions` is plumbed but not yet wired to the form (that lands in Phase 2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Extensions: address Phase 1 review (EMA-override test + envelope comment) - extensions.test.ts: add an EMA-specific assertion that an override for EMA_EXTENSION_KEY cannot advertise it (auth-mode only) — documents intent and guards against EMA being added to ADVERTISABLE_EXTENSIONS by mistake. - inspectorClient.ts: rewrite the withModernTaskEnvelope tasks-stamp comment so it stays accurate once Phase 2 lets a user disable general tasks advertisement — the force-stamp is load-bearing (the raw tasks/* channel requires the extension), not a redundant re-add. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * TUI: de-flake step-up OAuth frame assertions (raise poll ceiling) The step-up authorize tests intermittently timed out on the final "Step-up authorization succeeded" frame. Root cause is a test-infra timing budget, not product code: waitForFrame/waitUntil polled only 25× (25ms tick ≈ 625ms), and the step-up flow adds an extra async OAuth-runner hop plus React commits that can exceed that under CI load with v8 coverage. Hoist the poll count into a named POLL_TRIES = 100 (~2.5s ceiling). A poll exits the instant its predicate is true, so a higher ceiling never slows a passing assertion — it only widens the margin for the slow path. Verified 5× green (3 plain + 2 under --coverage), including the previously-flaky step-up cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * TUI: prettier-format the wrapped waitForFrame signature The POLL_TRIES change pushed waitForFrame's signature past the print width; Prettier wraps it. (Root has no aggregate format script — format is per-client, so this was missed locally.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
f4fb4b50c9 |
Forward RFC 9207 iss to the v2 SDK (fixes OAuth + EMA leg 1)
SDK v2 added RFC 9207 §2.4 authorization-response issuer validation
(`validateAuthorizationResponseIssuer` / `IssuerMismatchError`) as a mix-up
attack defense. It is strict: when AS metadata advertises
`authorization_response_iss_parameter_supported: true` and no `iss` reaches
`exchangeAuthorization`/`auth()`, the exchange throws.
v1.29.0 had no `iss` parameter and no RFC 9207 validation at all, so dropping
the callback `iss` was harmless. The migration swapped in the stricter SDK
without feeding it the new input, breaking the authorization-code exchange
against any RFC 9207 authorization server (Okta/xaa.dev advertise it):
Issuer mismatch in authorization response (RFC 9207):
expected "https://idp.xaa.dev", received undefined
Found by smoke-testing EMA against live xaa.dev staging; not caught by CI
because the mock IdP never advertised the parameter, so the SDK took its
lenient path (a missing `iss` is only fatal when advertised).
`iss` was dropped in three places, with the plumbing half-built —
`oauthManager`/`inspectorClient` already accepted `iss`, but nothing supplied
or forwarded it:
- `parseOAuthCallbackParams` never extracted `iss` from the callback query
- `mcpAuth` never forwarded `options.iss` to SDK `auth()` — its own comment
("forwarded on v2 upgrade; ignored by v1 SDK auth()") predicted this exact
gap, so standard OAuth was broken identically, not just EMA
- EMA leg 1 (`completeIdpOidcAuthorization`) never passed `iss` to
`exchangeAuthorization`
Thread `iss` end to end: callback query -> CallbackParams -> resumeAfterOAuth
-> completeOAuthFlow -> {mcpAuth | EMA leg 1} -> SDK. The Node callback server
and runner carry it for CLI/TUI.
Tests: `ema-mock-servers.ts` now advertises
`authorization_response_iss_parameter_supported: true` so the mock is at least
as strict as real IdPs — with it absent the SDK silently takes the lenient path
and a dropped `iss` goes unnoticed. Flipping it made three existing tests
reproduce the failure. Adds regression coverage for forwarding and for
rejecting a mismatched `iss` before any network call.
Verified end to end against live xaa.dev staging: EMA legs 1-3 (IdP SSO ->
ID-JAG mint -> resource token) now complete and the MCP session initializes.
web 3099, cli 212, tui 274 green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjMemXZohSR2EndBQYif2b
|
||
|
cliffhall
|
1f7ef9184c |
test(tui): truly assert the non-selected-server auth guards
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 |
||
|
cliffhall
|
f7ab287f6a |
test(tui): cover App.tsx mid-session auth flows to the ≥90% gate
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 |
||
|
Bob Dickinson
|
562bdd00f1 | All of the mid-session and step-up auth work | ||
|
Bob Dickinson
|
13dafe457b |
Guided-auth removal, OAuth smoke testing, and TUI/CLI support (#1531)
* 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> |
||
|
Cliff Hall
|
f43fafc08b |
test(tui): extend coverage gate to Ink components + App.tsx (closes #1501) (#1538)
* 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> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
775107776d | ||
|
|
9a8488681e | ||
|
|
a3bb679f1e | ||
|
|
463227c947 | ||
|
|
c0bcdc7e4a | ||
|
|
781aa69d19 | ||
|
|
7f27d7e9c7 | ||
|
|
65fd2b9e35 | ||
|
|
531cf84e60 | ||
|
|
21cf1bd6c3 | ||
|
|
91e08067dc | ||
|
|
d90a955164 | ||
|
|
0f628a8174 | ||
|
|
b33532c024 | ||
|
|
405d0f4d0d |
||
|
|
85e3215534 | ||
|
|
d830213796 | ||
|
|
5d8094357d | ||
|
|
519ecf49ee | ||
|
|
ae0e9d53a3 | ||
|
|
9c6e4aeb4e | ||
|
|
d9e4f444a2 |
||
|
|
d009fcca09 |
||
|
|
2f80958d3f |
||
|
|
f4fb4b50c9 | ||
|
|
1f7ef9184c | ||
|
|
f7ab287f6a | ||
|
|
562bdd00f1 | ||
|
|
13dafe457b |
||
|
|
f43fafc08b |