chore: close quality-gate coverage gaps (#1689)

[cliffhall]

Closes #1689

Closes the three pre-existing quality-gate gaps found while auditing the SDK v2 migration (#1688): cli/tui src was never typechecked, core/ had no format/lint gate, and core/json/core/client weren't in the coverage include.

1. cli & tui src are now typechecked

• Each client gains a typecheck script (tsc --noEmit -p tsconfig.json) folded into its validate.
• Their tsconfig.json now mirrors clients/web/tsconfig.app.json's effective options — DOM lib, moduleResolution: bundler, and no noUncheckedIndexedAccess (web's app config doesn't extend tsconfig.base, so re-enabling it would flag core/ issues web itself never gates). This validates the imported core/ sources the same way web does, collapsing ~130 tsconfig-noise errors down to the genuinely-real ones.
• Real errors fixed:
  • cli cli.ts: the managed*State locals are assigned inside the runMethod closure, so the outer finally still narrowed them to null and ?.destroy() typed as never. Routed teardown through a small typed destroyManagedState() helper (no cast).
  • tui InfoTab.tsx: was reading .headers off the transport MCPServerConfig, but headers live on the per-server InspectorServerSettings (pair-array). Threaded serverSettings in and read headers from there (a latent display bug — the block never rendered before).
  • tui schemaToForm.ts: widened the JsonSchemaObject.properties value type to unknown so the SDK's broadly-typed Tool["inputSchema"] is assignable (each value was already narrowed at the point of use).

2. core/ format + lint gate

• New root validate:core = format:check:core (prettier --check "core/**/*.ts") + lint:core (eslint "core/**/*.ts" via a new root eslint.config.js), run first in the root validate.
• 22 pre-existing unformatted core/ files reformatted (formatting-only).
• Root gains prettier/eslint/typescript-eslint devDependencies. The root eslint config treats an _-prefix as the intentionally-unused marker.

3. core/json + core/client coverage

• Extended web's coverage include to core/json/** and core/client/**.
• Added remote.test.ts (fetch-mocked load/save) and node-persistence.test.ts (keychain migration, unavailable-keychain bail, delete paths); extended the json/client tests for the newly-measured lines (toRecord, getClientConfigFilePath, loadRunnerClientConfig, CIMD validators, …).

Verification

All gates green locally:

• validate:core, cli/tui validate (typecheck + tests: 212 / 279)
• coverage:web — 4459 tests, 0 threshold failures (core/client 97.6%, core/json 100%), coverage:cli, coverage:tui all exit 0

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 5m 13s —— View job

---

Review: chore: close quality-gate coverage gaps (#1689)

• Gather context (diff vs origin/v2/main)
• Review tooling/config changes (root eslint, package.json, tsconfigs, vite coverage include)
• Review real code fixes (cli cli.ts, tui InfoTab.tsx, schemaToForm.ts)
• Review core/ reformat for semantic drift
• Review new/extended tests
• Post review

Verdict: solid, and the three real bug/type fixes are correct. I verified the whole core/ diff with git diff -w --ignore-blank-lines — it is purely formatting, no semantic drift anywhere. The InfoTab header fix is a genuine latent bug: MCPServerConfig (core/mcp/types.ts:64) really has no headers, and mcpConfigToServerEntries (core/mcp/serverList.ts:451) deliberately partitions headers onto ServerEntry.settings, so that display block could never have rendered. destroyManagedState is a clean, cast-free workaround for the closure-narrowing problem. Notes below are mostly non-blocking; CI is still running so I did not reproduce the gate locally (no node_modules in this checkout).

Findings (2 worth acting on, rest are nits / follow-ups)

1. No root format fixer to pair with the new format:check:core — likely to bite contributors

validate:core now fails the build on an unformatted core/**/*.ts, but the root has no aggregate format script; AGENTS.md says "ALWAYS do npm run format before committing", which at the root doesn't exist and per-client format scripts are scoped to their own dirs. A contributor who edits core/ and runs the documented command gets a green local check and a red validate:core. Suggest a root "format": "npm run format:core && cd clients/web && npm run format && …" (or at minimum call out npm run format:core in the pre-push section of AGENTS.md).
Fix this →

2. The tsconfig comment overstates what is mirrored

clients/cli/tsconfig.json:5 and clients/tui/tsconfig.json:5 say they "mirror clients/web/tsconfig.app.json's effective options", but they mirror only lib/module/moduleResolution/types/allowImportingTsExtensions. Web's app config additionally sets target: ES2023, noUnusedLocals, noUnusedParameters, verbatimModuleSyntax, erasableSyntaxOnly, noFallthroughCasesInSwitch, noUncheckedSideEffectImports, useDefineForClassFields. Net effect: core/ is validated the same way (good — that was the goal), but cli/tui's own src is checked more weakly than web's src, and noUncheckedIndexedAccess from tsconfig.base.json is now explicitly off for them. Either mirror the strictness flags or soften the comment to "mirror the module/lib resolution options". Also: the block is now duplicated verbatim in two files (three counting web) — a shared tsconfig.client.json would prevent drift.

3. schemaToForm cast is now unvalidated

clients/tui/src/utils/schemaToForm.ts:41 widens properties to Record<string, unknown>, then line 68 does const property = prop as JsonSchemaProperty. Since the declared type now genuinely admits anything, a malformed server schema with properties: { foo: null } reaches property.title and throws. The risk is pre-existing (the old type just lied), but a guard is one line:

const property = (typeof prop === "object" && prop !== null ? prop : {}) as JsonSchemaProperty;

4. loadRunnerClientConfig tests touch the real OS keychain

clients/web/src/test/core/client/runner.test.ts calls loadRunnerClientConfig, which unconditionally constructs new KeyringSecretStore() (core/client/runner.ts:31) with no injection point. KeyringSecretStore.get swallows unavailability and returns null, so this shouldn't flake on a bare CI runner — but it makes the test environment-dependent (on a dev box with a populated mcp-inspector keychain entry the loaded config differs; the assertions happen not to look at it). Consider adding an optional secretStore to LoadRunnerClientConfigOptions so the test can inject InMemorySecretStore, matching how node-persistence.test.ts does it.

5. Test-hygiene nits

• runner.test.ts afterEach does delete process.env.MCP_CLIENT_CONFIG_PATH, clobbering any pre-existing value from the developer's environment. vi.stubEnv + vi.unstubAllEnvs() restores instead.
• remote.test.ts fakeResponse uses as unknown as Response. AGENTS.md requires an inline justification on double casts — the JSDoc explains what it is but not why no better option exists. Either add that sentence or build the stub as a typed partial.

6. Remaining gaps of exactly the class this PR closes

Since this is the "close the gaps" PR, worth either fixing here or filing a follow-up:

• clients/web/server/** is linted (eslint . from clients/web matches **/*.{ts,tsx}) but not prettier-checked — format:check is prettier --check src.
• Root scripts/**, test-servers/src/**, vitest.shared.mts, and the new root eslint.config.js are neither formatted nor linted.
• typecheck in cli/tui covers src only (include is src, plus exclude: ["**/*.test.ts"]), so clients/{cli,tui}/__tests__ — the code that does the most casting — still gets no type check.
• lint:core / format:*:core globs are *.ts only; a future core/**/*.tsx would silently escape both (coverage include already uses {ts,tsx}).

7. Docs rule: README not updated alongside AGENTS.md

AGENTS.md was updated thoroughly, but README.md's Testing & the quality gate table still describes npm run validate as "format:check + lint + build + fast unit tests, per client" with no mention of validate:core or the cli/tui typecheck step. The repo's own maintenance rule asks for both.

8. pr-screenshots/ removal is out of scope and may lose content

The two extra commits delete pr-screenshots/ (318-line README + 40 PNGs) and add pr-screenshots to .gitignore. Three things to confirm:

• The commit message says "add it to .github", but .github/ on this branch contains only workflows/ — the README's content isn't anywhere in this diff. If it was meant to be relocated, it appears to have been dropped.
• Merged PR bodies that inline those images via raw.githubusercontent.com/.../v2/main/pr-screenshots/... will 404 once this lands. Probably acceptable, just intentional-or-not.
• .gitignore entry is unanchored, so it also ignores any nested pr-screenshots dir; /pr-screenshots/ is more precise.

This is unrelated to #1689 and isn't tracked by an issue — fine to keep if deliberate, but it makes the PR's scope wider than its title.

Things I checked and found correct

• Root eslint.config.js: flat-config shape matches the clients', root package.json is "type": "module" so the ESM config loads, globalIgnores is harmless, and the ^_ unused-vars marker is a reasonable convention (it's what core/mcp/__tests__/fakeInspectorClient.ts and friends already use). Clients' eslint . runs still resolve their own config from cwd, so no interference.
• No .prettierrc anywhere — root and all four clients use prettier defaults, so core/ formatting won't fight a client's.
• Removing outDir/rootDir from clients/tui/tsconfig.json is safe: tsup.config.ts sets outDir: 'build' itself and never type-checks.
• Root devDependencies don't affect the tarball — the files allowlist is unchanged and lists only build outputs.
• New tests are well-targeted: the node-persistence.test.ts keychain-migration matrix (empty keychain / existing value wins / KeychainUnavailableError bail / non-keychain rethrow / delete paths) maps 1:1 onto migrateClientPlaintextSecret's branches, and placement under src/test/core/client/ matches the existing config.test.ts precedent.
  · branch v2/quality-gate-gaps-1689

[cliffhall]

Thanks for the thorough review. Addressed in a58288a (also merged v2/main in):

Fixed:

• Progress notifications #1 root format fixer — added a root "format" script chaining format:core + each client's format; the "ALWAYS npm run format" rule in AGENTS + README now resolves at the root.
• Request logging #2 tsconfig comment overstates — softened the cli/tui comments to say they mirror web's module/lib resolution only, and call out explicitly that cli/tui's own src is checked more loosely than web's (extra strictness flags not mirrored). Left the shared tsconfig.client.json extraction as a possible follow-up rather than expanding this PR's surface.
• Completion requests #3 schemaToForm cast unvalidated — added the typeof prop === "object" && prop !== null guard (a properties: { foo: null } now degrades to a string field instead of throwing), with a test.
• SSE transport support #4 loadRunnerClientConfig touches real keychain — added optional secretStore to LoadRunnerClientConfigOptions; the content-asserting test now injects InMemorySecretStore, and one test intentionally omits it to keep the default new KeyringSecretStore() branch covered.
• OAuth support #5 test hygiene — runner.test.ts now uses vi.stubEnv + vi.unstubAllEnvs(); remote.test.ts's as unknown as Response now carries the required inline justification (why a direct as Response fails and why no lighter option exists).
• add readme #6 core globs — lint:core / format:*:core and the root eslint files are now {ts,tsx} so a future core/**/*.tsx can't slip past.
• Tool use #7 README not updated — updated the quality-gate table for validate:core and the cli/tui typecheck step.

Deferred (out of #1689's scope, noted for a follow-up): the remaining #6 gaps — prettier-checking clients/web/server, root scripts//test-servers/src/vitest.shared.mts/eslint.config.js, and typechecking clients/{cli,tui}/__tests__. Happy to file a tracking issue for those.

#8 pr-screenshots — those two commits are @cliffhall's own (deliberately removing the screenshots), not part of the #1689 work; flagging your note about the .github relocation and the raw.githubusercontent links so they can confirm it's intended.

Focused coverage after the changes: all core/json + core/client files still ≥90 (runner.ts branch 94.33%, remote/config 100%), cli/tui typecheck clean.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 5m 36s —— View job

---

Review: chore: close quality-gate coverage gaps (#1689) — round 2

• Gather context (full diff vs origin/v2/main)
• Verify the 7 fixes in a58288a9 land as described
• Re-check tooling/config (root format, eslint globs, tsconfigs, coverage include)
• Re-check real code fixes (cli.ts, InfoTab.tsx, schemaToForm.ts, runner.ts)
• Re-check new/extended tests
• Post review

Verdict: all seven review items are genuinely addressed, and I found one new real issue — the core/ reformat silently broke a /* v8 ignore next */ annotation in inspectorClient.ts. Everything else below is a nit or a doc-drift item.

CI run 30124149500 is still in progress, and this checkout has no node_modules, so I could not run the gate locally — findings are from reading the diff.

New finding: reformat broke a coverage annotation (worth fixing)

core/mcp/inspectorClient.ts:1830 — v8 ignore next no longer covers its target

Prettier split the annotated statement across two lines:

/* v8 ignore next -- fallback only if getProtocolVersion() is unset, which
   can't happen on the connected modern session this runs on. */
const protocolVersion =            // ← line 1832: the only line `next` ignores
  this.getProtocolVersion() ?? MODERN_PROTOCOL_VERSION;   // ← line 1833: the branch

v8 ignore next ignores exactly one line after the comment. The ?? fallback — the whole point of the annotation — is now on the second line, so it is counted as an uncovered branch again. inspectorClient.ts is large enough that one branch won't sink the file under 90 (consistent with your "0 threshold failures"), so this is not a gate break — but the annotation is now dead weight and the branch is silently un-ignored, which is exactly the state AGENTS.md's v8 ignore policy exists to prevent. Fix is v8 ignore next 2 (or reflow the statement back onto one line).

This is the generic hazard of prettier-formatting a previously-unformatted tree, so worth a targeted check rather than a general worry: I grepped every core/ file this PR touches and inspectorClient.ts is the only one containing v8 ignore comments, and this is the only spot where a reflow lands adjacent to one. The other nine v8 ignore next N sites in that file are untouched by the diff.

Fix this →

Verification of the seven round-1 fixes

#	Item	Status
1	Root format script	✅ chains format:core + all four clients; every client does have a format script, and the cd clients/web && … && cd ../cli && … relative chain resolves correctly.
2	tsconfig comments softened	✅ both now say "module/lib resolution options" and explicitly state cli/tui src is checked more loosely than web's.
3	schemaToForm guard	✅ typeof prop === "object" && prop !== null + a test asserting properties: { bad: null, worse: 42 } degrades to two string fields instead of throwing.
4	secretStore injection	✅ options?.secretStore ?? new KeyringSecretStore(). Worth noting the test that omits the store is fully deterministic anyway: its client.json is absent, so readClientConfigStore returns {} before ever calling secretStore.get(), and KeyringSecretStore has no constructor body — no real keychain touch on any platform.
5	Test hygiene	✅ vi.stubEnv + vi.unstubAllEnvs(); the as unknown as Response justification is accurate (Response has ~20 required members, so as Response really is a TS2352).
6	{ts,tsx} globs	✅ in lint:core / format:core / format:check:core and the root eslint files. (@typescript-eslint/parser enables JSX by extension, so a future core/*.tsx will at least parse — it just won't get react-hooks rules, which matches the config's own comment.)
7	README quality-gate table	✅ mentions validate:core and the cli/tui typecheck.

Nits / follow-ups (non-blocking)

a. AGENTS.md now contradicts package.json on two points. The new bullet still says format:check:core (prettier --check "core/**/*.ts") + lint:core (eslint "core/**/*.ts") — fix #6 widened both to {ts,tsx}. And the cli/tui bullet still claims the tsconfigs "mirror clients/web/tsconfig.app.json's effective options", which is the exact wording fix #2 softened in the code comments. Two one-line edits.
Fix this →

b. Prettier version skew across the repo. The root adds prettier: ^3.9.6 (lock resolves 3.9.6) while the clients' own locks pin 3.8.1 (web) / 3.8.4 (cli, tui, launcher). Nothing breaks — only root prettier ever formats core/, and only client prettier ever formats client dirs — but the repo now has two formatter versions producing two styles, visible in the reformat itself: core/ unions collapsed to the hugged form (export type ConnectionStatus =\n "disconnected" | "connecting" | …) while every client union is in the leading-| form. Pinning the root to the clients' range (or bumping all five together) keeps one style for the whole tree.

c. destroyManagedState silences the error without restoring the check. The helper is a clean, cast-free fix for the finally-block narrowing (clients/cli/src/cli.ts:130), and the doc comment explains it well. Be aware it isn't stronger than the cast it replaces: because every argument is narrowed to null at the call site, the { destroy(): void } parameter type never actually validates anything. If you want real checking, collect the states as they're created inside the closure (const teardowns: Array<{ destroy(): void }> = []; teardowns.push((managedToolsState = new ManagedToolsState(…)))) and iterate in finally. Fine to leave as-is for this PR.

d. InfoTab's header block is now duplicated verbatim across the sse and streamable-http branches (clients/tui/src/components/InfoTab.tsx:119-146). Since headerPairs is hoisted, both branches render byte-identical JSX — a small HeaderLine subcomponent (or hoisting the block above the ternary) would remove the copy. Pre-existing duplication, just now more obviously copy-pasted.

e. moduleResolution: "bundler" for cli/tui is right for tsup-bundled output but no longer models Node's own resolution (it accepts imports Node ESM would reject). Correct trade-off given the goal was to validate core/ the way web does — worth knowing that the typecheck gate won't catch a Node-resolution-only mistake.

f. Still open from round 1, deferred by agreement: prettier-checking clients/web/server, root scripts/ / test-servers/src / vitest.shared.mts / eslint.config.js, and typechecking clients/{cli,tui}/__tests__. Filing that tracking issue would be good — those are the same class of gap this PR closes.

g. pr-screenshots/ (out of scope, @cliffhall's own commits). Re-confirming what I flagged last round, now verified against the tree: .github/ on this branch contains only workflows/, so the 318-line README is not relocated anywhere in this diff. Nothing in the repo links to pr-screenshots/ any more (only .gitignore mentions it), so no broken in-repo links — but merged PR bodies that hotlink raw.githubusercontent.com/.../v2/main/pr-screenshots/… will 404. The .gitignore entry is also still unanchored (pr-screenshots vs /pr-screenshots/), so it ignores nested dirs of that name too.

Things I checked and found correct

• core/ diff is formatting-only. I diffed every one of the 22 changed core/ files at whitespace-insensitive level and read the full -w diff end to end: every change is prettier reflow (line joins/splits, trailing-comma removal on collapse, parenthesized cast reflow, one missing trailing newline in core/mcp/node/authChallengeFetch.ts). No identifier, operator, or control-flow change anywhere. The two non-formatting core/ edits are the intentional ones: runner.ts's secretStore option and the SecretStore type import.
• InfoTab fix remains correct. InspectorServerSettings.headers is { key: string; value: string }[] (core/mcp/types.ts:611) with no enabled flag, so rendering every pair is right; MCPServerConfig still has no headers, confirming the display block was previously dead. Tests now build settings through the real headersToServerSettings helper instead of a lying double cast — good change. App.tsx threads selectedServerEntry?.settings, which is the same entry the config already comes from.
• tsconfig mechanics. allowImportingTsExtensions is paired with noEmit (required); removing outDir/rootDir from tui is safe since tsup sets its own; paths in the child configs resolve relative to the child config file (no baseUrl), so ../../core/* and ./node_modules/@types/react both land correctly; core/ is checked transitively through cli/tui imports even though include is only src.
• New tests are well-targeted. node-persistence.test.ts covers all four migrateClientPlaintextSecret branches (empty keychain / existing value wins / KeychainUnavailableError bail leaving plaintext on disk / non-keychain rethrow) plus both delete paths; remote.test.ts covers 404-vs-500, header presence/absence, non-object JSON, and the globalThis.fetch fallback; the config.test.ts / jsonUtils.test.ts additions hit exactly the newly-measured lines (toRecord, getClientConfigFilePath, the three CIMD validator messages, convertParameterValue's empty-string short-circuit and JSON-parse fallback).
• Coverage include widening is a strict superset of the old core/json/xMcpHeader.ts entry, and matches the {ts,tsx} pattern used by its siblings.

· branch [`v2/quality-gate-gaps-1689`](https://github.com/modelcontextprotocol/inspector/tree/v2/quality-gate-gaps-1689)

[cliffhall]

Round 2 addressed in f572b0f:

New finding (fixed): the v8 ignore next in inspectorClient.ts — good catch, that was a real regression my reformat introduced. Changed to v8 ignore next 2 so both lines of the reflowed statement (the ?? MODERN_PROTOCOL_VERSION fallback branch) are ignored again. Re-ran the full web coverage gate: 4459 tests, 0 threshold failures.

Nit a (fixed): AGENTS.md no longer drifts from the code — the validate:core bullet now shows the {ts,tsx} globs, and the cli/tui bullet now says they match web's module/lib resolution options only (not "effective options") and calls out the looser strictness.

Nit b (fixed): pinned root prettier to ^3.8.1 (installs 3.8.4, matching the clients) and reformatted core/. The whole tree is now one style again — core/ unions went back to the leading-| form instead of 3.9.x's hugged form. This is why the diff now touches a few more core/ files (all pure reformat back to the client style).

Nit d (fixed): hoisted the duplicated sse/streamable-http header JSX into a single headersBlock const.

Nits c, e: left as-is per your "fine to leave" — noted. (c: destroyManagedState doesn't restore real checking, agreed; e: moduleResolution: bundler won't catch Node-resolution-only mistakes, agreed and intended.)

Nit f (tracked): filed #1767 for the remaining gap-closing items (prettier-check clients/web/server, root scripts/test-servers/src/vitest.shared.mts/eslint.config.js, typecheck cli/tui __tests__) and added it to the board.

#8 pr-screenshots: still @cliffhall's own commits — relaying your verified notes (the README isn't relocated into .github/, and the .gitignore entry is unanchored) for them to confirm.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 4m 41s —— View job

---

Review: chore: close quality-gate coverage gaps (#1689) — round 3

• Gather context (full diff vs origin/v2/main, focus on f572b0f6)
• Verify the four round-2 fixes land as described
• Re-check core/ reformat for semantic drift (prettier 3.8.4 pass)
• Re-audit every v8 ignore site in the touched core/ files
• Re-check tooling/config + new tests
• Post review

Verdict: ready to merge from my side. All four round-2 items are correctly addressed, the v8 ignore regression is genuinely fixed, and I re-verified the whole core/ diff is formatting-only. Four minor nits below, none blocking. CI run 30125164848 is still in progress and this checkout has no node_modules, so findings are from reading the tree, not from running the gate.

Verification of the round-2 fixes

Item	Status
v8 ignore next 2 in inspectorClient.ts:1830	✅ The annotated statement occupies exactly lines 1833–1834 (const protocolVersion = / this.getProtocolVersion() ?? MODERN_PROTOCOL_VERSION;), so next 2 covers the ?? branch again.
Prettier pinned to ^3.8.1	✅ Root and all four clients now declare the same range. Verified the effect: the net core/ diff no longer contains any union reformat — challenge.ts, types.ts, oauthUx.ts, connection-state.ts, urlElicitation.ts are all back to the leading-| client style, and fakeInspectorClient.ts / connection-state.ts / urlElicitation.ts dropped out of the diff entirely.
headersBlock hoist in InfoTab.tsx:34-44	✅ Single hoisted node, both sse / streamable-http branches render {headersBlock}; output is byte-identical to the old duplicated JSX (headerPairs was already hoisted).
AGENTS.md drift	✅ {ts,tsx} globs and the "module/lib resolution options" wording now match package.json and the tsconfig comments. README's quality-gate table matches too.

v8 ignore audit (the generalized version of last round's finding). I grepped every core/ file this PR touches for v8 ignore and checked each against the reflow locations, rather than trusting the one spot:

• core/mcp/inspectorClient.ts — 11 sites. The three other reflows in this file (content: [{…}] at 1866, the reject(new Error(…)) split at 1908, the if (isTerminalStatus(…) || …) split at 4448) have no adjacent annotation. The remaining 10 annotations (2215, 2251, 2339, 2646, 3087, 3107, 3140, 3165, 3186, 4757) are untouched by the diff.
• core/auth/node/runner-oauth-callback.ts — 6 sites (48, 56, 60, 61, 68, 77), all well below the only reflow in that file (the line-34 raw = … join). A join earlier in a file doesn't change any annotation's offset from its own target, so these are unaffected.
• core/mcp/node/authChallengeFetch.ts — 1 site; the only change to that file is the added trailing newline at EOF.
• Every other touched core/ file: zero v8 ignore comments.

Semantic drift re-check. git diff -w --ignore-blank-lines origin/v2/main...HEAD -- 'core/**' yields ~40 hunks and I read all of them: every one is a prettier reflow (join/split, trailing-comma removal on collapse, parenthesized-cast reflow, the EOF newline). The only non-formatting core/ edits are the intentional ones in core/client/runner.ts — the secretStore?: SecretStore option, the SecretStore type import, and options?.secretStore ?? new KeyringSecretStore().

Worth calling out: the new gate paid for itself immediately — core/mcp/node/authChallengeFetch.ts was missing its trailing newline, which nothing in the repo checked before.

Nits (non-blocking)

1. cli/tui tsconfig.json exclude is .ts-only — the same glob asymmetry fixed last round. clients/cli/tsconfig.json:23 and clients/tui/tsconfig.json:27 both use "exclude": ["node_modules", "**/*.test.ts", "build"]. tui has 15 .tsx files under src, so an in-src Foo.test.tsx would be pulled into typecheck while its .ts sibling is excluded. Latent today (there are zero test files under either client's src — all tests live in __tests__/, which include doesn't reach anyway), but it's the same .ts-vs-{ts,tsx} class as the lint:core glob fix. One-line change to **/*.test.{ts,tsx}.
Fix this →

2. Duplicate import in config.test.ts. @inspector/core/client/secrets.js is now imported twice — the pre-existing block at line 13 (mergeSecretsIntoClientConfig, …) and a new standalone import { hasClientPlaintextSecret } from "@inspector/core/client/secrets.js";. Passes lint (no-duplicate-imports isn't in recommended); just fold it into the existing block.

3. Prettier lock skew is narrowed, not eliminated. Declared ranges now agree (^3.8.1 everywhere), but the resolved versions still differ: root / cli / tui / launcher locks → 3.8.4, clients/web's lock → 3.8.1. In practice the styles agree (that's the point of the pin), so this is cosmetic — a npm install refresh in clients/web would converge it.

4. v8 ignore next 2 now encodes a reflow as a coverage contract. The comment is honest about why it's 2, which is the right call. The remaining fragility is the other direction: if a future prettier rejoins the statement onto one line, next 2 silently swallows the following return { line. A /* v8 ignore start */ … /* v8 ignore stop */ pair around the single statement is reflow-proof in both directions, and this repo already uses that form (core/auth/node/runner-oauth-callback.ts:56). Low priority — the current state is correct.

5. Placement note, no change requested. node-persistence.test.ts and the new loadRunnerClientConfig block in runner.test.ts do real filesystem work (mkdtemp, writeFile) from the unit (happy-dom) project rather than src/test/integration/. The neighbouring config.test.ts already does exactly this, so it matches precedent and I'm not asking for a move — flagging only because "unit test that touches the FS" is the thing the folder split exists to make visible.

Things I checked and found correct

• headersBlock hoist doesn't change behavior for stdio. It's computed unconditionally but only referenced in the two HTTP branches, and evaluates to null when headerPairs is empty. InfoTab's new serverSettings?: InspectorServerSettings | null prop defaults to null, and App.tsx:1663 passes selectedServerEntry?.settings ?? null — the same entry the config comes from.
• schemaToForm guard is right and tested. typeof prop === "object" && prop !== null ? prop : {} degrades properties: { bad: null, worse: 42 } to two string fields; the test asserts exactly that (name/type/label for both).
• secretStore injection. options?.secretStore ?? new KeyringSecretStore(), runner.test.ts injects InMemorySecretStore for the content-asserting test and deliberately omits it in the absent-file test to keep the default branch covered — and that default-branch test is deterministic anyway (no client.json → readClientConfigStore returns {} before secretStore.get() is reached).
• Test hygiene. runner.test.ts uses vi.stubEnv + vi.unstubAllEnvs(); remote.test.ts uses vi.stubGlobal + vi.unstubAllGlobals() and its as unknown as Response carries an accurate justification (Response really does have ~20 required members, so as Response is a TS2352).
• Coverage include widening (core/json/**, core/client/**) is a strict superset of the old single-file entry and matches the sibling {ts,tsx} pattern. core/client has 8 files, core/json has 2 — all with tests after this PR.
• Root tooling. format chain's relative cd ../cli && … resolves correctly (npm runs from the package root, and each client's own format doesn't cd); root eslint config's tseslint.configs.recommended disables no-undef for TS so the isomorphic browser+node globals set is sufficient; root typescript: ~5.9.3 matches all four clients, and each client resolves its own copy first anyway; root devDeps don't reach the tarball (files allowlist unchanged).
• tsconfig mechanics. noUncheckedIndexedAccess: false is the only strictness override vs tsconfig.base.json (strict: true still inherited); types: ["node"] doesn't affect the tests since both clients run globals: false and import from vitest explicitly; allowImportingTsExtensions is paired with noEmit; esbuild/tsup ignore moduleResolution, so flipping it to bundler can't affect the built bundles.
• Still-open / out-of-scope items are settled: the remaining gap-closers are tracked in Close remaining quality-gate gaps: web/server + root-file format, cli/tui __tests__ typecheck #1767, and the pr-screenshots removal remains @cliffhall's own commits (the 318-line README is still not relocated into .github/, and the .gitignore entry is still unanchored — both for them to confirm).

· branch [`v2/quality-gate-gaps-1689`](https://github.com/modelcontextprotocol/inspector/tree/v2/quality-gate-gaps-1689)

[cliffhall]

Thanks — round 3's nits addressed in e15515d (all four):

• Progress notifications #1 tsconfig exclude glob — widened to **/*.test.{ts,tsx} in both cli and tui, matching the lint:core glob class.
• Request logging #2 duplicate import — folded hasClientPlaintextSecret into the existing secrets.js import block in config.test.ts.
• Completion requests #3 prettier lock skew — converged: bumped clients/web to 3.8.4 so all five locks resolve the same version, while keeping every declared range at ^3.8.1.
• SSE transport support #4 v8-ignore reflow fragility — switched to a v8 ignore start/stop bracket around the single statement (the reflow-proof form the repo already uses in runner-oauth-callback.ts), so neither a future split nor a rejoin can mis-count the branch.

#5 (placement note) — left as-is per your "no change requested"; the FS-touching unit tests match the existing config.test.ts precedent.

Gate re-verified after the changes: web coverage 4459 tests / 0 threshold failures, validate:core + cli/tui typecheck clean.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 19s —— View job

---

Review: chore: close quality-gate coverage gaps (#1689) — round 4

• Gather context (diff vs origin/v2/main, focus on e15515d9)
• Verify the four round-3 fixes
• Re-check core/ diff for semantic drift after the prettier pin
• Re-audit v8 ignore sites
• Post review

Verdict: three of the four round-3 fixes are correct; the fourth doesn't work, and that one is my fault — my round-3 nit #1 gave bad advice. TypeScript's include/exclude globs don't support brace expansion, so **/*.test.{ts,tsx} is a literal filename pattern that matches nothing. Details and the correct form below; it's a one-line fix and has zero practical impact today.

Finding: **/*.test.{ts,tsx} in the cli/tui tsconfigs is a no-op — my round-3 nit was wrong

clients/cli/tsconfig.json:24 and clients/tui/tsconfig.json:29:

"exclude": ["node_modules", "**/*.test.{ts,tsx}", "build"]

tsconfig include/exclude support only *, ?, and **/ — brace expansion has never been implemented (microsoft/TypeScript#5583, still open). TS escapes { / } as literal regex characters, so this pattern matches a file literally named foo.test.{ts,tsx}. Net effect: the exclusion that did work before (**/*.test.ts) now excludes nothing.

I verified this empirically rather than from memory (throwaway project, src/foo.test.ts with a deliberate type error, --listFiles):

exclude entry	foo.test.ts in the program?
"**/*.test.{ts,tsx}"	yes — TS2322 reported
"**/*.test.ts", "**/*.test.tsx"	no — not listed, no error

(Run with the global tsc on this runner, 7.0.2, not the repo's pinned ~5.9.3 — the brace behavior is identical in both, since it's the absence of a feature rather than a version difference.)

The repo already has the correct precedent: clients/web/tsconfig.app.json:62-70 spells out both extensions separately (src/**/*.test.tsx, src/**/*.test.ts, ../../core/**/*.test.ts, ../../core/**/*.test.tsx), and clients/tui/vitest.config.ts does the same in its coverage exclude. Note vitest/tinyglobby does support braces — that asymmetry is exactly what makes this an easy trap, and it's why the {ts,tsx} widening was right for lint:core/format:core (prettier + eslint globs) but wrong here.

Practical impact today: none. include is src/**/* (cli) and index.ts / tui.tsx / src/**/* (tui), and there are no *.test.ts(x) files under either src — all tests live in __tests__/, which include never reaches. So the gate result you reported (cli/tui typecheck clean) is unaffected either way. The problem is that the guard is now inert: the first in-src test file would be pulled into typecheck unexpectedly, which is the exact scenario the entry exists to prevent.

Fix:

"exclude": ["node_modules", "**/*.test.ts", "**/*.test.tsx", "build"]

Fix this →

Verification of the round-3 fixes

#	Item	Status
1	tsconfig exclude glob widened	❌ See above — the brace form doesn't work in tsconfig. My advice, my error; the fix is two entries.
2	Duplicate secrets.js import folded	✅ config.test.ts now has exactly one from "@inspector/core/client/secrets.js" (line 15), with hasClientPlaintextSecret in the existing block. I also scanned all five touched/new test files for any other duplicate module specifier — none.
3	Prettier lock converged	✅ All five locks now resolve 3.8.4 (root, web, cli, tui, launcher) while every declared range stays ^3.8.1. The skew I flagged in round 2/3 is gone.
4	v8 ignore start/stop bracket	✅ Correct and tight: core/mcp/inspectorClient.ts:1830 opens, :1835 closes, wrapping only the const protocolVersion = … ?? MODERN_PROTOCOL_VERSION; statement. Counted the pairs in the file — exactly one start and one stop, so there's no unbalanced start silently ignoring the file tail. The other 10 annotations in the file are all next N and untouched.

Semantic-drift re-check (cumulative, not just e15515d9). Re-read the full git diff -w --ignore-blank-lines origin/v2/main...HEAD -- 'core/**' end to end. Every hunk is a prettier reflow — constructor-param joins, import-list collapse/expand, parenthesized-cast reflow, union splits, ternary joins. The only non-formatting core/ changes remain the three intentional ones in core/client/runner.ts (the secretStore?: SecretStore option, the type SecretStore import, options?.secretStore ?? new KeyringSecretStore()) plus the v8 comment rewrite above. No identifier, operator, or control-flow change anywhere.

Still standing from earlier rounds (all settled, no action)

• Nits c (destroyManagedState doesn't restore real type checking) and e (moduleResolution: bundler won't catch Node-resolution-only mistakes) — accepted as-is by agreement.
• Round-3 nit 5 (FS-touching tests in the unit project) — matches the config.test.ts precedent; no change requested.
• Remaining gap-closers tracked in Close remaining quality-gate gaps: web/server + root-file format, cli/tui __tests__ typecheck #1767 (prettier-check clients/web/server, root scripts/ / test-servers/src / vitest.shared.mts / eslint.config.js, typecheck cli/tui __tests__). Worth noting the brace-glob trap above is a good argument for including "audit the exclude globs when __tests__ joins typecheck" in that issue's scope.
• pr-screenshots/ removal remains @cliffhall's own two commits (1a798daa, 69a57193), outside Close quality-gate coverage gaps: typecheck cli/tui, format+lint core/, coverage core/json+core/client #1689's scope. Re-verified against the current tree: the 318-line README is still not present anywhere under .github/, and the .gitignore entry is still unanchored (pr-screenshots rather than /pr-screenshots/). Both are for them to confirm as intended.

I could not run the gate locally (no node_modules in this checkout), so all findings are from reading the tree plus the isolated tsc experiment above. Once the one-line exclude change lands, this looks good to merge from my side.
· branch v2/quality-gate-gaps-1689

[cliffhall]

Good catch — fixed in ea856b5. You're right that tsconfig include/exclude doesn't do brace expansion (unlike prettier/eslint/vitest globs), so **/*.test.{ts,tsx} was inert and silently dropped the .test.ts exclusion. Split into two entries (**/*.test.ts + **/*.test.tsx), matching clients/web/tsconfig.app.json's form.

Verified empirically: a broken src/__probe.test.ts is now excluded from typecheck (clean), while a broken src/__probe.ts control is still caught (1 error) — so the guard actually works again.

Everything else (the other three round-3 fixes, semantic-drift, the v8 ignore bracket balance) confirmed by your re-audit. No other changes.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 0s —— View job

---

Review: chore: close quality-gate coverage gaps (#1689) — round 5

• Gather context (diff vs origin/v2/main, focus on ea856b5d)
• Verify the round-4 exclude fix actually works (empirical tsc probe)
• Re-check the full non-core/ diff
• Re-audit core/ for semantic drift + v8 ignore balance
• Post review

Verdict: no findings — LGTM to merge. The round-4 exclude fix is correct, and I re-verified it empirically rather than by inspection. Nothing new surfaced in this pass; the four prior rounds' items are all settled.

Empirical verification of ea856b5d

Reproduced the three-way comparison in a throwaway project (src/foo.test.ts, src/bar.test.tsx, src/ok.ts, each test file carrying a deliberate TS2322), with include: ["src/**/*"]:

exclude entry	Result
"**/*.test.ts", "**/*.test.tsx" (this PR, ea856b5d)	✅ exit 0 — both test files excluded
"**/*.test.{ts,tsx}" (previous, e15515d9)	❌ exit 1 — both test files compiled and errored, i.e. the entry matched nothing
control: add a non-test src/ctrl.ts with the same error	✅ exit 1 — TS2322 reported, so the program isn't accidentally empty

That last row is the check that matters most: it proves the exclusion is narrow rather than swallowing the whole include. Matches your own probe result (src/__probe.test.ts excluded, src/__probe.ts caught), and matches clients/web/tsconfig.app.json:62-70's existing separate-entry form. Both clients/cli/tsconfig.json:24 and clients/tui/tsconfig.json:29 carry the two entries.

Final re-audit (cumulative, whole diff)

core/ is formatting-only. Re-ran git diff -w --ignore-blank-lines origin/v2/main...HEAD -- 'core/**' and read every hunk: constructor-param joins, import collapse/expand, union splits, parenthesized-cast reflow, ternary joins, one EOF newline. No identifier, operator, or control-flow change. The only non-formatting core/ edits remain the four intentional ones — core/client/runner.ts's secretStore?: SecretStore field, the type SecretStore import, options?.secretStore ?? new KeyringSecretStore(), and the v8 ignore comment rewrite in inspectorClient.ts.

Worth noting the one reflow that looks semantic but isn't: core/client/runner.ts:98 collapsed the oauthFromCli ternary condition onto a single line (cliOverrides?.clientId || cliOverrides?.clientSecret || clientMetadataUrl ? { … }). || binds tighter than ?:, so precedence is unchanged — it just reads more densely.

v8 ignore balance. core/mcp/inspectorClient.ts has exactly one start (1830) and one stop (1835), bracketing only the const protocolVersion = … ?? MODERN_PROTOCOL_VERSION; statement — no unbalanced start silently ignoring the file tail. The other 10 sites are all next N and untouched by the diff. Re-grepped the other touched core/ files: runner-oauth-callback.ts (6 sites, all above/below the single line-34 join, unaffected) and authChallengeFetch.ts (1 site, only change is the EOF newline); everything else has zero annotations.

Docs now match the code. AGENTS.md's validate:core bullet shows the {ts,tsx} globs and the "module/lib resolution options" wording; README's quality-gate table names validate:core and the cli/tui typecheck; the tsconfig comments say the same thing. No drift left between the three.

Root tooling. format chain (format:core → web → cli → tui → launcher) resolves correctly — the relative cd ../cli runs in the shell already inside clients/web, and no client's own format cds. Root eslint.config.js scopes files to core/**/*.{ts,tsx}, so client eslint . runs (which resolve their own config from cwd) are unaffected. Root devDeps don't reach the tarball (files allowlist unchanged).

Real code fixes re-read and unchanged from my round-1/2/3 verification: destroyManagedState (cast-free, well-documented), the InfoTab headers fix with the hoisted headersBlock, and schemaToForm's typeof prop === "object" && prop !== null guard with its test.

Still standing, all settled — no action

• Round-2 nits c (destroyManagedState doesn't restore real type checking — its parameter validates nothing, since every call-site argument is narrowed to null) and e (moduleResolution: bundler won't catch Node-resolution-only mistakes): accepted as-is by agreement.
• Round-3 nit 5 (FS-touching tests in the unit project rather than src/test/integration/): matches the config.test.ts precedent; no change requested.
• Remaining gap-closers tracked in Close remaining quality-gate gaps: web/server + root-file format, cli/tui __tests__ typecheck #1767 — prettier-check clients/web/server, root scripts/ / test-servers/src / vitest.shared.mts / eslint.config.js, typecheck cli/tui __tests__. The brace-glob trap is a good thing to fold into that issue's scope, since bringing __tests__ under typecheck means touching these same exclude globs.
• pr-screenshots/ removal remains @cliffhall's own two commits (1a798daa, 69a57193), outside Close quality-gate coverage gaps: typecheck cli/tui, format+lint core/, coverage core/json+core/client #1689's scope. Re-verified against the current tree: .github/ still contains only workflows/, so the 318-line README is not relocated anywhere; and .gitignore:23 is still the unanchored pr-screenshots rather than /pr-screenshots/. Both for @cliffhall to confirm as intended.

Caveat on scope of verification: this checkout has no node_modules (root or any client), so I could not run the repo's own gate — findings are from reading the tree plus the isolated tsc probe above (global tsc 7.0.2; the brace behavior is the absence of a feature, identical under the repo's pinned ~5.9.3). I'm relying on your reported gate results (web coverage 4459 tests / 0 threshold failures, validate:core + cli/tui typecheck clean). CI run 30126143641 is still in progress at time of writing — worth a glance before merging.
· branch v2/quality-gate-gaps-1689

[cliffhall]

Thanks for the five thorough rounds — appreciate the empirical tsc verification of the exclude fix in particular. No outstanding review items; PR is ready to merge whenever @cliffhall is.