From 3f73538f34aaead9e2cf045f97fabaf134a6baa6 Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 16 Jul 2026 19:04:27 -0300 Subject: [PATCH 1/3] =?UTF-8?q?feat(mcp):=20make=20/mcp=20a=20dumb=20pipe?= =?UTF-8?q?=20=E2=80=94=20serve=20the=20BEX=20catalog,=20pass=20content=20?= =?UTF-8?q?through?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vault hardcoded the MCP tool catalog and text-wrapped every tool result. Two consequences: each new BEX tool needed a cross-repo change (tools/list never mentioned it, tools/call rejected it at the TOOL_NAMES guard), and bex_screenshot was impossible — MCP returns images as {type:'image'} content blocks and the vault could only emit text. The nine browser-driving tools are merged and shipping in keepkey-client (#112/#113) and are inert until this lands. - tools/list proxies to bex_list_tools; TOOLS becomes FALLBACK_TOOLS, served only when the bridge is down so `claude mcp add` still succeeds and the agent can reach bex_status to find out why. - tools/call passes an MCP content-block result through untouched; a plain result is still text-wrapped as before. - Drop the TOOL_NAMES guard. The vault can no longer know the valid names and doesn't need to: the BEX's unknown_tool surfaces as an isError result via the existing catch. One rejection path, in the process that owns the catalog. Auth is untouched: /mcp stays bearer-authed, loopback-only, browser-excluded, which matters more now that bex_click can drive a logged-in tab. The BEX-side Agent-mode toggle (default off) remains the gate. Tests drive a fake BEX socket, so the bridge-up paths (proxied catalog, image passthrough, unknown_tool) are covered with no extension and no device. The four new assertions fail against the unpatched handler. Implements HANDOFF_vault_mcp_dumb_pipe.md (keepkey-client#114). --- projects/keepkey-vault/src/bun/mcp.test.ts | 77 ++++++++++++++++++++-- projects/keepkey-vault/src/bun/mcp.ts | 46 +++++++++---- 2 files changed, 107 insertions(+), 16 deletions(-) diff --git a/projects/keepkey-vault/src/bun/mcp.test.ts b/projects/keepkey-vault/src/bun/mcp.test.ts index 965c4fbc..7033cfe7 100644 --- a/projects/keepkey-vault/src/bun/mcp.test.ts +++ b/projects/keepkey-vault/src/bun/mcp.test.ts @@ -5,9 +5,9 @@ * * Run: bun test src/bun/mcp.test.ts */ -import { describe, test, expect } from 'bun:test' +import { describe, test, expect, afterEach } from 'bun:test' import { handleMcpRequest } from './mcp' -import { callBex, onBexOpen, onBexClose, bridgeConnected } from './bex-bridge' +import { callBex, onBexOpen, onBexClose, onBexMessage, bridgeConnected } from './bex-bridge' function post(body: unknown): Request { return new Request('http://localhost:1646/mcp', { @@ -78,7 +78,7 @@ describe('MCP JSON-RPC handler', () => { expect(json.result.protocolVersion).toBe('2025-06-18') }) - test('tools/list returns the read-only tool catalog', async () => { + test('tools/list serves the static fallback catalog with the bridge DOWN', async () => { const { json } = await call({ jsonrpc: '2.0', id: 2, method: 'tools/list' }) const names = json.result.tools.map((t: any) => t.name) expect(names).toContain('bex_status') @@ -90,9 +90,14 @@ describe('MCP JSON-RPC handler', () => { expect(json.error.code).toBe(-32601) }) - test('unknown tool → -32602', async () => { + test('unknown tool is no longer rejected here — it goes to the BEX, which owns the names', async () => { + // The vault has no catalog to check against, so an unknown name is NOT a + // -32602 anymore: it is forwarded, and the BEX's unknown_tool surfaces as + // an isError result. Bridge down here, so the disconnect is what surfaces — + // either way, a result, never a JSON-RPC error. const { json } = await call({ jsonrpc: '2.0', id: 4, method: 'tools/call', params: { name: 'nope' } }) - expect(json.error.code).toBe(-32602) + expect(json.error).toBeUndefined() + expect(json.result.isError).toBe(true) }) test('bex_status answers truthfully with the bridge DOWN (never hangs)', async () => { @@ -104,6 +109,68 @@ describe('MCP JSON-RPC handler', () => { }) }) +describe('MCP dumb pipe (bridge UP)', () => { + // bex-bridge is a module singleton, so a socket left open by a failing test + // leaks into the next one. Close in afterEach, not at the end of each test + // body, which a failed expect() would skip. + let open: any = null + afterEach(() => { + if (open) onBexClose(open) + open = null + }) + + // A fake BEX that answers each forwarded call with a canned reply, keyed by + // tool name. Lets us drive the proxied catalog + content passthrough with no + // extension and no device. + const fakeBex = (replies: Record) => { + const ws = { + send(raw: string) { + const { id, tool } = JSON.parse(raw) + const result = replies[tool] + queueMicrotask(() => + onBexMessage(ws as any, JSON.stringify( + result === undefined + ? { id, error: { code: 'unknown_tool', message: `unknown tool: ${tool}` } } + : { id, result }, + )), + ) + }, + close() {}, + } + return ws + } + + test('tools/list serves the BEX catalog, not the vault fallback', async () => { + const ws = open = fakeBex({ bex_list_tools: { tools: [{ name: 'bex_screenshot' }, { name: 'bex_click' }] } }) + onBexOpen(ws as any) + const { json } = await call({ jsonrpc: '2.0', id: 10, method: 'tools/list' }) + expect(json.result.tools.map((t: any) => t.name)).toEqual(['bex_screenshot', 'bex_click']) + }) + + test('a content-block result passes through untouched (image is not stringified)', async () => { + const shot = { content: [{ type: 'image', data: 'AAAA', mimeType: 'image/jpeg' }] } + const ws = open = fakeBex({ bex_screenshot: shot }) + onBexOpen(ws as any) + const { json } = await call({ jsonrpc: '2.0', id: 11, method: 'tools/call', params: { name: 'bex_screenshot' } }) + expect(json.result).toEqual(shot) // not double-encoded into a text block + }) + + test('a plain (non-content) result is still text-wrapped', async () => { + const ws = open = fakeBex({ bex_accounts: { ethereum: '0xabc' } }) + onBexOpen(ws as any) + const { json } = await call({ jsonrpc: '2.0', id: 12, method: 'tools/call', params: { name: 'bex_accounts' } }) + expect(JSON.parse(json.result.content[0].text)).toEqual({ ethereum: '0xabc' }) + }) + + test("the BEX's unknown_tool surfaces as an isError result", async () => { + const ws = open = fakeBex({}) + onBexOpen(ws as any) + const { json } = await call({ jsonrpc: '2.0', id: 13, method: 'tools/call', params: { name: 'nope' } }) + expect(json.result.isError).toBe(true) + expect(JSON.parse(json.result.content[0].text).error).toBe('unknown_tool') + }) +}) + describe('BEX bridge call lifecycle', () => { const fakeWs = () => ({ sent: [] as string[], closed: false, send(d: string) { this.sent.push(d) }, close() { this.closed = true } }) diff --git a/projects/keepkey-vault/src/bun/mcp.ts b/projects/keepkey-vault/src/bun/mcp.ts index f3299e56..411c849f 100644 --- a/projects/keepkey-vault/src/bun/mcp.ts +++ b/projects/keepkey-vault/src/bun/mcp.ts @@ -11,9 +11,12 @@ * responses (no SSE stream, no sessions; both optional per spec). Swap in the * SDK if we ever need notifications/resources. * - * All tools execute inside the BEX; this file only owns the tool CATALOG and - * the forwarding. Bridge down → structured bridge_disconnected error - * (bex_status instead answers truthfully so agents can always probe). + * All tools execute inside the BEX, and the BEX also owns the CATALOG: this + * file is a dumb pipe that serves whatever bex_list_tools reports and passes + * tool content straight through. A new BEX tool therefore ships without a vault + * release. Bridge down → structured bridge_disconnected error, and tools/list + * serves FALLBACK_TOOLS (bex_status instead answers truthfully so agents can + * always probe). * * AUTH: /mcp requires a valid pairing bearer token — the SAME API keys as the * rest of the REST API (auth.requireAuth in rest-api's /mcp block). A local @@ -34,8 +37,12 @@ const PROTOCOL_VERSION = '2025-06-18' // negotiation), else offer ours. const SUPPORTED_PROTOCOL_VERSIONS = new Set(['2025-06-18', '2025-03-26', '2024-11-05']) -// Tier-1 read-only tools (Phase 1). Tier-2 control tools land in Phase 2. -const TOOLS = [ +// FALLBACK ONLY — not the source of truth. The BEX owns the catalog and +// answers bex_list_tools; these five tier-1 entries are served by tools/list +// only when the bridge is down, so `claude mcp add` still succeeds and the +// agent can reach bex_status to find out why. Do NOT add tools here: a new tool +// belongs in the BEX alone (HANDOFF_vault_mcp_dumb_pipe.md, keepkey-client). +const FALLBACK_TOOLS = [ { name: 'bex_status', description: @@ -84,8 +91,6 @@ const TOOLS = [ }, ] -const TOOL_NAMES = new Set(TOOLS.map(t => t.name)) - const json = (body: unknown, status = 200, cors: Record = {}) => new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json', ...cors } }) @@ -146,15 +151,34 @@ export async function handleMcpRequest(req: Request, cors: Record Date: Thu, 16 Jul 2026 19:20:31 -0300 Subject: [PATCH 2/3] fix(mcp): validate the tool-name shape, and keep unknown_tool a protocol error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the dumb-pipe change. 1. Dropping the TOOL_NAMES guard also dropped the only check that `name` was a usable string. The BEX drops frames with a non-truthy `tool` WITHOUT replying (mcpBridge.ts `if (!msg?.id || !msg?.tool) return`), so a missing/empty name was forwarded and then sat for the full 30s CALL_TIMEOUT_MS — where it used to fail instantly with -32602. Validate the shape before forwarding: the vault owns the request shape, the BEX still owns which names are real. 2. The MCP tools spec treats an unknown tool as a PROTOCOL error (-32602); isError is for a valid tool that failed while executing. Map the BEX's unknown_tool back onto rpcError instead of dressing it up as a successful result. The BEX still decides — we only translate its verdict into the envelope the spec asks for. No hardcoded catalog comes back. The fake BEX in the tests now mirrors the real one's silent-drop behavior, so the bridge-up regression test genuinely reproduces the hang: with the guard removed it times out instead of asserting. --- projects/keepkey-vault/src/bun/mcp.test.ts | 42 +++++++++++++++++----- projects/keepkey-vault/src/bun/mcp.ts | 19 +++++++--- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/projects/keepkey-vault/src/bun/mcp.test.ts b/projects/keepkey-vault/src/bun/mcp.test.ts index 7033cfe7..7a2c35f8 100644 --- a/projects/keepkey-vault/src/bun/mcp.test.ts +++ b/projects/keepkey-vault/src/bun/mcp.test.ts @@ -90,16 +90,26 @@ describe('MCP JSON-RPC handler', () => { expect(json.error.code).toBe(-32601) }) - test('unknown tool is no longer rejected here — it goes to the BEX, which owns the names', async () => { - // The vault has no catalog to check against, so an unknown name is NOT a - // -32602 anymore: it is forwarded, and the BEX's unknown_tool surfaces as - // an isError result. Bridge down here, so the disconnect is what surfaces — - // either way, a result, never a JSON-RPC error. + test('an unknown (but well-formed) tool name is forwarded, not rejected on the catalog', async () => { + // The vault has no catalog to check against, so the name itself is the + // BEX's call. Bridge is down here, so what surfaces is the disconnect — + // an execution failure, hence isError rather than a protocol error. const { json } = await call({ jsonrpc: '2.0', id: 4, method: 'tools/call', params: { name: 'nope' } }) expect(json.error).toBeUndefined() expect(json.result.isError).toBe(true) }) + test.each([ + ['missing', undefined], + ['empty', ''], + ['non-string', 42], + ])('a %s tool name → -32602 immediately, never a 30s bridge timeout', async (_label, name) => { + // The BEX drops frames with a falsy `tool` without ever replying, so + // forwarding one would hang until CALL_TIMEOUT_MS. Shape is ours to check. + const { json } = await call({ jsonrpc: '2.0', id: 6, method: 'tools/call', params: { name } }) + expect(json.error.code).toBe(-32602) + }) + test('bex_status answers truthfully with the bridge DOWN (never hangs)', async () => { // No socket connected → callBex rejects bridge_disconnected → bex_status // special-cases it into a { bridge: "down" } result, not an error. @@ -126,6 +136,10 @@ describe('MCP dumb pipe (bridge UP)', () => { const ws = { send(raw: string) { const { id, tool } = JSON.parse(raw) + // Mirrors the real BEX: a frame without a truthy id/tool is dropped with + // NO reply (mcpBridge.ts `if (!msg?.id || !msg?.tool) return`). That + // silence is exactly what makes an unvalidated name hang for 30s. + if (!id || !tool) return const result = replies[tool] queueMicrotask(() => onBexMessage(ws as any, JSON.stringify( @@ -162,13 +176,25 @@ describe('MCP dumb pipe (bridge UP)', () => { expect(JSON.parse(json.result.content[0].text)).toEqual({ ethereum: '0xabc' }) }) - test("the BEX's unknown_tool surfaces as an isError result", async () => { + test("the BEX's unknown_tool becomes a -32602 protocol error, per the MCP tools spec", async () => { + // isError is for a valid tool that failed while executing; an unknown tool + // is a protocol error. The BEX still decides — we only translate. const ws = open = fakeBex({}) onBexOpen(ws as any) const { json } = await call({ jsonrpc: '2.0', id: 13, method: 'tools/call', params: { name: 'nope' } }) - expect(json.result.isError).toBe(true) - expect(JSON.parse(json.result.content[0].text).error).toBe('unknown_tool') + expect(json.result).toBeUndefined() + expect(json.error.code).toBe(-32602) }) + + test('an empty tool name fails fast rather than hanging on a BEX that never replies', async () => { + // Regression: without the shape guard this forwards to a BEX that silently + // drops it, and the call sits for the full CALL_TIMEOUT_MS (30s). If this + // ever starts timing out instead of asserting, the guard is gone. + const ws = open = fakeBex({ bex_status: { ok: true } }) + onBexOpen(ws as any) + const { json } = await call({ jsonrpc: '2.0', id: 14, method: 'tools/call', params: { name: '' } }) + expect(json.error.code).toBe(-32602) + }, 1000) // must resolve WAY inside the 30s bridge timeout }) describe('BEX bridge call lifecycle', () => { diff --git a/projects/keepkey-vault/src/bun/mcp.ts b/projects/keepkey-vault/src/bun/mcp.ts index 411c849f..c08bd55b 100644 --- a/projects/keepkey-vault/src/bun/mcp.ts +++ b/projects/keepkey-vault/src/bun/mcp.ts @@ -168,10 +168,14 @@ export async function handleMcpRequest(req: Request, cors: Record Date: Thu, 16 Jul 2026 19:22:05 -0300 Subject: [PATCH 3/3] chore(mcp): drop the unused post() test helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dead since before this PR: post() builds a Request and returns it, but nothing calls it — every test goes through call(), which does the same construction and then actually invokes the handler. tsc flagged it TS6133. --- projects/keepkey-vault/src/bun/mcp.test.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/projects/keepkey-vault/src/bun/mcp.test.ts b/projects/keepkey-vault/src/bun/mcp.test.ts index 7a2c35f8..9630eb4b 100644 --- a/projects/keepkey-vault/src/bun/mcp.test.ts +++ b/projects/keepkey-vault/src/bun/mcp.test.ts @@ -9,13 +9,6 @@ import { describe, test, expect, afterEach } from 'bun:test' import { handleMcpRequest } from './mcp' import { callBex, onBexOpen, onBexClose, onBexMessage, bridgeConnected } from './bex-bridge' -function post(body: unknown): Request { - return new Request('http://localhost:1646/mcp', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: typeof body === 'string' ? body : JSON.stringify(body), - }) -} const call = async (body: unknown, headers?: Record) => { const req = new Request('http://localhost:1646/mcp', { method: 'POST',