diff --git a/packages/coding-agent/src/extensibility/extensions/wrapper.ts b/packages/coding-agent/src/extensibility/extensions/wrapper.ts index b62833a36..a4bc0665d 100644 --- a/packages/coding-agent/src/extensibility/extensions/wrapper.ts +++ b/packages/coding-agent/src/extensibility/extensions/wrapper.ts @@ -157,15 +157,63 @@ export class ExtensionToolWrapper, context?: AgentToolContext, ): Promise> { - // 1. Check approval policy (before extension handlers). - // CLI `--auto-approve` / `--yolo` sets approval mode to yolo. - // User `tools.approval.` policies are still applied in all modes. + // Resolve approval settings up front. A `deny` on the original input short-circuits before the + // runner is touched — an already-denied tool never emits `tool_call` — while the full gate below + // re-resolves against the (possibly revised) input so a handler cannot rewrite into a denied or + // newly prompt-gated command and have it run unapproved. const cliAutoApprove = context?.autoApprove === true; const settings: Settings | undefined = context?.settings; const configuredMode = (settings?.get("tools.approvalMode") ?? "yolo") as ApprovalMode; const approvalMode: ApprovalMode = cliAutoApprove ? "yolo" : configuredMode; const userPolicies = (settings?.get("tools.approval") ?? {}) as Record; - const resolvedArgs = approvalArgs(params, context); + if (resolveApproval(this.tool, approvalArgs(params, context), approvalMode, userPolicies).policy === "deny") { + throw new Error( + `Tool "${this.tool.name}" is blocked by user policy.\n` + + `To allow: remove "tools.approval.${this.tool.name}: deny" from config.`, + ); + } + + // 1. Emit tool_call event first - extensions can block execution or revise the input the tool + // runs with. Doing this BEFORE the approval gate means approval (below) resolves against the + // input that actually executes, closing the "approve one thing, run another" gap: the prompt + // text, policy resolution, and provider safety checks all see `effectiveParams`. + let effectiveParams = params; + if (this.runner.hasHandlers("tool_call")) { + try { + const callResult = (await this.runner.emitToolCall({ + type: "tool_call", + toolName: this.tool.name, + toolCallId, + input: normalizeToolEventInput( + this.tool.name, + resolveToolEventInput(this.tool, toolEventArgs(params, context)), + ), + })) as ToolCallEventResult | undefined; + + if (callResult?.block) { + const reason = callResult.reason || "Tool execution was blocked by an extension"; + throw new Error(reason); + } + // A non-blocking handler may replace the execution input. The returned object is the raw + // input passed to `execute` (handler-owned; not re-normalized). Skipped for `computer` + // tool calls, whose event input is a synthetic {actions,pendingSafetyChecks} view + // (see toolEventArgs) rather than the real execution params. + if (callResult?.input !== undefined && context?.toolCall?.providerMetadata?.type !== "computer") { + effectiveParams = callResult.input as typeof params; + } + } catch (err) { + if (err instanceof Error) { + throw err; + } + throw new Error(`Extension failed, blocking execution: ${String(err)}`); + } + } + + // 2. Full approval gate against the (possibly revised) input that will actually run — resolves + // policy and prompts on `effectiveParams`, so the user approves exactly what executes. A revised + // input that newly resolves to `deny` is caught here even though the original passed the + // short-circuit above. + const resolvedArgs = approvalArgs(effectiveParams, context); const resolved = resolveApproval(this.tool, resolvedArgs, approvalMode, userPolicies); if (resolved.policy === "deny") { throw new Error( @@ -202,7 +250,7 @@ export class ExtensionToolWrapper { + const emitApprovalResolved = async (approved: boolean, reason?: string) => { if (!hasApprovalHandlers) return; await this.runner.emit({ type: "tool_approval_resolved", @@ -218,7 +266,7 @@ export class ExtensionToolWrapper 0) { throw new Error( `Tool "${this.tool.name}" has pending provider safety checks but no interactive UI is available.`, @@ -243,11 +291,11 @@ export class ExtensionToolWrapper; let executionError: Error | undefined; diff --git a/packages/coding-agent/src/extensibility/shared-events.ts b/packages/coding-agent/src/extensibility/shared-events.ts index 05e07a4ba..cea60e6c7 100644 --- a/packages/coding-agent/src/extensibility/shared-events.ts +++ b/packages/coding-agent/src/extensibility/shared-events.ts @@ -300,9 +300,9 @@ export interface ToolCallEventResult { * owns its correctness) — not the normalized `event.input` view, which may carry derived * gate-only fields (e.g. hashline `edit` `path`/`paths`) that are not real parameters. When * multiple handlers set `input`, the last one wins; handlers do not observe each other's - * revisions (each sees the original `event.input`). Not applied to `computer` tool calls. On an - * approval-gated tool the revised input is re-checked against approval policy, so a revision that - * newly resolves to `deny` (or, outside yolo, newly requires a prompt) is blocked rather than run. + * revisions (each sees the original `event.input`). Not applied to `computer` tool calls. The + * `tool_call` event fires before the approval gate, so an approval-gated tool prompts for and + * resolves policy against the revised input — the user always approves what actually runs. */ input?: Record; } diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index a2654014f..11cf45b4e 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -1993,7 +1993,52 @@ describe("ExtensionRunner", () => { settings: { get: (key: string) => (key === "tools.approvalMode" ? "yolo" : {}) }, } as never; - it("blocks a revised input that a deny policy would have rejected (re-checks approval)", async () => { + // Minimal runtime init so the approval gate's interactive `select` is wired for prompt-path tests. + const initApprovalRunner = ( + runner: ExtensionRunner, + select: (title: string, options: string[]) => Promise, + ) => { + runner.initialize( + { + sendMessage: () => {}, + sendUserMessage: () => {}, + appendEntry: () => {}, + setLabel: () => {}, + getActiveTools: () => [], + getAllTools: () => [], + setActiveTools: async () => {}, + getCommands: () => [], + setModel: async () => false, + getThinkingLevel: () => undefined, + setThinkingLevel: () => {}, + getSessionName: () => undefined, + setSessionName: async () => {}, + } as never, + { + getModel: () => undefined, + isIdle: () => true, + abort: () => {}, + hasPendingMessages: () => false, + shutdown: () => {}, + getContextUsage: () => undefined, + compact: async () => {}, + getSystemPrompt: () => [], + } as never, + undefined, + { select, notify: () => {} } as never, + ); + }; + const alwaysAskContext = { + sessionManager, + modelRegistry, + model: undefined, + isIdle: () => true, + hasQueuedMessages: () => false, + abort: () => {}, + settings: { get: (key: string) => (key === "tools.approvalMode" ? "always-ask" : {}) }, + } as never; + + it("blocks a revised input that resolves to a deny policy (approval gates the revised args)", async () => { const recordPath = path.join(tempDir.path(), "regate-blocked.jsonl"); const extCode = ` export default function(pi) { @@ -2015,11 +2060,12 @@ describe("ExtensionRunner", () => { ); const wrapped = new ExtensionToolWrapper(createArgGatedTool(recordPath), runner); - // Original "echo original" resolves to exec (allowed under yolo); the handler rewrites it to - // "rm -rf", which the tool's approval declares deny — the re-check must block it. + // Original "echo original" resolves to exec; the handler rewrites it to "rm -rf", which the + // tool's approval declares deny. Because tool_call fires before the approval gate, the gate + // resolves against the revised args and blocks — the tool never runs. await expect( wrapped.execute("tool-call-id", { command: "echo original" }, undefined, undefined, yoloContext), - ).rejects.toThrow(/blocked by policy/); + ).rejects.toThrow(/blocked by user policy/); expect(fs.existsSync(recordPath)).toBe(false); // tool never executed }); @@ -2096,6 +2142,121 @@ describe("ExtensionRunner", () => { .map(line => JSON.parse(line)); expect(executed).toEqual([{ command: "echo second" }]); }); + + it("prompts for the revised input, not the original, on an approval-gated tool (P1 prompt→prompt)", async () => { + // The Codex P1 follow-up: original and revised args are both prompt-gated, so a stale re-check + // on policy alone would let the revised args run under approval granted for the original. + // Because tool_call fires before the approval gate, the prompt must reflect the revised args. + const extCode = ` + export default function(pi) { + pi.on("tool_call", async (event) => { + if (event.toolName !== "prompt_tool") return; + return { input: { command: "revised-command" } }; + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "tool-call-prompt-revise.ts"), extCode); + + const result = await loadTestExtensions(); + const runner = new ExtensionRunner( + result.extensions, + result.runtime, + tempDir.path(), + sessionManager, + modelRegistry, + ); + let promptedWith = ""; + const select = vi.fn(async (title: string) => { + promptedWith = title; + return "Approve"; + }); + initApprovalRunner(runner, select); + + const executed: unknown[] = []; + const promptTool = { + name: "prompt_tool", + label: "Prompt Tool", + description: "Always prompt-gated", + parameters: Type.Object({ command: Type.String() }), + strict: true, + approval: "exec" as const, + formatApprovalDetails: (args: unknown) => + args && typeof args === "object" && "command" in args ? String(args.command) : "", + execute: async (_id: string, params: unknown) => { + executed.push(params); + return { content: [{ type: "text", text: "ran" }] }; + }, + } as AgentTool; + const wrapped = new ExtensionToolWrapper(promptTool, runner); + + await (wrapped as ExtensionToolWrapper).execute( + "call-p2p", + { command: "original-command" }, + undefined, + undefined, + alwaysAskContext, + ); + + // The user was prompted for the revised command, and that is what executed. + expect(promptedWith).toContain("revised-command"); + expect(promptedWith).not.toContain("original-command"); + expect(executed).toEqual([{ command: "revised-command" }]); + }); + + it("emits tool_call before the approval prompt so approval sees the final input", async () => { + const order: string[] = []; + const extCode = ` + export default function(pi) { + pi.on("tool_call", async (event) => { + if (event.toolName !== "prompt_tool") return; + globalThis.__orderEvents.push("tool_call"); + return { input: { command: "revised" } }; + }); + pi.on("tool_approval_requested", async () => { + globalThis.__orderEvents.push("tool_approval_requested"); + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "tool-call-order.ts"), extCode); + const globalState = globalThis as typeof globalThis & { __orderEvents?: string[] }; + globalState.__orderEvents = order; + + const result = await loadTestExtensions(); + const runner = new ExtensionRunner( + result.extensions, + result.runtime, + tempDir.path(), + sessionManager, + modelRegistry, + ); + const select = vi.fn(async () => { + order.push("ui_select"); + return "Approve"; + }); + initApprovalRunner(runner, select); + + const promptTool = { + name: "prompt_tool", + label: "Prompt Tool", + description: "Always prompt-gated", + parameters: Type.Object({ command: Type.String() }), + strict: true, + approval: "exec" as const, + execute: async () => ({ content: [{ type: "text", text: "ran" }] }), + } as AgentTool; + const wrapped = new ExtensionToolWrapper(promptTool, runner); + + await (wrapped as ExtensionToolWrapper).execute( + "call-order", + { command: "original" }, + undefined, + undefined, + alwaysAskContext, + ); + + expect(order).toEqual(["tool_call", "tool_approval_requested", "ui_select"]); + delete globalState.__orderEvents; + }); }); describe("hasHandlers", () => { it("returns true when handlers exist for event type", async () => {