test(mcp): add test suite for mcp.ts handler and fix session header bug - #189
Conversation
|
Hi @niallroche @mttrbrts — this PR adds the first test |
|
Nice work on this @Satvik77777. A few thoughts: InMemoryTransport approach is the right call. Testing the MCP handler via supertest would have been a slog given the StreamableHTTP stream mechanics. Using Session-id header fix is a real bug. Validating Coordination note on follow-up tests: I have a planned follow-up PR that adds contract tests for the No line conflict with my open #155 or #184. LGTM from my side modulo the test-the-fix nit above. |
|
Thanks @JayDS22 — really like the thorough review. Pushed a regression test that locks in the session-id Good to know about your planned buildApiErrorMessage
That way your follow-up PR has a clean place to append |
github-actions
Bot
added
the
maintainer-engaged
|
This PR is stale because it has been open with no activity. Remove the stale label or comment to keep it active. Only items with maintainer engagement are auto-closed. |
|
Thanks for this @Satvik77777 in the interests of cleaning up multiple issues and getting a clean merge with other PRs that are in the process of being merged can I suggest as this PR bundles three distinct things: the test suite, the session cleanup (also in #194), and a standalone bug fix — the mcp-session-id response header never being set on new session initialization. That header fix is genuinely independent and valuable on its own. My suggested split: Extract the res.setHeader('mcp-session-id', newSessionId) change plus a focused test for it as a separate PR. That can merge immediately with no conflicts. |
|
Hi @Satvik77777, flagging Niall's suggestion in the merge plan thread: splitting the session-header fix (the 6-line change in If you're busy with #187 / #192 / #194 I'm happy to do the split off your branch (preserving authorship via One other thing: Either way works for me, just want to keep the merge train moving per Niall's plan. |
|
Thanks @niallroche @JayDS22 — driving the split
Will rebase this test suite onto both once #194 and #203 land |
65930b9 to
a216259
Compare
There was a problem hiding this comment.
Pull request overview
Adds initial automated coverage for the MCP handler and fixes MCP Streamable HTTP initialization to return an mcp-session-id header so clients can persist the session.
Changes:
- Export
getServer()fromserver/handlers/mcp.tsto enable direct, transport-independent testing of MCP tool/resource registration. - Fix Streamable HTTP initialization to send
mcp-session-idback to clients. - Add a new Jest test suite for MCP HTTP boundary behavior and in-memory tool execution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
server/handlers/mcp.ts |
Exports getServer() and returns mcp-session-id on initialization responses. |
server/handlers/mcp.test.ts |
Introduces tests for MCP HTTP boundary validation and tool/resource registration via InMemoryTransport. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import request from 'supertest'; | ||
| import express from 'express'; | ||
| import { jest } from '@jest/globals'; | ||
| import * as crypto from 'crypto'; | ||
|
|
| app = express(); | ||
| app.use(express.json()); | ||
| app.use('/mcp', mcpRouter); | ||
| }); |
| const response = await request(app) | ||
| .post('/mcp/mcp') | ||
| .send({ jsonrpc: '2.0', method: 'tools/list', id: 1 }) | ||
| .expect(400); |
| it('returns mcp-session-id header on initialization response', async () => { | ||
| const res = await request(app) | ||
| .post('/mcp/mcp') | ||
| .send({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| method: 'initialize', | ||
| params: { | ||
| protocolVersion: '2024-11-05', | ||
| capabilities: {}, | ||
| clientInfo: { name: 'test', version: '1.0' } | ||
| } | ||
| }); | ||
| // The mcp-session-id header must be present on init response | ||
| // This locks in the bug fix against future regression | ||
| expect(res.headers['mcp-session-id']).toBeDefined(); | ||
| }); |
| it('GET /mcp/sse returns an SSE stream with the correct content type', async () => { | ||
| const response = await request(app) | ||
| .get('/mcp/sse') | ||
| .buffer(false) |
| it('POST /mcp/messages returns 400 when sessionId query param is missing or invalid', async () => { | ||
| const response = await request(app) | ||
| .post('/mcp/messages') | ||
| .query({ sessionId: 'nonexistent' }) | ||
| .send({ jsonrpc: '2.0', method: 'ping', id: 1 }) | ||
| .expect(400); |
|
@Satvik77777 apologies for the wait on this one. My Your #194 and #203 look clean on a quick pass and should be able to move independently on their own review cycles. Left a peer note on #203 with a couple of small suggestions. |
|
@Satvik77777 quick heads-up: #199 and #202 both landed today, so main has moved past what your branch was rebased against. #189 should be able to rebase cleanly against post-#199 main now (the conflicts should localize to the imports block and the mcp.test.ts describe blocks, similar to what I hit when I rebased #199 and just did on #201). Ping me if you want to pair on it, otherwise I will keep an eye on it and offer feedback once you have a fresh push. |
8caefcb to
ea98591
Compare
|
@JayDS22 — rebased #189 against latest main (post #199/#202). Conflicts were localized to mcp.test.ts as you predicted — combined both test suites (upstream's error-handling/typed-error/Concerto tests + this PR's HTTP boundary/InMemoryTransport tests). Also dropped the old 9342f8c commit entirely (both its test and its mcp.ts hunk), since that session-header fix now lives solely in #203 to avoid duplicate logic. All 23 tests passing. Ready whenever you get a chance to take a look. |
|
can you resolve the test and I will merge @Satvik77777 |
ea98591 to
8f04f85
Compare
|
Hey @Satvik77777 — quick steer on why CI is red, since it's not what it looks like. The failure isn't the Copilot mount-path comment — you already fixed that. The test now mounts at / and hits /mcp / /sse / /messages directly, which is the correct production surface (index.ts mounts the router at /, and the /mcp prefix comes from the route declaration in mcp.ts). That part's good. The actual failure is a syntax error in mcp.test.ts — tsc reports handlers/mcp.test.ts(497,1): error TS1005: '}' expected. Line 497 is EOF, so there are unclosed blocks: the brace/paren balance is short by two }), i.e. two describe blocks are missing their closers. tsc fails to compile, so CI dies in ~40s before any test runs. Looks like it slipped in when the mcp.test.ts conflict got resolved during the rebase — you and #203 both rewrite that file heavily, so it's an easy one to drop. Adding the two missing }) before EOF should turn it green. Two things while you're in there: This branch still conflicts with #203 in mcp.test.ts, and #189 depends on #203 landing first anyway — so once the syntax fix is in, please also rebase onto current main after #203 merges, not before, to avoid a second round. |
8f04f85 to
92ff5cd
Compare
|
@niallroche — thanks for catching that, matches exactly what I found independently (two missing closing braces in mcp.test.ts from the rebase conflict resolution). Fixed and pushed (92ff5cd) — all 14 checks passing now. Holding off on rebasing onto current main again until #203 merges, per your note, to avoid a second conflict round. |
|
this should be ready if you rebase @Satvik77777 |
Resolves test coverage gap for the new MCP handler. - Extracted getServer() for testability. - Wrote tests for HTTP boundary error cases (400 responses). - Wrote internal tests using InMemoryTransport to verify getTemplate, getAgreement, and trigger-agreement tools directly without battling strict StreamableHTTPServerTransport payload requirements. Signed-off-by: Satvik77777 <satviksaini02@gmail.com>
92ff5cd to
a8e2cd9
Compare
|
@niallroche — rebased onto current main (post-#216/#214 service-layer slices), force-pushed (same single commit rebased, 92ff5cd → a8e2cd9). The getServer conflict needed both the export keyword (for testability) and the new db: Database parameter (from the service-layer work) — merged both in. Test file combined cleanly: kept the session-header test plus this PR's HTTP boundary/internal logic suites, using a createMockDb() factory (same pattern already established in templateService.test.ts) wired via res.locals.db middleware. Clean tsc --noEmit, 148/148 tests passing. Should be ready for merge. |
🎉 Thank you for your contribution! 🎉Dear @Satvik77777, Your pull request has been successfully merged into the project! We greatly appreciate your efforts and the time you've dedicated to improving our repository. What happens next?
Once again, thank you for being part of our community! Best regards, |
Summary
Adds the first test suite for mcp.ts which previously had 0% test
coverage. The session-header bug fix and session cleanup discovered
during testing have been split into separate PRs (#203 and #194) per
@niallroche's suggestion.
What this PR does
Refactor — export getServer() for testability
tested without fighting the StreamableHTTPServerTransport stream
mechanics
Tests (7 new, 60 total passing)
HTTP stream mechanics entirely
Why InMemoryTransport over supertest
StreamableHTTPServerTransport requires specific Accept headers and
stream mechanics that supertest cannot emulate. Using InMemoryTransport
from the official @modelcontextprotocol/sdk tests the actual tool logic
with 100% confidence.
References
landing first
Author Checklist