test: run v1.5-ported integration tests under node-env vitest project (#1307)

[cliffhall]

Summary

Closes #1307. Brings the 13 v1.5-ported integration tests (~10K LOC, excluded under #1302 because happy-dom can't host them) back online under a new integration vitest project, drops the broad coverage.exclude globs for v1.5-ported source, and fills coverage gaps across nearly every newly-measured file.

• Adds the integration vitest project: environment: 'node', 30s test/hook timeouts (matches v1.5's core/vitest.config.ts), no setupFiles. Runs the 13 files listed in integrationTests in clients/web/vite.config.ts.
• Resolves the SDK auth-module mock identity issue by pinning @modelcontextprotocol/sdk/client/auth.js to one canonical path and deduping the SDK (the v2 install hoists a second copy at the repo root). vi.mock(...) now hooks the same module the source imports, so discovery.test.ts, state-machine.test.ts, and inspectorClient-oauth-fetchFn.test.ts pass.
• Restores test-servers/ as a buildable package (matches v1.5): adds test-servers/tsconfig.json and a test-servers:build npm script. The Vite alias @modelcontextprotocol/inspector-test-server now points at test-servers/build/index.js so getTestMcpServerPath() returns a .js path Node can spawn directly.
• Wires test:integration and test:integration:watch scripts and folds the integration project into test:coverage so the per-file gate is enforced across both projects in one run. npm run validate and CI's existing test:coverage step now cover both. No new CI step needed.
• Drops coverage.exclude entries for v1.5-ported source (core/mcp/inspectorClient.ts, core/mcp/oauthManager.ts, core/mcp/fetchTracking.ts, core/mcp/messageTrackingTransport.ts, core/mcp/config.ts, core/mcp/node/**, core/mcp/remote/**, core/auth/**, core/storage/**, core/logging/**) and expands coverage.include to capture core/auth/**, core/storage/**, core/logging/**.
• Adds new tests filling coverage gaps in 13 files: file-storage.ts, core/auth/storage.ts, fetchTracking.ts, createRemoteLogger.ts, oauth-callback-server.ts, state-machine.ts, storage-node.ts, storage-browser.ts, browser/providers.ts, storage-remote.ts, createRemoteFetch.ts, remote/sessionStorage.ts, remoteClientTransport.ts, remote/node/server.ts (via new endpoint-error tests in remote-transport.test.ts), and node/config.ts (new 38-test file). All v1.5-ported source files in core/auth/**, core/storage/**, core/logging/** now meet the 90/85/80/50 per-file gate.

Two files keep narrow coverage.exclude entries marked TODO(#1310): inspectorClient.ts (73% lines despite 4005 LOC of integration tests; gaps in OAuth/subscription/error paths) and remote/node/server.ts (88% lines; gaps in transport-failure / 401-propagation paths and the private createTokenAuthProvider helper). Tracked in #1310.

Manual smoke flows from #1302 AC

All three are covered by passing integration tests in this PR (no separate manual sign-off needed):

• ✅ Stdio InspectorClient against real stdio MCP server — inspectorClient.test.ts "should connect to server" and the entire "Tool Methods" / "Resource Methods" suites against getTestMcpServerCommand().
• ✅ Streamable-HTTP InspectorClient — inspectorClient.test.ts HTTP transport tests + transport.test.ts against createTestServerHttp({ serverType: 'streamable-http' }).
• ✅ OAuth flow against a real provider — inspectorClient-oauth-e2e.test.ts (43 tests across SSE + streamable-HTTP × static/CIMD/DCR) against TestServerOAuth.

Test plan

• npm run validate (lint + format + build + unit + integration with coverage): 126 files / 1403 tests pass, per-file gate green.
• npm run test:integration standalone: 13 files / 298 tests pass.
• npm run test:storybook: 67 files / 298 tests pass.
• clients/web/vite.config.ts has no remaining TODO(#1302 follow-up) markers (the two remaining TODOs reference the new follow-up issue Backfill coverage for InspectorClient and remote-server core paths (#1307 follow-up) #1310).
• Follow-up issue Backfill coverage for InspectorClient and remote-server core paths (#1307 follow-up) #1310 filed for the two large files still under narrow per-file exclusions.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 24s —— View job

---

PR Review

Overall: this is a high-quality, well-scoped PR. The vitest-project refactor in clients/web/vite.config.ts is tight, the rationale for each decision (SDK dedupe, auth.js anchored alias, node-env inlining, repoRoot project root) is captured in comments, and every new test file targets a specific source file and exercises both happy and error paths deliberately. Coverage gates and CI wiring match the claim in the PR body.

Below is one concrete concern worth addressing before merge and a handful of smaller observations.

---

🟡 test-servers/tsconfig.json uses deprecated baseUrl — breaks test:integration / test:coverage / validate under TS ≥ 6

Reproduced locally: with TypeScript 6.0.3 in PATH (any future bump will hit this too), tsc -p ../../test-servers --noCheck emits:

error TS5101: Option 'baseUrl' is deprecated and will stop functioning in TypeScript 7.0.
  Specify compilerOption '"ignoreDeprecations": "6.0"' to silence this error.

tsc exits with code 2 — even though the build/ artifacts are still emitted. Because every integration-touching npm script short-circuits on &&:

"test:integration": "npm run test-servers:build && vitest run --project=integration"
"test:coverage":    "npm run test-servers:build && vitest run --project=unit --project=integration --coverage"
"validate":         "... && npm run test:coverage"

…the vitest command never runs once the repo's pinned TS (~5.9.3) crosses into 6.x. The current green runs only work because TS 5.9 treats this as a warning-only. Minimal fix in test-servers/tsconfig.json:

"compilerOptions": {
  ...
  "ignoreDeprecations": "6.0",
  ...
}

Alternatively, drop baseUrl and rewrite paths with project-relative values (the resolution targets are already relative). Fix this →

---

Smaller observations (non-blocking)

1. remoteClientTransport.test.ts:73 mutates private closed state via cast. (transport as unknown as { closed: boolean }).closed = true; covers the "Transport is closed" branch but bypasses the real path the branch guards. The transport_error test two blocks later already observes this transition end-to-end (it checks that onclose fires). If coverage of the send()-after-closed branch is the goal, consider driving it via the transport_error SSE event and then calling send() — that makes the test describe the actual bug it's preventing rather than the line it's hitting.

2. endpoint error paths in remote-transport.test.ts:964-1100 spins up a fresh hono server per it (10 tests). Each startRemoteServer(0) binds/closes a socket. Folding the common setup into a beforeEach at the describe scope (single server, each test just fires a request with the shared baseUrl/authToken) would trim ~150ms and make the intent clearer — none of these tests mutate server state, they just verify response-shape validation. Fine to land as-is and clean up later.

3. test-servers/tsconfig.json couples to clients/web/node_modules. typeRoots: ["../clients/web/node_modules/@types"] and the paths entries assume the top-level install layout. Given the monorepo is still coalescing this is OK, but worth revisiting if test-servers/ gains its own package.json.

4. createRemoteLogger.test.ts uses await new Promise(r => setTimeout(r, 0)) to settle the fire-and-forget fetch. Works because pino transmit is synchronous and the fetch resolves on the next microtask, but await Promise.resolve() or setImmediate would be a hair more deterministic under some runners. Not worth blocking on.

5. Developer trap in the integrationTests list. A new file in src/test/core/ that isn't added to integrationTests silently falls into the unit project and runs under happy-dom. There's a comment acknowledging this; consider a lightweight guard (e.g., include: ['src/test/core/**/*.test.ts'] on the integration project + exclude: integrationTests on unit inverted into a positive manifest) as a future cleanup.

---

What looks great

• Alias refactor (sharedAliases, nodeModulesAliases, projectResolve) is clean and the comments explain why each piece exists (SDK hoist, auth.js mock identity, bare-module resolution from repoRoot).
• Integration project config (environment: 'node', 30s timeouts, SDK inlining, no setupFiles) matches v1.5 and is justified in-line.
• Coverage-exclude carve-outs for inspectorClient.ts / remote/node/server.ts are quantified and tagged with TODO(#1310); the rest of core/auth/**, core/storage/**, core/logging/** are now measured.
• New tests systematically target error paths: fetchTracking clone/toString throws, remoteClientTransport SSE stream error vs transport_error vs JSON parse, sessionStorage 404 tolerance, config invalid-JSON/missing-mcpServers/multiple-servers, state-machine missing-clientInfo + terminal-state guard, remote-server endpoint validation.
• AGENTS.md, .gitignore, and project-structure docs are in sync with the test-servers/build/ move.

---

Todo list

• Read PR diff against origin/v2/main
• Review clients/web/vite.config.ts
• Review new/modified test files
• Review test-servers/tsconfig.json + reproduce locally
• Review package.json scripts + coverage wiring
• Review AGENTS.md + .gitignore updates
• Check coverage-exclude posture and TODO(Backfill coverage for InspectorClient and remote-server core paths (#1307 follow-up) #1310) markers
• Post review
  · Branch: v2/1307-integration-vitest-project

[cliffhall]

Thanks for the review! Addressed in 76a4122:

🟡 test-servers/tsconfig.json deprecated baseUrl — Fixed. Rather than masking with ignoreDeprecations, I dropped baseUrl / paths / typeRoots / declaration* entirely. They were only used for type resolution, and test-servers:build already runs with --noCheck, so they were inert at build time. The simplified tsconfig now emits cleanly on any TS version and the && chain stays sound under TS 6+. Verified: tsc -p ../../test-servers --noCheck exits 0, npm run test:integration and npm run validate still pass.

#1 — Mutating private closed via cast — Fixed. Reworked the test to drive closed=true through an actual transport_error SSE event and vi.waitFor(onclose) instead of casting through unknown. The test now describes the real path the if (this.closed) throw guard protects, not just the line that hits it.

The other observations are well-taken; tracking them as non-blocking notes:

#2 — endpoint error paths spinning up a fresh server per it — Agree, a shared beforeEach at describe scope would trim ~150ms and clarify intent (none of these mutate server state). Deferring to a follow-up.

#3 — test-servers/tsconfig.json coupling to clients/web/node_modules — Moot now that I dropped the path mappings, but the underlying concern stands: if test-servers/ ever gains its own package.json, the coupling reappears as a hoist assumption. Worth revisiting then.

#4 — setTimeout(0) vs Promise.resolve() / setImmediate in createRemoteLogger.test.ts — Noted. Going to leave as-is since pino transmit + fire-and-forget fetch is deterministic enough in practice, but happy to swap if it ever flakes.

#5 — Developer trap in integrationTests list — Good call. A positive manifest for unit (include: ['src/test/core/**/*.test.ts'] + flipping integration to be the override) would prevent silent misclassification. Will file a follow-up for the broader test-organization cleanup.

[cliffhall]

@claude review

[claude]

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

---

PR Review (follow-up pass — commit 76a41220)

Both blocking items from my previous review are addressed cleanly. Verified locally: tsc -p ../../test-servers --noCheck exits 0, npm run test:integration is 13/13 files & 298/298 tests green, npm run validate is 126/126 files & 1403/1403 tests green.

---

✅ test-servers/tsconfig.json — resolved

Rather than silencing TS5101 with ignoreDeprecations, cliffhall dropped baseUrl / paths / typeRoots / declaration* / strict / noUncheckedIndexedAccess entirely. Since test-servers:build runs with --noCheck, those type-checking options had no effect on emit. The resulting config is minimal and future-proof against TS 6+.

Minor note (non-blocking): if anyone later drops --noCheck to type-check test-servers/, they'd rediscover the module resolution coupling. Given the PR body already states the move is temporary until test-servers/ gets its own package.json, this is fine.

✅ remoteClientTransport.test.ts "send() after close" — resolved

The reworked test now drives the transport into closed=true via a real transport_error SSE event (data: {"type":"transport_error",...}) and synchronizes with vi.waitFor(() => onclose toHaveBeenCalled()). Cross-checked against remoteClientTransport.ts:238-249 — exactly the path the branch guards. _sessionId stays set because close() is never called, so the subsequent send() correctly hits if (this.closed) throw "Transport is closed" at line 277-279 rather than the !this._sessionId branch above it. No more as unknown as { closed: boolean } cast.

---

Non-blocking follow-ups (acknowledged by author)

• Shared server in remote-transport.test.ts endpoint-error block — deferred.
• test-servers/ node_modules coupling — moot now that paths are gone; revisit if test-servers/ gains its own package.json.
• setTimeout(0) in createRemoteLogger.test.ts — leaving as-is.
• Positive manifest for integration vs unit projects — good suggestion, filed as follow-up.

Verdict

LGTM. Ship it.

Todo list

• Fetch PR head and diff since previous review
• Review test-servers/tsconfig.json simplification
• Verify test-servers:build exits 0 and still emits usable artifacts
• Review the reworked remoteClientTransport.test.ts closed-path test
• Run npm run test:integration / npm run validate locally
• Post review

---

· Branch: v2/1307-integration-vitest-project