test: raise per-file coverage gate to 90 across all clients (closes #1271)

[cliffhall]

Closes #1271.

Summary

This PR hardens the test-coverage gate in two stages, both on this branch:

1. Audit per-file branch-coverage outliers and consider raising the 50% gate #1271 — web branch-coverage audit (50 → 70). The web per-file gate had drifted red: the recently-landed EMA auth feature (core/auth/ema/*) and ClientSettings components shipped undertested, and the 50% branch floor let real business-logic branches go untested (CI doesn't run the coverage gate, so it accumulated unnoticed). Every per-file branch outlier was audited and covered.
2. Codebase-wide raise to 90/90/90/90. Building on that, the entire per-file gate is raised to ≥ 90 on all four dimensions — lines, statements, functions, branches — across web, cli, tui, and launcher (core/ is measured under the web suite), with real tests added for every outlier.

The gate

All four vitest/vite coverage configs now enforce perFile: 90/90/90/90. Two pure-type modules (core/mcp/import/types.ts, clients/web/server/types.ts) were added to the web exclude list, matching the existing type-only exclusions.

Tests added

~50 test files were extended or created, fanned out across parallel agents over disjoint clusters:

Area	Highlights
core auth / EMA	full EMA OAuth flow (emaFlow, idpOidc, idpSession, tokenEndpoint, transportProvider, resourceContext, wire, jwt), providers, state-machine, node callback-server / secret-store / storage
core mcp / remote	inspectorClient.ts + remoteClientTransport.ts + Hono remote server.ts backfill, messageTrackingTransport, sessionStorage, resolveSource, state reducers, store-io
core react	useServers, useServerListWritable, useSandboxUrl, useClientSettingsDraft, useEmaIdpLoginState
web components / screens / server	every Mantine control's handler/branch paths (Server*/Prompt*/Resource*/Tool*/Log*/ViewHeader/InspectorView/badges), the three screens, sandbox-controller
cli / tui	unsupported---method + argv-fallback paths; tui logger + uriTemplateToForm

Unreachable branches

Genuinely-unreachable branches are annotated at the source with justified /* v8 ignore -- <reason> */ comments rather than lowering the gate — happy-dom-inherent paths (Mantine portal, useMediaQuery, typeof window SSR guards), React StrictMode effect replay, and provably-dead defensive guards (?? fallbacks the types make impossible, Select.onChange values outside the allowed list).

No runtime logic changed. The only structural source edits are two behavior-preserving refactors — ResourceTemplatePanel hoists a field value to a local so a JSX-attribute ignore applies, and ToolsScreen swaps a ternary for an if so the dead arm's ignore takes effect — plus prettier line-wrapping. A flaky real-timer debounce assertion in PromptArgumentsForm was also made deterministic (it asserted an exact sibling-typing interleaving that raced the 300 ms debounce; now asserts the load-bearing fact — a non-empty fire-time sibling capture).

Verification

• All four client coverage gates green at 90/90/90/90 (web: 3094 tests; global web branch 88.73% → ~91%, lines → 97%).
• npm run validate (all clients) — clean (format / lint / build / typecheck).
• npm run test:storybook — 370 play-tests pass.
• npm run smoke — launcher / cli / tui / web all pass.
• Caveat: connect-crash.test.ts (a pre-existing integration test, untouched here) can hit its 60 s subprocess timeout under the heavy parallel load of a full coverage run, but passes in isolation (~0.5 s) and under CI's lighter test:integration step — which is what CI runs (CI does not run coverage).

Docs

AGENTS.md updated to describe the uniform 90 gate and the v8 ignore policy for genuinely-unreachable branches.

🤖 Generated with Claude Code

[cliffhall]

Update — scope expanded: raise the whole gate to 90/90/90/90 across all clients

Building on the #1271 branch-coverage audit (web branches 50 → 70), a second commit (9373ef9) raises the entire per-file gate to ≥ 90 on all four dimensions — lines, statements, functions, branches — for web, cli, tui, and launcher (core/ is measured under the web suite).

What changed

• Configs: all four vitest/vite coverage configs set to 90/90/90/90. Two pure-type modules (core/mcp/import/types.ts, clients/web/server/types.ts) added to the web exclude list (matching existing type-only exclusions).
• ~50 test files extended/created across core (auth, ema, mcp, remote, state, storage, react), web components/screens/server, and the cli/tui clients — fanned out across parallel agents over disjoint clusters.
• Genuinely-unreachable branches are annotated at the source with justified /* v8 ignore -- <reason> */ comments rather than lowering the gate: happy-dom-inherent paths (Mantine portal, useMediaQuery, typeof window SSR guards), React StrictMode effect replay, and provably-dead defensive guards. No runtime logic changed — the only structural edits are two behavior-preserving refactors (ResourceTemplatePanel hoists a field value so the JSX ignore applies; ToolsScreen swaps a ternary for an if) plus prettier line-wrapping.
• Fixed a flaky real-timer debounce assertion in PromptArgumentsForm (it asserted an exact sibling-typing interleaving that raced the 300 ms debounce; now asserts the load-bearing fact — a non-empty fire-time sibling capture).
• AGENTS.md updated to describe the uniform 90 gate and the v8-ignore policy.

Verification

• All four client coverage gates green at 90/90/90/90 (web: 3094 tests).
• npm run validate (all clients) green; npm run test:storybook green (370); npm run smoke green (launcher/cli/tui/web).
• Note: connect-crash.test.ts (a pre-existing integration test, untouched here) can time out at 60 s under the heavy parallel load of a full coverage run but passes in isolation (~0.5 s) and under CI's lighter test:integration step.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 35s —— View job

---

Review

• Read PR context & changed source files
• Review coverage config changes
• Review the structural source edits (refactors + v8 ignores)
• Review representative test additions
• Post review feedback

I focused on the non-test source changes (the risk surface), the four coverage configs, and a sample of the new tests. The ~50 test files are additive; the only runtime-adjacent edits are two behavior-preserving refactors plus v8 ignore annotations.

Verdict: 👍 Looks good to merge

This is a clean, well-disciplined coverage hardening. The gate raise is uniform, the unreachable-branch annotations are individually justified at the source rather than papered over by lowering the gate, and the new tests exercise real branches (rejection paths, debounce cancellation, focus-fire streams) rather than gaming the metric.

What I verified

Structural refactors are behavior-preserving ✅

• ResourceTemplatePanel.tsx — hoisting variables[varName] ?? "" into a per-iteration fieldValue and returning the ternary is equivalent; fieldValue feeds both the Autocomplete and TextInput arms identically.
• ToolsScreen.tsx — tool ? collectSchemaDefaults(...) : {} → let formValues = {}; if (tool) formValues = … is a faithful 1:1 rewrite.

Coverage configs ✅ — all four (web, cli, tui, launcher) now 90/90/90/90. The two new web excludes (core/mcp/import/types.ts, clients/web/server/types.ts) are pure-type modules, consistent with the existing type-only / server/ exclusions (the bare-relative clients/web/server/types.ts matches the sibling server/*.ts excludes on lines 120–122, so the path-style mix is intentional, not a bug).

v8 ignore annotations ✅ — spot-checked each; all are defensible defensive guards or framework-replay paths (TCP address() always AddressInfo, StrictMode effect double-invoke, String.split().pop() non-empty, Mantine Transition render-prop must return an element, allowDeselect={false} guaranteeing isSortDirection). Each carries a specific, accurate reason.

Flaky fix ✅ — PromptArgumentsForm now asserts the load-bearing fact (a non-empty fire-time sibling capture via /^es?$/) instead of an exact debounce-race interleaving. Good call; the two added tests (rejection → empty dropdown, re-focus cancels pending debounce) are genuine.

Minor notes (non-blocking)

1. Cross-package invariant in clients/cli/src/cli.ts:142 — the v8 ignore on the else arm rests on an invariant inside InspectorClient ("attemptToolCall throws if finalResult is falsy"). That invariant lives in core/, a different package. If it ever changes, this annotated dead branch would silently swallow a real result === null path with no test to catch it. Low risk and consistent with the stated policy, but it's the one ignore whose justification depends on code outside this file — worth a mental note for future core/ refactors.

2. v8 ignore surface area — this PR adds a fair number of ignores. They're all justified today, but each is a small "trust me" that future readers must re-verify. The inline reasons mitigate this well; no action needed, just flagging that the gate's strength now partly rests on the discipline of keeping these comments honest.

3. connect-crash.test.ts timeout under full coverage — already disclosed in the PR body, and it's a pre-existing test untouched here. Since CI runs validate + standalone test:integration (not coverage), this won't block CI. Fine as-is, but if the full-coverage flakiness becomes a local-DX annoyance, bumping that one subprocess timeout would be a cheap follow-up.

Nice work — the audit-and-annotate approach (rather than lowering floors) is exactly what the AGENTS.md policy calls for, and the documentation updates keep it self-explaining.

[cliffhall]

Thanks for the review! Glad the audit-and-annotate approach reads cleanly. Acknowledging the three non-blocking notes — no changes made, but they're noted:

1. cli.ts:142 cross-package invariant — good catch that this is the one ignore whose justification (attemptToolCall throws on a falsy finalResult) lives in core/ rather than this file. Logged as a thing to re-verify if that InspectorClient path is ever refactored.
2. v8 ignore surface area — agreed; the inline reasons are the safeguard, and AGENTS.md now documents the policy so future additions stay held to the same "provably unreachable + specific reason" bar.
3. connect-crash.test.ts full-coverage timeout — leaving as-is for this PR (pre-existing, untouched, doesn't affect CI which runs test:integration not coverage). Bumping that one subprocess timeout is a reasonable cheap follow-up if the local-DX flakiness proves annoying.

No actionable items, so not re-requesting a review.

[cliffhall]

Addressed finding 3 (the connect-crash.test.ts timeout under full coverage) in commit d08b944.

Root cause: readSseEvents looped until the server closed the SSE stream (done). The server closes it immediately after writing the terminal transport_error, but under a full coverage run's parallel scheduler pressure that close can lag past the 30s test timeout — a hang, not a wrong result. The reader now stops as soon as the terminal transport_error event arrives (cancelling the reader) rather than waiting only for done; that event is always the last one the session queue emits, so nothing is lost, and the done break is kept as a fallback.

Verified green across two consecutive full npm run test:coverage runs (3094 tests) where it previously hung, plus isolation (~0.5s).

@claude review

[claude]

Claude finished @cliffhall's task in 56s —— View job

---

Review — commit d08b944 (connect-crash SSE reader fix)

• Read PR context & the trigger comment
• Review the connect-crash.test.ts change in d08b944
• Verify the "terminal event is always last" invariant against the server
• Post review feedback

Verdict: 👍 Correct and well-scoped — good to merge

This is a clean fix that targets the actual root cause (waiting on stream-close timing) without weakening any assertion. The test still verifies exactly what it did before — the queued stdio_log "boom from script" plus the closing transport_error (connect-crash.test.ts:159-170).

What I verified

The "transport_error is always the last event" invariant holds at the source ✅

• RemoteSession.markTransportDead() pushes transport_error as the final queued event (core/mcp/remote/node/remote-session.ts:46-59), and the events endpoint drains the queued stderr + transport_error, yields once to flush, then returns — closing the stream (core/mcp/remote/node/server.ts:690-697). So transport_error is genuinely terminal, and stopping on it cannot drop a later event. The fix's central claim checks out.

The bail logic is correct ✅

• sawTerminal is declared fresh per outer read iteration (:83), and the inner while drains all complete \n\n-delimited blocks already in the buffer before the if (sawTerminal || done) break (:84-101). So if a chunk contains both the stderr event and transport_error, both are pushed before breaking — no off-by-one, no lost trailing event. Good.

Cleanup is safe ✅

• Wrapping the loop in try { … } finally { await reader.cancel().catch(() => {}) } (:77-105) cancels the underlying stream on the early-bail path (previously the reader was just abandoned). .catch(() => {}) correctly swallows the benign "already closed / cancelled" rejection. Cancelling a reader that already saw done is a no-op, so the done path is unaffected.

Backward-compatible signature ✅

• stopAtType = "transport_error" defaults to the only call site's need (:154 calls readSseEvents(eventsRes) with no second arg), and parameterizing it keeps the helper reusable if a future test needs a different terminal event. No behavior change for existing callers.

Minor notes (non-blocking)

1. Last-resort safety net — the loop now has two exit conditions (sawTerminal, done) but still no independent timeout inside the helper; it continues to rely on the 30s vitest test timeout as the ultimate backstop. That's fine — the whole point of this fix is that the deterministic sawTerminal bail makes the timeout unreachable on the happy path, and a genuinely-stuck stream (no events at all) is a real failure worth surfacing as a timeout. No change needed; just noting the helper itself is unbounded by design.

2. Naming — stopAtType reads slightly as a verb; terminalEventType or stopAtEvent would be marginally clearer, but the JSDoc makes the intent obvious. Purely cosmetic.

Nice, surgical fix — it removes the load-dependent flakiness by anchoring on a guaranteed-terminal event rather than on stream-close scheduling, and the JSDoc captures the why so a future reader won't "simplify" it back to a done-only loop. This resolves finding 3 cleanly.

[cliffhall]

Thanks for the thorough re-review and for verifying the terminal-event invariant against the source (remote-session.ts queue order + the events endpoint's drain-then-close). Acknowledging both non-blocking notes:

1. Helper has no internal timeout — agreed, intentional. The deterministic sawTerminal bail makes the 30s vitest timeout unreachable on the happy path, and a genuinely-stuck stream should surface as a timeout failure rather than be masked.
2. stopAtType naming — fair, purely cosmetic; leaving it as-is since the JSDoc carries the intent and a rename would only churn the diff.

No actionable items, so not re-requesting. Finding 3 resolved — thanks!