From 61cad0e42fb82b3b54961cec3e56223a5d0da8b4 Mon Sep 17 00:00:00 2001 From: finalerock44 Date: Mon, 9 Nov 2026 17:05:23 +0000 Subject: [PATCH] fix(mcp): say why a tool is missing in read-only mode With --read-only, dcd mcp asks the hosted server for its read-only tools, and the server then doesn't list the others at all. A call to one of them, such as run, came back as a bare "Unknown tool: run". In read-only mode an unknown tool now says the server offers only the tools that change nothing. The test stand-in for the hosted server now honours ?read_only=true as the API does, which is how this slipped through. A second test plays an API that lists every tool, so the local refusal stays covered. --- src/mcp/bridge.ts | 17 +++++-- test/unit/mcp-bridge.test.ts | 98 ++++++++++++++++++++++++------------ 2 files changed, 78 insertions(+), 37 deletions(-) diff --git a/src/mcp/bridge.ts b/src/mcp/bridge.ts index be2d395..bc446b7 100644 --- a/src/mcp/bridge.ts +++ b/src/mcp/bridge.ts @@ -222,8 +222,8 @@ class RemoteDevices { const { apiUrl } = await getContext(); const client = new Client({ name: 'dcd-mcp-bridge', version: getCliVersion() }); // In read-only mode, ask the hosted server for its read-only tools too, so - // it refuses the rest as well. The filter here stays: an API without the - // flag ignores it, and a refused call still gets the read-only message. + // it refuses the rest as well. The filter here stays for an API without + // the flag, which ignores it and lists every tool. const url = new URL(`${apiUrl}/mcp${isReadOnly() ? '?read_only=true' : ''}`); await client.connect( new StreamableHTTPClientTransport(url, { @@ -311,8 +311,17 @@ export async function createBridgeServer( return runTool(`device ${name}`, async () => { await remote.list(); - if (!remote.has(name)) return errorResult(`Unknown tool: ${name}`); - // The list hides these in read-only mode; refuse them if called anyway. + if (!remote.has(name)) { + // In read-only mode the hosted server lists only its read-only tools, + // so a device tool that changes something is unknown here as well. + return errorResult( + readOnly + ? `Unknown tool: ${name}. In read-only mode this server offers only the tools that change nothing.` + : `Unknown tool: ${name}`, + ); + } + + // An API without the read-only flag lists every tool; refuse these here. if (readOnly && !remote.isReadOnly(name)) { return errorResult(`${name} is not available in read-only mode.`); } diff --git a/test/unit/mcp-bridge.test.ts b/test/unit/mcp-bridge.test.ts index 14c5b69..c15683e 100644 --- a/test/unit/mcp-bridge.test.ts +++ b/test/unit/mcp-bridge.test.ts @@ -28,13 +28,21 @@ function startHosted(): Promise<{ calls: { args: unknown; auth?: string; name: string }[]; failStart: boolean; http: HttpServer; + ignoreReadOnly: boolean; inits: number; paths: string[]; sessions: Record[]; url: string; }> { const calls: { args: unknown; auth?: string; name: string }[] = []; - const hosted = { calls, failStart: false, inits: 0, paths: [] as string[], sessions: [] as Record[] }; + const hosted = { + calls, + failStart: false, + ignoreReadOnly: false, + inits: 0, + paths: [] as string[], + sessions: [] as Record[], + }; const http = createHttpServer(async (req, res) => { // The open sessions, as GET /device/sessions answers. if (req.method === 'GET' && req.url === '/device/sessions') { @@ -64,37 +72,46 @@ function startHosted(): Promise<{ calls.push({ name, args, auth }); return { content: [{ type: 'text' as const, text: JSON.stringify({ ok: name }) }] }; }; - server.registerTool( - 'install_app', - { - description: 'Install an app.', - inputSchema: { session_id: z.string().optional(), app_binary_id: z.string().optional() }, - }, - record('install_app'), - ); - server.registerTool( - 'run', - { description: 'Run Maestro.', inputSchema: { yaml: z.string().optional(), flow_upload_id: z.string().optional() } }, - record('run'), - ); - server.registerTool( - 'get_logs', - { description: 'Slow.', inputSchema: { ms: z.number() }, annotations: { readOnlyHint: true } }, - async ({ ms }) => { - await new Promise((r) => setTimeout(r, ms)); - return { content: [{ type: 'text' as const, text: 'late' }] }; - }, - ); - server.registerTool( - 'start_device', - { description: 'Start a device.', inputSchema: { platform: z.string() } }, - record('start_device'), - ); - server.registerTool( - 'inspect_screen', - { description: 'Inspect.', inputSchema: {}, annotations: { readOnlyHint: true } }, - record('inspect_screen'), - ); + const registered = [ + server.registerTool( + 'install_app', + { + description: 'Install an app.', + inputSchema: { session_id: z.string().optional(), app_binary_id: z.string().optional() }, + }, + record('install_app'), + ), + server.registerTool( + 'run', + { description: 'Run Maestro.', inputSchema: { yaml: z.string().optional(), flow_upload_id: z.string().optional() } }, + record('run'), + ), + server.registerTool( + 'get_logs', + { description: 'Slow.', inputSchema: { ms: z.number() }, annotations: { readOnlyHint: true } }, + async ({ ms }) => { + await new Promise((r) => setTimeout(r, ms)); + return { content: [{ type: 'text' as const, text: 'late' }] }; + }, + ), + server.registerTool( + 'start_device', + { description: 'Start a device.', inputSchema: { platform: z.string() } }, + record('start_device'), + ), + server.registerTool( + 'inspect_screen', + { description: 'Inspect.', inputSchema: {}, annotations: { readOnlyHint: true } }, + record('inspect_screen'), + ), + ]; + // As the API does: /mcp?read_only=true serves only the tools that change + // nothing. ignoreReadOnly plays an older API, which lists every tool. + const readOnlyAsked = new URL(req.url ?? '/', 'http://hosted').searchParams.get('read_only') === 'true'; + if (readOnlyAsked && !hosted.ignoreReadOnly) { + for (const tool of registered) if (tool.annotations?.readOnlyHint !== true) tool.remove(); + } + const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: undefined }); await server.connect(transport); await transport.handleRequest(req, res, body); @@ -246,7 +263,7 @@ describe('dcd mcp bridge', () => { // Hidden is not enough: a call made anyway is refused, never forwarded. const refused = await client.callTool({ name: 'run', arguments: { yaml: 'x' } }); expect(refused.isError).to.equal(true); - expect(JSON.stringify(refused.content)).to.include('read-only mode'); + expect(JSON.stringify(refused.content)).to.include('Unknown tool: run. In read-only mode'); expect(hosted.calls).to.deep.equal([]); await client.callTool({ name: 'inspect_screen', arguments: {} }); expect(hosted.calls.map((c) => c.name)).to.deep.equal(['inspect_screen']); @@ -256,6 +273,21 @@ describe('dcd mcp bridge', () => { await client.close(); }); + it('read-only mode still refuses on an API that lists every tool', async () => { + hosted.ignoreReadOnly = true; + process.env.DCD_MCP_READONLY = '1'; + const client = await connect(); + const names = (await client.listTools()).tools.map((t) => t.name); + expect(names).to.include('inspect_screen'); + expect(names).to.not.include.members(['install_app', 'run', 'start_device']); + + const refused = await client.callTool({ name: 'run', arguments: { yaml: 'x' } }); + expect(refused.isError).to.equal(true); + expect(JSON.stringify(refused.content)).to.include('run is not available in read-only mode'); + expect(hosted.calls).to.deep.equal([]); + await client.close(); + }); + it('still serves the dcd_* tools when the hosted endpoint is unreachable', async () => { process.env.DCD_API_URL = 'http://127.0.0.1:9'; clearContextCache();