fix(#83): default MCP_SERVER_URL to dev Kapa AI endpoint#85
Conversation
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Thanks for picking this up @imtushar01! The ticket may have been misinterpreted slightly. Just defaulting the URL is not enough to get real Kapa AI results because the MCP integration in callKapaAI() is still a TODO stub that throws 'MCP integration not yet implemented'. So setting useMockMCP: false in research.ts will crash npm run research.
While working on the code generation layer (#86), we needed higher quality output from the research layer, so a full MCPClient was implemented there (src/mcp/client.ts) with real JSON-RPC calls, timeout handling, error handling, and response parsing. Since the research agent can be operated independently, I think the scope of this ticket can be extended to pull in those components so the research agent works end to end on main. If that feels like too much scope, the minimum requirement would be to wire up an actual call to the MCP server in callKapaAI() so that flipping useMockMCP: false doesn't crash.
Specifically from #86:
src/mcp/client.tsandsrc/mcp/index.ts(theMCPClientclass)- The related types in
src/types/index.ts(MCPClientConfig,MCPSearchDocsParams, etc.) - The updated
DocumentationSearchAgentconstructor andcallKapaAImethod that usesMCPClientinstead of the stub
The existing tests will also need updates:
- The
'should throw error when MCP is disabled and not implemented'test asserts the current throw behavior and would need to be replaced with a test that verifies a real (or mocked HTTP) call is made - New tests for the MCP client call (success, timeout, error responses, malformed responses)
- The
'cached' sourcetest may need updating depending on how the response source is reported after wiring up real calls
This would fully satisfy the acceptance criteria in #83 and reduce merge conflicts when #86 lands later.
| const supervisor = new ResearchSupervisor({ | ||
| modelName: 'claude-sonnet-4-20250514', | ||
| useMockMCP: true, // Using mocked MCP for now | ||
| useMockMCP: false, // Use real MCP by default |
There was a problem hiding this comment.
issue (blocking): This will crash because callKapaAI() throws when useMockMCP is false. The real MCP call needs to be wired up before flipping this flag.
| // Model will be used when MCP integration is complete | ||
| // For now, we use mocked responses | ||
| this.useMockMCP = options.useMockMCP !== false; // Default to true for POC | ||
| this.mcpServerUrl = options.mcpServerUrl ?? process.env.MCP_SERVER_URL ?? 'https://mcp-docs.dev.medicmobile.org/mcp'; |
There was a problem hiding this comment.
suggestion (non-blocking): Consider reading process.env.MCP_SERVER_URL in research.ts (where dotenv is loaded) and passing it via constructor options instead of reading it here. Keeps config resolution in one place.
| constructor(options: { modelName?: string; useMockMCP?: boolean; mcpServerUrl?: string } = {}) { | ||
| // Model will be used when MCP integration is complete | ||
| // For now, we use mocked responses | ||
| this.useMockMCP = options.useMockMCP !== false; // Default to true for POC |
There was a problem hiding this comment.
suggestion (non-blocking): options.useMockMCP === true would be clearer. Currently undefined silently becomes true, which means callers that don't set it get mocked responses without realizing.
| return this.mockKapaAIResponse(query, domain); | ||
| } | ||
|
|
||
| console.log(`[Documentation Search Agent] Calling MCP server: ${this.mcpServerUrl}`); |
There was a problem hiding this comment.
issue: mcpServerUrl is logged but never used in a request. The method throws right below. This needs a real MCP client call using the URL.
| return relatedDomains; | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
nitpick (non-blocking): Trailing newline was removed.
|
Thanks for the detailed review @sugat009! You're right I had misread the ticket scope. I'll extend this PR to wire up a real MCP call in I'll pull in the relevant pieces from #86:
For tests, I'll:
Will push an update shortly. |
|
Cool, thanks @imtushar01 ! |
2 participants
Description
Default MCP_SERVER_URL to https://mcp-docs.dev.medicmobile.org/mcp when not explicitly set.
Added console.log to display the MCP server URL being used when the documentation search agent makes a call.
#83
Code review checklist
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.