diff --git a/AGENTS.md b/AGENTS.md index 79fbed94..e74d1190 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -589,9 +589,12 @@ server instead of re-declaring each upstream server to Claude: - `GET /tools` → the full server-prefixed tool list (names + descriptions + JSON Schema). Returns `[]` (never an error) when no enabled servers are healthy. - `POST /tools/call` → executes one tool by name through the app's tool-approval - policy SERVER-SIDE, so a headless run never hangs on an interactive prompt - (`yolo` runs everything; `ask`/`wait` run only whitelisted tools, otherwise return - a structured "not approved" envelope, never a transport error). + policy SERVER-SIDE: `yolo` runs everything; `ask`/`wait` gate on the whitelist — + `ask` returns a structured "not approved" envelope for anything not whitelisted + (never prompts, since a headless caller can't answer), while `wait` raises the + SAME approval dialog+countdown the OpenAI backend shows and blocks the call until + the user responds or the countdown auto-approves (never a transport error either + way). - **Wiring**: `McpToolBridge` (`src/backend/services/mcp/mcp-tool-bridge.ts`) flattens the AI-SDK `ToolSet` into wire summaries and enforces the approval policy; the front door (`mcp-serve.ts`) maps a `{ok:false}` envelope to an MCP `isError` result. diff --git a/src/backend/services/cli-bridge/bridge-router.ts b/src/backend/services/cli-bridge/bridge-router.ts index 04fbb1e9..05bb028b 100644 --- a/src/backend/services/cli-bridge/bridge-router.ts +++ b/src/backend/services/cli-bridge/bridge-router.ts @@ -52,6 +52,7 @@ export interface BridgeServices { name: string, args?: Record, serverFilter?: string[], + shaveId?: string, ): Promise; }; } @@ -325,6 +326,7 @@ async function routeTools( parsed.data.name, parsed.data.arguments ?? {}, parsed.data.serverFilter, + parsed.data.shaveId, ); // A tool-level failure (incl. a structured "not approved") is still a // successful BRIDGE response — the envelope carries {ok:false,...} so the diff --git a/src/backend/services/cli-bridge/cli-bridge-server.ts b/src/backend/services/cli-bridge/cli-bridge-server.ts index b5d68f1e..91ea6689 100644 --- a/src/backend/services/cli-bridge/cli-bridge-server.ts +++ b/src/backend/services/cli-bridge/cli-bridge-server.ts @@ -16,6 +16,7 @@ import { McpToolBridge } from "../mcp/mcp-tool-bridge"; import { applyOpenAtLoginSetting } from "../settings/login-item"; import { LlmStorage } from "../storage/llm-storage"; import { UserSettingsStorage } from "../storage/user-settings-storage"; +import { UserInteractionService } from "../user-interaction/user-interaction-service"; import { type BridgeServices, routeRequest } from "./bridge-router"; const MAX_BODY_BYTES = 256 * 1024; // 256 KB is plenty for config payloads. @@ -313,8 +314,8 @@ export function createDefaultServices(): BridgeServices { async listTools(serverFilter) { return (await getToolBridge()).listTools(serverFilter); }, - async callTool(name, args, serverFilter) { - return (await getToolBridge()).callTool(name, args, serverFilter); + async callTool(name, args, serverFilter, shaveId) { + return (await getToolBridge()).callTool(name, args, serverFilter, shaveId); }, }, }; @@ -323,7 +324,9 @@ export function createDefaultServices(): BridgeServices { /** Build a {@link McpToolBridge} wired to the live singletons. */ async function getToolBridge(): Promise { const manager = await MCPServerManager.getInstanceAsync(); - return new McpToolBridge(manager, { - getSettingsAsync: () => UserSettingsStorage.getInstance().getSettingsAsync(), - }); + return new McpToolBridge( + manager, + { getSettingsAsync: () => UserSettingsStorage.getInstance().getSettingsAsync() }, + UserInteractionService.getInstance(), + ); } diff --git a/src/backend/services/cli-bridge/front-door.integration.test.ts b/src/backend/services/cli-bridge/front-door.integration.test.ts index de2bab21..80c83e2f 100644 --- a/src/backend/services/cli-bridge/front-door.integration.test.ts +++ b/src/backend/services/cli-bridge/front-door.integration.test.ts @@ -1,11 +1,16 @@ import { createServer, type Server as HttpServer } from "node:http"; import type { AddressInfo } from "node:net"; import type { ToolSet } from "ai"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { z } from "zod"; import { BridgeClient } from "../../../cli/bridge-client"; import { callToolViaBridge, listToolsViaBridge } from "../../../cli/mcp-serve"; -import { McpToolBridge, type ToolBridgeManager } from "../mcp/mcp-tool-bridge"; +import type { ToolApprovalDecision } from "../../../shared/types/mcp"; +import { + McpToolBridge, + type ToolBridgeManager, + type ToolBridgeUserInteraction, +} from "../mcp/mcp-tool-bridge"; import { type BridgeServices, routeRequest } from "./bridge-router"; /** @@ -79,10 +84,18 @@ function makeManager(whitelist: string[]): ToolBridgeManager { } /** Build the real BridgeServices, with only the tools front-door wired to a real bridge. */ -function makeServices(approvalMode: "yolo" | "ask" | "wait", whitelist: string[]): BridgeServices { - const bridge = new McpToolBridge(makeManager(whitelist), { - getSettingsAsync: async () => ({ toolApprovalMode: approvalMode }), - }); +function makeServices( + approvalMode: "yolo" | "ask" | "wait", + whitelist: string[], + userInteraction: ToolBridgeUserInteraction = { + requestToolApproval: async () => ({ kind: "approve" }), + }, +): BridgeServices { + const bridge = new McpToolBridge( + makeManager(whitelist), + { getSettingsAsync: async () => ({ toolApprovalMode: approvalMode }) }, + userInteraction, + ); // Only the routes this test exercises need real backing; the rest can throw. const notUsed = () => { throw new Error("not part of the front-door path"); @@ -99,7 +112,12 @@ function makeServices(approvalMode: "yolo" | "ask" | "wait", whitelist: string[] settings: { getSettingsAsync: notUsed, updateSettingsAsync: notUsed }, tools: { listTools: () => bridge.listTools(), - callTool: (name: string, args?: Record) => bridge.callTool(name, args), + callTool: ( + name: string, + args?: Record, + serverFilter?: string[], + shaveId?: string, + ) => bridge.callTool(name, args, serverFilter, shaveId), }, } as unknown as BridgeServices; } @@ -210,6 +228,84 @@ describe("front-door integration — mcp-serve → BridgeClient → router → M expect(result.content).toEqual([{ type: "text", text: "Created issue: OK" }]); }); + it("raises the approval prompt for a non-whitelisted tool under 'wait' and runs it once APPROVED (#920)", async () => { + const requestToolApproval = vi + .fn() + .mockResolvedValue({ kind: "approve" } satisfies ToolApprovalDecision); + ({ server, port } = await startBridge( + makeServices("wait", /* whitelist */ [], { requestToolApproval }), + )); + const client = makeClient(port); + + const result = await callToolViaBridge(client, "GitHub__create_issue", { title: "Bug" }); + + // The bug this closes (#920): Claude Code mode used to run this tool WITHOUT ever consulting + // the approval flow. Proving the mock was actually invoked, over the real HTTP round-trip, is + // the regression guard. + expect(requestToolApproval).toHaveBeenCalledTimes(1); + expect(requestToolApproval).toHaveBeenCalledWith( + "GitHub__create_issue", + { title: "Bug" }, + expect.objectContaining({}), + ); + expect(result.isError).toBe(false); + expect(result.content).toEqual([{ type: "text", text: "Created issue: Bug" }]); + }); + + it("does NOT run a non-whitelisted tool under 'wait' when the user denies it", async () => { + const requestToolApproval = vi + .fn() + .mockResolvedValue({ kind: "deny_stop", feedback: "no" } satisfies ToolApprovalDecision); + ({ server, port } = await startBridge( + makeServices("wait", /* whitelist */ [], { requestToolApproval }), + )); + const client = makeClient(port); + + const result = await callToolViaBridge(client, "GitHub__create_issue", { title: "Bug" }); + + expect(requestToolApproval).toHaveBeenCalledTimes(1); + expect(result.isError).toBe(true); + expect((result.content[0] as { text?: string }).text).toMatch(/not approved/i); + }); + + it("runs a WHITELISTED tool under 'wait' without prompting", async () => { + const requestToolApproval = vi.fn(); + ({ server, port } = await startBridge( + makeServices("wait", ["GitHub__create_issue"], { requestToolApproval }), + )); + const client = makeClient(port); + + const result = await callToolViaBridge(client, "GitHub__create_issue", { title: "OK" }); + + expect(requestToolApproval).not.toHaveBeenCalled(); + expect(result.isError).toBe(false); + expect(result.content).toEqual([{ type: "text", text: "Created issue: OK" }]); + }); + + it("forwards shaveId end-to-end from callToolViaBridge to McpToolBridge (#920 per-shave auto-approve)", async () => { + const requestToolApproval = vi + .fn() + .mockResolvedValue({ kind: "approve" } satisfies ToolApprovalDecision); + ({ server, port } = await startBridge( + makeServices("wait", /* whitelist */ [], { requestToolApproval }), + )); + const client = makeClient(port); + + await callToolViaBridge( + client, + "GitHub__create_issue", + { title: "Bug" }, + undefined, + "shave-42", + ); + + expect(requestToolApproval).toHaveBeenCalledWith( + "GitHub__create_issue", + { title: "Bug" }, + expect.objectContaining({ shaveId: "shave-42" }), + ); + }); + it("rejects an unauthenticated request at the HTTP boundary", async () => { await boot("yolo"); const badClient = new BridgeClient({ diff --git a/src/backend/services/mcp/local-claude-orchestrator.test.ts b/src/backend/services/mcp/local-claude-orchestrator.test.ts index 33ee7a9e..b4ff242e 100644 --- a/src/backend/services/mcp/local-claude-orchestrator.test.ts +++ b/src/backend/services/mcp/local-claude-orchestrator.test.ts @@ -215,11 +215,14 @@ describe("LocalClaudeOrchestrator", () => { expect(argv).not.toContain("bypassPermissions"); }); - it("wait -> --permission-mode bypassPermissions (headless can't defer the prompt)", () => { + it("wait -> --permission-mode bypassPermissions at the CLI layer (MCP tools are gated by the bridge instead, #920)", () => { const orch = new LocalClaudeOrchestrator(); const argv = orch.buildArgv("/tmp/cfg.json", "/tmp/prompt.txt", "wait", []); - // main maps `wait` -> bypassPermissions (headless can't show the deferred dialog), so an empty - // whitelist still runs every tool. There is no --allowedTools because none was passed. + // `wait` still bypasses Claude's OWN built-in-tool permission system at the CLI layer (there is + // no bridge in front of those to defer to) — there is no --allowedTools because none was + // passed. This does NOT mean MCP tools run unchecked: McpToolBridge.callTool enforces the real + // wait-mode approval dialog+countdown server-side, independent of this flag (see + // mcp-tool-bridge.test.ts). expect(argv).not.toContain("--allowedTools"); expect(argv).toContain("bypassPermissions"); }); diff --git a/src/backend/services/mcp/local-claude-orchestrator.ts b/src/backend/services/mcp/local-claude-orchestrator.ts index 9f82282b..d0779169 100644 --- a/src/backend/services/mcp/local-claude-orchestrator.ts +++ b/src/backend/services/mcp/local-claude-orchestrator.ts @@ -6,6 +6,7 @@ import { join } from "node:path"; import { CLI_BRIDGE_PORT_ENV, CLI_BRIDGE_SERVER_FILTER_ENV, + CLI_BRIDGE_SHAVE_ID_ENV, CLI_BRIDGE_TOKEN_ENV, } from "../../../shared/cli-bridge/protocol"; import type { ToolApprovalMode } from "../../../shared/types/user-settings"; @@ -187,7 +188,8 @@ export class LocalClaudeOrchestrator implements IBacklogOrchestrator { await this.ensureClaudeAvailable(); const manager = this.serverManager ?? (await MCPServerManager.getInstanceAsync()); - const frontDoor = this.frontDoor ?? (await resolveFrontDoorConfig(options.serverFilter)); + const frontDoor = + this.frontDoor ?? (await resolveFrontDoorConfig(options.serverFilter, options.shaveId)); const approvalMode = (await this.getSettingsStorage().getSettingsAsync()).toolApprovalMode; const systemPrompt = this.buildSystemPrompt( @@ -265,18 +267,10 @@ export class LocalClaudeOrchestrator implements IBacklogOrchestrator { console.warn(`[LocalClaudeOrchestrator] ${msg}`); } - // Under "wait", the OpenAI path shows a 15s approval dialog the user can still deny within. A - // headless `claude -p` can't render that deferred prompt, so "wait" runs EVERY tool immediately - // (effectively YOLO) under the local backend. Tell the user so the safe-looking "wait" setting - // doesn't silently widen tool execution without any signal. - if (approvalMode === "wait") { - const msg = - 'Claude Code (local) cannot show the "wait" approval dialog because it runs headlessly, ' + - "so under this backend every tool runs immediately without a chance to deny (the same as " + - '"YOLO"). Switch to "ask" to restrict the run to whitelisted tools only.'; - notices.push(msg); - console.warn(`[LocalClaudeOrchestrator] ${msg}`); - } + // "wait" needs no caveat here (#920): a non-whitelisted MCP tool call is gated by + // `McpToolBridge.callTool` in the main process, which raises the same approval dialog+countdown + // the OpenAI backend shows and blocks the run until the user responds or the countdown + // auto-approves — the same behaviour as the OpenAI path, not a silent widening to YOLO. for (const text of notices) { onStep?.({ type: "reasoning", reasoning: JSON.stringify({ type: "text", text }) }); @@ -366,17 +360,25 @@ Embed this URL in the task content that you create. Follow user requirements STR /** * Maps the approval mode to Claude Code's permission flags. * - * - `yolo` → `--permission-mode bypassPermissions` (run every tool immediately). - * - `wait` → ALSO `--permission-mode bypassPermissions`. In the OpenAI backend `wait` shows the - * approval prompt but AUTO-APPROVES after a delay, so a non-whitelisted tool the user doesn't - * dismiss still RUNS. A headless `claude -p` can't render a deferred prompt, so the faithful - * analogue of "auto-approve after a delay" is to bypass permissions — otherwise `wait` would - * silently HARD-DENY non-whitelisted tools, diverging from the OpenAI behaviour for the same - * setting. (See requestToolApproval's `wait` -> auto-approve path.) + * These flags only control Claude's OWN built-in tools (Read/Write/Bash/etc.) at the CLI layer. + * MCP tools (everything YakShaver actually cares about — GitHub/Jira/Azure DevOps/etc.) do NOT + * stop at this layer: every `tools/call` for the single `yakshaver` front-door is proxied to + * {@link McpToolBridge.callTool} in the main process, which enforces the REAL approval policy + * server-side regardless of `--permission-mode` (#920) — including, under `wait`, raising the + * same approval dialog+countdown the OpenAI backend shows. + * + * - `yolo` → `--permission-mode bypassPermissions` (run every built-in tool immediately; MCP + * tools run immediately too, per the bridge's own `yolo` handling). + * - `wait` → ALSO `--permission-mode bypassPermissions`, so Claude's own built-in tools aren't + * hard-denied at the CLI layer (there is no bridge in front of those to defer to). MCP tools are + * NOT auto-approved here — the bridge gates them on the whitelist and prompts via the approval + * dialog for anything not already whitelisted, so `wait` behaves the same as the OpenAI backend + * for the tools that matter. * - `ask` → `--allowedTools ` + `--permission-mode dontAsk`. `--allowedTools` only * auto-approves the listed tools; on its own (default permission mode) a non-whitelisted tool * would trigger an interactive prompt the headless run can't answer and the run can hang/abort. - * `dontAsk` converts any such prompt into an outright denial, so the run never blocks. + * `dontAsk` converts any such prompt into an outright denial, so the run never blocks. (MCP + * tools are additionally gated by the bridge itself, same as always.) * * `--strict-mcp-config` ensures Claude only uses the servers we serialized into `--mcp-config`, * ignoring any ambient project `.mcp.json` / `~/.claude` MCP config so the embedded run is @@ -706,7 +708,10 @@ Embed this URL in the task content that you create. Follow user requirements STR * Lazily imports electron + the bridge server so the pure argv/config unit tests (which always * inject a `frontDoor`) never touch these singletons. */ -async function resolveFrontDoorConfig(serverFilter?: string[]): Promise { +async function resolveFrontDoorConfig( + serverFilter?: string[], + shaveId?: string, +): Promise { const { app } = await import("electron"); const { CliBridgeServer } = await import("../cli-bridge/cli-bridge-server"); @@ -723,6 +728,11 @@ async function resolveFrontDoorConfig(serverFilter?: string[]): Promise 0) { env[CLI_BRIDGE_SERVER_FILTER_ENV] = serverFilter.join(","); } + // Forward the shave id so a `wait`-mode approval prompt raised via the bridge (#920) can honour + // the same per-shave auto-approve override the OpenAI backend supports. + if (shaveId) { + env[CLI_BRIDGE_SHAVE_ID_ENV] = shaveId; + } return { command: process.execPath, cliEntryPath, env }; } diff --git a/src/backend/services/mcp/mcp-tool-bridge.test.ts b/src/backend/services/mcp/mcp-tool-bridge.test.ts index 953f161f..aa7eb92b 100644 --- a/src/backend/services/mcp/mcp-tool-bridge.test.ts +++ b/src/backend/services/mcp/mcp-tool-bridge.test.ts @@ -1,7 +1,13 @@ import type { ToolSet } from "ai"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { z } from "zod"; -import { McpToolBridge, type ToolBridgeManager, type ToolBridgeSettings } from "./mcp-tool-bridge"; +import type { ToolApprovalDecision } from "../../../shared/types/mcp"; +import { + McpToolBridge, + type ToolBridgeManager, + type ToolBridgeSettings, + type ToolBridgeUserInteraction, +} from "./mcp-tool-bridge"; /** * Build a ToolSet where each tool carries an AI-SDK-shaped {description, inputSchema, execute}. @@ -38,9 +44,20 @@ function makeSettings(mode: "yolo" | "ask" | "wait"): ToolBridgeSettings { return { getSettingsAsync: vi.fn().mockResolvedValue({ toolApprovalMode: mode }) }; } +/** A stub {@link ToolBridgeUserInteraction} whose decision is scripted per test. */ +function makeUserInteraction( + decision: ToolApprovalDecision = { kind: "approve" }, +): ToolBridgeUserInteraction & { requestToolApproval: ReturnType } { + return { requestToolApproval: vi.fn().mockResolvedValue(decision) }; +} + describe("McpToolBridge.listTools", () => { it("flattens the aggregated toolset INCLUDING internal servers, with JSON-Schema input", async () => { - const bridge = new McpToolBridge(makeManager(makeToolSet()), makeSettings("ask")); + const bridge = new McpToolBridge( + makeManager(makeToolSet()), + makeSettings("ask"), + makeUserInteraction(), + ); const tools = await bridge.listTools(); const names = tools.map((t) => t.name).sort(); @@ -56,7 +73,7 @@ describe("McpToolBridge.listTools", () => { it("forwards the serverFilter to the manager so only selected servers are collected", async () => { const manager = makeManager(makeToolSet()); - const bridge = new McpToolBridge(manager, makeSettings("ask")); + const bridge = new McpToolBridge(manager, makeSettings("ask"), makeUserInteraction()); await bridge.listTools(["srv-1", "srv-2"]); expect(manager.collectToolsForSelectedServersAsync).toHaveBeenCalledWith(["srv-1", "srv-2"]); }); @@ -65,7 +82,11 @@ describe("McpToolBridge.listTools", () => { const tools = { Bare__tool: { description: "no schema", execute: vi.fn() }, } as unknown as ToolSet; - const bridge = new McpToolBridge(makeManager(tools), makeSettings("yolo")); + const bridge = new McpToolBridge( + makeManager(tools), + makeSettings("yolo"), + makeUserInteraction(), + ); const [summary] = await bridge.listTools(); expect(summary.inputSchema).toEqual({ type: "object", properties: {} }); }); @@ -80,7 +101,7 @@ describe("McpToolBridge.listTools", () => { manager.collectToolsForSelectedServersAsync.mockRejectedValue( new Error("[MCPServerManager]: No MCP clients available"), ); - const bridge = new McpToolBridge(manager, makeSettings("ask")); + const bridge = new McpToolBridge(manager, makeSettings("ask"), makeUserInteraction()); await expect(bridge.listTools()).resolves.toEqual([]); }); }); @@ -96,6 +117,7 @@ describe("McpToolBridge.callTool - approval policy enforcement", () => { const bridge = new McpToolBridge( makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), /* whitelist */ []), makeSettings("yolo"), + makeUserInteraction(), ); const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); expect(executeSpy).toHaveBeenCalledOnce(); @@ -106,6 +128,7 @@ describe("McpToolBridge.callTool - approval policy enforcement", () => { const bridge = new McpToolBridge( makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), ["GitHub__create_issue"]), makeSettings("ask"), + makeUserInteraction(), ); const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); expect(executeSpy).toHaveBeenCalledOnce(); @@ -113,30 +136,98 @@ describe("McpToolBridge.callTool - approval policy enforcement", () => { }); it("ask: a NON-whitelisted tool returns a structured not-approved result and does NOT run (no hang)", async () => { + const userInteraction = makeUserInteraction(); const bridge = new McpToolBridge( makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), /* whitelist */ []), makeSettings("ask"), + userInteraction, ); const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); expect(executeSpy).not.toHaveBeenCalled(); + // "ask" never prompts — a headless caller couldn't answer an interactive dialog anyway. + expect(userInteraction.requestToolApproval).not.toHaveBeenCalled(); expect(res.ok).toBe(false); if (res.ok) throw new Error("expected a not-approved failure"); expect(res.notApproved).toBe(true); expect(res.error).toMatch(/not approved/i); }); - it("wait: runs a NON-whitelisted tool (wait = auto-approve, matching buildArgv's bypassPermissions)", async () => { + it("wait: runs a whitelisted tool without prompting", async () => { + const userInteraction = makeUserInteraction(); const bridge = new McpToolBridge( - makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), /* whitelist */ []), + makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), ["GitHub__create_issue"]), makeSettings("wait"), + userInteraction, ); const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); + expect(userInteraction.requestToolApproval).not.toHaveBeenCalled(); expect(executeSpy).toHaveBeenCalledOnce(); expect(res.ok).toBe(true); }); + it("wait: a NON-whitelisted tool raises the approval dialog and runs only once APPROVED (#920)", async () => { + const userInteraction = makeUserInteraction({ kind: "approve" }); + const bridge = new McpToolBridge( + makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), /* whitelist */ []), + makeSettings("wait"), + userInteraction, + ); + const res = await bridge.callTool( + "GitHub__create_issue", + { title: "Bug" }, + undefined, + "shave-1", + ); + + expect(userInteraction.requestToolApproval).toHaveBeenCalledWith( + "GitHub__create_issue", + { title: "Bug" }, + expect.objectContaining({ shaveId: "shave-1" }), + ); + expect(executeSpy).toHaveBeenCalledOnce(); + expect(res).toEqual({ ok: true, result: "Created #5" }); + }); + + it("wait: a DENIED tool does NOT run and returns a structured not-approved result", async () => { + const userInteraction = makeUserInteraction({ kind: "deny_stop", feedback: "no thanks" }); + const bridge = new McpToolBridge( + makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), /* whitelist */ []), + makeSettings("wait"), + userInteraction, + ); + const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); + + expect(executeSpy).not.toHaveBeenCalled(); + expect(res.ok).toBe(false); + if (res.ok) throw new Error("expected a not-approved failure"); + expect(res.notApproved).toBe(true); + }); + + it("wait: a 'request_changes' decision does NOT run and surfaces the feedback", async () => { + const userInteraction = makeUserInteraction({ + kind: "request_changes", + feedback: "use a different title", + }); + const bridge = new McpToolBridge( + makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), /* whitelist */ []), + makeSettings("wait"), + userInteraction, + ); + const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); + + expect(executeSpy).not.toHaveBeenCalled(); + expect(res.ok).toBe(false); + if (res.ok) throw new Error("expected a not-approved failure"); + expect(res.notApproved).toBe(true); + expect(res.error).toContain("use a different title"); + }); + it("returns a clear error for an unknown tool", async () => { - const bridge = new McpToolBridge(makeManager(makeToolSet()), makeSettings("yolo")); + const bridge = new McpToolBridge( + makeManager(makeToolSet()), + makeSettings("yolo"), + makeUserInteraction(), + ); const res = await bridge.callTool("Nope__missing", {}); expect(res).toEqual({ ok: false, error: "Unknown tool: Nope__missing" }); }); @@ -146,6 +237,7 @@ describe("McpToolBridge.callTool - approval policy enforcement", () => { const bridge = new McpToolBridge( makeManager(makeToolSet({ GitHub__create_issue: boom }), ["GitHub__create_issue"]), makeSettings("ask"), + makeUserInteraction(), ); const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); expect(res).toEqual({ ok: false, error: "boom" }); @@ -160,7 +252,7 @@ describe("McpToolBridge.callTool - approval policy enforcement", () => { manager.collectToolsForSelectedServersAsync.mockRejectedValue( new Error("[MCPServerManager]: No tools available from selected/healthy servers"), ); - const bridge = new McpToolBridge(manager, makeSettings("yolo")); + const bridge = new McpToolBridge(manager, makeSettings("yolo"), makeUserInteraction()); const res = await bridge.callTool("GitHub__create_issue", { title: "Bug" }); expect(res.ok).toBe(false); if (res.ok) throw new Error("expected a structured failure"); @@ -171,6 +263,7 @@ describe("McpToolBridge.callTool - approval policy enforcement", () => { const bridge = new McpToolBridge( makeManager(makeToolSet({ GitHub__create_issue: executeSpy }), []), makeSettings("yolo"), + makeUserInteraction(), ); await bridge.callTool("GitHub__create_issue"); expect(executeSpy).toHaveBeenCalledWith( diff --git a/src/backend/services/mcp/mcp-tool-bridge.ts b/src/backend/services/mcp/mcp-tool-bridge.ts index ee5ba6c8..7b948ae8 100644 --- a/src/backend/services/mcp/mcp-tool-bridge.ts +++ b/src/backend/services/mcp/mcp-tool-bridge.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto"; import { asSchema, type ToolSet } from "ai"; import type { BridgeToolSummary, ToolCallResult } from "../../../shared/cli-bridge/protocol"; +import type { ToolApprovalDecision } from "../../../shared/types/mcp"; import type { ToolApprovalMode } from "../../../shared/types/user-settings"; /** @@ -25,23 +26,41 @@ export interface ToolBridgeSettings { getSettingsAsync(): Promise<{ toolApprovalMode: ToolApprovalMode }>; } +/** + * The slice of {@link UserInteractionService} the bridge needs to raise the SAME approval + * dialog+countdown the OpenAI backend uses (#920) — kept narrow so unit tests can inject a mock + * without dragging in Electron/BrowserWindow. + */ +export interface ToolBridgeUserInteraction { + requestToolApproval( + toolName: string, + args: unknown, + options?: { message?: string; shaveId?: string }, + ): Promise; +} + /** * Bridges the app's aggregated MCP toolset to the localhost bridge endpoints * (`GET /tools`, `POST /tools/call`) consumed by the single `yakshaver` MCP * front-door (#915). * - * Two responsibilities: + * Three responsibilities: * 1. Flatten the AI-SDK `ToolSet` (server-prefixed keys → `description` + * JSON-Schema `inputSchema`) into wire-friendly summaries. - * 2. Apply the app's tool-approval policy SERVER-SIDE on every call so a - * headless run never hangs on an interactive prompt: `yolo` runs everything; - * `ask`/`wait` run only whitelisted tools, otherwise return a STRUCTURED - * "not approved" result. + * 2. Apply the app's tool-approval policy SERVER-SIDE on every call: `yolo` runs everything; + * `ask` runs only whitelisted tools, otherwise returns a STRUCTURED "not approved" result + * immediately (never prompts — a headless caller can't answer an interactive dialog). + * 3. Under `wait`, raise the SAME approval dialog+countdown the OpenAI backend shows (#920) for a + * non-whitelisted tool. This call blocks until the user responds or the countdown + * auto-approves — safe here because the bridge is a plain in-process HTTP call with no + * transport timeout (the front-door's `POST /tools/call` simply waits, exactly as it would for + * any slow tool). */ export class McpToolBridge { constructor( private readonly manager: ToolBridgeManager, private readonly settings: ToolBridgeSettings, + private readonly userInteraction: ToolBridgeUserInteraction, ) {} /** Aggregated tool list for `GET /tools`. Names/descriptions/schemas only. */ @@ -61,18 +80,18 @@ export class McpToolBridge { /** * Execute a single tool by its server-prefixed name for `POST /tools/call`. * - * Enforces the approval policy first, NEVER hanging. Only `ask` gates on the - * whitelist (a non-whitelisted tool returns `{ok:false, notApproved:true}` - * rather than waiting on a prompt the headless caller can't answer). `yolo` and - * `wait` both run everything — matching the orchestrator's `buildArgv`, which - * maps `wait` to `bypassPermissions` because the OpenAI backend's `wait` is - * "auto-approve after a delay", not a hard deny. Treating `wait` like `ask` here - * would hard-deny every non-whitelisted tool and break wait-mode runs. + * Enforces the approval policy first. `yolo` runs everything immediately. `ask` gates on the + * whitelist and never prompts — a non-whitelisted tool returns `{ok:false, notApproved:true}` + * rather than waiting on a dialog a headless caller couldn't answer anyway. `wait` also gates on + * the whitelist, but — since this call DOES have an interactive user on the other end of the + * bridge (the desktop app itself) — a non-whitelisted tool raises the same approval + * dialog+countdown OpenAI mode shows (#920) instead of hard-denying or silently running. */ async callTool( name: string, args: Record = {}, serverFilter?: string[], + shaveId?: string, ): Promise { // Resolve against the SAME filtered toolset `listTools` exposes, so a tool from an unselected // project isn't reachable even if the model guesses its name (the server-side gate for #915). @@ -83,14 +102,34 @@ export class McpToolBridge { } const approvalMode = (await this.settings.getSettingsAsync()).toolApprovalMode; - if (approvalMode === "ask") { + if (approvalMode === "ask" || approvalMode === "wait") { const whitelist = new Set(await this.manager.getWhitelistWithServerPrefixAsync()); if (!whitelist.has(name)) { - return { - ok: false, - notApproved: true, - error: `Tool '${name}' is not approved under the 'ask' approval mode. Whitelist it in YakShaver, or switch the approval mode to 'yolo'.`, - }; + if (approvalMode === "ask") { + return { + ok: false, + notApproved: true, + error: `Tool '${name}' is not approved under the 'ask' approval mode. Whitelist it in YakShaver, or switch the approval mode to 'yolo'.`, + }; + } + + // wait: raise the same approval dialog+countdown the OpenAI backend uses, and block until + // the user responds (or the countdown auto-approves). Never hangs forever — the dialog + // itself auto-resolves after WAIT_MODE_AUTO_APPROVE_DELAY_MS. + const decision = await this.userInteraction.requestToolApproval(name, args, { + message: `Approval required to run ${name}`, + shaveId, + }); + if (decision.kind !== "approve") { + const feedback = decision.kind === "request_changes" ? decision.feedback : undefined; + return { + ok: false, + notApproved: true, + error: feedback + ? `Tool '${name}' was not approved by the user: ${feedback}` + : `Tool '${name}' was not approved by the user.`, + }; + } } } diff --git a/src/cli/mcp-serve.test.ts b/src/cli/mcp-serve.test.ts index 4253168d..94f35fe7 100644 --- a/src/cli/mcp-serve.test.ts +++ b/src/cli/mcp-serve.test.ts @@ -77,6 +77,35 @@ describe("mcp-serve front-door — tools/call proxy", () => { expect(result.content).toEqual([{ type: "text", text: "Created #5" }]); }); + it("forwards shaveId to POST /tools/call when provided (#920, wait-mode per-shave auto-approve)", async () => { + const post = vi.fn().mockResolvedValue({ ok: true, result: "Created #5" } as ToolCallResult); + + await callToolViaBridge( + { post }, + "GitHub__create_issue", + { title: "Bug" }, + undefined, + "shave-1", + ); + + expect(post).toHaveBeenCalledWith("/tools/call", { + name: "GitHub__create_issue", + arguments: { title: "Bug" }, + shaveId: "shave-1", + }); + }); + + it("omits shaveId from the POST body when not provided", async () => { + const post = vi.fn().mockResolvedValue({ ok: true, result: "Created #5" } as ToolCallResult); + + await callToolViaBridge({ post }, "GitHub__create_issue", { title: "Bug" }); + + expect(post).toHaveBeenCalledWith("/tools/call", { + name: "GitHub__create_issue", + arguments: { title: "Bug" }, + }); + }); + it("stringifies a non-string tool result", async () => { const post = vi.fn().mockResolvedValue({ ok: true, result: { id: 5 } } as ToolCallResult); const result = await callToolViaBridge({ post }, "X__y", {}); diff --git a/src/cli/mcp-serve.ts b/src/cli/mcp-serve.ts index 23f70009..96522d1a 100644 --- a/src/cli/mcp-serve.ts +++ b/src/cli/mcp-serve.ts @@ -9,6 +9,7 @@ import { type BridgeToolSummary, CLI_BRIDGE_PORT_ENV, CLI_BRIDGE_SERVER_FILTER_ENV, + CLI_BRIDGE_SHAVE_ID_ENV, CLI_BRIDGE_TOKEN_ENV, type ToolCallResult, } from "../shared/cli-bridge/protocol"; @@ -42,6 +43,12 @@ export interface McpServeOptions { * selected servers. Undefined/empty means every enabled server. */ serverFilter?: string[]; + /** + * The current shave's id, forwarded on `POST /tools/call` so a `wait`-mode approval prompt can + * honour the per-shave auto-approve override (#920). Injectable for testing; in production it is + * read from {@link CLI_BRIDGE_SHAVE_ID_ENV}. + */ + shaveId?: string; } /** @@ -54,6 +61,7 @@ export function createMcpServer(options: McpServeOptions = {}): Server { // The orchestrator injects the project's selected servers; the front-door forwards them so the // app restricts the toolset (and tool execution) to that project, not every configured server. const serverFilter = options.serverFilter ?? readServerFilterFromEnv(); + const shaveId = options.shaveId ?? readShaveIdFromEnv(); const server = new Server( { name: "yakshaver", version: "1.0.0" }, @@ -62,7 +70,7 @@ export function createMcpServer(options: McpServeOptions = {}): Server { server.setRequestHandler(ListToolsRequestSchema, () => listToolsViaBridge(client, serverFilter)); server.setRequestHandler(CallToolRequestSchema, (request) => - callToolViaBridge(client, request.params.name, request.params.arguments, serverFilter), + callToolViaBridge(client, request.params.name, request.params.arguments, serverFilter, shaveId), ); return server; @@ -116,12 +124,17 @@ type McpToolResult = Pick; * - a transport failure (app quit/restart, socket drop, non-JSON body), * - a tool-level `isError` on an `ok:true` result — passed through verbatim so a * tool FAILURE is never masked as success (matches the in-process orchestrator). + * + * Under `wait` approval mode, the bridge itself may block this call awaiting the user's response to + * an approval dialog (#920) — safe because `BridgeClient.post` sets no request timeout, so this + * simply waits like it would for any slow tool. */ export async function callToolViaBridge( client: Pick, name: string, args: Record | undefined, serverFilter?: string[], + shaveId?: string, ): Promise { let result: ToolCallResult; try { @@ -129,6 +142,7 @@ export async function callToolViaBridge( name, arguments: args ?? {}, ...(serverFilter && serverFilter.length > 0 ? { serverFilter } : {}), + ...(shaveId ? { shaveId } : {}), }); } catch (err) { const message = err instanceof Error ? err.message : String(err); @@ -220,6 +234,12 @@ function readServerFilterFromEnv(): string[] | undefined { return ids.length > 0 ? ids : undefined; } +/** Read the current shave id the orchestrator injected, if any (#920). */ +function readShaveIdFromEnv(): string | undefined { + const raw = process.env[CLI_BRIDGE_SHAVE_ID_ENV]; + return raw && raw.trim().length > 0 ? raw.trim() : undefined; +} + /** Encode a server filter as a `?serverFilter=a,b` query suffix, or "" when there is none. */ function serverFilterQuery(serverFilter?: string[]): string { if (!serverFilter || serverFilter.length === 0) return ""; diff --git a/src/shared/cli-bridge/protocol.ts b/src/shared/cli-bridge/protocol.ts index d7570315..92506c26 100644 --- a/src/shared/cli-bridge/protocol.ts +++ b/src/shared/cli-bridge/protocol.ts @@ -48,6 +48,15 @@ export const CLI_BRIDGE_TOKEN_ENV = "YAKSHAVER_BRIDGE_TOKEN"; */ export const CLI_BRIDGE_SERVER_FILTER_ENV = "YAKSHAVER_BRIDGE_SERVER_FILTER"; +/** + * The current shave's id, injected the same way as {@link CLI_BRIDGE_SERVER_FILTER_ENV}. The + * front-door forwards it on `POST /tools/call` so the bridge's `wait`-mode approval prompt can key + * off the same per-shave "auto-approve for this shave" override the OpenAI backend supports + * (`UserInteractionService.setShaveAutoApprove`), and so an approval dialog raised from a headless + * Claude Code run is attributable to the shave that triggered it. + */ +export const CLI_BRIDGE_SHAVE_ID_ENV = "YAKSHAVER_BRIDGE_SHAVE_ID"; + /** Placeholder shown instead of any secret value. */ export const REDACTED = "***redacted***"; @@ -174,6 +183,12 @@ export const ToolCallInputSchema = z.object({ * an unselected project even if the model guesses its name. */ serverFilter: z.array(z.string()).optional(), + /** + * The current shave's id, forwarded from {@link CLI_BRIDGE_SHAVE_ID_ENV} so a `wait`-mode + * approval prompt raised by this call can honour the same per-shave auto-approve override the + * OpenAI backend supports. + */ + shaveId: z.string().optional(), }); export type ToolCallInput = z.infer;