diff --git a/shared/glean/mcp/src/skill-writer.ts b/shared/glean/mcp/src/skill-writer.ts index 42b0472..f943503 100644 --- a/shared/glean/mcp/src/skill-writer.ts +++ b/shared/glean/mcp/src/skill-writer.ts @@ -34,6 +34,33 @@ function parseFrontmatter(content: string): Record { return result; } +/** + * Keep approval requirements out of the local skill cache. The remote + * get_tool_approval lookup is the only source of truth, so a stale or hand-edited + * skill file must not retain a second approval setting for the plugin or the model + * to read. Other tool metadata, especially inputSchema, remains cached for argument + * shaping and prompt construction. + */ +function sanitizeSkillFile(filePath: string, text: string): string { + if (!/^tools[\\/]\S+\.json$/.test(filePath)) return text; + + try { + const parsed = JSON.parse(text) as unknown; + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + return text; + } + const { requires_approval: _ignored, ...metadata } = parsed as Record< + string, + unknown + >; + return JSON.stringify(metadata); + } catch { + // Leave malformed/non-object tool files alone; run_tool will not use them as + // approval state, and preserving the original content keeps diagnostics intact. + return text; + } +} + type LogFn = (label: string, detail?: Record) => void; /** @@ -99,8 +126,10 @@ export async function writeSkillsToDisk( continue; } await fs.mkdir(path.dirname(fullPath), { recursive: true }); - const text = - typeof content === "string" ? content : JSON.stringify(content); + const text = sanitizeSkillFile( + filePath, + typeof content === "string" ? content : JSON.stringify(content), + ); await fs.writeFile(fullPath, text, "utf-8"); writtenFiles.push(fullPath); } diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 2436b6a..4b55eb7 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -160,7 +160,6 @@ export async function resolveFileArgs( } interface ToolMetadata { - requires_approval?: boolean; name?: string; description?: string; server_id?: string; @@ -229,6 +228,54 @@ async function buildApprovalMessage( return message.join("\n"); } +// This follow-up only controls approval for future calls. +const alwaysAllowFollowUpTimeoutMs = 5_000; + +interface AlwaysAllowFollowUpResult { + accepted: boolean; + timedOut: boolean; +} + +async function requestAlwaysAllowFollowUp( + mcpServer: Server, + toolName: string, +): Promise { + const startedAt = Date.now(); + try { + const result = await mcpServer.elicitInput( + { + message: + `Always allow ${toolName} for future calls?\n\n` + + `(Auto-declines in 5 seconds)`, + // Empty form preserves the host-native Yes/No actions. + requestedSchema: { type: "object", properties: {} } as any, + }, + { timeout: alwaysAllowFollowUpTimeoutMs }, + ); + const elapsedMs = Date.now() - startedAt; + return { + accepted: result.action === "accept", + timedOut: + result.action !== "accept" && + elapsedMs >= alwaysAllowFollowUpTimeoutMs * 0.9, + }; + } catch { + return { + accepted: false, + timedOut: + Date.now() - startedAt >= alwaysAllowFollowUpTimeoutMs * 0.9, + }; + } +} + +function alwaysAllowFollowUpTimeoutMessage(toolName: string): string { + return ( + `The Always Allow prompt for ${toolName} timed out after 5 seconds ` + + `(auto-declined). The current action was approved, but it was not saved ` + + `for future calls; they will ask for approval again.` + ); +} + // A WeakSet so a short-lived server in tests doesn't leak, // and so the burn happens exactly once per server instance. const elicitationIdPrimed = new WeakSet(); @@ -312,6 +359,91 @@ export interface RunToolPolicy { fileArgs: boolean; } +class ToolApprovalError extends Error { + constructor(message: string) { + super(message); + this.name = "ToolApprovalError"; + } +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function approvalResponsePayload(result: CallToolResult): unknown { + const structured = (result as CallToolResult & { + structuredContent?: unknown; + }).structuredContent; + if (structured !== undefined) return structured; + + const text = result.content.find((item) => item.type === "text"); + if (!text || text.type !== "text") return undefined; + try { + return JSON.parse(text.text); + } catch { + return undefined; + } +} + +/** + * Ask the remote control plane whether this downstream tool requires approval. + * + * This is deliberately a per-call lookup. The answer is not read from skill files, + * stored in this process, or persisted locally. A missing, malformed, or failed + * response fails closed so the downstream `run_tool` call cannot proceed without a + * current remote decision. + */ +export async function getToolApproval( + remoteClient: Client, + serverId: string, + toolName: string, +): Promise { + let result: CallToolResult; + try { + result = await callRemoteTool(remoteClient, "get_tool_approval", { + server_id: serverId, + tool_name: toolName, + }); + } catch (err) { + const detail = err instanceof Error ? err.message : String(err); + throw new ToolApprovalError(`remote lookup failed: ${detail}`); + } + + if (result.isError) { + const text = result.content.find((item) => item.type === "text"); + const detail = text?.type === "text" ? text.text : "remote lookup returned an error"; + throw new ToolApprovalError(detail); + } + + const payload = approvalResponsePayload(result); + if (!isRecord(payload) || typeof payload.requires_approval !== "boolean") { + throw new ToolApprovalError( + "remote response did not contain boolean requires_approval", + ); + } + return payload.requires_approval; +} + +function approvalLookupFailure( + toolName: string, + error: unknown, +): CallToolResult { + const detail = error instanceof Error ? error.message : String(error); + console.error(`[get_tool_approval] ${toolName}: ${detail}`); + return { + content: [ + { + type: "text", + text: + `Could not determine whether ${toolName} requires approval from the ` + + `remote settings. The action was NOT executed. Retry when the approval ` + + `settings are available.`, + }, + ], + isError: true, + }; +} + export async function handleRunTool( remoteClient: Client, mcpServer: Server, @@ -331,9 +463,9 @@ export async function handleRunTool( }; } - // Load the downstream tool's metadata once, up front: its inputSchema drives - // file_args JSON-parsing (object/array params) and its requires_approval - // drives the HITL gate. Both paths must see it regardless of ENABLE_HITL. + // Load the downstream tool's metadata only for inputSchema. Approval is not + // taken from this file; it is fetched from the remote control plane below for + // every attempted downstream call. const toolMeta = await findToolJson(skillsBaseDir, toolName); // Refuse before reading any model-supplied path. Disabled file_args must be @@ -369,18 +501,14 @@ export async function handleRunTool( throw err; } + let requiresApproval: boolean; + try { + requiresApproval = await getToolApproval(remoteClient, serverId, toolName); + } catch (err) { + return approvalLookupFailure(toolName, err); + } + const hitlEnabled = process.env.ENABLE_HITL === "true"; - // Fail CLOSED when the tool's approval requirement is unknown. The gate used - // to key on `toolMeta?.requires_approval`; a missing or unparseable tool JSON - // (evicted by evictStaleSkills after a week, called from memory without a - // fresh find_skills_and_tools, or corrupt) made that falsy, so the gate - // was skipped and — with the native prompt already suppressed via - // readOnlyHint — the tool executed with ZERO approval. Only skip the gate - // when we can positively confirm the tool is read-only. - const requiresApproval = - typeof toolMeta?.requires_approval === "boolean" - ? toolMeta.requires_approval - : true; // Cursor is deliberately not excepted: current Cursor builds can use the same // local elicitation gate as other capable hosts. Older builds that drop the // prompt fail closed, and the timeout response explains the upgrade path. @@ -424,6 +552,43 @@ export async function handleRunTool( ], }; } + + const alwaysAllow = await requestAlwaysAllowFollowUp( + mcpServer, + toolName, + ); + if (alwaysAllow.accepted) { + try { + await callRemoteTool(remoteClient, "set_tool_approval", { + server_id: serverId, + tool_name: toolName, + value: "ALWAYS_ALLOWED", + }); + } catch (err) { + const detail = err instanceof Error ? err.message : String(err); + console.error( + `[set_tool_approval] failed to persist "${toolName}" to Glean: ${detail}`, + ); + } + } + + if (alwaysAllow.timedOut) { + const downstreamResult = await callRemoteTool( + remoteClient, + "run_tool", + buildRemoteArgs(serverId, toolName, resolvedArgs), + ); + return { + ...downstreamResult, + content: [ + { + type: "text", + text: alwaysAllowFollowUpTimeoutMessage(toolName), + }, + ...downstreamResult.content, + ], + }; + } } catch (err) { // Fail CLOSED. An approval gate that executes the action when the // prompt times out or errors defeats its own purpose — and the SDK diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index e1ef2a4..e5e1cfe 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -6,6 +6,7 @@ import { resolveFileArgs, buildRemoteArgs, FileArgsError, + getToolApproval, handleRunTool, runToolAnnotations, elicitationFailureText, @@ -248,15 +249,44 @@ describe("buildRemoteArgs", () => { }); }); -function makeRemote() { +function makeRemote(opts: { + requiresApproval?: boolean; + approvalResult?: unknown; + approvalError?: Error; +} = {}) { + const downstreamCall = vi.fn().mockResolvedValue({ + content: [{ type: "text", text: "ok" }], + }); + const callTool = vi.fn().mockImplementation(async (request: { name: string }) => { + if (request.name === "get_tool_approval") { + if (opts.approvalError) throw opts.approvalError; + return opts.approvalResult ?? { + content: [ + { + type: "text", + text: JSON.stringify({ + requires_approval: opts.requiresApproval ?? true, + }), + }, + ], + }; + } + return downstreamCall(request); + }); return { - callTool: vi.fn().mockResolvedValue({ - content: [{ type: "text", text: "ok" }], - }), + callTool, + downstreamCall, close: vi.fn(), } as any; } +function acceptThenDecline() { + return vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValue({ action: "decline" }); +} + function makeServer(opts: { elicitation?: boolean; clientName?: string; @@ -270,7 +300,7 @@ function makeServer(opts: { getClientVersion: vi .fn() .mockReturnValue({ name: opts.clientName ?? "claude-code", version: "1" }), - elicitInput: opts.elicit ?? vi.fn().mockResolvedValue({ action: "accept" }), + elicitInput: opts.elicit ?? acceptThenDecline(), // Used by primeElicitationCancellation to burn request id 0. request: opts.request ?? vi.fn().mockResolvedValue({}), } as any; @@ -306,6 +336,21 @@ async function writeModeMarker( ); } +describe("getToolApproval", () => { + it("accepts a structured remote response", async () => { + const remote = makeRemote({ + approvalResult: { + content: [], + structuredContent: { requires_approval: true }, + }, + }); + + await expect( + getToolApproval(remote, "server-1", "tool-1"), + ).resolves.toBe(true); + }); +}); + describe("handleRunTool (HITL)", () => { let tmpDir: string; const baseArgs = { @@ -332,49 +377,25 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(server.elicitInput).not.toHaveBeenCalled(); - expect(remote.callTool).toHaveBeenCalledTimes(1); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); - it("does not elicit when the tool does not require approval", async () => { + it("does not elicit when the remote says the tool does not require approval", async () => { vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); + const remote = makeRemote({ requiresApproval: false }); const server = makeServer({ elicitation: true }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(server.elicitInput).not.toHaveBeenCalled(); - expect(remote.callTool).toHaveBeenCalledTimes(1); - }); - - it("fails closed when the tool's approval requirement is unknown", async () => { - vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); - const server = makeServer({ elicitation: true, elicit }); - - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - - expect(elicit).toHaveBeenCalledTimes(1); - expect(remote.callTool).toHaveBeenCalledTimes(1); - }); - - it("does not execute unknown-approval tools when the user declines", async () => { - vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "decline" }); - const server = makeServer({ elicitation: true, elicit }); - - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - - expect(elicit).toHaveBeenCalledTimes(1); - expect(remote.callTool).not.toHaveBeenCalled(); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); it("sanitizes argument keys so newlines cannot forge prompt labels", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); @@ -400,7 +421,7 @@ describe("handleRunTool (HITL)", () => { it("DOES elicit for Cursor — our prompt is the single gate there too", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, clientName: "cursor-vscode", @@ -410,10 +431,9 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - // Cursor is no longer excluded: it gets readOnlyHint like every other - // elicitation-capable host, so this prompt is the only approval gate. - expect(elicit).toHaveBeenCalledTimes(1); - expect(remote.callTool).toHaveBeenCalledTimes(1); + // Cursor gets the same initial gate plus always-allow follow-up as other hosts. + expect(elicit).toHaveBeenCalledTimes(2); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); // Cursor used to render the tool and its arguments itself, so its prompt was only a @@ -422,7 +442,7 @@ describe("handleRunTool (HITL)", () => { it("spells out action and arguments for Cursor too, since it no longer shows them", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, clientName: "cursor-vscode", @@ -445,7 +465,7 @@ describe("handleRunTool (HITL)", () => { it("spells out action and arguments for a host that does not render them", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); @@ -486,7 +506,7 @@ describe("handleRunTool (HITL)", () => { expect(result.isError).toBe(true); expect(text).toContain("3.15"); expect(text).toContain("NOT executed"); - expect(remote.callTool).not.toHaveBeenCalled(); + expect(remote.downstreamCall).not.toHaveBeenCalled(); }); // A timeout cannot distinguish "prompt shown, nobody answered" from "prompt never @@ -546,7 +566,7 @@ describe("handleRunTool (HITL)", () => { expect(result.isError).toBe(true); expect(text).not.toContain("3.15"); expect(text).toContain("Ask the user to confirm"); - expect(remote.callTool).not.toHaveBeenCalled(); + expect(remote.downstreamCall).not.toHaveBeenCalled(); }); it("never mentions Cursor to another host, even on a full-timeout hang", async () => { @@ -565,7 +585,7 @@ describe("handleRunTool (HITL)", () => { const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect((result.content[0] as { text: string }).text).not.toContain("3.15"); - expect(remote.callTool).not.toHaveBeenCalled(); + expect(remote.downstreamCall).not.toHaveBeenCalled(); }); // fileArgs disabled by remote policy. The refusal lives here rather than at the call @@ -625,7 +645,7 @@ describe("handleRunTool (HITL)", () => { }); expect(result.isError).toBeUndefined(); - expect(remote.callTool).toHaveBeenCalledTimes(1); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); it("treats a spec-compliant cancel as a cancel, not a failure", async () => { @@ -645,13 +665,13 @@ describe("handleRunTool (HITL)", () => { expect(text).toContain("cancelled by the user"); expect(text).not.toContain("3.15"); expect(result.isError).toBeUndefined(); - expect(remote.callTool).not.toHaveBeenCalled(); + expect(remote.downstreamCall).not.toHaveBeenCalled(); }); it("prompts with action name + arguments and forwards on accept", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true, @@ -667,7 +687,7 @@ describe("handleRunTool (HITL)", () => { expect(params.message).not.toContain("Search Jira issues"); expect(params.message).not.toContain("**"); expect(options.timeout).toBe(300_000); - expect(remote.callTool).toHaveBeenCalledTimes(1); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); it("pings to burn request id 0 before the first elicitation (so timeout cancellation is honored), once per server", async () => { @@ -677,7 +697,12 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const request = vi.fn().mockResolvedValue({}); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValueOnce({ action: "decline" }) + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValueOnce({ action: "decline" }); const server = makeServer({ elicitation: true, elicit, request }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); @@ -687,13 +712,12 @@ describe("handleRunTool (HITL)", () => { // Ping fired exactly once for this server, and it is a ping. expect(request).toHaveBeenCalledTimes(1); expect(request.mock.calls[0][0]).toEqual({ method: "ping" }); - // Both prompts still ran. - expect(elicit).toHaveBeenCalledTimes(2); + expect(elicit).toHaveBeenCalledTimes(4); }); - it("does not ping when the tool requires no approval (no elicitation)", async () => { + it("does not ping when the remote says the tool requires no approval", async () => { vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); + const remote = makeRemote({ requiresApproval: false }); const request = vi.fn().mockResolvedValue({}); const server = makeServer({ elicitation: true, request }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: false }); @@ -707,7 +731,7 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("HITL_TIMEOUT_MS", "5000"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); @@ -723,7 +747,7 @@ describe("handleRunTool (HITL)", () => { for (const bad of ["0", "-1", "abc", ""]) { vi.stubEnv("HITL_TIMEOUT_MS", bad); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); @@ -741,7 +765,7 @@ describe("handleRunTool (HITL)", () => { const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(remote.callTool).not.toHaveBeenCalled(); + expect(remote.downstreamCall).not.toHaveBeenCalled(); expect((result.content[0] as { text: string }).text).toContain("declined"); }); @@ -754,7 +778,7 @@ describe("handleRunTool (HITL)", () => { const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(remote.callTool).not.toHaveBeenCalled(); + expect(remote.downstreamCall).not.toHaveBeenCalled(); expect(result.isError).toBe(true); const text = (result.content[0] as { text: string }).text; expect(text).toContain("not approved"); @@ -764,7 +788,7 @@ describe("handleRunTool (HITL)", () => { it("spills large arguments to a file and keeps the prompt short", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "create_doc", { requires_approval: true }); @@ -795,7 +819,7 @@ describe("handleRunTool (HITL)", () => { it("surfaces file_args content in the approval prompt", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "create_doc", { requires_approval: true }); const bodyFile = path.join(tmpDir, "draft.md"); @@ -811,7 +835,7 @@ describe("handleRunTool (HITL)", () => { const message = elicit.mock.calls[0][0].message as string; expect(message).toContain("TITLE: Doc"); expect(message).toContain("BODY: FILE_SOURCED_BODY"); // file-sourced arg shown - expect(remote.callTool).toHaveBeenCalledTimes(1); // executed on accept + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); // executed on accept }); it("parses an object-typed file_arg from the tool schema and forwards it as structured data", async () => { @@ -832,7 +856,7 @@ describe("handleRunTool (HITL)", () => { file_args: { spec: specFile }, }, ALL_ON); - const call = remote.callTool.mock.calls[0][0]; + const call = remote.downstreamCall.mock.calls[0][0]; expect(call.name).toBe("run_tool"); expect(call.arguments.arguments.spec).toEqual({ name: "my-agent", @@ -859,6 +883,156 @@ describe("handleRunTool (HITL)", () => { expect(remote.callTool).not.toHaveBeenCalled(); }); + it("uses the remote approval result on every attempted downstream call", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote({ requiresApproval: false }); + const server = makeServer({ elicitation: true }); + // This stale local value must not affect the remote-only decision. + await writeToolJson(tmpDir, "remote_only_tool", { requires_approval: true }); + const args = { ...baseArgs, tool_name: "remote_only_tool" }; + + await handleRunTool(remote, server, tmpDir, args, ALL_ON); + await handleRunTool(remote, server, tmpDir, args, ALL_ON); + + expect(server.elicitInput).not.toHaveBeenCalled(); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get_tool_approval", + "run_tool", + "get_tool_approval", + "run_tool", + ]); + expect(remote.callTool.mock.calls[0][0].arguments).toEqual({ + server_id: baseArgs.server_id, + tool_name: "remote_only_tool", + }); + }); + + it("prompts when the remote requires approval even if local metadata says false", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote({ requiresApproval: true }); + const elicit = acceptThenDecline(); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "remote_required_tool", { requires_approval: false }); + + await handleRunTool( + remote, + server, + tmpDir, + { ...baseArgs, tool_name: "remote_required_tool" }, + ALL_ON, + ); + + expect(elicit).toHaveBeenCalledTimes(2); + expect(elicit.mock.calls[1][0].message).toBe( + "Always allow remote_required_tool for future calls?\n\n" + + "(Auto-declines in 5 seconds)", + ); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get_tool_approval", + "run_tool", + ]); + }); + + it("persists an explicit always-allow decision before running the tool", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValueOnce({ action: "accept" }); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "always_tool", { requires_approval: true }); + + await handleRunTool( + remote, + server, + tmpDir, + { ...baseArgs, tool_name: "always_tool" }, + ALL_ON, + ); + + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get_tool_approval", + "set_tool_approval", + "run_tool", + ]); + expect(remote.callTool.mock.calls[1][0].arguments).toEqual({ + server_id: baseArgs.server_id, + tool_name: "always_tool", + value: "ALWAYS_ALLOWED", + }); + }); + + it("reports when the always-allow follow-up times out", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValueOnce({ action: "decline" }); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "timeout_tool", { requires_approval: true }); + const now = vi + .spyOn(Date, "now") + .mockReturnValueOnce(0) + .mockReturnValueOnce(5_000); + + const result = await handleRunTool( + remote, + server, + tmpDir, + { ...baseArgs, tool_name: "timeout_tool" }, + ALL_ON, + ); + now.mockRestore(); + + expect(result.isError).not.toBe(true); + expect((result.content[0] as { text: string }).text).toContain( + "timed out after 5 seconds", + ); + expect((result.content[0] as { text: string }).text).toContain( + "future calls; they will ask for approval again", + ); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get_tool_approval", + "run_tool", + ]); + }); + + it("fails closed when the remote approval lookup is malformed", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote({ + approvalResult: { content: [{ type: "text", text: "{}" }] }, + }); + const server = makeServer({ elicitation: true }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + + expect(result.isError).toBe(true); + expect((result.content[0] as { text: string }).text).toContain( + "requires approval", + ); + expect(server.elicitInput).not.toHaveBeenCalled(); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get_tool_approval", + ]); + }); + + it("fails closed when the remote approval lookup errors", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote({ approvalError: new Error("503 unavailable") }); + const server = makeServer({ elicitation: true }); + + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + + expect(result.isError).toBe(true); + expect((result.content[0] as { text: string }).text).toContain( + "The action was NOT executed", + ); + expect(server.elicitInput).not.toHaveBeenCalled(); + expect(remote.callTool).toHaveBeenCalledTimes(1); + }); + it("skips the elicitation gate and executes directly in bypassPermissions mode", async () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("CLAUDE_PLUGIN_DATA", tmpDir); @@ -872,7 +1046,7 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); expect(elicit).not.toHaveBeenCalled(); - expect(remote.callTool).toHaveBeenCalledTimes(1); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); it("still elicits when the session's permission mode is not bypass", async () => { @@ -882,13 +1056,13 @@ describe("handleRunTool (HITL)", () => { await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); await writeModeMarker(tmpDir, "sess-default", "default"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(1); - expect(remote.callTool).toHaveBeenCalledTimes(1); + expect(elicit).toHaveBeenCalledTimes(2); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); it("still elicits when no permission-mode marker exists (fails toward the gate)", async () => { @@ -898,12 +1072,12 @@ describe("handleRunTool (HITL)", () => { await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); // Deliberately write no marker. const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(1); + expect(elicit).toHaveBeenCalledTimes(2); }); it("ignores a bypass marker written for a different session (no cross-session leak)", async () => { @@ -914,12 +1088,12 @@ describe("handleRunTool (HITL)", () => { // Another concurrent session opted into bypass; ours did not. await writeModeMarker(tmpDir, "sess-B", "bypassPermissions"); const remote = makeRemote(); - const elicit = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(1); // gate preserved for THIS session + expect(elicit).toHaveBeenCalledTimes(2); // initial gate + always-allow follow-up }); }); diff --git a/shared/glean/mcp/tests/skill-writer.test.ts b/shared/glean/mcp/tests/skill-writer.test.ts index f70a8a0..2d85ead 100644 --- a/shared/glean/mcp/tests/skill-writer.test.ts +++ b/shared/glean/mcp/tests/skill-writer.test.ts @@ -61,6 +61,28 @@ describe("writeSkillsToDisk", () => { expect(toolJson.input_schema.properties.query.type).toBe("string"); }); + it("does not persist local approval requirements in tool metadata", async () => { + const skills: SkillsMap = { + "remote-approval": { + "tools/action.json": JSON.stringify({ + requires_approval: true, + inputSchema: { properties: { title: { type: "string" } } }, + }), + }, + }; + + await writeSkillsToDisk(skills, tmpDir); + + const toolJson = JSON.parse( + await fs.readFile( + path.join(tmpDir, "remote-approval", "tools", "action.json"), + "utf-8", + ), + ); + expect(toolJson.requires_approval).toBeUndefined(); + expect(toolJson.inputSchema.properties.title.type).toBe("string"); + }); + it("creates nested directories from slash-separated paths", async () => { const skills: SkillsMap = { "code-review": {