test: share remote-server setup across response-shape describes (#1313)

[cliffhall]

Summary

• Closes Share remote-server setup across endpoint-error tests (#1307 follow-up) #1313.
• Hoist startRemoteServer(0) out of per-it() bodies into describe-scope beforeAll/afterAll for authentication, when dangerouslyOmitAuth is true, endpoint error paths, and Origin validation in clients/web/src/test/integration/mcp/remote/transport.test.ts. Each it() in these blocks only fires a single request and asserts response shape, so one shared server per describe is safe.
• Origin validation is split into two nested describes (with allowedOrigins configured / without allowedOrigins configured) because one test needs a server with different options.

Scope notes

• The issue example showed beforeEach, but the acceptance criterion "runtime drops measurably (~150ms)" only holds with beforeAll — beforeEach still spawns one server per test and saves no socket bind/close time. Using beforeAll makes the cleanup local (own afterAll) so the outer afterEach's if (remoteServer) check skips these servers naturally.
• storage and additional coverage paths describes are intentionally not changed:
  • storage: 6 of 13 tests read/write the same path /api/storage/test-store and would collide on a shared server. Refactoring would require unique storeIds per test, which is reorganizing the suite.
  • additional coverage paths: tests with no options are interspersed with tests using logger/storageDir/mcpHttpServer. Sharing would require splitting into option-grouped sub-describes — also a reorganization.
• Both align with the issue's explicit "Reorganizing remote-transport.test.ts more broadly. This is a targeted cleanup." out-of-scope note.

Result

• 23 socket bind/close ops eliminated (28 → 5 across the four refactored describes).
• npm run test:integration cumulative test time: 85.80s → 84.80s (≈1s saved; e2e tests dominate per-file numbers).
• All 331 integration tests still pass; npm run validate and npm run test:storybook both pass.

Test plan

• npm run test:integration passes
• npm run validate passes
• npm run test:storybook passes

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

Code Review

• Read the changed file in full
• Compare against origin/v2/main to understand the exact diff
• Check CLAUDE.md / AGENTS.md for relevant testing guidance
• Assess correctness, test isolation, and side effects of shared server setup
• Post the review

Summary

This is a clean, well-scoped cleanup that matches what the PR description promises. The four targeted describes (authentication, when dangerouslyOmitAuth is true, endpoint error paths, Origin validation) only fire one request per it() and assert response shape, so sharing a single remote server via beforeAll/afterAll is safe. The inline comments explaining the pattern (especially at transport.test.ts:392-395 and 1107-1108) are valuable for future readers. The decision to skip storage and additional coverage paths is well-reasoned and called out in the PR body.

Correctness

• Outer afterEach interaction (transport.test.ts:88-103): The shared-server describes intentionally don't assign to the outer remoteServer variable, so the if (remoteServer) check skips them and their own afterAll handles cleanup. The comment at line 392-395 documents this contract. Good.
• Test isolation within shared servers: I walked through each shared describe and confirmed no test creates server-side state another test could observe:
  • authentication: all tests short-circuit at 401 before any handler runs.
  • dangerouslyOmitAuth: /api/mcp/connect with url: "http://localhost:3000" may start an async transport attempt, but no test in the block reads sessions; /api/log and /api/storage GET are read/write-independent.
  • endpoint error paths: every test targets error branches — invalid JSON, missing fields, unknown session IDs. No session or storage writes.
  • Origin validation (configured): preflight + 403 paths create no sessions; the two 200-ish paths may start hanging upstream connections to http://localhost:3000, but no subsequent test depends on that state.
• Origin validation split (transport.test.ts:1105-1254): Splitting into with allowedOrigins configured and without allowedOrigins configured is the right move — different server options can't share a server. The "without" branch only has one test, so it doesn't save any spawns, but symmetric beforeAll/afterAll in both children is easier to read than mixing styles, and the cost of one extra describe is negligible.

Minor observations (non-blocking)

• sharedServer: ServerType; declared uninitialized (e.g. transport.test.ts:396): If beforeAll throws before assignment, afterAll runs sharedServer.close(...) on undefined and masks the real error. Vitest will still report the beforeAll failure, so in practice this is fine; a ServerType | undefined with an if (sharedServer) guard in afterAll would just give a marginally cleaner stack. Not worth changing unless you're touching the file for another reason.
• Duplicated cleanup shape: Each of the four shared describes repeats essentially the same afterAll promise-wrap of server.close(...). A small closeServer(server: ServerType) helper next to startRemoteServer would de-duplicate ~12 lines, but introducing it for four call sites is a judgment call — the current form keeps each block self-contained, which pairs well with the explanatory comments. Happy either way.
• // Missing "Bearer " prefix comment at transport.test.ts:450: Still accurate after the refactor — no action needed, just flagging I checked.

Verification

• PR body states npm run test:integration, npm run validate, and npm run test:storybook all pass. Per AGENTS.md this is the full gate for an integration-test change, so no additional checks are warranted from my side. The git diff --stat confirms the change is isolated to clients/web/src/test/integration/mcp/remote/transport.test.ts (+185/-164).

Verdict

Looks good to merge. The refactor does exactly what the linked issue asks for, within the scope the issue explicitly set. Non-refactored blocks (storage, additional coverage paths) have correct justifications.
• Branch: v2/1313-share-remote-server-setup