diff --git a/bin/cli.js b/bin/cli.js index 06a5cd1..5f9e8c8 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -544,7 +544,11 @@ switch (command) { case 'mcp-serve': { // Start the tool-retrieval MCP stdio server. // Launched by opencode via: "command": ["opencode-workspace", "mcp-serve"] - require('../src/mcp/tool-retrieval-server.js'); + const { startServer } = require('../src/mcp/tool-retrieval-server.js'); + startServer().catch(err => { + process.stderr.write(`tool-retrieval-server: fatal error: ${err.message}\n`); + process.exit(1); + }); break; } diff --git a/docs/tool-retrieval-mcp.feature b/docs/tool-retrieval-mcp.feature index b775bda..6d05399 100644 --- a/docs/tool-retrieval-mcp.feature +++ b/docs/tool-retrieval-mcp.feature @@ -57,3 +57,22 @@ Feature: On-Demand Tool Retrieval MCP Tool And an opencode session is active with no specific browser tools in context When the agent calls search_tools with query "click a button in a web page" Then at least one tool from the "playwright" server appears in the results + + # ── Server wiring tests (the gap that let the startup crash go undetected) ── + + Scenario: The MCP server wires request handlers with Zod schemas not plain objects + When the MCP server is configured with a mock SDK + Then setRequestHandler was called twice + And the list-tools handler schema is a valid Zod schema + And the call-tool handler schema is a valid Zod schema + + Scenario: list-tools returns the search_tools manifest over the wire protocol + When the MCP server handles a list-tools request via in-memory transport + Then the response contains a tool named "search_tools" + And the search_tools tool declares a required "query" input parameter + + Scenario: call-tool returns a valid CallToolResult envelope over the wire protocol + Given the tool corpus has not been built + When the MCP server handles a call-tool request for "search_tools" via in-memory transport + Then the response is a valid CallToolResult with a content array + And the content text is a non-empty string diff --git a/src/mcp/tool-retrieval-server.js b/src/mcp/tool-retrieval-server.js index 66edd08..5d64df9 100644 --- a/src/mcp/tool-retrieval-server.js +++ b/src/mcp/tool-retrieval-server.js @@ -22,14 +22,33 @@ * 2. Complements the TUI first-message hook (lib/tool-retrieval.plugin.js) * by giving the agent on-demand access to the retrieval pipeline at any * point in the conversation. + * + * Exports: + * createMcpServer(sdk) — builds and returns a configured Server instance. + * Accepts SDK dependencies explicitly so it can be + * unit-tested with a mock Server (no stdio transport). + * startServer() — loads the real SDK, calls createMcpServer, attaches + * a StdioServerTransport, and connects. Called by + * bin/cli.js for the mcp-serve command. */ const { handleSearchTools } = require('./search-tools-handler'); -async function startServer() { - const { Server } = await import('@modelcontextprotocol/sdk/server/index.js'); - const { StdioServerTransport } = await import('@modelcontextprotocol/sdk/server/stdio.js'); +// ── server factory ──────────────────────────────────────────────────────────── +/** + * Create and configure the MCP Server instance. + * + * Accepts the SDK constructors/schemas as explicit parameters so this function + * can be unit-tested without spawning a real stdio transport. + * + * @param {object} sdk + * @param {Function} sdk.Server - Server constructor + * @param {object} sdk.ListToolsRequestSchema - Zod schema for tools/list + * @param {object} sdk.CallToolRequestSchema - Zod schema for tools/call + * @returns {object} configured MCP Server instance + */ +function createMcpServer({ Server, ListToolsRequestSchema, CallToolRequestSchema }) { const server = new Server( { name: 'tool-retrieval', version: '1.0.0' }, { capabilities: { tools: {} } }, @@ -37,7 +56,7 @@ async function startServer() { // ── tool list ───────────────────────────────────────────────────────────── server.setRequestHandler( - { method: 'tools/list' }, + ListToolsRequestSchema, async () => ({ tools: [ { @@ -71,7 +90,7 @@ async function startServer() { // ── tool call ───────────────────────────────────────────────────────────── server.setRequestHandler( - { method: 'tools/call' }, + CallToolRequestSchema, async (request) => { const { name, arguments: args } = request.params; @@ -86,7 +105,26 @@ async function startServer() { }, ); - // ── transport ───────────────────────────────────────────────────────────── + return server; +} + +// ── transport entry point ───────────────────────────────────────────────────── + +/** + * Load the real MCP SDK, build the server via createMcpServer(), attach a + * StdioServerTransport, and connect. + * + * Called explicitly by bin/cli.js for the `mcp-serve` command — NOT invoked + * automatically at module load time so that unit tests can safely require() + * this module and call createMcpServer() without triggering stdio binding. + */ +async function startServer() { + const { Server } = await import('@modelcontextprotocol/sdk/server/index.js'); + const { StdioServerTransport } = await import('@modelcontextprotocol/sdk/server/stdio.js'); + const { ListToolsRequestSchema, CallToolRequestSchema } = + await import('@modelcontextprotocol/sdk/types.js'); + + const server = createMcpServer({ Server, ListToolsRequestSchema, CallToolRequestSchema }); const transport = new StdioServerTransport(); await server.connect(transport); @@ -94,7 +132,4 @@ async function startServer() { process.on('SIGINT', () => server.close()); } -startServer().catch(err => { - process.stderr.write(`tool-retrieval-server: fatal error: ${err.message}\n`); - process.exit(1); -}); +module.exports = { createMcpServer, startServer }; diff --git a/unit-tests/step-definitions/tool-retrieval-mcp.steps.js b/unit-tests/step-definitions/tool-retrieval-mcp.steps.js index dbfa4f7..cd42a1d 100644 --- a/unit-tests/step-definitions/tool-retrieval-mcp.steps.js +++ b/unit-tests/step-definitions/tool-retrieval-mcp.steps.js @@ -172,3 +172,98 @@ Then('at least one tool from the {string} server appears in the results', functi `Expected a tool from "${serverName}" in results.\nActual:\n${text}`, ); }); + +// ─── Server-wiring When ──────────────────────────────────────────────────────── + +When('the MCP server is configured with a mock SDK', async function () { + await this.runStartServer(); +}); + +When('the MCP server handles a list-tools request via in-memory transport', async function () { + await this.runWireListTools(); +}); + +When('the MCP server handles a call-tool request for {string} via in-memory transport', + async function (toolName) { + await this.runWireCallTool(toolName, { query: 'test query' }); + }, +); + +// ─── Server-wiring Then ──────────────────────────────────────────────────────── + +Then('setRequestHandler was called twice', function () { + assert.equal( + this._serverCalls?.length, + 2, + `Expected setRequestHandler to be called exactly 2 times, got: ${this._serverCalls?.length}`, + ); +}); + +Then('the list-tools handler schema is a valid Zod schema', function () { + const schema = this._serverCalls?.[0]?.schema; + assert.ok( + typeof schema?.parse === 'function', + 'Expected list-tools schema to be a Zod schema with a .parse() method; ' + + 'got a plain object — this is the bug: setRequestHandler needs a Zod schema, not { method: "..." }', + ); + const result = schema.safeParse({ method: 'tools/list', params: {} }); + assert.ok( + result.success, + `Expected list-tools schema to accept a { method: "tools/list" } request.\n` + + `safeParse error: ${JSON.stringify(result.error)}`, + ); +}); + +Then('the call-tool handler schema is a valid Zod schema', function () { + const schema = this._serverCalls?.[1]?.schema; + assert.ok( + typeof schema?.parse === 'function', + 'Expected call-tool schema to be a Zod schema with a .parse() method; ' + + 'got a plain object — this is the bug: setRequestHandler needs a Zod schema, not { method: "..." }', + ); + const result = schema.safeParse({ method: 'tools/call', params: { name: 'search_tools', arguments: {} } }); + assert.ok( + result.success, + `Expected call-tool schema to accept a tools/call request.\n` + + `safeParse error: ${JSON.stringify(result.error)}`, + ); +}); + +Then('the response contains a tool named {string}', function (toolName) { + const tools = this._wireToolsList?.tools ?? []; + const found = tools.find(t => t.name === toolName); + assert.ok( + found, + `Expected a tool named "${toolName}" in the tools/list response.\n` + + `Got: ${tools.map(t => t.name).join(', ') || '(empty)'}`, + ); +}); + +Then('the search_tools tool declares a required {string} input parameter', function (paramName) { + const tools = this._wireToolsList?.tools ?? []; + const tool = tools.find(t => t.name === 'search_tools'); + assert.ok(tool, 'Expected search_tools to be present in tools/list response'); + const required = tool.inputSchema?.required ?? []; + assert.ok( + required.includes(paramName), + `Expected "${paramName}" to be in inputSchema.required.\nGot: ${JSON.stringify(required)}`, + ); +}); + +Then('the response is a valid CallToolResult with a content array', function () { + const result = this._wireCallResult; + assert.ok(result, 'Expected _wireCallResult to be set (tools/call produced no response)'); + assert.ok( + Array.isArray(result.content), + `Expected result.content to be an array.\nGot: ${JSON.stringify(result)}`, + ); + assert.ok(result.content.length > 0, 'Expected result.content to be non-empty'); +}); + +Then('the content text is a non-empty string', function () { + const text = this._wireCallResult?.content?.[0]?.text; + assert.ok( + typeof text === 'string' && text.trim().length > 0, + `Expected content[0].text to be a non-empty string.\nGot: ${JSON.stringify(text)}`, + ); +}); diff --git a/unit-tests/support/world.js b/unit-tests/support/world.js index a5d6c0e..f05e775 100644 --- a/unit-tests/support/world.js +++ b/unit-tests/support/world.js @@ -38,6 +38,9 @@ class OWWorld extends World { this._hookHandler = null; // reusable handler from createFirstMessageHandler this._hookLastQuery = null; // last text passed to _searchFn in runHook this._searchToolsResult = null; // result from runSearchTools + this._serverCalls = null; // setRequestHandler calls captured by runStartServer + this._wireToolsList = null; // listTools() result from runWireListTools + this._wireCallResult = null; // callTool() result from runWireCallTool } // ── helpers ───────────────────────────────────────────────────────────────── @@ -298,7 +301,96 @@ class OWWorld extends World { const args = { query, ...(opts.k !== undefined ? { k: opts.k } : {}) }; this._searchToolsResult = await handleSearchTools(args, { _searchFn: realSearch }); } -} + + /** + * Invoke createMcpServer() with a mock SDK to validate request-handler + * registration without connecting a real stdio transport. + * + * The mock Server records every setRequestHandler() call so that Then steps + * can assert that proper Zod schemas — not plain objects — were passed. + * Captures results in this._serverCalls = [{ schema, handler }, ...]. + */ + async runStartServer() { + const { ListToolsRequestSchema, CallToolRequestSchema } = + await import('@modelcontextprotocol/sdk/types.js'); + + const calls = []; + + class MockServer { + constructor() {} + setRequestHandler(schema, handler) { + calls.push({ schema, handler }); + } + } + + const { createMcpServer } = require('../../src/mcp/tool-retrieval-server'); + createMcpServer({ Server: MockServer, ListToolsRequestSchema, CallToolRequestSchema }); + + this._serverCalls = calls; + } + + /** + * Run a full wire-protocol tools/list round-trip using InMemoryTransport. + * + * Creates a real MCP Server (via createMcpServer) and a real Client, links + * them in-process, then calls client.listTools(). No stdio, no subprocess. + * Result stored in this._wireToolsList. + */ + async runWireListTools() { + const { Server } = await import('@modelcontextprotocol/sdk/server/index.js'); + const { Client } = await import('@modelcontextprotocol/sdk/client/index.js'); + const { InMemoryTransport } = await import('@modelcontextprotocol/sdk/inMemory.js'); + const { ListToolsRequestSchema, CallToolRequestSchema } = + await import('@modelcontextprotocol/sdk/types.js'); + + const { createMcpServer } = require('../../src/mcp/tool-retrieval-server'); + const server = createMcpServer({ Server, ListToolsRequestSchema, CallToolRequestSchema }); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + + const client = new Client({ name: 'test-client', version: '1.0.0' }); + await client.connect(clientTransport); + try { + this._wireToolsList = await client.listTools(); + } finally { + await client.close(); + } + } + + /** + * Run a full wire-protocol tools/call round-trip using InMemoryTransport. + * + * Intentionally uses an empty corpus so handleSearchTools() returns its + * graceful "corpus is empty" response without invoking the embedder. + * This makes the test self-contained and fast. + * Result stored in this._wireCallResult. + * + * @param {string} toolName - tool to call (e.g. 'search_tools') + * @param {object} args - arguments to pass to the tool + */ + async runWireCallTool(toolName, args) { + const { Server } = await import('@modelcontextprotocol/sdk/server/index.js'); + const { Client } = await import('@modelcontextprotocol/sdk/client/index.js'); + const { InMemoryTransport } = await import('@modelcontextprotocol/sdk/inMemory.js'); + const { ListToolsRequestSchema, CallToolRequestSchema } = + await import('@modelcontextprotocol/sdk/types.js'); + + const { createMcpServer } = require('../../src/mcp/tool-retrieval-server'); + const server = createMcpServer({ Server, ListToolsRequestSchema, CallToolRequestSchema }); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + + const client = new Client({ name: 'test-client', version: '1.0.0' }); + await client.connect(clientTransport); + try { + this._wireCallResult = await client.callTool({ name: toolName, arguments: args }); + } finally { + await client.close(); + } + } +} // end OWWorld // ── Expose ExitError as a global so step files can catch it ──────────────────