smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(extensions): resolve tool approval against the revised tool input

Browse Source 
Follow-up to the earlier approval re-check, which missed the
prompt-to-prompt case: if the original and revised inputs both resolve
to `prompt`, a handler could swap in different prompt-gated args that ran
under approval granted for the original.

Emit the `tool_call` event before the approval gate instead of after, so
the gate resolves policy and shows the interactive prompt against the
input that actually executes. The user always approves what runs, and
deny/allow-to-prompt/prompt-to-prompt transitions are all covered by one
gate rather than special-cased. A `deny` on the original input still
short-circuits before the runner is touched, so an already-denied tool
never emits `tool_call`.

Tests: the approval prompt reflects the revised input, `tool_call` fires
before `tool_approval_requested`, plus the existing deny/computer/
multi-handler cases.
This commit is contained in:
Larry Gordon
2026-07-26 00:30:38 -07:00
parent 8916e7de9f
commit d683cf90e7
3 changed files with 224 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/extensibility/extensions/wrapper.ts
+56 -60
View File
@@ -157,15 +157,63 @@ export class ExtensionToolWrapper<TParameters extends TSchema = TSchema, TDetail
onUpdate?: AgentToolUpdateCallback<TDetails, TParameters>,
context?: AgentToolContext,
): Promise<AgentToolResult<TDetails, TParameters>> {
// 1. Check approval policy (before extension handlers).
// CLI `--auto-approve` / `--yolo` sets approval mode to yolo.
// User `tools.approval.<tool>` 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<string, unknown>;
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<TParameters extends TSchema = TSchema, TDetail
});
}
const resolveApproval = async (approved: boolean, reason?: string) => {
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<TParameters extends TSchema = TSchema, TDetail
// ordinary tier approval, no setting or yolo mode may bypass this gate.
if (!this.runner.hasUI()) {
const reason = "no interactive UI available";
await resolveApproval(false, reason);
await emitApprovalResolved(false, reason);
if (pendingSafetyChecks.length > 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<TParameters extends TSchema = TSchema, TDetail
try {
choice = await uiContext.select(safetyPrompt, ["Approve", "Deny"]);
} catch (err) {
await resolveApproval(false, err instanceof Error ? err.message : "approval aborted");
await emitApprovalResolved(false, err instanceof Error ? err.message : "approval aborted");
throw err;
}
const approved = choice === "Approve";
await resolveApproval(approved, approved ? undefined : "denied by user");
await emitApprovalResolved(approved, approved ? undefined : "denied by user");
if (!approved) {
throw new Error(`Tool call denied by user: ${this.tool.name}`);
}
@@ -257,58 +305,6 @@ export class ExtensionToolWrapper<TParameters extends TSchema = TSchema, TDetail
}
}
// 2. Emit tool_call event - extensions can block execution or revise the input the tool runs with
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;
// The approval/safety gate above resolved against the original `params`. Re-resolve the
// policy on the revised input so a handler cannot rewrite approved args into ones a
// `deny`/critical policy would have blocked. This re-checks policy only (no second
// interactive prompt): a revised arg that newly resolves to `deny` — or, outside yolo,
// newly requires a prompt the original didn't — is blocked rather than run unapproved.
const revisedArgs = approvalArgs(effectiveParams, context);
const revised = resolveApproval(this.tool, revisedArgs, approvalMode, userPolicies);
if (revised.policy === "deny") {
throw new Error(
`Tool "${this.tool.name}" revised input is blocked by policy` +
`${revised.reason ? `: ${revised.reason}` : "."}`,
);
}
if (approvalMode !== "yolo" && revised.policy === "prompt" && resolved.policy !== "prompt") {
throw new Error(
`Tool "${this.tool.name}" revised input requires approval that the original did not; ` +
`blocking the unapproved revision.`,
);
}
}
} catch (err) {
if (err instanceof Error) {
throw err;
}
throw new Error(`Extension failed, blocking execution: ${String(err)}`);
}
}
// Execute the actual tool
let result: AgentToolResult<TDetails, TParameters>;
let executionError: Error | undefined;
packages/coding-agent/src/extensibility/shared-events.ts
+3 -3
View File
@@ -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<string, unknown>;
}
packages/coding-agent/test/extensions-runner.test.ts
+165 -4
View File
@@ -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<string | undefined>,
) => {
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<any>).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<any>).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 () => {