diff --git a/packages/cli/src/commands/mcp.mjs b/packages/cli/src/commands/mcp.mjs index ff27b6b..eb29cd3 100644 --- a/packages/cli/src/commands/mcp.mjs +++ b/packages/cli/src/commands/mcp.mjs @@ -355,7 +355,8 @@ function stdioSpecFromServer(target, server) { function listMcpTools(spec, timeoutMs) { return new Promise((resolve, reject) => { - const child = spawn(spec.command, spec.args || [], mcpSpawnOptions(spec.env)); + const childSpec = mcpSpawnCommand(spec); + const child = spawn(childSpec.command, childSpec.args, mcpSpawnOptions(spec.env)); let stderr = ""; let settled = false; let timer; @@ -447,10 +448,33 @@ export function mcpSpawnOptions(env = {}, os = platform()) { return { env: { ...process.env, ...(env || {}) }, stdio: ["pipe", "pipe", "pipe"], - shell: os === "win32", + shell: false, }; } +export function mcpSpawnCommand(spec, os = platform()) { + const args = spec.args || []; + if (os !== "win32" || isWindowsExecutable(spec.command)) { + return { command: spec.command, args }; + } + + return { + command: process.env.ComSpec || "cmd.exe", + args: ["/d", "/s", "/c", wrapWindowsCommandLine([spec.command, ...args])], + }; +} + +function isWindowsExecutable(command) { + return /\.(?:exe|com)$/i.test(command); +} + +function wrapWindowsCommandLine(parts) { + const commandLine = parts + .map((part) => shellQuoteForPlatform(part, "win32")) + .join(" "); + return `"${commandLine}"`; +} + function extractServer(target, config) { if (target === "cursor" || target === "claude-desktop") return config?.mcpServers?.qveris; if (target === "opencode") return config?.mcp?.qveris; diff --git a/packages/cli/test/mcp.test.mjs b/packages/cli/test/mcp.test.mjs index 0635edc..3fd7fd6 100644 --- a/packages/cli/test/mcp.test.mjs +++ b/packages/cli/test/mcp.test.mjs @@ -8,6 +8,7 @@ import test from "node:test"; import { extractEnvAssignmentValue, + mcpSpawnCommand, mcpSpawnOptions, resolveProbeTimeoutMs, shellQuoteForPlatform, @@ -58,9 +59,9 @@ test("mcp configure enforces owner-only permissions on existing config files", ( } }); -test("mcp live probe enables shell execution on Windows only", () => { +test("mcp live probe disables shell execution for spawn options", () => { const windowsOptions = mcpSpawnOptions({ QVERIS_API_KEY: "sk-test" }, "win32"); - assert.equal(windowsOptions.shell, true); + assert.equal(windowsOptions.shell, false); assert.equal(windowsOptions.env.QVERIS_API_KEY, "sk-test"); assert.deepEqual(windowsOptions.stdio, ["pipe", "pipe", "pipe"]); @@ -68,6 +69,37 @@ test("mcp live probe enables shell execution on Windows only", () => { assert.equal(posixOptions.shell, false); }); +test("mcp live probe runs Windows executables directly", () => { + const spec = mcpSpawnCommand( + { + command: "C:\\Program Files\\nodejs\\node.exe", + args: ["fake server.mjs", "--package=@qverisai/mcp"], + }, + "win32", + ); + + assert.equal(spec.command, "C:\\Program Files\\nodejs\\node.exe"); + assert.deepEqual(spec.args, ["fake server.mjs", "--package=@qverisai/mcp"]); +}); + +test("mcp live probe wraps Windows command shims through cmd.exe without shell option", () => { + const spec = mcpSpawnCommand( + { + command: "npx", + args: ["fake server.mjs", "--package=@qverisai/mcp"], + }, + "win32", + ); + + assert.equal(spec.command.endsWith("cmd.exe"), true); + assert.deepEqual(spec.args, [ + "/d", + "/s", + "/c", + "\"\"npx\" \"fake server.mjs\" \"--package=@qverisai/mcp\"\"", + ]); +}); + test("mcp command quoting follows the target shell platform", () => { assert.equal(shellQuoteForPlatform("L'Ondon", "darwin"), `'L'\\''Ondon'`); assert.equal(shellQuoteForPlatform('say "hi"', "win32"), `"say ""hi"""`); diff --git a/packages/mcp/src/index.test.ts b/packages/mcp/src/index.test.ts index 741ac87..a595f92 100644 --- a/packages/mcp/src/index.test.ts +++ b/packages/mcp/src/index.test.ts @@ -119,6 +119,16 @@ describe('MCP public tool interface', () => { 'call', { tool_id: 'weather.tool.v1' }, ); + const invalidCallParams = await executeQverisMcpTool( + client, + 'session-1', + 'call', + { + tool_id: 'weather.tool.v1', + search_id: 'search-1', + params_to_tool: '{"city":"London"}', + }, + ); const unknown = await executeQverisMcpTool(client, 'session-1', 'not_a_tool', {}); const apiError = await executeQverisMcpTool( client, @@ -131,6 +141,8 @@ describe('MCP public tool interface', () => { expect(payload(missingQuery).error).toContain('query'); expect(missingCallParams.isError).toBe(true); expect(payload(missingCallParams).error).toContain('search_id'); + expect(invalidCallParams.isError).toBe(true); + expect(payload(invalidCallParams).error).toContain('JSON object'); expect(unknown.isError).toBe(true); expect(payload(unknown).available_tools).toEqual([ 'discover', diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 47243e1..95534ff 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -232,7 +232,7 @@ export async function executeQverisMcpTool( const missingFields: string[] = []; if (!input.tool_id) missingFields.push('tool_id'); if (!input.search_id) missingFields.push('search_id'); - if (!input.params_to_tool) missingFields.push('params_to_tool'); + if (input.params_to_tool === undefined) missingFields.push('params_to_tool'); if (missingFields.length > 0) { return { diff --git a/packages/mcp/src/tools/execute.test.ts b/packages/mcp/src/tools/execute.test.ts index c56759c..50c9daa 100644 --- a/packages/mcp/src/tools/execute.test.ts +++ b/packages/mcp/src/tools/execute.test.ts @@ -53,7 +53,7 @@ describe('call (execute_tool)', () => { vi.restoreAllMocks(); }); - it('should parse JSON params and call client.executeTool', async () => { + it('should pass object params and call client.executeTool', async () => { const mockResponse: ExecuteResponse = { execution_id: 'exec-123', tool_id: 'weather-tool', @@ -67,13 +67,13 @@ describe('call (execute_tool)', () => { const result = await executeExecuteTool( mockClient, - { - tool_id: 'weather-tool', - search_id: 'search-123', - params_to_tool: '{"city": "Tokyo", "units": "metric"}', - }, - 'default-session' - ); + { + tool_id: 'weather-tool', + search_id: 'search-123', + params_to_tool: { city: 'Tokyo', units: 'metric' }, + }, + 'default-session' + ); expect(executeToolMock).toHaveBeenCalledWith('weather-tool', { search_id: 'search-123', @@ -96,13 +96,13 @@ describe('call (execute_tool)', () => { await executeExecuteTool( mockClient, - { - tool_id: 'tool-1', - search_id: 'search-123', - params_to_tool: '{}', - session_id: 'custom-session', - }, - 'default-session' + { + tool_id: 'tool-1', + search_id: 'search-123', + params_to_tool: {}, + session_id: 'custom-session', + }, + 'default-session' ); expect(executeToolMock).toHaveBeenCalledWith('tool-1', { @@ -124,13 +124,13 @@ describe('call (execute_tool)', () => { await executeExecuteTool( mockClient, - { - tool_id: 'tool-1', - search_id: 'search-123', - params_to_tool: '{}', - max_response_size: 102400, - }, - 'default-session' + { + tool_id: 'tool-1', + search_id: 'search-123', + params_to_tool: {}, + max_response_size: 102400, + }, + 'default-session' ); expect(executeToolMock).toHaveBeenCalledWith('tool-1', { @@ -141,35 +141,24 @@ describe('call (execute_tool)', () => { }); }); - it('should throw error for invalid JSON in params_to_tool', async () => { - await expect( - executeExecuteTool( - mockClient, - { - tool_id: 'tool-1', - search_id: 'search-123', - params_to_tool: 'not valid json', - }, - 'default-session' - ) - ).rejects.toThrow('Invalid JSON in params_to_tool'); - }); - - it('should include original params in JSON parse error message', async () => { - await expect( - executeExecuteTool( - mockClient, - { - tool_id: 'tool-1', - search_id: 'search-123', - params_to_tool: '{broken', - }, - 'default-session' - ) - ).rejects.toThrow('Received: {broken'); - }); - - it('should handle complex nested parameters', async () => { + it.each(['not valid json', null, ['city']])( + 'should reject non-object params_to_tool: %s', + async (paramsToTool) => { + await expect( + executeExecuteTool( + mockClient, + { + tool_id: 'tool-1', + search_id: 'search-123', + params_to_tool: paramsToTool as unknown as Record, + }, + 'default-session' + ) + ).rejects.toThrow('params_to_tool must be a JSON object'); + } + ); + + it('should handle complex nested parameters', async () => { const complexParams = { filters: { category: 'electronics', @@ -189,12 +178,12 @@ describe('call (execute_tool)', () => { await executeExecuteTool( mockClient, - { - tool_id: 'search-products', - search_id: 'search-123', - params_to_tool: JSON.stringify(complexParams), - }, - 'default-session' + { + tool_id: 'search-products', + search_id: 'search-123', + params_to_tool: complexParams, + }, + 'default-session' ); expect(executeToolMock).toHaveBeenCalledWith('search-products', { @@ -212,13 +201,13 @@ describe('call (execute_tool)', () => { await expect( executeExecuteTool( mockClient, - { - tool_id: 'tool-1', - search_id: 'search-123', - params_to_tool: '{}', - }, - 'session' - ) + { + tool_id: 'tool-1', + search_id: 'search-123', + params_to_tool: {}, + }, + 'session' + ) ).rejects.toEqual(error); }); }); diff --git a/packages/mcp/src/tools/execute.ts b/packages/mcp/src/tools/execute.ts index 83ab3b5..33f253c 100644 --- a/packages/mcp/src/tools/execute.ts +++ b/packages/mcp/src/tools/execute.ts @@ -26,15 +26,14 @@ export interface ExecuteToolInput { */ search_id: string; - /** - * Dictionary of parameters to pass to the remote tool. - * Keys are parameter names, values can be of any type. - * Can be provided as an object directly or as a JSON string (legacy). - * - * @example {"city": "London", "units": "metric"} - * @example {"query": "AI news", "limit": 10} - */ - params_to_tool: Record | string; + /** + * Dictionary of parameters to pass to the remote tool. + * Keys are parameter names, values can be of any type. + * + * @example {"city": "London", "units": "metric"} + * @example {"query": "AI news", "limit": 10} + */ + params_to_tool: Record; /** * Session identifier for tracking user sessions. @@ -103,37 +102,28 @@ export const executeToolSchema = { * @param input - Execution parameters * @param defaultSessionId - Fallback session ID if not provided in input * @returns Execution result from the tool - * @throws {Error} If params_to_tool is not valid JSON - */ -export async function executeExecuteTool( - client: QverisClient, - input: ExecuteToolInput, - defaultSessionId: string -): Promise { - // Resolve parameters: accept object directly or JSON string (legacy compat) - let parameters: Record; - if (typeof input.params_to_tool === 'string') { - try { - parameters = JSON.parse(input.params_to_tool) as Record; - } catch (error) { - throw new Error( - `Invalid JSON in params_to_tool: ${error instanceof Error ? error.message : 'Unknown parse error'}. ` + - `Received: ${input.params_to_tool}` - ); - } - } else if (typeof input.params_to_tool === 'object' && input.params_to_tool !== null) { - parameters = input.params_to_tool; - } else { - parameters = {}; - } - - const response = await client.executeTool(input.tool_id, { - search_id: input.search_id, - session_id: input.session_id ?? defaultSessionId, - parameters, - max_response_size: input.max_response_size, - }); - - return response; -} + * @throws {Error} If params_to_tool is not a JSON object + */ +export async function executeExecuteTool( + client: QverisClient, + input: ExecuteToolInput, + defaultSessionId: string +): Promise { + if (!isParamsObject(input.params_to_tool)) { + throw new Error('params_to_tool must be a JSON object.'); + } + + const response = await client.executeTool(input.tool_id, { + search_id: input.search_id, + session_id: input.session_id ?? defaultSessionId, + parameters: input.params_to_tool, + max_response_size: input.max_response_size, + }); + + return response; +} + +function isParamsObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +}