test(core): backfill coverage on inspectorClient + remote server

[cliffhall]

Closes #1310.

Drops the two TODO(#1310) entries from clients/web/vite.config.ts (coverage.exclude). Both files now pass the per-file gate (lines >= 90, statements >= 85, functions >= 80, branches >= 50).

Coverage delta

	before	after
core/mcp/inspectorClient.ts	73.48 / 72.82 / 72.09 / 60.57	91.16 / 90.76 / 89.92 / 75.53
core/mcp/remote/node/server.ts	88.15 / 87.50 / 78.37 / 73.52	97.29 / 97.33 / 100.00 / 82.08

(lines / statements / functions / branches)

What changed

core/mcp/remote/node/server.ts

• Extracted createTokenAuthProvider to its own file (core/mcp/remote/node/tokenAuthProvider.ts) so the 7 no-op OAuthClientProvider stubs can be unit-tested directly — remote-tokenAuthProvider.test.ts exercises all of them. (The issue authorizes refactoring private helpers to be testable.)
• Added integration tests in remote-transport.test.ts for the remaining server.ts gaps: createTransportNode sync-throw on connect, transport-dead path on send, /api/log non-JSON catch, forwardLogEvent unknown-level / no-messages / string-first-message fallbacks, /api/fetch missing-url + upstream-throw, /api/storage POST invalid storeId / invalid JSON, and /api/storage GET on a corrupt store file. 11 new tests, all green.

core/mcp/inspectorClient.ts

Added a single "Coverage backfill (#1310)" describe block to inspectorClient.test.ts with focused tests for the previously-uncovered paths:

• OAuth + getter guards (no server): all OAuth methods on a client constructed without environment.oauth; the trivial session / roots / subscription / instructions accessors before connect; setOAuthConfig and authenticate / runGuidedAuth / proceedOAuthStep throws via ensureOAuthManager; authenticate() on stdio transports (covers getServerUrl()'s stdio throw).
• Subscribe / unsubscribe: error paths against a server without the subscribe capability and the happy path against one with it (URI fixed to test://resource_1 to match createNumberedResources).
• getPrompt + readResourceFromTemplate: happy path against a server with prompts and templates, plus the URI-template expansion-failure path.
• setLoggingLevel: "Server does not support logging" guard against a logging-less server.
• getAppRendererClient: null before connect, cached proxy after.
• listChanged notifications: uses fixture add_tool / add_resource / add_prompt tools to make the server emit tools/list_changed / resources/list_changed / prompts/list_changed and asserts the matching dispatched events. (I tried resources/updated too but the streamable-http delivery was flaky in 5s; the other three are enough to lift the gate.)
• Misc tiny branches: connect() no-op when already connected, connect() throwing when this.client is null (nullify), getTaskCapabilities() returning undefined against a server without a tasks capability, the elicit: { form: true } constructor branch.
• Receiver-task internals (focused unit pass the issue calls out): drives createReceiverTask / listReceiverTasks / getReceiverTask / getReceiverTaskPayload / cancelReceiverTask via (instance as unknown as { ... }) to exercise the entire cancelReceiverTask body plus the TTL setTimeout cleanup branch. Includes the terminal-status fast-path and the unknown-taskId throws.
• Defensive if (!this.client) throw guards: one nullify-and-call test that exercises 16 sites in a single instance — listTools, listResources, listResourceTemplates, listPrompts, callTool, callToolStream, readResource, readResourceFromTemplate, getPrompt, setLoggingLevel, setRoots, subscribeToResource, unsubscribeFromResource, getRequestorTask / getRequestorTaskResult / cancelRequestorTask / listRequestorTasks, ping, plus getCompletions's { values: [] } short-circuit.

Test plan

• npm run validate (format + lint + build + unit + integration coverage; per-file gate now enforced on both files)
• npm run test:storybook (67 stories / 298 play-tests)
• New tests: 11 in remote-transport.test.ts, 22 in inspectorClient.test.ts, 3 in the new remote-tokenAuthProvider.test.ts — all green

Out of scope

• Raising the project-wide branches threshold (tracked in Audit per-file branch-coverage outliers and consider raising the 50% gate #1271).
• Wider coverage.include expansion beyond what Run v1.5-ported integration tests under a node-env vitest project (#1302 follow-up) #1307 already added.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

Claude PR Review

• Read changed files and understand the scope
• Review tokenAuthProvider.ts extraction + its tests
• Review remote-transport.test.ts additions
• Review inspectorClient.test.ts additions
• Verify vite.config.ts coverage changes
• Post review feedback

---

Summary

Solid, focused coverage backfill. The extraction of createTokenAuthProvider to its own file is a clean refactor that enables direct unit testing without over‑expanding the module's public surface, and the coverage delta on both files clears the per‑file gate comfortably. The new tests stay close to behavior (endpoint status codes, event dispatches, capability guards) and lean on real test servers rather than brittle mocking. Overall this is approval‑worthy; notes below are mostly nits.

Strengths

• Good refactor scope. createTokenAuthProvider is moved verbatim into core/mcp/remote/node/tokenAuthProvider.ts with an explanatory comment that justifies the no‑op stubs; the server.ts change is -40/+1 and purely an import swap. Low-risk, high-testability win.
• Endpoint error paths on server.ts are well covered. remote-transport.test.ts now exercises the createTransportNode sync‑throw path (L1236–1252), the /api/log non‑JSON catch (L1302–1316), the three forwardLogEvent branches (unknown‑level / no‑messages / string‑first, L1318–1404), /api/fetch missing‑url and upstream‑throw (L1406–1437), and /api/storage invalid‑id / invalid‑JSON / corrupt‑file‑GET (L1439–1501). These are the gaps server.ts actually had.
• inspectorClient.test.ts tests drive real behavior — subscribe/unsubscribe against a server with subscriptions: true, list‑changed dispatches via the add_tool / add_resource / add_prompt fixtures with waitForEvent, and getPrompt / readResourceFromTemplate round‑trips — rather than smoke‑touching internals.
• Receiver‑task cancel + TTL cleanup (L4415–4493) is a legitimately hard area to cover from the outside, and the as unknown as { ... } private‑surface drive is the right call (the existing receiver‑task e2e tests don't exercise the cancel path). The test even pre‑attaches .catch(...) on payloadPromise to avoid an unhandled rejection, which is a subtle but correct detail.

Suggestions / Nits

1. Misleading describe + test name at inspectorClient.test.ts:4198–4226.
   The block is titled "listChanged notifications" and the test is "dispatches toolsListChanged when server advertises tools.listChanged and notifies", but the body never triggers a notification or asserts any dispatch — it only asserts client.getCapabilities() round-trips the listChanged/subscribe flags after connect. The real dispatch tests live in the next block ("list_changed + resourceUpdated notifications" at L4275). Either:

   • rename this to something like "capability detection: reports listChanged + subscribe flags after connect", or
   • fold its single getCapabilities() assertion into the connect‑path of one of the dispatch tests below and delete the block.

   The subscriptions: true + resources + prompts fixtures registered here are also unused by the assertions, which adds to the confusion. Fix this →

2. remote-transport.test.ts:1283 has a 250 ms sleep to let the transport notice the upstream close before asserting the send fails. The assertion is permissive (>= 500), so it won't flake on the status code, but the sleep is the only thing gating whether the dead‑transport short‑circuit vs. a downstream send error is exercised. Not a blocker — consistent with existing timing patterns in this file — but worth a note so future maintainers know it's intentional best‑effort coverage, not a precise assertion.

3. Contrived‑null test is honest about what it's doing. The nullify() helper at inspectorClient.test.ts:4501–4503 is a reasonable way to exercise the 16 if (!this.client) throw guards in one pass, and the preceding comment explains why. Reasonable tradeoff; just flagging that if any of those guards are removed in future cleanup (Audit per-file branch-coverage outliers and consider raising the 50% gate #1271 branches work or similar), this test will silently under‑cover those lines rather than fail — worth a // keep this in sync with defensive guards in inspectorClient.ts pointer so the link is explicit.

4. tokenAuthProvider.ts:42 retains the as unknown as OAuthClientProvider widening cast from the original helper. That's fine for now (the type in the SDK has narrower method signatures than the stubs), but it's the reason remote-tokenAuthProvider.test.ts:25–36 has to re‑narrow with its own explicit shape. If you wanted to drop one layer of cast gymnastics, you could type the returned object with satisfies once the SDK OAuthClientProvider type stabilizes — no action needed here, just context for anyone touching this file later.

5. remote-transport.test.ts:1302 comment ("/api/log accepts non-JSON body silently via the catch fallback") is a nice pointer, but it references an anonymous .catch(() => ({})) arrow rather than a named helper. If you'd prefer the coverage target to be more discoverable, extracting that arrow into a small safeJson helper in server.ts would give the test a named target. Not worth doing in this PR.

Things explicitly not an issue

• The authenticate()‑on‑stdio test at L4569–4596 correctly relies on getServerUrl() throwing before the OAuth env stubs are ever reached, so the {} as never casts for storage/navigation/redirectUrlProvider are safe.
• The receiver‑task test includes both the unknown‑id throws and the TTL‑cleanup setTimeout branch — good.
• The getCompletions { values: [] } short‑circuit is correctly expected to resolve (not throw) at L4563–4565.
• The vite.config.ts deletion drops the two TODO(#1310) exclusions without touching the inspectorClientEventTarget.ts TODO(v2 core/ hook layer: port v1.5 useInspectorClient + useManaged* + state machinery #1243), which is the right scope.

Verdict

LGTM modulo the naming issue in (1). The refactor is clean, the tests target the right gaps, and the stated coverage delta is realistic for the scope of changes.
• Branch: v2/issue-1310-coverage-backfill

[cliffhall]

Thanks for the review. Applied the three actionable nits in 2f7230d.

1. Misnamed listChanged notifications describe. Renamed to "capability detection after connect" and the test to "round-trips listChanged + subscribe flags via getCapabilities()" — the body never asserted on dispatch, only on getCapabilities() round-trip, which is what the new names say. Kept the resources / prompts fixtures with an inline note explaining they are required for the test server to actually emit those capability blocks (omitting them collapses the capability rather than advertising an empty one; dropping them in the first pass of this fix turned caps?.resources?.listChanged from true to undefined).

2. 250 ms sleep in the dead-transport test. Expanded the comment to call out that the >= 500 assertion is intentionally permissive across both the dead-transport short-circuit at /api/mcp/send and the downstream send-failure branch — either path covers a 5xx branch.

3. nullify() helper. Added a "keep this in sync" pointer above the helper noting that if a future cleanup removes any of the if (!this.client) throw guards in inspectorClient.ts (e.g. when the field becomes non-null Client), this test will silently under-cover rather than fail, and the matching call should be removed at the same time.

4 + 5 (the SDK type cast in tokenAuthProvider + the anonymous .catch(() => ({})) arrow in /api/log) — left as-is per the bot's note that they don't need action in this PR.

npm run validate and npm run test:storybook still pass.

[cliffhall]

@claude review

[claude]

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

---

Re-review of follow-up commit 2f7230d

• Check out the current branch state
• Review the fix commit (2f7230d) addressing prior feedback
• Re-verify the remaining files for any missed issues
• Post review feedback

---

Verdict: LGTM ✅

All three actionable nits from the prior review are addressed cleanly. No new concerns on the rest of the diff.

What was addressed

1. Renamed misleading describe / test at inspectorClient.test.ts:4198–4228. The block is now "capability detection after connect" with test "round-trips listChanged + subscribe flags via getCapabilities()", which matches what the body actually asserts. The resources / prompts fixtures are kept with a helpful inline note explaining that empty arrays collapse the capability rather than advertising it — that context is exactly what was missing before and makes the test's setup self-documenting.

2. 250 ms sleep comment at remote-transport.test.ts:1283–1287 now explicitly calls out that the >= 500 assertion is intentionally permissive across both the dead-transport short-circuit and the downstream send-failure branch. Future maintainers will not misread the sleep as a precise timing dependency.

3. nullify() helper pointer at inspectorClient.test.ts:4504–4508 adds a "keep this in sync" note linking back to the if (!this.client) throw sites in inspectorClient.ts, including the specific scenario (field becoming non-nullable Client) that would cause the test to silently under-cover. Exactly the right level of detail.

Items intentionally left as-is

The prior nits 4 (as unknown as OAuthClientProvider cast in tokenAuthProvider.ts) and 5 (anonymous .catch(() => ({})) in /api/log) were flagged in the previous review as "no action needed" — leaving them alone here was the right call.

Everything else in the PR

Spot-checked the unchanged parts of the PR (receiver-task private-surface drive, /api/storage corrupt-file GET, forwardLogEvent fallbacks, createTokenAuthProvider extraction): all still look correct and scoped tightly to the stated coverage gaps. Nothing new to flag.

Ship it.
• Branch: v2/issue-1310-coverage-backfill