From 9241658e68f2b894eee0ba921c4bc07fa990d9d7 Mon Sep 17 00:00:00 2001 From: KazKozDev Date: Thu, 8 Oct 2026 18:28:28 +0200 Subject: [PATCH] fix(mcp): make the HTTP fallback work and stop it masking bridge errors The MCP server falls back to the bridge's HTTP API when the socket fails. It sent the extension's request types (`browser.snapshot`) to `/v1/tools/call`, which takes tool names (`browser_snapshot`), so every tool came back "Unknown tool" and the fallback had never worked. It was also taken on any socket failure, including an error the bridge itself returned. The real error was then replaced by the retry's, and a socket timeout would have sent the same action a second time. The fallback now runs only when the socket cannot be opened. scripts/mcp-smoke.mjs drives the real mcp-server.js over stdio against a real bridge and a recording HTTP server, and runs from check.sh. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 ++ scripts/check.sh | 1 + scripts/mcp-smoke.mjs | 186 +++++++++++++++++++++++++++++++ weboperator-bridge/mcp-server.js | 42 +++++-- 4 files changed, 228 insertions(+), 8 deletions(-) create mode 100644 scripts/mcp-smoke.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f35024..1ee6b21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,13 @@ evals. ### Fixed +- The MCP server's HTTP fallback, which never worked: it sent the extension's + request types (`browser.snapshot`) to an API that takes tool names + (`browser_snapshot`), so with the socket unavailable every tool came back + "Unknown tool". It now uses the routes the bridge has. The fallback is also + taken only when the socket cannot be opened; an error the bridge returned is + reported as it is, where it used to be replaced by the failed retry's, and a + socket timeout no longer sends the same click or keystroke a second time. - Page content could break out of the block that marks it as untrusted. The observation fence used fixed, public markers, so a page that printed the closing marker in its own text had everything after it read as instruction diff --git a/scripts/check.sh b/scripts/check.sh index b44d4e9..d883d82 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -5,6 +5,7 @@ ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" node "$ROOT/scripts/eval-fixtures.mjs" node "$ROOT/scripts/bridge-smoke.mjs" +node "$ROOT/scripts/mcp-smoke.mjs" cd "$ROOT/core" npm run typecheck diff --git a/scripts/mcp-smoke.mjs b/scripts/mcp-smoke.mjs new file mode 100644 index 0000000..0930f96 --- /dev/null +++ b/scripts/mcp-smoke.mjs @@ -0,0 +1,186 @@ +#!/usr/bin/env node +// The MCP server reaches the bridge over a Unix socket and falls back to the +// bridge's HTTP API when the socket cannot be opened. This drives the real +// mcp-server.js over stdio and checks both halves of that arrangement. +import assert from 'node:assert/strict'; +import { once } from 'node:events'; +import { spawn } from 'node:child_process'; +import http from 'node:http'; +import readline from 'node:readline'; + +const root = new URL('..', import.meta.url); +const port = 21000 + Math.floor(Math.random() * 1000); +const token = 'test-token'; +const socketPath = `/tmp/weboperator-mcp-smoke-${process.pid}.sock`; +const noSocket = `/tmp/weboperator-mcp-smoke-${process.pid}-absent.sock`; + +const TOOL_CALLS = [ + ['browser_snapshot', {}], + ['browser_navigate', { url: 'https://example.com' }], + ['browser_click', { index: 1 }], + ['browser_type', { index: 1, text: 'hello' }], + ['browser_press', { key: 'Enter' }], + ['browser_scroll', { direction: 'down' }], + ['browser_screenshot', {}], + ['browser_extract', { instruction: 'the title' }], + ['browser_solve_captcha', { type: 'auto' }], + ['weboperator_execute_goal', { goal: 'read the page' }], +]; + +// A real bridge with no extension attached: every request it understands +// fails the same way, and one it does not understand fails differently. +const bridge = spawn(process.execPath, ['weboperator-bridge/bridge.js'], { + cwd: root, + env: { + ...process.env, + WEBOPERATOR_BRIDGE_PORT: String(port), + WEBOPERATOR_AGENT_SOCKET: socketPath, + WEBOPERATOR_BRIDGE_LOG: '/tmp/weboperator-mcp-smoke.log', + WEBOPERATOR_API_TOKEN: token, + }, + stdio: ['ignore', 'ignore', 'inherit'], +}); + +// Stands in for the bridge's HTTP API where the routes themselves are the +// thing under test, and records what it was asked. +const seen = []; +const fake = http.createServer((req, res) => { + let raw = ''; + req.on('data', (chunk) => { raw += chunk; }); + req.on('end', () => { + const body = raw ? JSON.parse(raw) : {}; + seen.push({ method: req.method, path: req.url, body }); + res.setHeader('content-type', 'application/json'); + if (req.url === '/v1/tasks') return res.end(JSON.stringify({ id: 'task 1' })); + if (req.url === '/v1/tasks/task%201/wait') { + return res.end(JSON.stringify({ id: 'task 1', status: 'completed', steps: [] })); + } + if (req.url === '/v1/tools/call') return res.end(JSON.stringify({ ok: true, tool: body.tool })); + res.statusCode = 404; + return res.end(JSON.stringify({ error: `Unknown endpoint: ${req.method} ${req.url}` })); + }); +}); +fake.listen(0, '127.0.0.1'); +await once(fake, 'listening'); +const fakePort = fake.address().port; + +const servers = []; + +try { + await waitForServer(`http://127.0.0.1:${port}/health`); + + // 1. No socket, real bridge: every tool has to arrive as a request the + // bridge recognises. The fallback used to send `browser.snapshot` where + // the HTTP API takes `browser_snapshot`, so all ten came back + // "Unknown tool" and the fallback could never work. + const overHttp = startMcp({ socket: noSocket, httpPort: port }); + await overHttp.request('initialize', {}); + const listed = await overHttp.request('tools/list', {}); + assert.deepEqual(listed.tools.map((tool) => tool.name), TOOL_CALLS.map(([name]) => name)); + for (const [name, args] of TOOL_CALLS) { + const result = await overHttp.request('tools/call', { name, arguments: args }); + const text = result.content[0].text; + assert.equal(result.isError, true, `${name} cannot succeed without an extension`); + assert.match(text, /extension is not connected/i, `${name} over HTTP: ${text}`); + } + + // 2. No socket, recording server: a goal is two requests, and neither is + // a tool call. + const recorded = startMcp({ socket: noSocket, httpPort: fakePort }); + await recorded.request('initialize', {}); + const goal = await recorded.request('tools/call', { + name: 'weboperator_execute_goal', arguments: { goal: 'read the page', timeoutMs: 5000 }, + }); + assert.notEqual(goal.isError, true, goal.content[0].text); + assert.deepEqual(seen.map((entry) => `${entry.method} ${entry.path}`), [ + 'POST /v1/tasks', + 'POST /v1/tasks/task%201/wait', + ]); + assert.deepEqual(seen[0].body, { goal: 'read the page', timeoutMs: 5000 }); + assert.deepEqual(seen[1].body, { timeoutMs: 5000 }); + + seen.length = 0; + const click = await recorded.request('tools/call', { + name: 'browser_click', arguments: { index: 3 }, + }); + assert.notEqual(click.isError, true, click.content[0].text); + assert.equal(seen.length, 1); + assert.equal(seen[0].path, '/v1/tools/call'); + assert.equal(seen[0].body.tool, 'browser_click'); + assert.equal(seen[0].body.arguments.index, 3); + + // 3. Socket open, bridge answers with an error: that error is the result. + // Falling back here replaced it with the second attempt's, and after a + // timeout would have run the action twice. + seen.length = 0; + const overSocket = startMcp({ socket: socketPath, httpPort: fakePort }); + await overSocket.request('initialize', {}); + const refused = await overSocket.request('tools/call', { name: 'browser_click', arguments: { index: 1 } }); + assert.equal(refused.isError, true); + assert.match(refused.content[0].text, /extension is not connected/i); + assert.equal(seen.length, 0, 'an error from the bridge must not be retried over HTTP'); + + console.log('MCP smoke tests passed'); +} finally { + for (const server of servers) await stopChild(server.child); + await stopChild(bridge); + fake.close(); +} + +function startMcp({ socket, httpPort }) { + const child = spawn(process.execPath, ['weboperator-bridge/mcp-server.js'], { + cwd: root, + env: { + ...process.env, + WEBOPERATOR_AGENT_SOCKET: socket, + WEBOPERATOR_BRIDGE_PORT: String(httpPort), + WEBOPERATOR_API_TOKEN: token, + }, + stdio: ['pipe', 'pipe', 'inherit'], + }); + const waiting = new Map(); + let nextId = 1; + readline.createInterface({ input: child.stdout }).on('line', (line) => { + const message = JSON.parse(line); + const settle = waiting.get(message.id); + if (!settle) return; + waiting.delete(message.id); + if (message.error) settle.reject(new Error(message.error.message)); + else settle.resolve(message.result); + }); + const server = { + child, + request(method, params) { + const id = nextId++; + return new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error(`No answer to ${method}`)), 15_000); + waiting.set(id, { + resolve: (value) => { clearTimeout(timer); resolve(value); }, + reject: (err) => { clearTimeout(timer); reject(err); }, + }); + child.stdin.write(`${JSON.stringify({ jsonrpc: '2.0', id, method, params })}\n`); + }); + }, + }; + servers.push(server); + return server; +} + +async function waitForServer(url) { + const started = Date.now(); + while (Date.now() - started < 5000) { + try { + const res = await fetch(url); + if (res.ok) return; + } catch {} + await new Promise((resolve) => setTimeout(resolve, 100)); + } + throw new Error(`Timed out waiting for ${url}`); +} + +async function stopChild(childProcess) { + if (childProcess.exitCode !== null || childProcess.signalCode !== null) return; + const exited = once(childProcess, 'exit').catch(() => {}); + childProcess.kill('SIGTERM'); + await exited; +} diff --git a/weboperator-bridge/mcp-server.js b/weboperator-bridge/mcp-server.js index 9b2c21e..8b1ca6f 100755 --- a/weboperator-bridge/mcp-server.js +++ b/weboperator-bridge/mcp-server.js @@ -196,16 +196,42 @@ async function callBridge(type, payload = {}, timeoutMs = 60_000) { // Try Unix Domain Socket first try { return await callSocket(type, payload, timeoutMs); - } catch { - // Fallback to HTTP API + } catch (err) { + // Once the bridge has taken the request, its answer is the answer. A + // retry over HTTP would hide the real error behind whatever the second + // attempt says, and after a timeout it would run the action again — a + // second click, a second form submission. + if (err && err.reachedBridge) throw err; + // The socket could not be opened at all: fall back to the HTTP API. return await callHttp(type, payload, timeoutMs); } } +// The socket takes the extension's own request types; the HTTP API has its +// own routes and tool names. This is the translation between the two. +function httpRequestFor(type, payload, timeoutMs) { + if (type === 'tasks.start') { + return { path: '/v1/tasks', body: payload }; + } + if (type === 'tasks.wait') { + return { + path: `/v1/tasks/${encodeURIComponent(payload.id)}/wait`, + body: { timeoutMs: payload.timeoutMs }, + }; + } + // browser.snapshot -> browser_snapshot, and so on for every browser tool. + return { + path: '/v1/tools/call', + body: { tool: type.replace('.', '_'), arguments: payload, timeoutMs }, + }; +} + function callSocket(type, payload, timeoutMs) { return new Promise((resolve, reject) => { let resolved = false; + let connected = false; + const fail = (err) => reject(Object.assign(err, { reachedBridge: connected })); const socket = net.createConnection(SOCKET_PATH); const id = randomUUID(); const message = { id, type, payload, timeoutMs, ...(API_TOKEN ? { token: API_TOKEN } : {}) }; @@ -214,7 +240,7 @@ function callSocket(type, payload, timeoutMs) { if (!resolved) { resolved = true; socket.destroy(); - reject(new Error(`Bridge socket timeout for ${type}`)); + fail(new Error(`Bridge socket timeout for ${type}`)); } }, timeoutMs); @@ -222,6 +248,7 @@ function callSocket(type, payload, timeoutMs) { let nextLength = null; socket.on('connect', () => { + connected = true; const body = Buffer.from(JSON.stringify(message), 'utf8'); const header = Buffer.alloc(4); header.writeUInt32LE(body.length, 0); @@ -248,7 +275,7 @@ function callSocket(type, payload, timeoutMs) { resolved = true; clearTimeout(timer); socket.end(); - if (msg.error) reject(new Error(msg.error)); + if (msg.error) fail(new Error(msg.error)); else resolve(msg.result); } } catch (parseErr) { @@ -256,7 +283,7 @@ function callSocket(type, payload, timeoutMs) { resolved = true; clearTimeout(timer); socket.destroy(); - reject(parseErr); + fail(parseErr); } } } @@ -266,7 +293,7 @@ function callSocket(type, payload, timeoutMs) { if (!resolved) { resolved = true; clearTimeout(timer); - reject(err); + fail(err); } }); }); @@ -274,9 +301,8 @@ function callSocket(type, payload, timeoutMs) { function callHttp(type, payload, timeoutMs) { return new Promise((resolve, reject) => { - let path = '/v1/tools/call'; + const { path, body: bodyObj } = httpRequestFor(type, payload, timeoutMs); const method = 'POST'; - const bodyObj = { tool: type, arguments: payload, timeoutMs }; const headers = { 'content-type': 'application/json',