Bring back MCP endpoint, scoped to the two /ask tools#246
Merged
Conversation
Exposes search_all and get_full_document over Streamable HTTP at /mcp, reusing the same executeSearchAll / executeGetFullDocument helpers that back the /ask agent loop so both stay in sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for jam-search2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Model Context Protocol (MCP) endpoint: new MCP server and HTTP handler, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as MCP Handler
participant Transport as WebStandardStreamableHTTPServerTransport
participant Server as MCP Server
participant Tools as Tool Execution
participant DB as SearchDB
Client->>Handler: POST /mcp (initialize / call-tool)
activate Handler
Handler->>Transport: create transport (per-request)
Handler->>Server: createMcpServer(db, dataDir)
activate Server
Server->>Transport: server.connect(transport)
Handler->>Transport: transport.handleRequest(rawReq)
activate Transport
Transport->>Server: dispatch initialize or callTool
activate Server
alt callTool (search_all / get_full_document)
Server->>Tools: validate args (Zod) & execute
activate Tools
Tools->>DB: query or fetch document (fulltext or embedding-controlled)
DB-->>Tools: results/document
Tools-->>Server: formatted JSON/text result
deactivate Tools
else unknown tool / error
Server-->>Server: produce error response
end
Server-->>Transport: MCP response stream
deactivate Server
Transport-->>Handler: stream/response
Handler-->>Client: HTTP response (SSE / JSON-RPC)
deactivate Transport
deactivate Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/src/mcp/handler.ts (1)
72-87: Unreachable branch:existingTransportcheck aftertransports.has()is always truthy.Line 72 already verifies
transports.has(sessionId)before callingtransports.get(). The subsequentif (!existingTransport)check on line 74 is dead code sinceMap.get()will always return a defined value afterhas()returns true.♻️ Simplify by using non-null assertion or restructuring
if (sessionId && transports.has(sessionId)) { - const existingTransport = transports.get(sessionId); - if (!existingTransport) { - return c.json( - { - jsonrpc: "2.0", - error: { - code: -32000, - message: "Session not found", - }, - id: (body as { id?: unknown })?.id ?? null, - }, - 400 - ); - } - transport = existingTransport; + transport = transports.get(sessionId)!; } else if (!sessionId && isInitializeRequest(body)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/mcp/handler.ts` around lines 72 - 87, The null-check on existingTransport is unreachable because you already ensure transports.has(sessionId) is true; remove the redundant branch and either (a) drop the has() call and handle the undefined from transports.get(sessionId) (use the existing error response when get returns undefined), or (b) keep has(sessionId) and replace transports.get(sessionId) with a non-null assertion (e.g., existingTransport = transports.get(sessionId)!); update the code paths around transports, sessionId, and existingTransport in handler.ts accordingly so there is a single clear existence check.backend/src/index.ts (1)
71-71: Transport cleanup is fire-and-forget; may leave connections unclean.
cleanupMcpTransports()callstransport.close().catch(console.error)without awaiting. The server then closes immediately, potentially interrupting in-flight MCP sessions.Consider making cleanup async and awaiting transport closures before closing the HTTP server:
♻️ Suggested change to await cleanup
In
backend/src/mcp/handler.ts:-export function cleanupMcpTransports(): void { +export async function cleanupMcpTransports(): Promise<void> { + const closePromises: Promise<void>[] = []; for (const [sessionId, transport] of transports) { console.log(`Closing MCP transport for session ${sessionId}`); - transport.close().catch(console.error); + closePromises.push(transport.close().catch(console.error)); } + await Promise.all(closePromises); transports.clear(); }In
backend/src/index.ts:- cleanupMcpTransports(); + await cleanupMcpTransports();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/index.ts` at line 71, cleanupMcpTransports() currently invokes transport.close().catch(console.error) fire-and-forget, so make cleanupMcpTransports async and await all transport closures (e.g., replace per-transport .catch with collecting promises and await Promise.all(transports.map(t => t.close())) and handle errors appropriately), then in backend/src/index.ts await cleanupMcpTransports() before shutting down the HTTP server (i.e., await cleanupMcpTransports() prior to calling server.close()/process exit) so in-flight MCP sessions are allowed to finish.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/package.json`:
- Line 19: Update the incorrect MCP SDK version in package.json: replace the
dependency entry "@modelcontextprotocol/sdk": "^1.29.0" with the valid published
version (e.g. "@modelcontextprotocol/sdk": "^1.9.0") so installs succeed; locate
the dependency string in package.json and change the version specifier
accordingly or confirm and use the intended published version.
---
Nitpick comments:
In `@backend/src/index.ts`:
- Line 71: cleanupMcpTransports() currently invokes
transport.close().catch(console.error) fire-and-forget, so make
cleanupMcpTransports async and await all transport closures (e.g., replace
per-transport .catch with collecting promises and await
Promise.all(transports.map(t => t.close())) and handle errors appropriately),
then in backend/src/index.ts await cleanupMcpTransports() before shutting down
the HTTP server (i.e., await cleanupMcpTransports() prior to calling
server.close()/process exit) so in-flight MCP sessions are allowed to finish.
In `@backend/src/mcp/handler.ts`:
- Around line 72-87: The null-check on existingTransport is unreachable because
you already ensure transports.has(sessionId) is true; remove the redundant
branch and either (a) drop the has() call and handle the undefined from
transports.get(sessionId) (use the existing error response when get returns
undefined), or (b) keep has(sessionId) and replace transports.get(sessionId)
with a non-null assertion (e.g., existingTransport =
transports.get(sessionId)!); update the code paths around transports, sessionId,
and existingTransport in handler.ts accordingly so there is a single clear
existence check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4cd7cfb1-1cb6-4dfc-8b5d-f9052b211401
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
backend/package.jsonbackend/src/api.tsbackend/src/index.tsbackend/src/mcp/handler.tsbackend/src/mcp/server.ts
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Mark `limit` as `.default(10).optional()` so it drops out of the JSON-schema `required` list — strict MCP clients shouldn't have to pass a tunable. - Register /mcp route handlers directly instead of wrapping in arrow functions. - Add a header comment pointing mcp/server.ts readers at ask/tools.ts, where the actual search implementations live. - Add a vitest that wires Client + Server via InMemoryTransport and covers: tools/list names, the `limit` non-required invariant, search_all and get_full_document happy paths, and unknown-id isError on get_full_document. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/src/mcp/server.ts (1)
73-74: Use compact JSON for MCP tool payloads.Pretty-printing increases response size without functional benefit for tool consumers.
Payload-size optimization diff
- content: [{ type: "text", text: JSON.stringify(results, null, 2) }], + content: [{ type: "text", text: JSON.stringify(results) }], @@ - content: [{ type: "text", text: JSON.stringify(doc, null, 2) }], + content: [{ type: "text", text: JSON.stringify(doc) }],Also applies to: 88-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/mcp/server.ts` around lines 73 - 74, The payloads in backend/src/mcp/server.ts are using pretty-printed JSON (JSON.stringify(results, null, 2)) which increases response size; replace those calls with compact JSON serialization (use JSON.stringify(results) instead) wherever the content array is built (the entries producing content: [{ type: "text", text: JSON.stringify(results, null, 2) }] and the similar occurrence around lines 88-89) so MCP tool payloads send compact JSON; ensure both spots that create the content text field use the compact JSON form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/__tests__/mcp/server.test.ts`:
- Around line 52-57: The test currently assumes result.content[0] exists before
calling JSON.parse, which can throw a TypeError; update the test around the
result/content parsing (the const content = result.content ... and subsequent
JSON.parse lines) to first assert that result.content is an array and that
result.content[0] is defined and has type "text" (e.g.,
expect(Array.isArray(result.content)).toBe(true);
expect(result.content.length).toBeGreaterThan(0);
expect(result.content[0]).toBeDefined();) before parsing, then parse
result.content[0].text into parsed and continue with the existing assertions
about parsed and parsed[0].id.
- Around line 7-17: connectClient currently creates a server (createMcpServer)
and a client (new Client) with transports (InMemoryTransport.createLinkedPair)
but never returns or closes the server/client, leaking handles; modify
connectClient to return both { client, server, db } (or a cleanup function) and
ensure tests call await Promise.all([client.close(), server.close()]) in an
afterEach() hook (or invoke the returned cleanup) so transport resources are
deterministically torn down; note that `@modelcontextprotocol/sdk` v1.29.0 close
methods are idempotent so calling client.close and server.close is safe.
In `@backend/src/mcp/server.ts`:
- Around line 97-102: Avoid returning raw exception messages to MCP clients: in
the catch block that currently constructs `message` and returns `content: [{
type: "text", text: \`Error: ${message}\` }]` with `isError: true`, replace that
client-facing string with a generic, non-sensitive message (e.g., "An internal
error occurred") and move the detailed `message` (or the full `error`) into a
server-side log call (use the existing logger or `console.error`) so
implementation details are recorded but not sent to the client.
---
Nitpick comments:
In `@backend/src/mcp/server.ts`:
- Around line 73-74: The payloads in backend/src/mcp/server.ts are using
pretty-printed JSON (JSON.stringify(results, null, 2)) which increases response
size; replace those calls with compact JSON serialization (use
JSON.stringify(results) instead) wherever the content array is built (the
entries producing content: [{ type: "text", text: JSON.stringify(results, null,
2) }] and the similar occurrence around lines 88-89) so MCP tool payloads send
compact JSON; ensure both spots that create the content text field use the
compact JSON form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f9ca01f-f1bf-470d-9c8e-2e843fb32967
📒 Files selected for processing (3)
backend/src/__tests__/mcp/server.test.tsbackend/src/api.tsbackend/src/mcp/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/api.ts
- Stateless /mcp: fresh Server + Transport per request via createMcpHandler
factory. No session map, no module-level state, no GET/DELETE lifecycle.
Registered as app.all("/mcp", ...) so any method hits the transport.
- CORS scoped to non-MCP routes so /mcp sends no Access-Control-* headers.
- executeSearchAll gains a useEmbeddings option (default true). MCP passes
false so anonymous MCP traffic cannot spend the server's OpenAI quota.
- TOOL_SPECS in ask/tools.ts is now the single source of truth for both
the OpenAI tool-call format (TOOL_DEFINITIONS) and the MCP tools/list
shape (derived in mcp/server.ts). No more parallel schema definitions.
- Tests: handler integration test asserts stateless behaviour and
verifies /mcp has no CORS header while /health still does. ask/tools
test pins the TOOL_DEFINITIONS <- TOOL_SPECS derivation.
- README updated to describe the three design choices (stateless,
no CORS, no embeddings).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/src/mcp/server.ts (1)
88-93:⚠️ Potential issue | 🟠 MajorDo not expose raw exception messages to MCP clients.
Line 89-Line 92 currently echoes internal error text. Return a generic client-safe message and log details server-side instead.
Proposed hardening diff
} catch (error) { - const message = error instanceof Error ? error.message : String(error); + console.error("[mcp] tool execution failed", { tool: name, error }); return { - content: [{ type: "text", text: `Error: ${message}` }], + content: [{ type: "text", text: "Tool execution failed" }], isError: true, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/mcp/server.ts` around lines 88 - 93, The catch block returning error details to MCP clients exposes internal exception messages; modify the handler in backend/src/mcp/server.ts (the catch that builds the returned object with content/isError) to return a generic client-safe message like "An internal error occurred" instead of `Error: ${message}`, and move the original error logging to server-side (e.g., use processLogger.error or console.error to log the full `error` or `message` and stack) so internal details are recorded but not sent to the client; update the returned object created in that catch (the content array and isError flag) accordingly.
🧹 Nitpick comments (1)
backend/src/__tests__/mcp/handler.test.ts (1)
43-52: Also assert SSEContent-Typeon successful initialize.Body text checks are useful; adding a header assertion makes the contract tighter.
Suggested test hardening
expect(res.status).toBe(200); + expect(res.headers.get("content-type")).toContain("text/event-stream"); // CORS must not be advertised for server-to-server endpoint. expect(res.headers.get("access-control-allow-origin")).toBeNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/__tests__/mcp/handler.test.ts` around lines 43 - 52, The test in handler.test.ts currently verifies status and some headers but omits asserting the SSE Content-Type; update the test (after checking mcp-session-id) to assert that res.headers.get("content-type") indicates an SSE response (e.g., equals or matches "text/event-stream" including possible charset), so the successful initialize response explicitly guarantees SSE Content-Type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/src/mcp/server.ts`:
- Around line 88-93: The catch block returning error details to MCP clients
exposes internal exception messages; modify the handler in
backend/src/mcp/server.ts (the catch that builds the returned object with
content/isError) to return a generic client-safe message like "An internal error
occurred" instead of `Error: ${message}`, and move the original error logging to
server-side (e.g., use processLogger.error or console.error to log the full
`error` or `message` and stack) so internal details are recorded but not sent to
the client; update the returned object created in that catch (the content array
and isError flag) accordingly.
---
Nitpick comments:
In `@backend/src/__tests__/mcp/handler.test.ts`:
- Around line 43-52: The test in handler.test.ts currently verifies status and
some headers but omits asserting the SSE Content-Type; update the test (after
checking mcp-session-id) to assert that res.headers.get("content-type")
indicates an SSE response (e.g., equals or matches "text/event-stream" including
possible charset), so the successful initialize response explicitly guarantees
SSE Content-Type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60c01aa7-911c-4a4b-a57e-9053b528ef32
📒 Files selected for processing (7)
backend/README.mdbackend/src/__tests__/ask/tools.test.tsbackend/src/__tests__/mcp/handler.test.tsbackend/src/api.tsbackend/src/ask/tools.tsbackend/src/mcp/handler.tsbackend/src/mcp/server.ts
✅ Files skipped from review due to trivial changes (1)
- backend/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/api.ts
- backend/src/mcp/handler.ts
- MCP tool error handler now distinguishes caller-facing ZodError (safe to surface — describes the caller's own arguments) from unexpected errors (logged server-side, returned as a generic "Tool execution failed" to avoid leaking paths / DB / internal detail). - server.test.ts: add afterEach teardown that calls client.close() and server.close() so InMemoryTransport pairs don't accumulate across the test suite. SDK close() is idempotent. Addresses CodeRabbit review comments on #246. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST/GET/DELETE /mcpusing Streamable HTTP with session management./askagent uses:search_allandget_full_document. The MCP tool handlers call the sameexecuteSearchAll/executeGetFullDocumenthelpers frombackend/src/ask/tools.ts, so both surfaces stay in sync.@modelcontextprotocol/sdk@^1.29.0to the backend.The original MCP server (commit
ecac46b, removed 2026-04-17) exposed five per-source search tools. This version intentionally restricts the surface to the unified tool pair, matching the current/askagent.Test plan
npm run typecheckcleaninitializehandshake,tools/listreturns exactlysearch_all+get_full_document,tools/call search_allreturns real indexed results🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Change
Documentation