From 3f7df5a1e82107f413e7f7e78e47c041967d62f2 Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Wed, 2 Sep 2026 12:11:36 +0530 Subject: [PATCH 01/11] feat(glean): persist always-allow tool approvals --- shared/glean/mcp/src/tools/run-tool.ts | 51 +++++- shared/glean/mcp/tests/run-tool.test.ts | 215 +++++++++++++++++++++--- 2 files changed, 241 insertions(+), 25 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 2436b6a..bc8175b 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -18,6 +18,12 @@ const DEFAULT_FILE_ARG_MAX_BYTES = 5 * 1024 * 1024; // pass an explicit (longer) value the prompt errors out from under the user. const defaultHitlTimeoutMs = 300_000; +// Skip repeat prompts until discovery reflects the persisted grant. +const sessionApproved = new Set(); +function approvalKey(serverId: string, toolName: string): string { + return JSON.stringify([serverId, toolName]); +} + export class FileArgsError extends Error { constructor(message: string) { super(message); @@ -229,6 +235,28 @@ async function buildApprovalMessage( return message.join("\n"); } +// This follow-up only controls approval for future calls. +const alwaysAllowFollowUpTimeoutMs = 5_000; + +async function requestAlwaysAllowFollowUp( + mcpServer: Server, + toolName: string, +): Promise { + try { + const result = await mcpServer.elicitInput( + { + message: `Always allow ${toolName} for future calls?`, + // Empty form preserves the host-native Yes/No actions. + requestedSchema: { type: "object", properties: {} } as any, + }, + { timeout: alwaysAllowFollowUpTimeoutMs }, + ); + return result.action === "accept"; + } catch { + return false; + } +} + // 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(); @@ -397,7 +425,8 @@ export async function handleRunTool( // call and never leaks across sessions. Any other or unknown mode keeps the // gate. Only bypassPermissions is skipped (deliberately narrow). const bypass = (await currentPermissionMode()) === "bypassPermissions"; - if (!bypass) { + const preApproved = sessionApproved.has(approvalKey(serverId, toolName)); + if (!bypass && !preApproved) { const message = await buildApprovalMessage(toolName, resolvedArgs); const timeout = hitlTimeoutMs(); @@ -424,6 +453,26 @@ export async function handleRunTool( ], }; } + + const alwaysAllow = await requestAlwaysAllowFollowUp( + mcpServer, + toolName, + ); + if (alwaysAllow) { + try { + await callRemoteTool(remoteClient, "set_tool_approval", { + server_id: serverId, + tool_name: toolName, + value: "ALWAYS_ALLOWED", + }); + sessionApproved.add(approvalKey(serverId, toolName)); + } catch (err) { + const detail = err instanceof Error ? err.message : String(err); + console.error( + `[set_tool_approval] failed to persist "${toolName}" to Glean: ${detail}`, + ); + } + } } 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..cda2002 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -257,6 +257,13 @@ function makeRemote() { } as any; } +function acceptThenDecline() { + return vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValue({ action: "decline" }); +} + function makeServer(opts: { elicitation?: boolean; clientName?: string; @@ -270,7 +277,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; @@ -350,12 +357,12 @@ describe("handleRunTool (HITL)", () => { 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 elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(1); + expect(elicit).toHaveBeenCalledTimes(2); expect(remote.callTool).toHaveBeenCalledTimes(1); }); @@ -374,7 +381,7 @@ describe("handleRunTool (HITL)", () => { 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 +407,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,9 +417,8 @@ 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); + // Cursor gets the same two-step gate as other elicitation-capable hosts. + expect(elicit).toHaveBeenCalledTimes(2); expect(remote.callTool).toHaveBeenCalledTimes(1); }); @@ -422,7 +428,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 +451,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 }); @@ -651,7 +657,7 @@ describe("handleRunTool (HITL)", () => { 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, @@ -677,7 +683,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,8 +698,7 @@ 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 () => { @@ -707,7 +717,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 +733,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); @@ -764,7 +774,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 +805,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"); @@ -859,6 +869,163 @@ describe("handleRunTool (HITL)", () => { expect(remote.callTool).not.toHaveBeenCalled(); }); + it("uses a plain approval form, then asks the optional follow-up", 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, "jirasearch", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); + + expect(elicit).toHaveBeenCalledTimes(2); + expect(elicit.mock.calls[0][0].requestedSchema).toEqual({ + type: "object", + properties: {}, + }); + expect(elicit.mock.calls[1][0].message).toContain( + "Always allow jirasearch for future calls?", + ); + expect(elicit.mock.calls[1][1].timeout).toBe(5_000); + }); + + it("runs once without persisting when the follow-up is declined", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = acceptThenDecline(); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "no_tool", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, { + ...baseArgs, + tool_name: "no_tool", + }, ALL_ON); + + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "run_tool", + ]); + }); + + it("runs once without persisting when the follow-up times out", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockRejectedValueOnce(new Error("Request timed out")); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "timeout_tool", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, { + ...baseArgs, + tool_name: "timeout_tool", + }, ALL_ON); + + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "run_tool", + ]); + }); + + it("runs once without persisting when the follow-up is cancelled", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValueOnce({ action: "cancel" }); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "cancel_tool", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, { + ...baseArgs, + tool_name: "cancel_tool", + }, ALL_ON); + + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "run_tool", + ]); + }); + + it("persists an accepted follow-up before running and skips future prompts", 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 }); + const args = { + server_id: "composio/jira-pack", + tool_name: "always_tool", + arguments: {}, + }; + await writeToolJson(tmpDir, "always_tool", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, args, ALL_ON); + + expect(remote.callTool.mock.calls.slice(0, 2).map((c: any) => c[0].name)).toEqual([ + "set_tool_approval", + "run_tool", + ]); + expect(remote.callTool.mock.calls[0][0].arguments).toEqual({ + server_id: "composio/jira-pack", + tool_name: "always_tool", + value: "ALWAYS_ALLOWED", + }); + + await handleRunTool(remote, server, tmpDir, args, ALL_ON); + expect(elicit).toHaveBeenCalledTimes(2); + }); + + it("does not block execution when persistence fails", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const remote = makeRemote(); + remote.callTool.mockImplementation(async (req: any) => { + if (req.name === "set_tool_approval") throw new Error("403 no scope"); + return { content: [{ type: "text", text: "ok" }] }; + }); + const elicit = vi + .fn() + .mockResolvedValueOnce({ action: "accept" }) + .mockResolvedValueOnce({ action: "accept" }); + const server = makeServer({ elicitation: true, elicit }); + const args = { + server_id: "s", + tool_name: "always_fail_tool", + arguments: {}, + }; + await writeToolJson(tmpDir, "always_fail_tool", { requires_approval: true }); + + const result = await handleRunTool(remote, server, tmpDir, args, ALL_ON); + + expect(result.isError).toBeFalsy(); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toContain( + "run_tool", + ); + errSpy.mockRestore(); + }); + + it("treats accepted follow-ups without content as one-time approval", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = acceptThenDecline(); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "contentless_tool", { requires_approval: true }); + + await handleRunTool(remote, server, tmpDir, { + ...baseArgs, + tool_name: "contentless_tool", + }, ALL_ON); + + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "run_tool", + ]); + }); + it("skips the elicitation gate and executes directly in bypassPermissions mode", async () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("CLAUDE_PLUGIN_DATA", tmpDir); @@ -882,12 +1049,12 @@ 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(elicit).toHaveBeenCalledTimes(2); expect(remote.callTool).toHaveBeenCalledTimes(1); }); @@ -898,12 +1065,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 +1081,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); // gate preserved for THIS session }); }); From e711a523add2f04b310993aa624d3be1fe2819ab Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Wed, 2 Sep 2026 14:54:54 +0530 Subject: [PATCH 02/11] Fetch tool approval from remote before execution --- shared/glean/mcp/src/skill-writer.ts | 33 ++- shared/glean/mcp/src/tools/run-tool.ts | 161 ++++++----- shared/glean/mcp/tests/run-tool.test.ts | 297 ++++++++------------ shared/glean/mcp/tests/skill-writer.test.ts | 22 ++ 4 files changed, 266 insertions(+), 247 deletions(-) diff --git a/shared/glean/mcp/src/skill-writer.ts b/shared/glean/mcp/src/skill-writer.ts index 42b0472..4ffdc98 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 bc8175b..7e2d72a 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -18,12 +18,6 @@ const DEFAULT_FILE_ARG_MAX_BYTES = 5 * 1024 * 1024; // pass an explicit (longer) value the prompt errors out from under the user. const defaultHitlTimeoutMs = 300_000; -// Skip repeat prompts until discovery reflects the persisted grant. -const sessionApproved = new Set(); -function approvalKey(serverId: string, toolName: string): string { - return JSON.stringify([serverId, toolName]); -} - export class FileArgsError extends Error { constructor(message: string) { super(message); @@ -166,7 +160,6 @@ export async function resolveFileArgs( } interface ToolMetadata { - requires_approval?: boolean; name?: string; description?: string; server_id?: string; @@ -235,28 +228,6 @@ async function buildApprovalMessage( return message.join("\n"); } -// This follow-up only controls approval for future calls. -const alwaysAllowFollowUpTimeoutMs = 5_000; - -async function requestAlwaysAllowFollowUp( - mcpServer: Server, - toolName: string, -): Promise { - try { - const result = await mcpServer.elicitInput( - { - message: `Always allow ${toolName} for future calls?`, - // Empty form preserves the host-native Yes/No actions. - requestedSchema: { type: "object", properties: {} } as any, - }, - { timeout: alwaysAllowFollowUpTimeoutMs }, - ); - return result.action === "accept"; - } catch { - return false; - } -} - // 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(); @@ -340,6 +311,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, @@ -359,9 +415,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 @@ -397,18 +453,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. @@ -425,8 +477,7 @@ export async function handleRunTool( // call and never leaks across sessions. Any other or unknown mode keeps the // gate. Only bypassPermissions is skipped (deliberately narrow). const bypass = (await currentPermissionMode()) === "bypassPermissions"; - const preApproved = sessionApproved.has(approvalKey(serverId, toolName)); - if (!bypass && !preApproved) { + if (!bypass) { const message = await buildApprovalMessage(toolName, resolvedArgs); const timeout = hitlTimeoutMs(); @@ -453,26 +504,6 @@ export async function handleRunTool( ], }; } - - const alwaysAllow = await requestAlwaysAllowFollowUp( - mcpServer, - toolName, - ); - if (alwaysAllow) { - try { - await callRemoteTool(remoteClient, "set_tool_approval", { - server_id: serverId, - tool_name: toolName, - value: "ALWAYS_ALLOWED", - }); - sessionApproved.add(approvalKey(serverId, toolName)); - } catch (err) { - const detail = err instanceof Error ? err.message : String(err); - console.error( - `[set_tool_approval] failed to persist "${toolName}" to Glean: ${detail}`, - ); - } - } } 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 cda2002..63357e7 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,11 +249,33 @@ 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; } @@ -313,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 = { @@ -339,43 +377,19 @@ 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 = acceptThenDecline(); - const server = makeServer({ elicitation: true, elicit }); - - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - - expect(elicit).toHaveBeenCalledTimes(2); - 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 () => { @@ -417,9 +431,9 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - // Cursor gets the same two-step gate as other elicitation-capable hosts. - expect(elicit).toHaveBeenCalledTimes(2); - expect(remote.callTool).toHaveBeenCalledTimes(1); + // Cursor gets the same single gate as other elicitation-capable hosts. + expect(elicit).toHaveBeenCalledTimes(1); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); // Cursor used to render the tool and its arguments itself, so its prompt was only a @@ -492,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 @@ -552,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 () => { @@ -571,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 @@ -631,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 () => { @@ -651,7 +665,7 @@ 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 () => { @@ -673,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 () => { @@ -698,12 +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" }); - expect(elicit).toHaveBeenCalledTimes(4); + expect(elicit).toHaveBeenCalledTimes(2); }); - 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 }); @@ -751,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"); }); @@ -764,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"); @@ -821,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 () => { @@ -842,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", @@ -869,161 +883,84 @@ describe("handleRunTool (HITL)", () => { expect(remote.callTool).not.toHaveBeenCalled(); }); - it("uses a plain approval form, then asks the optional follow-up", 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, "jirasearch", { requires_approval: true }); - - await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - - expect(elicit).toHaveBeenCalledTimes(2); - expect(elicit.mock.calls[0][0].requestedSchema).toEqual({ - type: "object", - properties: {}, - }); - expect(elicit.mock.calls[1][0].message).toContain( - "Always allow jirasearch for future calls?", - ); - expect(elicit.mock.calls[1][1].timeout).toBe(5_000); - }); - - it("runs once without persisting when the follow-up is declined", async () => { + it("uses the remote approval result on every attempted downstream call", async () => { vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); - const elicit = acceptThenDecline(); - const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "no_tool", { requires_approval: 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, { - ...baseArgs, - tool_name: "no_tool", - }, ALL_ON); + 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", - ]); - }); - - it("runs once without persisting when the follow-up times out", async () => { - vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); - const elicit = vi - .fn() - .mockResolvedValueOnce({ action: "accept" }) - .mockRejectedValueOnce(new Error("Request timed out")); - const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "timeout_tool", { requires_approval: true }); - - await handleRunTool(remote, server, tmpDir, { - ...baseArgs, - tool_name: "timeout_tool", - }, ALL_ON); - - expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "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("runs once without persisting when the follow-up is cancelled", async () => { + it("prompts when the remote requires approval even if local metadata says false", async () => { vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); - const elicit = vi - .fn() - .mockResolvedValueOnce({ action: "accept" }) - .mockResolvedValueOnce({ action: "cancel" }); + const remote = makeRemote({ requiresApproval: true }); + const elicit = vi.fn().mockResolvedValue({ action: "accept" }); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "cancel_tool", { requires_approval: true }); + await writeToolJson(tmpDir, "remote_required_tool", { requires_approval: false }); - await handleRunTool(remote, server, tmpDir, { - ...baseArgs, - tool_name: "cancel_tool", - }, ALL_ON); + await handleRunTool( + remote, + server, + tmpDir, + { ...baseArgs, tool_name: "remote_required_tool" }, + ALL_ON, + ); + expect(elicit).toHaveBeenCalledTimes(1); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get-tool-approval", "run_tool", ]); }); - it("persists an accepted follow-up before running and skips future prompts", 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 }); - const args = { - server_id: "composio/jira-pack", - tool_name: "always_tool", - arguments: {}, - }; - await writeToolJson(tmpDir, "always_tool", { requires_approval: true }); - - await handleRunTool(remote, server, tmpDir, args, ALL_ON); - - expect(remote.callTool.mock.calls.slice(0, 2).map((c: any) => c[0].name)).toEqual([ - "set_tool_approval", - "run_tool", - ]); - expect(remote.callTool.mock.calls[0][0].arguments).toEqual({ - server_id: "composio/jira-pack", - tool_name: "always_tool", - value: "ALWAYS_ALLOWED", - }); - - await handleRunTool(remote, server, tmpDir, args, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(2); - }); - - it("does not block execution when persistence fails", async () => { + it("fails closed when the remote approval lookup is malformed", async () => { vi.stubEnv("ENABLE_HITL", "true"); - const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - const remote = makeRemote(); - remote.callTool.mockImplementation(async (req: any) => { - if (req.name === "set_tool_approval") throw new Error("403 no scope"); - return { content: [{ type: "text", text: "ok" }] }; + const remote = makeRemote({ + approvalResult: { content: [{ type: "text", text: "{}" }] }, }); - const elicit = vi - .fn() - .mockResolvedValueOnce({ action: "accept" }) - .mockResolvedValueOnce({ action: "accept" }); - const server = makeServer({ elicitation: true, elicit }); - const args = { - server_id: "s", - tool_name: "always_fail_tool", - arguments: {}, - }; - await writeToolJson(tmpDir, "always_fail_tool", { requires_approval: true }); + const server = makeServer({ elicitation: true }); - const result = await handleRunTool(remote, server, tmpDir, args, ALL_ON); + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(result.isError).toBeFalsy(); - expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toContain( - "run_tool", + expect(result.isError).toBe(true); + expect((result.content[0] as { text: string }).text).toContain( + "requires approval", ); - errSpy.mockRestore(); + expect(server.elicitInput).not.toHaveBeenCalled(); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get-tool-approval", + ]); }); - it("treats accepted follow-ups without content as one-time approval", async () => { + it("fails closed when the remote approval lookup errors", async () => { vi.stubEnv("ENABLE_HITL", "true"); - const remote = makeRemote(); - const elicit = acceptThenDecline(); - const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "contentless_tool", { requires_approval: true }); + const remote = makeRemote({ approvalError: new Error("503 unavailable") }); + const server = makeServer({ elicitation: true }); - await handleRunTool(remote, server, tmpDir, { - ...baseArgs, - tool_name: "contentless_tool", - }, ALL_ON); + const result = await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ - "run_tool", - ]); + 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 () => { @@ -1039,7 +976,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 () => { @@ -1054,8 +991,8 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(2); - expect(remote.callTool).toHaveBeenCalledTimes(1); + expect(elicit).toHaveBeenCalledTimes(1); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); it("still elicits when no permission-mode marker exists (fails toward the gate)", async () => { @@ -1070,7 +1007,7 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(2); + expect(elicit).toHaveBeenCalledTimes(1); }); it("ignores a bypass marker written for a different session (no cross-session leak)", async () => { @@ -1086,7 +1023,7 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(2); // gate preserved for THIS session + expect(elicit).toHaveBeenCalledTimes(1); // gate preserved for THIS session }); }); 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": { From d8835607e3083ebb06d55ea2d60a90234a65af85 Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Wed, 2 Sep 2026 17:26:48 +0530 Subject: [PATCH 03/11] Fetch tool approval before each tool call --- shared/glean/mcp/src/skill-writer.ts | 2 +- shared/glean/mcp/src/tools/run-tool.ts | 45 +++++++++++++++++++- shared/glean/mcp/tests/run-tool.test.ts | 56 +++++++++++++++++++------ 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/shared/glean/mcp/src/skill-writer.ts b/shared/glean/mcp/src/skill-writer.ts index 4ffdc98..f943503 100644 --- a/shared/glean/mcp/src/skill-writer.ts +++ b/shared/glean/mcp/src/skill-writer.ts @@ -36,7 +36,7 @@ function parseFrontmatter(content: string): Record { /** * 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 + * 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. diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 7e2d72a..3d6c77b 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -228,6 +228,28 @@ async function buildApprovalMessage( return message.join("\n"); } +// This follow-up only controls approval for future calls. +const alwaysAllowFollowUpTimeoutMs = 5_000; + +async function requestAlwaysAllowFollowUp( + mcpServer: Server, + toolName: string, +): Promise { + try { + const result = await mcpServer.elicitInput( + { + message: `Always allow ${toolName} for future calls?`, + // Empty form preserves the host-native Yes/No actions. + requestedSchema: { type: "object", properties: {} } as any, + }, + { timeout: alwaysAllowFollowUpTimeoutMs }, + ); + return result.action === "accept"; + } catch { + return false; + } +} + // 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(); @@ -352,7 +374,7 @@ export async function getToolApproval( ): Promise { let result: CallToolResult; try { - result = await callRemoteTool(remoteClient, "get-tool-approval", { + result = await callRemoteTool(remoteClient, "get_tool_approval", { server_id: serverId, tool_name: toolName, }); @@ -381,7 +403,7 @@ function approvalLookupFailure( error: unknown, ): CallToolResult { const detail = error instanceof Error ? error.message : String(error); - console.error(`[get-tool-approval] ${toolName}: ${detail}`); + console.error(`[get_tool_approval] ${toolName}: ${detail}`); return { content: [ { @@ -504,6 +526,25 @@ export async function handleRunTool( ], }; } + + const alwaysAllow = await requestAlwaysAllowFollowUp( + mcpServer, + toolName, + ); + if (alwaysAllow) { + 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}`, + ); + } + } } 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 63357e7..cea36d9 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -258,7 +258,7 @@ function makeRemote(opts: { content: [{ type: "text", text: "ok" }], }); const callTool = vi.fn().mockImplementation(async (request: { name: string }) => { - if (request.name === "get-tool-approval") { + if (request.name === "get_tool_approval") { if (opts.approvalError) throw opts.approvalError; return opts.approvalResult ?? { content: [ @@ -431,8 +431,8 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - // Cursor gets the same single gate as other elicitation-capable hosts. - expect(elicit).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); }); @@ -712,7 +712,7 @@ 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" }); - expect(elicit).toHaveBeenCalledTimes(2); + expect(elicit).toHaveBeenCalledTimes(4); }); it("does not ping when the remote says the tool requires no approval", async () => { @@ -896,9 +896,9 @@ describe("handleRunTool (HITL)", () => { expect(server.elicitInput).not.toHaveBeenCalled(); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ - "get-tool-approval", + "get_tool_approval", "run_tool", - "get-tool-approval", + "get_tool_approval", "run_tool", ]); expect(remote.callTool.mock.calls[0][0].arguments).toEqual({ @@ -910,7 +910,7 @@ describe("handleRunTool (HITL)", () => { 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 = vi.fn().mockResolvedValue({ action: "accept" }); + const elicit = acceptThenDecline(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "remote_required_tool", { requires_approval: false }); @@ -922,11 +922,41 @@ describe("handleRunTool (HITL)", () => { ALL_ON, ); - expect(elicit).toHaveBeenCalledTimes(1); + expect(elicit).toHaveBeenCalledTimes(2); + 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", + "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("fails closed when the remote approval lookup is malformed", async () => { @@ -944,7 +974,7 @@ describe("handleRunTool (HITL)", () => { ); expect(server.elicitInput).not.toHaveBeenCalled(); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ - "get-tool-approval", + "get_tool_approval", ]); }); @@ -991,7 +1021,7 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(1); + expect(elicit).toHaveBeenCalledTimes(2); expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); @@ -1007,7 +1037,7 @@ describe("handleRunTool (HITL)", () => { 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 () => { @@ -1023,7 +1053,7 @@ describe("handleRunTool (HITL)", () => { 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 }); }); From be26c48acccdade169767e9aa1e2ab72215fb889 Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Thu, 3 Sep 2026 17:47:07 +0530 Subject: [PATCH 04/11] Report always-allow prompt timeouts --- shared/glean/mcp/src/tools/run-tool.ts | 44 ++++++++++++++++++++++--- shared/glean/mcp/tests/run-tool.test.ts | 36 ++++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 3d6c77b..9ae5098 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -231,10 +231,16 @@ async function buildApprovalMessage( // 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 { +): Promise { + const startedAt = Date.now(); try { const result = await mcpServer.elicitInput( { @@ -244,12 +250,24 @@ async function requestAlwaysAllowFollowUp( }, { timeout: alwaysAllowFollowUpTimeoutMs }, ); - return result.action === "accept"; + return { accepted: result.action === "accept", timedOut: false }; } catch { - return false; + 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(); @@ -531,7 +549,7 @@ export async function handleRunTool( mcpServer, toolName, ); - if (alwaysAllow) { + if (alwaysAllow.accepted) { try { await callRemoteTool(remoteClient, "set_tool_approval", { server_id: serverId, @@ -545,6 +563,24 @@ export async function handleRunTool( ); } } + + 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 cea36d9..0fab558 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -959,6 +959,42 @@ describe("handleRunTool (HITL)", () => { }); }); + 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" }) + .mockRejectedValueOnce(new Error("Request timed out")); + 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({ From 2deb8507dcf32122fc5b58063514bbdc4fa51f97 Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Thu, 3 Sep 2026 18:25:46 +0530 Subject: [PATCH 05/11] Handle auto-declined always-allow prompts --- shared/glean/mcp/src/tools/run-tool.ts | 8 +++++++- shared/glean/mcp/tests/run-tool.test.ts | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 9ae5098..b7b176d 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -250,7 +250,13 @@ async function requestAlwaysAllowFollowUp( }, { timeout: alwaysAllowFollowUpTimeoutMs }, ); - return { accepted: result.action === "accept", timedOut: false }; + const elapsedMs = Date.now() - startedAt; + return { + accepted: result.action === "accept", + timedOut: + result.action !== "accept" && + elapsedMs >= alwaysAllowFollowUpTimeoutMs * 0.9, + }; } catch { return { accepted: false, diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index 0fab558..8796fe6 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -965,7 +965,7 @@ describe("handleRunTool (HITL)", () => { const elicit = vi .fn() .mockResolvedValueOnce({ action: "accept" }) - .mockRejectedValueOnce(new Error("Request timed out")); + .mockResolvedValueOnce({ action: "decline" }); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "timeout_tool", { requires_approval: true }); const now = vi From 00a6ae84f70d483f62a3287aa8189442b841efdc Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Fri, 4 Sep 2026 15:20:19 +0530 Subject: [PATCH 06/11] Show auto-decline timing in approval prompt --- shared/glean/mcp/src/tools/run-tool.ts | 4 +++- shared/glean/mcp/tests/run-tool.test.ts | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index b7b176d..4b55eb7 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -244,7 +244,9 @@ async function requestAlwaysAllowFollowUp( try { const result = await mcpServer.elicitInput( { - message: `Always allow ${toolName} for future calls?`, + 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, }, diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index 8796fe6..e5e1cfe 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -923,6 +923,10 @@ describe("handleRunTool (HITL)", () => { ); 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", From 07741e23fa689a6e3d518b05a8c94339054d3aee Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Mon, 7 Sep 2026 14:21:45 +0530 Subject: [PATCH 07/11] Use a single form for tool approval --- shared/glean/mcp/src/tools/run-tool.ts | 176 +++++++----------- shared/glean/mcp/tests/run-tool.test.ts | 227 ++++++++++++------------ 2 files changed, 180 insertions(+), 223 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 4b55eb7..7db0a35 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -7,7 +7,6 @@ import os from "node:os"; import path from "node:path"; import { callRemoteTool } from "../remote-client.js"; import { FILE_ARGS_DISABLED_TEXT } from "../policy/enforce.js"; -import { buildCompactArgs, writeApprovalArgsFile } from "./approval-args.js"; import { resolveSessionId } from "../session-id.js"; import { hostSharedDataDir } from "../data-dir.js"; @@ -197,83 +196,57 @@ export function isCursorClient(mcpServer: Server): boolean { .startsWith("cursor"); } -// Plain text, NOT Markdown: every host, including Cursor, gets the action and -// arguments in the elicitation itself. Depending on a host to render them above -// the prompt left Cursor's review text pointing at content that no longer -// appeared in newer builds. -async function buildApprovalMessage( - toolName: string, - args: unknown, -): Promise { - const { lines, needsFile } = buildCompactArgs(args); - // Indent argument lines under "Arguments:" so the structural labels stay - // distinct from values; keys are uppercased (in compactArgLine) so a key - // reads distinctly from its value — plain-text cues that cost no vertical - // space. - const message = [ - `Action: ${toolName}`, - "Arguments:", - ...lines.map((line) => ` ${line}`), - ]; - if (needsFile) { - // Best-effort: a failed spill (e.g. a sandbox blocking writes outside the - // project dir) must never break the approval gate, so fall back to a note. - try { - const filePath = await writeApprovalArgsFile(toolName, args); - message.push(` Full arguments: ${filePath}`); - } catch { - message.push(" (some arguments truncated; full-args file unavailable)"); - } - } - 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, +// Keep this form aligned with Scio's run_tool approval UX: one required enum, +// with Always Allow first and selected by default. +const approvalField = "approval"; +const approvalAlwaysAllow = "Always Allow"; +const approvalAllow = "Allow"; +const approvalDeny = "Deny"; +const approvalCancel = "cancel"; +const approvalChoices = [ + approvalAlwaysAllow, + approvalAllow, + approvalDeny, +] as const; +type ApprovalChoice = (typeof approvalChoices)[number]; +type ApprovalDecision = ApprovalChoice | typeof approvalCancel; + +function runToolApprovalForm(toolName: string) { + return { + mode: "form" as const, + message: `Allow running the write tool ${toolName}?`, + requestedSchema: { + type: "object", + required: [approvalField], + properties: { + [approvalField]: { + type: "string", + title: "Approval", + description: `Whether to run ${toolName}.`, + enum: [...approvalChoices], + default: approvalChoices[0], + }, }, - { 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, - }; - } + } as any, + }; } -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.` - ); +function approvalDecision(result: { + action: string; + content?: unknown; +}): ApprovalDecision | null { + if (result.action === "decline") return approvalDeny; + if (result.action === "cancel") return approvalCancel; + if (result.action !== "accept") return null; + if ( + typeof result.content !== "object" || + result.content === null || + Array.isArray(result.content) + ) { + return null; + } + const choice = (result.content as Record)[approvalField]; + return approvalChoices.find((candidate) => candidate === choice) ?? null; } // A WeakSet so a short-lived server in tests doesn't leak, @@ -477,9 +450,8 @@ export async function handleRunTool( }; } - // Resolve file_args up front so the approval prompt shows the COMPLETE input - // (file-sourced values included, not just the inline `arguments`), and so an - // unreadable file_args path fails before we prompt the user. + // Resolve file_args before approval so the approved call uses the complete + // input and an unreadable model-supplied path fails before we prompt the user. const baseArgs = args.arguments != null && typeof args.arguments === "object" ? (args.arguments as Record) @@ -526,7 +498,6 @@ export async function handleRunTool( // gate. Only bypassPermissions is skipped (deliberately narrow). const bypass = (await currentPermissionMode()) === "bypassPermissions"; if (!bypass) { - const message = await buildApprovalMessage(toolName, resolvedArgs); const timeout = hitlTimeoutMs(); // Make a dummy empty request to burn JSON-RPC request id 0 @@ -535,29 +506,36 @@ export async function handleRunTool( const startedAt = Date.now(); try { const result = await mcpServer.elicitInput( - { - message, - requestedSchema: { type: "object", properties: {} } as any, - }, + runToolApprovalForm(toolName), { timeout }, ); + const decision = approvalDecision(result); - if (result.action !== "accept") { + if (decision === approvalDeny || decision === approvalCancel) { + return { + content: [ + { + type: "text", + text: `Action ${toolName} was ${decision === approvalDeny ? "declined" : "cancelled"} by the user.`, + }, + ], + }; + } + if (decision === null) { return { content: [ { type: "text", - text: `Action ${toolName} was ${result.action === "decline" ? "declined" : "cancelled"} by the user.`, + text: + `Action ${toolName} was not approved — the approval form ` + + `response was invalid. The action was NOT executed.`, }, ], + isError: true, }; } - const alwaysAllow = await requestAlwaysAllowFollowUp( - mcpServer, - toolName, - ); - if (alwaysAllow.accepted) { + if (decision === approvalAlwaysAllow) { try { await callRemoteTool(remoteClient, "set_tool_approval", { server_id: serverId, @@ -571,24 +549,6 @@ export async function handleRunTool( ); } } - - 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 e5e1cfe..ee0a473 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -280,11 +280,12 @@ function makeRemote(opts: { } as any; } -function acceptThenDecline() { - return vi - .fn() - .mockResolvedValueOnce({ action: "accept" }) - .mockResolvedValue({ action: "decline" }); +function approvalResult(choice: "Always Allow" | "Allow" | "Deny") { + return { action: "accept", content: { approval: choice } }; +} + +function allowOnce() { + return vi.fn().mockResolvedValue(approvalResult("Allow")); } function makeServer(opts: { @@ -300,7 +301,7 @@ function makeServer(opts: { getClientVersion: vi .fn() .mockReturnValue({ name: opts.clientName ?? "claude-code", version: "1" }), - elicitInput: opts.elicit ?? acceptThenDecline(), + elicitInput: opts.elicit ?? allowOnce(), // Used by primeElicitationCancellation to burn request id 0. request: opts.request ?? vi.fn().mockResolvedValue({}), } as any; @@ -392,10 +393,10 @@ describe("handleRunTool (HITL)", () => { expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); - it("sanitizes argument keys so newlines cannot forge prompt labels", async () => { + it("keeps model-supplied arguments out of approval form labels", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); @@ -406,22 +407,22 @@ describe("handleRunTool (HITL)", () => { { server_id: "s", tool_name: "jirasearch", - arguments: { "note\nACTION: read_only_lookup": "x" }, + arguments: { "note\nAPPROVAL: Always Allow": "x" }, }, ALL_ON, ); - const message = elicit.mock.calls[0][0].message as string; - expect(message).not.toMatch(/^\s*ACTION: READ_ONLY_LOOKUP/m); - expect( - message.split("\n").filter((line) => line.startsWith("Action:")), - ).toHaveLength(1); + const params = elicit.mock.calls[0][0]; + expect(params.message).toBe("Allow running the write tool jirasearch?"); + expect(JSON.stringify(params.requestedSchema)).not.toContain( + "note\\nAPPROVAL", + ); }); it("DOES elicit for Cursor — our prompt is the single gate there too", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, clientName: "cursor-vscode", @@ -431,18 +432,15 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - // Cursor gets the same initial gate plus always-allow follow-up as other hosts. - expect(elicit).toHaveBeenCalledTimes(2); + // Cursor gets the same single approval form as other hosts. + expect(elicit).toHaveBeenCalledTimes(1); expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); - // Cursor used to render the tool and its arguments itself, so its prompt was only a - // review ask pointing at them. It stopped doing that (confirmed by screenshot, Aug - // 2026), so it now gets the same self-describing text as every other host. - it("spells out action and arguments for Cursor too, since it no longer shows them", async () => { + it("renders the same approval form for Cursor", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, clientName: "cursor-vscode", @@ -450,34 +448,40 @@ describe("handleRunTool (HITL)", () => { }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, { - ...baseArgs, - arguments: { project: "ENG", summary: "ship it" }, - }, ALL_ON); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - const message = elicit.mock.calls[0][0].message as string; - expect(message).toContain("Action: jirasearch"); - expect(message).toContain("ENG"); - // Would point at something Cursor no longer draws. - expect(message).not.toContain("shown above"); + const params = elicit.mock.calls[0][0]; + expect(params.message).toBe("Allow running the write tool jirasearch?"); + expect(params.requestedSchema.properties.approval.title).toBe("Approval"); }); - it("spells out action and arguments for a host that does not render them", async () => { + it("offers a required Approval enum with Always Allow selected by default", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); - await handleRunTool(remote, server, tmpDir, { - ...baseArgs, - arguments: { project: "ENG" }, - }, ALL_ON); + await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - const message = elicit.mock.calls[0][0].message as string; - expect(message).toContain("Action: jirasearch"); - expect(message).toContain("ENG"); - expect(message).not.toContain("shown above"); + expect(elicit).toHaveBeenCalledTimes(1); + expect(elicit.mock.calls[0][0]).toEqual({ + mode: "form", + message: "Allow running the write tool jirasearch?", + requestedSchema: { + type: "object", + required: ["approval"], + properties: { + approval: { + type: "string", + title: "Approval", + description: "Whether to run jirasearch.", + enum: ["Always Allow", "Allow", "Deny"], + default: "Always Allow", + }, + }, + }, + }); }); // Cursor's pre-3.15 bug can drop the prompt, so the request burns the whole @@ -668,25 +672,22 @@ describe("handleRunTool (HITL)", () => { expect(remote.downstreamCall).not.toHaveBeenCalled(); }); - it("prompts with action name + arguments and forwards on accept", async () => { + it("forwards exactly once when the form choice is Allow", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); - await writeToolJson(tmpDir, "jirasearch", { - requires_approval: true, - description: "Search Jira issues", - }); + await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); const [params, options] = elicit.mock.calls[0]; - expect(params.message).toContain("Action: jirasearch"); - expect(params.message).toContain("PROJECT: ABC"); - expect(params.message).not.toContain("Server:"); - expect(params.message).not.toContain("Search Jira issues"); - expect(params.message).not.toContain("**"); + expect(params.message).toBe("Allow running the write tool jirasearch?"); expect(options.timeout).toBe(300_000); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get_tool_approval", + "run_tool", + ]); expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); @@ -697,12 +698,7 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const request = vi.fn().mockResolvedValue({}); - const elicit = vi - .fn() - .mockResolvedValueOnce({ action: "accept" }) - .mockResolvedValueOnce({ action: "decline" }) - .mockResolvedValueOnce({ action: "accept" }) - .mockResolvedValueOnce({ action: "decline" }); + const elicit = vi.fn().mockResolvedValue(approvalResult("Allow")); const server = makeServer({ elicitation: true, elicit, request }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); @@ -712,7 +708,7 @@ 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" }); - expect(elicit).toHaveBeenCalledTimes(4); + expect(elicit).toHaveBeenCalledTimes(2); }); it("does not ping when the remote says the tool requires no approval", async () => { @@ -731,7 +727,7 @@ describe("handleRunTool (HITL)", () => { vi.stubEnv("ENABLE_HITL", "true"); vi.stubEnv("HITL_TIMEOUT_MS", "5000"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); @@ -747,7 +743,7 @@ describe("handleRunTool (HITL)", () => { for (const bad of ["0", "-1", "abc", ""]) { vi.stubEnv("HITL_TIMEOUT_MS", bad); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); @@ -785,10 +781,10 @@ describe("handleRunTool (HITL)", () => { expect(text).toContain("NOT executed"); }); - it("spills large arguments to a file and keeps the prompt short", async () => { + it("does not embed large model-supplied arguments in the approval form", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "create_doc", { requires_approval: true }); @@ -799,27 +795,16 @@ describe("handleRunTool (HITL)", () => { arguments: { title: "Report", body: bigBody }, }, ALL_ON); - const message = elicit.mock.calls[0][0].message as string; - expect(message).toContain("Action: create_doc"); - expect(message).toContain("TITLE: Report"); - expect(message.split("\n").length).toBeLessThanOrEqual(10); - - const fileLine = message - .split("\n") - .find((l) => l.includes("Full arguments: ")); - expect(fileLine).toBeDefined(); - const marker = "Full arguments: "; - const filePath = fileLine!.slice(fileLine!.indexOf(marker) + marker.length).trim(); - const fileContent = await fs.readFile(filePath, "utf-8"); - expect(fileContent).toContain(bigBody); - expect(fileContent).toContain("## body"); - await fs.rm(filePath, { force: true }); + const params = elicit.mock.calls[0][0]; + expect(params.message).toBe("Allow running the write tool create_doc?"); + expect(JSON.stringify(params.requestedSchema)).not.toContain(bigBody); + expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); - it("surfaces file_args content in the approval prompt", async () => { + it("resolves file_args before approval and forwards them after Allow", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "create_doc", { requires_approval: true }); const bodyFile = path.join(tmpDir, "draft.md"); @@ -832,10 +817,13 @@ describe("handleRunTool (HITL)", () => { file_args: { body: bodyFile }, }, ALL_ON); - 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.downstreamCall).toHaveBeenCalledTimes(1); // executed on accept + expect(elicit.mock.calls[0][0].message).toBe( + "Allow running the write tool create_doc?", + ); + expect(remote.downstreamCall.mock.calls[0][0].arguments.arguments).toEqual({ + title: "Doc", + body: "FILE_SOURCED_BODY", + }); }); it("parses an object-typed file_arg from the tool schema and forwards it as structured data", async () => { @@ -910,7 +898,7 @@ describe("handleRunTool (HITL)", () => { 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 elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "remote_required_tool", { requires_approval: false }); @@ -922,11 +910,12 @@ describe("handleRunTool (HITL)", () => { 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(elicit).toHaveBeenCalledTimes(1); + expect(elicit.mock.calls[0][0].requestedSchema.properties.approval.enum).toEqual([ + "Always Allow", + "Allow", + "Deny", + ]); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ "get_tool_approval", "run_tool", @@ -938,8 +927,7 @@ describe("handleRunTool (HITL)", () => { const remote = makeRemote(); const elicit = vi .fn() - .mockResolvedValueOnce({ action: "accept" }) - .mockResolvedValueOnce({ action: "accept" }); + .mockResolvedValue(approvalResult("Always Allow")); const server = makeServer({ elicitation: true, elicit }); await writeToolJson(tmpDir, "always_tool", { requires_approval: true }); @@ -963,39 +951,48 @@ describe("handleRunTool (HITL)", () => { }); }); - it("reports when the always-allow follow-up times out", async () => { + it("does not execute when the accepted form choice is Deny", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); - const elicit = vi - .fn() - .mockResolvedValueOnce({ action: "accept" }) - .mockResolvedValueOnce({ action: "decline" }); + const elicit = vi.fn().mockResolvedValue(approvalResult("Deny")); 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); + await writeToolJson(tmpDir, "denied_tool", { requires_approval: true }); const result = await handleRunTool( remote, server, tmpDir, - { ...baseArgs, tool_name: "timeout_tool" }, + { ...baseArgs, tool_name: "denied_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("declined"); + expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ + "get_tool_approval", + ]); + }); + + it("fails closed when an accepted form response is missing Approval", async () => { + vi.stubEnv("ENABLE_HITL", "true"); + const remote = makeRemote(); + const elicit = vi.fn().mockResolvedValue({ action: "accept", content: {} }); + const server = makeServer({ elicitation: true, elicit }); + await writeToolJson(tmpDir, "malformed_tool", { requires_approval: true }); + + const result = await handleRunTool( + remote, + server, + tmpDir, + { ...baseArgs, tool_name: "malformed_tool" }, + ALL_ON, ); + + expect(result.isError).toBe(true); expect((result.content[0] as { text: string }).text).toContain( - "future calls; they will ask for approval again", + "approval form response was invalid", ); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ "get_tool_approval", - "run_tool", ]); }); @@ -1056,12 +1053,12 @@ describe("handleRunTool (HITL)", () => { await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); await writeModeMarker(tmpDir, "sess-default", "default"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(2); + expect(elicit).toHaveBeenCalledTimes(1); expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); @@ -1072,12 +1069,12 @@ describe("handleRunTool (HITL)", () => { await writeToolJson(tmpDir, "jirasearch", { requires_approval: true }); // Deliberately write no marker. const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(2); + expect(elicit).toHaveBeenCalledTimes(1); }); it("ignores a bypass marker written for a different session (no cross-session leak)", async () => { @@ -1088,12 +1085,12 @@ describe("handleRunTool (HITL)", () => { // Another concurrent session opted into bypass; ours did not. await writeModeMarker(tmpDir, "sess-B", "bypassPermissions"); const remote = makeRemote(); - const elicit = acceptThenDecline(); + const elicit = allowOnce(); const server = makeServer({ elicitation: true, elicit }); await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); - expect(elicit).toHaveBeenCalledTimes(2); // initial gate + always-allow follow-up + expect(elicit).toHaveBeenCalledTimes(1); // one form carries all choices }); }); From c7189801cfc812220b65d0f38c0ba6661f9e6293 Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Mon, 7 Sep 2026 15:48:57 +0530 Subject: [PATCH 08/11] Explain the default approval form choice --- shared/glean/mcp/src/tools/run-tool.ts | 6 +++++- shared/glean/mcp/tests/run-tool.test.ts | 21 +++++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 7db0a35..3862c5b 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -214,7 +214,11 @@ type ApprovalDecision = ApprovalChoice | typeof approvalCancel; function runToolApprovalForm(toolName: string) { return { mode: "form" as const, - message: `Allow running the write tool ${toolName}?`, + message: + `Allow running the write tool ${toolName}?\n\n` + + `Always Allow is selected by default. Accepting with this selection ` + + `saves approval for future calls to this tool. To change it, select a ` + + `different Approval option below.`, requestedSchema: { type: "object", required: [approvalField], diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index ee0a473..85ec5ce 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -288,6 +288,15 @@ function allowOnce() { return vi.fn().mockResolvedValue(approvalResult("Allow")); } +function expectedApprovalMessage(toolName: string): string { + return ( + `Allow running the write tool ${toolName}?\n\n` + + `Always Allow is selected by default. Accepting with this selection ` + + `saves approval for future calls to this tool. To change it, select a ` + + `different Approval option below.` + ); +} + function makeServer(opts: { elicitation?: boolean; clientName?: string; @@ -413,7 +422,7 @@ describe("handleRunTool (HITL)", () => { ); const params = elicit.mock.calls[0][0]; - expect(params.message).toBe("Allow running the write tool jirasearch?"); + expect(params.message).toBe(expectedApprovalMessage("jirasearch")); expect(JSON.stringify(params.requestedSchema)).not.toContain( "note\\nAPPROVAL", ); @@ -451,7 +460,7 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); const params = elicit.mock.calls[0][0]; - expect(params.message).toBe("Allow running the write tool jirasearch?"); + expect(params.message).toBe(expectedApprovalMessage("jirasearch")); expect(params.requestedSchema.properties.approval.title).toBe("Approval"); }); @@ -467,7 +476,7 @@ describe("handleRunTool (HITL)", () => { expect(elicit).toHaveBeenCalledTimes(1); expect(elicit.mock.calls[0][0]).toEqual({ mode: "form", - message: "Allow running the write tool jirasearch?", + message: expectedApprovalMessage("jirasearch"), requestedSchema: { type: "object", required: ["approval"], @@ -682,7 +691,7 @@ describe("handleRunTool (HITL)", () => { await handleRunTool(remote, server, tmpDir, baseArgs, ALL_ON); const [params, options] = elicit.mock.calls[0]; - expect(params.message).toBe("Allow running the write tool jirasearch?"); + expect(params.message).toBe(expectedApprovalMessage("jirasearch")); expect(options.timeout).toBe(300_000); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ "get_tool_approval", @@ -796,7 +805,7 @@ describe("handleRunTool (HITL)", () => { }, ALL_ON); const params = elicit.mock.calls[0][0]; - expect(params.message).toBe("Allow running the write tool create_doc?"); + expect(params.message).toBe(expectedApprovalMessage("create_doc")); expect(JSON.stringify(params.requestedSchema)).not.toContain(bigBody); expect(remote.downstreamCall).toHaveBeenCalledTimes(1); }); @@ -818,7 +827,7 @@ describe("handleRunTool (HITL)", () => { }, ALL_ON); expect(elicit.mock.calls[0][0].message).toBe( - "Allow running the write tool create_doc?", + expectedApprovalMessage("create_doc"), ); expect(remote.downstreamCall.mock.calls[0][0].arguments.arguments).toEqual({ title: "Doc", From 60535c22a5e89b5470a7e9bf4f3a64e387b53386 Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Tue, 8 Sep 2026 00:43:42 +0530 Subject: [PATCH 09/11] Add visual indicators to approval choices --- shared/glean/mcp/src/tools/run-tool.ts | 6 +++++- shared/glean/mcp/tests/run-tool.test.ts | 14 +++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 3862c5b..248ad6f 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -227,7 +227,11 @@ function runToolApprovalForm(toolName: string) { type: "string", title: "Approval", description: `Whether to run ${toolName}.`, - enum: [...approvalChoices], + oneOf: [ + { const: approvalAlwaysAllow, title: "🟢 Always Allow" }, + { const: approvalAllow, title: "⚪ Allow" }, + { const: approvalDeny, title: "🔴 Deny" }, + ], default: approvalChoices[0], }, }, diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index 85ec5ce..5775685 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -485,7 +485,11 @@ describe("handleRunTool (HITL)", () => { type: "string", title: "Approval", description: "Whether to run jirasearch.", - enum: ["Always Allow", "Allow", "Deny"], + oneOf: [ + { const: "Always Allow", title: "🟢 Always Allow" }, + { const: "Allow", title: "⚪ Allow" }, + { const: "Deny", title: "🔴 Deny" }, + ], default: "Always Allow", }, }, @@ -920,10 +924,10 @@ describe("handleRunTool (HITL)", () => { ); expect(elicit).toHaveBeenCalledTimes(1); - expect(elicit.mock.calls[0][0].requestedSchema.properties.approval.enum).toEqual([ - "Always Allow", - "Allow", - "Deny", + expect(elicit.mock.calls[0][0].requestedSchema.properties.approval.oneOf).toEqual([ + { const: "Always Allow", title: "🟢 Always Allow" }, + { const: "Allow", title: "⚪ Allow" }, + { const: "Deny", title: "🔴 Deny" }, ]); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ "get_tool_approval", From cf8628efe26a50b40197a9905ef6ec1889959dca Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Tue, 8 Sep 2026 00:52:59 +0530 Subject: [PATCH 10/11] Clarify persistent approval choices --- shared/glean/mcp/src/tools/run-tool.ts | 10 +++++----- shared/glean/mcp/tests/run-tool.test.ts | 24 +++++++++++++----------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 248ad6f..0b48bbe 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -218,19 +218,19 @@ function runToolApprovalForm(toolName: string) { `Allow running the write tool ${toolName}?\n\n` + `Always Allow is selected by default. Accepting with this selection ` + `saves approval for future calls to this tool. To change it, select a ` + - `different Approval option below.`, + `different Persistent approval option below.`, requestedSchema: { type: "object", required: [approvalField], properties: { [approvalField]: { type: "string", - title: "Approval", + title: "Persistent approval", description: `Whether to run ${toolName}.`, oneOf: [ - { const: approvalAlwaysAllow, title: "🟢 Always Allow" }, - { const: approvalAllow, title: "⚪ Allow" }, - { const: approvalDeny, title: "🔴 Deny" }, + { const: approvalAlwaysAllow, title: "Always allow" }, + { const: approvalAllow, title: "Allow once" }, + { const: approvalDeny, title: "Deny" }, ], default: approvalChoices[0], }, diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index 5775685..c047330 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -293,7 +293,7 @@ function expectedApprovalMessage(toolName: string): string { `Allow running the write tool ${toolName}?\n\n` + `Always Allow is selected by default. Accepting with this selection ` + `saves approval for future calls to this tool. To change it, select a ` + - `different Approval option below.` + `different Persistent approval option below.` ); } @@ -461,10 +461,12 @@ describe("handleRunTool (HITL)", () => { const params = elicit.mock.calls[0][0]; expect(params.message).toBe(expectedApprovalMessage("jirasearch")); - expect(params.requestedSchema.properties.approval.title).toBe("Approval"); + expect(params.requestedSchema.properties.approval.title).toBe( + "Persistent approval", + ); }); - it("offers a required Approval enum with Always Allow selected by default", async () => { + it("offers a required Persistent approval choice with Always allow selected by default", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const elicit = allowOnce(); @@ -483,12 +485,12 @@ describe("handleRunTool (HITL)", () => { properties: { approval: { type: "string", - title: "Approval", + title: "Persistent approval", description: "Whether to run jirasearch.", oneOf: [ - { const: "Always Allow", title: "🟢 Always Allow" }, - { const: "Allow", title: "⚪ Allow" }, - { const: "Deny", title: "🔴 Deny" }, + { const: "Always Allow", title: "Always allow" }, + { const: "Allow", title: "Allow once" }, + { const: "Deny", title: "Deny" }, ], default: "Always Allow", }, @@ -925,9 +927,9 @@ describe("handleRunTool (HITL)", () => { expect(elicit).toHaveBeenCalledTimes(1); expect(elicit.mock.calls[0][0].requestedSchema.properties.approval.oneOf).toEqual([ - { const: "Always Allow", title: "🟢 Always Allow" }, - { const: "Allow", title: "⚪ Allow" }, - { const: "Deny", title: "🔴 Deny" }, + { const: "Always Allow", title: "Always allow" }, + { const: "Allow", title: "Allow once" }, + { const: "Deny", title: "Deny" }, ]); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ "get_tool_approval", @@ -985,7 +987,7 @@ describe("handleRunTool (HITL)", () => { ]); }); - it("fails closed when an accepted form response is missing Approval", async () => { + it("fails closed when an accepted form response is missing its approval choice", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const elicit = vi.fn().mockResolvedValue({ action: "accept", content: {} }); From 19ada28300ce445df4624bf302f780f4c7b6e110 Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Tue, 8 Sep 2026 01:04:03 +0530 Subject: [PATCH 11/11] Align approval form labels --- shared/glean/mcp/src/tools/run-tool.ts | 10 +++------- shared/glean/mcp/tests/run-tool.test.ts | 24 +++++++++--------------- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/shared/glean/mcp/src/tools/run-tool.ts b/shared/glean/mcp/src/tools/run-tool.ts index 0b48bbe..3862c5b 100644 --- a/shared/glean/mcp/src/tools/run-tool.ts +++ b/shared/glean/mcp/src/tools/run-tool.ts @@ -218,20 +218,16 @@ function runToolApprovalForm(toolName: string) { `Allow running the write tool ${toolName}?\n\n` + `Always Allow is selected by default. Accepting with this selection ` + `saves approval for future calls to this tool. To change it, select a ` + - `different Persistent approval option below.`, + `different Approval option below.`, requestedSchema: { type: "object", required: [approvalField], properties: { [approvalField]: { type: "string", - title: "Persistent approval", + title: "Approval", description: `Whether to run ${toolName}.`, - oneOf: [ - { const: approvalAlwaysAllow, title: "Always allow" }, - { const: approvalAllow, title: "Allow once" }, - { const: approvalDeny, title: "Deny" }, - ], + enum: [...approvalChoices], default: approvalChoices[0], }, }, diff --git a/shared/glean/mcp/tests/run-tool.test.ts b/shared/glean/mcp/tests/run-tool.test.ts index c047330..3994923 100644 --- a/shared/glean/mcp/tests/run-tool.test.ts +++ b/shared/glean/mcp/tests/run-tool.test.ts @@ -293,7 +293,7 @@ function expectedApprovalMessage(toolName: string): string { `Allow running the write tool ${toolName}?\n\n` + `Always Allow is selected by default. Accepting with this selection ` + `saves approval for future calls to this tool. To change it, select a ` + - `different Persistent approval option below.` + `different Approval option below.` ); } @@ -461,12 +461,10 @@ describe("handleRunTool (HITL)", () => { const params = elicit.mock.calls[0][0]; expect(params.message).toBe(expectedApprovalMessage("jirasearch")); - expect(params.requestedSchema.properties.approval.title).toBe( - "Persistent approval", - ); + expect(params.requestedSchema.properties.approval.title).toBe("Approval"); }); - it("offers a required Persistent approval choice with Always allow selected by default", async () => { + it("offers a required Approval enum with Always Allow selected by default", async () => { vi.stubEnv("ENABLE_HITL", "true"); const remote = makeRemote(); const elicit = allowOnce(); @@ -485,13 +483,9 @@ describe("handleRunTool (HITL)", () => { properties: { approval: { type: "string", - title: "Persistent approval", + title: "Approval", description: "Whether to run jirasearch.", - oneOf: [ - { const: "Always Allow", title: "Always allow" }, - { const: "Allow", title: "Allow once" }, - { const: "Deny", title: "Deny" }, - ], + enum: ["Always Allow", "Allow", "Deny"], default: "Always Allow", }, }, @@ -926,10 +920,10 @@ describe("handleRunTool (HITL)", () => { ); expect(elicit).toHaveBeenCalledTimes(1); - expect(elicit.mock.calls[0][0].requestedSchema.properties.approval.oneOf).toEqual([ - { const: "Always Allow", title: "Always allow" }, - { const: "Allow", title: "Allow once" }, - { const: "Deny", title: "Deny" }, + expect(elicit.mock.calls[0][0].requestedSchema.properties.approval.enum).toEqual([ + "Always Allow", + "Allow", + "Deny", ]); expect(remote.callTool.mock.calls.map((c: any) => c[0].name)).toEqual([ "get_tool_approval",