From fd57eaaa456e02fba3b5a4e1cd0ac63ba46d4b03 Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Fri, 30 Oct 2026 14:16:43 +0000 Subject: [PATCH] fix(mcp): a tools/call whose arguments aren't an object gets invalid params, not a crash (#3071) A tools/call request passes its arguments as an object. MCPSession took params.arguments || {} without checking that, so a string or number went straight to the tool handler, where the session-view check ('_cgExploreSession' in args) threw. The call then came back as a tool result flagged isError: "This is an internal codegraph error, retry the call once; if it persists, continue without codegraph". That reports a client's malformed request as codegraph's own malfunction, with advice to stop using it. An array went through as if it were the arguments. The stdio proxy, serving in process while the daemon is unavailable, had the same unchecked read. Both now read the arguments through one helper, toolCallArguments(), and answer anything but an object with a JSON-RPC invalid-params error (-32602, "Tool arguments must be an object"), as a call with no tool name already is; no tool runs, and the proxy starts no engine for it. Missing, null or another empty value still means no arguments. Validation: new mcp-tool-call-arguments.test.ts covers the session (the direct stdio server and every daemon client) and the real proxy entry point in a child process with the daemon unavailable: red on main (2 of 3 failed; the well-formed control passed), green with the fix. explore-session-state and proxy-shutdown pass. Driving serve --mcp over stdio: on main, codegraph_explore, codegraph_node and codegraph_search with a string argument returned isError "internal codegraph error" and codegraph_status accepted [1], 42 and true; with the fix those six get -32602, while "", null, {} and absent arguments run as before. Reimplements PR #2986 by @kjgggggg. Fixes #2981 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + __tests__/mcp-tool-call-arguments.test.ts | 142 ++++++++++++++++++++++ src/mcp/proxy.ts | 11 +- src/mcp/session.ts | 23 +++- 4 files changed, 173 insertions(+), 4 deletions(-) create mode 100644 __tests__/mcp-tool-call-arguments.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 06cc4570c..94ace836b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - An index left incomplete by an indexing run that was stopped partway, by a crash, an out-of-memory kill or the server's own hang check, no longer gets the background server killed with "Main thread unresponsive" every time it starts: on a large project, restoring the database indexes the run had set aside took longer than the hang check allows, so each restart died at the same point and the index was never repaired. They are now restored in the background, and the project is served once they are back. Thanks @cbeaulieu-gt for the report and the first fix. (#1887) - A new agent session no longer waits up to about 12 minutes for its first answer when the background server is stuck: a server whose process still holds its connection open but never responds made the session wait out a long series of retries. The session now gives up within seconds and answers on its own, and switches back to the background server once that has been restarted. - An MCP session whose agent stops reading its answers, while keeping the session open, now shuts down the normal way the next time it answers: it stops its work, frees the project for the next session and exits cleanly, and other sessions sharing the background server carry on. Before, that answer crashed it with "Uncaught exception: Error: EPIPE". Thanks @iiijiashu for the report and the first fix. (#2991) +- A tool call whose arguments aren't an object, like `"arguments": "alpha"` from a hand-written MCP client or script, now gets the standard invalid-params error saying the arguments must be an object, and no tool runs. Before, a string or a number made most tools fail and come back as an internal CodeGraph error that told the agent to retry and then to continue without CodeGraph, and an array was run as if it were the arguments. Thanks @inth3shadows for the report and @kjgggggg for the first fix. (#2981) - The bundled Node runtime moves to 24.22.1, Node's October security release, and every release now checks the Node it bundles against the checksums nodejs.org publishes. The new runtime also fixes a crash on Windows as a command like `codegraph sync` exits, after its work is done, printing `Assertion failed: !(handle->flags & UV_HANDLE_CLOSING)`, which made a script or CI run that had succeeded fail. Thanks @Ancienttwo for the report and the first fix, and @IdealBoy709 for the report on the security release. (#2617, #2780) - Building CodeGraph from source with bun, as in `bun run build`, no longer runs until it is stopped: bun ran one step of the build as the whole build again, so it kept starting itself over. Building with npm is unchanged. Thanks @bompus. - When CodeGraph can't open a project's database, it now says what went wrong: SQLite's error, the file's path, and what to check, such as another program holding the file open, a folder you can't write to, or a network share. Before, every such failure read "CodeGraph requires node:sqlite (Node.js 22.5+)", and an agent was told no project was loaded. That hint now appears only when Node really can't load its built-in SQLite, a case that used to end in an unhandled error instead. diff --git a/__tests__/mcp-tool-call-arguments.test.ts b/__tests__/mcp-tool-call-arguments.test.ts new file mode 100644 index 000000000..4ae753a7e --- /dev/null +++ b/__tests__/mcp-tool-call-arguments.test.ts @@ -0,0 +1,142 @@ +/** + * A `tools/call` whose `arguments` is not an object is a malformed request, + * answered with a JSON-RPC invalid-params error (-32602) like a call that + * names no tool, and never run (#2981). + * + * Before, the session handed a string or number straight to the tool handler, + * where `'_cgExploreSession' in args` threw, and the call came back as a tool + * result flagged isError saying "This is an internal codegraph error — retry + * the call once; if it persists, continue without codegraph": a client's bug + * reported as codegraph's own, with advice to stop using it. An array went + * through as if it were the arguments. No arguments, or null, still mean none. + * + * Both ways a call is served are covered: the session (the direct stdio server + * and every client of the shared daemon), and the stdio proxy serving in + * process while the daemon is unavailable. + */ +import { describe, it, expect } from 'vitest'; +import { spawn } from 'child_process'; +import * as path from 'path'; +import { MCPSession } from '../src/mcp/session'; +import type { MCPEngine } from '../src/mcp/engine'; +import type { JsonRpcTransport, JsonRpcRequest, JsonRpcNotification } from '../src/mcp/transport'; + +const MALFORMED: unknown[] = ['alpha', 42, true, [1], [{ query: 'q' }]]; +const NONE: unknown[] = [undefined, null, '']; + +describe('a tools/call whose arguments are not an object (#2981)', () => { + function session() { + let handle: ((m: JsonRpcRequest | JsonRpcNotification) => Promise) | null = null; + const results: Array<{ id: unknown; result: unknown }> = []; + const errors: Array<{ id: unknown; code: number; message: string }> = []; + const executed: unknown[] = []; + const transport: JsonRpcTransport = { + start(h) { handle = h as typeof handle; }, + stop() { /* nothing to tear down */ }, + send() { /* unused */ }, + notify() { /* unused */ }, + async request() { return {}; }, + sendResult(id, result) { results.push({ id, result }); }, + sendError(id, code, message) { errors.push({ id, code, message }); }, + }; + const engine = { + ensureInitialized: async () => { /* already open */ }, + hasDefaultCodeGraph: () => true, + getProjectPath: () => '/repo', + retryInitializeSync: () => { /* nothing to retry */ }, + getToolHandler: () => ({ + getTools: () => [], + execute: async (_name: string, args: unknown) => { + executed.push(args); + return { content: [{ type: 'text' as const, text: 'ok' }] }; + }, + }), + } as unknown as MCPEngine; + new MCPSession(transport, engine).start(); + const call = (id: number, args: unknown) => + handle!({ + jsonrpc: '2.0', id, method: 'tools/call', + params: args === undefined ? { name: 'codegraph_explore' } : { name: 'codegraph_explore', arguments: args }, + } as JsonRpcRequest); + return { call, results, errors, executed }; + } + + it('is answered with invalid params, and never reaches a tool', async () => { + const s = session(); + for (const [i, args] of MALFORMED.entries()) await s.call(i, args); + expect(s.errors).toEqual(MALFORMED.map((_, i) => ({ id: i, code: -32602, message: 'Tool arguments must be an object' }))); + expect(s.executed).toEqual([]); + expect(s.results).toEqual([]); + }); + + it('runs a call with an object, or with no arguments at all', async () => { + const s = session(); + await s.call(1, { query: 'q' }); + for (const [i, args] of NONE.entries()) await s.call(10 + i, args); + expect(s.errors).toEqual([]); + expect(s.executed).toEqual([{ query: 'q' }, {}, {}, {}]); + }); + + it('is answered the same way by the proxy while the daemon is unavailable', async () => { + const proxyModule = path.resolve(__dirname, '../dist/mcp/proxy.js'); + // The real proxy entry point, with no daemon and an engine that says when + // a tool runs. + const script = ` + const { runLocalHandshakeProxy } = require(${JSON.stringify(proxyModule)}); + const say = (s) => require('fs').writeSync(1, s + '\\n'); + runLocalHandshakeProxy({ + root: process.cwd(), + getDaemonSocket: async () => null, + makeEngine: () => ({ + ensureInitialized: async () => {}, + isReadOnly: () => true, + getToolHandler: () => ({ execute: async (name, args) => { + say('EXECUTED ' + JSON.stringify(args)); + return { content: [{ type: 'text', text: 'ok' }] }; + } }), + stop: async () => {}, + }), + }); + `; + const child = spawn(process.execPath, ['-e', script], { + env: { + ...process.env, + CODEGRAPH_TELEMETRY: '0', DO_NOT_TRACK: '1', CODEGRAPH_NO_PROMPT_HOOK: '1', + CODEGRAPH_NO_WATCHDOG: '1', CODEGRAPH_PPID_POLL_MS: '0', + CODEGRAPH_STARTUP_HANDSHAKE_TIMEOUT_MS: '0', CODEGRAPH_DAEMON_RETRY_MS: '60000', + }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + let out = ''; + let err = ''; + child.stdout!.on('data', (d) => { out += d; }); + child.stderr!.on('data', (d) => { err += d; }); + const exited = new Promise((resolve) => child.once('exit', () => resolve())); + const send = (msg: object) => child.stdin!.write(JSON.stringify({ jsonrpc: '2.0', ...msg }) + '\n'); + const reply = (id: number) => + out.split('\n').filter((l) => l.startsWith('{')).map((l) => JSON.parse(l)).find((m) => m.id === id); + const until = async (check: () => boolean) => { + const deadline = Date.now() + 15_000; + while (!check()) { + if (Date.now() > deadline) throw new Error(`no reply: ${out}\n${err}`); + await new Promise((r) => setTimeout(r, 25)); + } + }; + try { + send({ id: 1, method: 'initialize', params: {} }); + await until(() => !!reply(1)); + send({ id: 2, method: 'tools/call', params: { name: 'codegraph_explore', arguments: 'alpha' } }); + send({ id: 3, method: 'tools/call', params: { name: 'codegraph_status', arguments: [1] } }); + send({ id: 4, method: 'tools/call', params: { name: 'codegraph_status' } }); + await until(() => !!reply(2) && !!reply(3) && !!reply(4)); + expect(reply(2).error).toEqual({ code: -32602, message: 'Tool arguments must be an object' }); + expect(reply(3).error).toEqual({ code: -32602, message: 'Tool arguments must be an object' }); + expect(reply(4).result).toEqual({ content: [{ type: 'text', text: 'ok' }] }); + expect(out.match(/^EXECUTED .*$/gm)).toEqual(['EXECUTED {}']); + } finally { + child.stdin!.end(); + child.kill('SIGKILL'); + await exited; + } + }, 30_000); +}); diff --git a/src/mcp/proxy.ts b/src/mcp/proxy.ts index 5c0e166e3..ce3a59924 100644 --- a/src/mcp/proxy.ts +++ b/src/mcp/proxy.ts @@ -28,7 +28,7 @@ import { armStartupHandshakeTimeout } from './startup-handshake'; import { treatStdinFailureAsShutdown } from './stdin-teardown'; import { treatStdoutFailureAsShutdown } from './stdout-teardown'; import { CodeGraphPackageVersion, isOlderRelease } from './version'; -import { SERVER_INFO, PROTOCOL_VERSION, initializeInstructions } from './session'; +import { SERVER_INFO, PROTOCOL_VERSION, initializeInstructions, toolCallArguments, TOOL_ARGUMENTS_NOT_OBJECT } from './session'; import { SERVER_INSTRUCTIONS } from './server-instructions'; import { getStaticTools } from './tools'; import { ErrorCodes } from './transport'; @@ -361,10 +361,15 @@ export async function runLocalHandshakeProxy(deps: LocalHandshakeDeps): Promise< let msg: JsonRpc; try { msg = JSON.parse(line) as JsonRpc; } catch { return; } const id = msg.id; if (msg.method === 'tools/call' && id !== undefined) { + const params = (msg.params || {}) as { name: string; arguments?: unknown }; + const args = toolCallArguments(params); + if (!args) { + writeClient({ jsonrpc: '2.0', id, error: { code: ErrorCodes.InvalidParams, message: TOOL_ARGUMENTS_NOT_OBJECT } }); + return; + } try { const local = await ensureEngine(); - const params = (msg.params || {}) as { name: string; arguments?: Record }; - const result = await local.getToolHandler().execute(params.name, params.arguments || {}, exploreSession); + const result = await local.getToolHandler().execute(params.name, args, exploreSession); writeClient({ jsonrpc: '2.0', id, result }); getTelemetry().recordUsage('mcp_tool', params.name, !result.isError, telemetryClient); } catch (err) { diff --git a/src/mcp/session.ts b/src/mcp/session.ts index d4b54ff79..610c342a3 100644 --- a/src/mcp/session.ts +++ b/src/mcp/session.ts @@ -58,6 +58,23 @@ export function initializeInstructions(base: string, notice: string | null = get /** MCP Protocol Version (latest the server claims). */ export const PROTOCOL_VERSION = '2024-11-05'; +/** The invalid-params message for {@link toolCallArguments}' null. */ +export const TOOL_ARGUMENTS_NOT_OBJECT = 'Tool arguments must be an object'; + +/** + * The arguments a `tools/call` passes, or null when they are not an object. + * A string, number or array there is a malformed request, the client's to + * fix, so it is answered with a JSON-RPC invalid-params error, as a call that + * names no tool is, and never run: run, a string made the tool handler throw, + * which came back as an internal codegraph error to retry (#2981). Exported so + * the proxy, serving in process while the daemon is unavailable, answers it + * the same way. No arguments (missing, null or another empty value) are none. + */ +export function toolCallArguments(params: { arguments?: unknown }): Record | null { + const args = params.arguments || {}; + return typeof args === 'object' && !Array.isArray(args) ? (args as Record) : null; +} + /** * How long to wait for the client's `roots/list` response before giving up * and falling back to the process cwd. @@ -296,7 +313,11 @@ export class MCPSession { } const toolName = params.name; - const toolArgs = params.arguments || {}; + const toolArgs = toolCallArguments(params); + if (!toolArgs) { + this.transport.sendError(request.id, ErrorCodes.InvalidParams, TOOL_ARGUMENTS_NOT_OBJECT); + return; + } const tool = tools.find((t) => t.name === toolName); if (!tool) {