diff --git a/docs/hooks.md b/docs/hooks.md index cb060d73..aaaaf8e6 100644 --- a/docs/hooks.md +++ b/docs/hooks.md @@ -75,6 +75,9 @@ It runs before codeoid's approval gate, so: - A hook **block** short-circuits — the user is never prompted, the autonomous turn budget is never spent, and an info message explains which hook blocked and why. - A block wins even for auto-approved safe tools (Read/Grep/Glob) — the gate is uniform. - A hook **mutation** replaces the tool input before the approval prompt renders, so the user approves what will actually run. An info message records the mutation. +- A mutation applies only on a backend that runs the rewritten input: claude, pi, openai, gemini, and llama.cpp. + codex, the Gemini CLI, and qwen answer an approval with yes or no and run the input they proposed, so on those a rewritten call is **refused** (audited as `session.hook_rewrite_refused`) rather than shown and then run as originally proposed. + To change what such a backend runs, block the call and let the agent retry. This matches pi's `tool_call` extension semantics. diff --git a/packages/protocol/src/approvals.ts b/packages/protocol/src/approvals.ts index 4a2f63b9..a5d980aa 100644 --- a/packages/protocol/src/approvals.ts +++ b/packages/protocol/src/approvals.ts @@ -9,11 +9,12 @@ * is parked. Only calls actually waiting are listed — never a leftover * `waiting_confirmation` row in the scrollback from a past run. * - * Known limits, inherited from the backends (tracked separately): codex and - * the Gemini CLI (ACP) answer an approval with yes/no only and run their own - * input, so a hook REWRITE shown here does not apply on them; and codex ties - * an MCP elicitation to its call by server name, so with two calls in flight - * to one server the input shown can be the other call's. + * A hook REWRITE is listed only on backends that run the rewritten input; + * on one that runs its own input (codex, the Gemini CLI, qwen) the call is + * refused before it parks, so it never appears here (#367). Known limit + * (tracked in #368): codex ties an MCP elicitation to its call by server + * name, so with two calls in flight to one server the input shown can be the + * other call's. */ // ── Messages (client → daemon) ──────────────────────────────────────────────── diff --git a/src/daemon/providers/acp/index.ts b/src/daemon/providers/acp/index.ts index 106e9632..b31c9f6d 100644 --- a/src/daemon/providers/acp/index.ts +++ b/src/daemon/providers/acp/index.ts @@ -79,6 +79,11 @@ export interface GeminiAcpProviderInit { } export class GeminiAcpProvider implements SessionProvider { + /** + * An ACP permission reply only selects an option — the Gemini CLI runs the + * input it proposed, so a rewrite can't apply (#367). + */ + readonly appliesToolInputRewrite = false; readonly id = "gemini-cli"; readonly displayName = "Gemini CLI (Google)"; diff --git a/src/daemon/providers/claude/index.ts b/src/daemon/providers/claude/index.ts index 5f0ff1e0..e23efaf2 100644 --- a/src/daemon/providers/claude/index.ts +++ b/src/daemon/providers/claude/index.ts @@ -185,6 +185,8 @@ export const MAX_CARRYOVER_EVENTS = 100; // ── ClaudeProvider ──────────────────────────────────────────────────────────── export class ClaudeProvider implements SessionProvider { + /** The Agent SDK runs the permission result's `updatedInput` (see AgentProvider.appliesToolInputRewrite). */ + readonly appliesToolInputRewrite = true; readonly id = "claude"; /** The CLI runs the main agent itself when background work settles (#adoptTurn). */ readonly continuesAfterBackgroundWork = true; diff --git a/src/daemon/providers/codex/index.ts b/src/daemon/providers/codex/index.ts index d36467d4..7d52d2e9 100644 --- a/src/daemon/providers/codex/index.ts +++ b/src/daemon/providers/codex/index.ts @@ -236,6 +236,11 @@ export function describeCodexError(params: Record): string { } export class CodexProvider implements SessionProvider { + /** + * codex's approval replies carry only a decision (`approved` / `accept`) — + * codex runs the input IT proposed, so a rewrite can't apply (#367). + */ + readonly appliesToolInputRewrite = false; readonly id = "codex"; readonly displayName = "Codex (OpenAI)"; diff --git a/src/daemon/providers/gemini/index.ts b/src/daemon/providers/gemini/index.ts index 55ab6334..d6c22bd0 100644 --- a/src/daemon/providers/gemini/index.ts +++ b/src/daemon/providers/gemini/index.ts @@ -67,6 +67,8 @@ export interface GeminiProviderInit { } export class GeminiProvider implements AgentProvider { + /** tool-loop runs `verdict.updatedInput ?? args` (see AgentProvider.appliesToolInputRewrite). */ + readonly appliesToolInputRewrite = true; readonly id = "gemini"; readonly displayName = "Gemini (Google)"; diff --git a/src/daemon/providers/interface.ts b/src/daemon/providers/interface.ts index a15e7c63..001db25e 100644 --- a/src/daemon/providers/interface.ts +++ b/src/daemon/providers/interface.ts @@ -491,6 +491,16 @@ export interface AgentProvider { * true only after its tool mount is confirmed. */ readonly supportsMemoryTools?: boolean; + /** + * True when the backend runs the `updatedInput` an `allow` verdict carries — + * a hook's rewrite of a tool call, or an approver's form answers — instead + * of the input it proposed. Absent = false: a backend that only hears + * yes/no runs its OWN input, and showing a rewrite it then ignores would + * have the user approve one thing while another runs. Session refuses such + * a call instead (#367). Declare it only after confirming the backend + * applies the new input to what it actually executes. + */ + readonly appliesToolInputRewrite?: boolean; dispose(): Promise; } diff --git a/src/daemon/providers/mock/session-provider.ts b/src/daemon/providers/mock/session-provider.ts index 77d20bd8..a3c57c2a 100644 --- a/src/daemon/providers/mock/session-provider.ts +++ b/src/daemon/providers/mock/session-provider.ts @@ -38,6 +38,8 @@ import type { CanonicalTurn, HistorySeedResult } from "../canonical.js"; export { mockResult } from "./index.js"; export class MockSessionProvider implements SessionProvider { + /** Tests flip this to model a backend that can't apply a rewrite (#367). */ + appliesToolInputRewrite = true; readonly id: string; readonly displayName: string; diff --git a/src/daemon/providers/openai/index.ts b/src/daemon/providers/openai/index.ts index 8ffbee23..cfd0695c 100644 --- a/src/daemon/providers/openai/index.ts +++ b/src/daemon/providers/openai/index.ts @@ -101,6 +101,8 @@ const OPENAI_FALLBACK_MODELS: ModelInfo[] = [ ]; export class OpenAIProvider implements AgentProvider { + /** tool-loop runs `verdict.updatedInput ?? args` (see AgentProvider.appliesToolInputRewrite). */ + readonly appliesToolInputRewrite = true; readonly id: string; readonly displayName: string; diff --git a/src/daemon/providers/pi/bridge.ts b/src/daemon/providers/pi/bridge.ts index b59cb3cb..1854d20d 100644 --- a/src/daemon/providers/pi/bridge.ts +++ b/src/daemon/providers/pi/bridge.ts @@ -79,7 +79,13 @@ const BRIDGE_BASE_BODY = ` // Readiness handshake: PiProvider fails a turn clos } if (decision.updatedInput && typeof decision.updatedInput === "object") { // pi contract: mutations to event.input feed the actual execution. + // updatedInput is the WHOLE input (codeoid#367) — replace, don't merge: + // a rewrite that drops a key must drop it from what runs too. + for (const key of Object.keys(event.input)) { + if (!Object.prototype.hasOwnProperty.call(decision.updatedInput, key)) delete event.input[key]; + } for (const key of Object.keys(decision.updatedInput)) { + if (key === "__proto__") continue; // never re-point event.input's prototype event.input[key] = decision.updatedInput[key]; } } diff --git a/src/daemon/providers/pi/index.ts b/src/daemon/providers/pi/index.ts index 68dca68f..76fd954a 100644 --- a/src/daemon/providers/pi/index.ts +++ b/src/daemon/providers/pi/index.ts @@ -90,6 +90,8 @@ const EMPTY_STATS: PiSessionStats = { }; export class PiProvider implements SessionProvider { + /** The bridge replaces pi's live tool input with `updatedInput` (see AgentProvider.appliesToolInputRewrite). */ + readonly appliesToolInputRewrite = true; readonly id = "pi"; readonly displayName = "pi (pi.dev)"; diff --git a/src/daemon/providers/qwen/index.ts b/src/daemon/providers/qwen/index.ts index 6e1a028b..fabcc9fc 100644 --- a/src/daemon/providers/qwen/index.ts +++ b/src/daemon/providers/qwen/index.ts @@ -88,6 +88,12 @@ export interface QwenProviderInit { } export class QwenProvider implements SessionProvider { + /** + * qwen-code's SDK path assigns `updatedInput` to `request.args` but executes + * the invocation it built from the ORIGINAL args before asking (it never + * rebuilds it), so a rewrite does not apply (#367). + */ + readonly appliesToolInputRewrite = false; readonly id = "qwen"; readonly displayName = "Qwen Code (Alibaba)"; diff --git a/src/daemon/providers/stateless.ts b/src/daemon/providers/stateless.ts index c7d9384d..134d957e 100644 --- a/src/daemon/providers/stateless.ts +++ b/src/daemon/providers/stateless.ts @@ -43,6 +43,10 @@ export class StatelessSessionProvider implements SessionProvider { return this.#inner.supportsMemoryTools ?? false; } + get appliesToolInputRewrite(): boolean { + return this.#inner.appliesToolInputRewrite ?? false; + } + get backingSessionId(): string { return this.#backingSessionId; } diff --git a/src/daemon/session.ts b/src/daemon/session.ts index 856c7091..12966d76 100644 --- a/src/daemon/session.ts +++ b/src/daemon/session.ts @@ -4303,6 +4303,10 @@ export class Session { } #makeCanUseToolFn(sender: AuthContext): ToolApprovalFn { + // The backend this gate is handed to — the one that will ASK. Captured + // here, not at call time, so the rewrite check below refers to it even if + // the session's provider has changed by the time a call arrives. + const provider = this.#provider; return async (toolId, approvalId, toolName, inputObj, signal) => { // Yield once so the tool_start event is processed by the event consumer // (creating the SessionMessage) before hooks run or the approval @@ -4348,7 +4352,42 @@ export class Session { this.#broadcastRaw(infoMsg); return { behavior: "deny" as const, message: `Blocked by hook "${hookName}": ${reason}` }; } - if (hookResult.updatedInput) { + // A "rewrite" identical to the input is no rewrite: what runs is + // exactly what is shown, on every backend. + if (hookResult.updatedInput && !Bun.deepEquals(hookResult.updatedInput, inputObj)) { + // A backend that only hears yes/no runs the input IT proposed: the + // rewrite would be shown (and approved) while the original runs. + // Refuse instead (#367) — before the rewrite reaches any display. + if (!provider.appliesToolInputRewrite) { + const hooks = hookResult.mutatedBy.map((n) => `"${n}"`).join(", "); + const reason = `${provider.id} can't run a rewritten input, so the call was refused rather than run as originally proposed`; + this.#approvalPatchKeys.delete(approvalId); + this.#resolveToolCallMessage(approvalId, { + phase: "cancelled", + reason: "denied", + message: `Hook ${hooks} rewrote ${toolName}: ${reason}`, + }); + this.#store.audit( + sender.sub, + "session.hook_rewrite_refused", + this.id, + `tool=${toolName} provider=${provider.id} hooks=${hookResult.mutatedBy.join(",")}`, + ); + const infoMsg = this.#makeMessage( + "info", + `🪝 Hook ${hooks} rewrote ${toolName}, but ${reason}`, + SYSTEM_IDENTITY, + undefined, + undefined, + { event: "hook.rewrite_refused", hooks: hookResult.mutatedBy, tool: toolName, provider: provider.id }, + ); + this.#persistAndBuffer(infoMsg); + this.#broadcastRaw(infoMsg); + return { + behavior: "deny" as const, + message: `Blocked: hook ${hooks} rewrote this ${toolName} call and this backend can't run the rewritten input`, + }; + } effectiveInput = hookResult.updatedInput; this.#applyHookInputMutation(approvalId, effectiveInput); const infoMsg = this.#makeMessage( @@ -4418,6 +4457,28 @@ export class Session { this.#approvalInputs.delete(approvalId); } const { approved, updatedInput } = decision; + + // The approver's answers (a form patch) would be dropped by a backend + // that runs its own input — don't run the call without them (#367). + // Decided BEFORE the call is marked running or audited as approved. + const patchableKeys = this.#approvalPatchKeys.get(approvalId); + this.#approvalPatchKeys.delete(approvalId); + const sanitizedPatch = approved ? sanitizeApprovalPatch(toolName, updatedInput, patchableKeys) : null; + if (sanitizedPatch && !provider.appliesToolInputRewrite) { + if (this.#turnActive()) this.#setStatus("thinking"); + else this.#settleBetweenTurns(); + this.#resolveToolCallMessage(approvalId, { + phase: "cancelled", + reason: "denied", + message: `${provider.id} can't apply the answers given to ${toolName}`, + }); + this.#store.audit(sender.sub, "session.patch_refused", this.id, `tool=${toolName} provider=${provider.id}`); + return { + behavior: "deny" as const, + message: `Blocked: this backend can't apply the answers given to ${toolName}`, + }; + } + if (this.#turnActive()) this.#setStatus(approved ? "tool_running" : "thinking"); else this.#settleBetweenTurns(); @@ -4431,11 +4492,8 @@ export class Session { : { phase: "cancelled", reason: "denied" }, ); - const patchableKeys = this.#approvalPatchKeys.get(approvalId); - this.#approvalPatchKeys.delete(approvalId); if (approved) { this.#store.audit(sender.sub, "session.approve", this.id, `tool=${toolName} approvalId=${approvalId}`); - const sanitizedPatch = sanitizeApprovalPatch(toolName, updatedInput, patchableKeys); const merged: Record = sanitizedPatch ? { ...effectiveInput, ...sanitizedPatch } : effectiveInput; return { behavior: "allow" as const, updatedInput: merged }; } diff --git a/src/tests/fixtures/fake-pi.ts b/src/tests/fixtures/fake-pi.ts index 70e9a5ce..5c5c57be 100644 --- a/src/tests/fixtures/fake-pi.ts +++ b/src/tests/fixtures/fake-pi.ts @@ -75,7 +75,8 @@ async function runPrompt(message: string): Promise { decision = {}; } if (decision.behavior === "allow") { - const effective = { ...input, ...(decision.updatedInput ?? {}) }; + // Mirrors the real bridge: updatedInput REPLACES the input (codeoid#367). + const effective = decision.updatedInput ?? input; emit({ type: "tool_execution_start", toolCallId, toolName: "bash", args: effective }); emit({ type: "tool_execution_end", diff --git a/src/tests/pi-bridge.test.ts b/src/tests/pi-bridge.test.ts index 685a3431..1f2df8c8 100644 --- a/src/tests/pi-bridge.test.ts +++ b/src/tests/pi-bridge.test.ts @@ -49,3 +49,58 @@ describe("pi bridge source", () => { expect(buildBridgeSource(false, [])).not.toContain(MCP_TOOL_TITLE); }); }); + +describe("pi bridge tool_call gate — runtime (#367)", () => { + /** Load the REAL generated bridge with a stub `pi`, and return its tool_call handler. */ + async function loadToolCallHandler(): Promise<(event: Record, ctx: unknown) => Promise> { + const { mkdtempSync, writeFileSync } = await import("node:fs"); + const { tmpdir } = await import("node:os"); + const { join } = await import("node:path"); + const dir = mkdtempSync(join(tmpdir(), "pi-bridge-rt-")); + const path = join(dir, "bridge.mjs"); + writeFileSync(path, buildBridgeSource(false)); + const mod = (await import(path)) as { default: (pi: unknown) => void }; + const handlers: Record, c: unknown) => Promise> = {}; + mod.default({ + on: (evt: string, fn: (e: Record, c: unknown) => Promise) => { + handlers[evt] = fn; + }, + }); + return handlers.tool_call!; + } + const ctxAnswering = (decision: unknown) => ({ hasUI: true, ui: { input: async () => JSON.stringify(decision) } }); + + test("updatedInput REPLACES pi's input — a key the rewrite drops is dropped from what runs", async () => { + const handler = await loadToolCallHandler(); + const event: { toolCallId: string; toolName: string; input: Record } = { + toolCallId: "c1", + toolName: "bash", + input: { command: "rm -rf ~/x", force: true }, + }; + const verdict = await handler(event, ctxAnswering({ behavior: "allow", updatedInput: { command: "echo SAFE" } })); + expect(verdict).toBeUndefined(); // allowed + expect(event.input).toEqual({ command: "echo SAFE" }); + }); + + test("prototype-named keys are handled as plain keys", async () => { + const handler = await loadToolCallHandler(); + const event: { toolCallId: string; toolName: string; input: Record } = { + toolCallId: "c3", + toolName: "bash", + input: { command: "x", constructor: "dropped-by-rewrite" }, + }; + const updated = JSON.parse('{"command":"y","__proto__":{"polluted":true}}'); + await handler(event, ctxAnswering({ behavior: "allow", updatedInput: updated })); + expect(Object.keys(event.input)).toEqual(["command"]); + expect(event.input.command).toBe("y"); + expect((event.input as { polluted?: unknown }).polluted).toBeUndefined(); + }); + + test("no updatedInput leaves the input as proposed; deny blocks", async () => { + const handler = await loadToolCallHandler(); + const event = { toolCallId: "c2", toolName: "bash", input: { command: "ls" } }; + expect(await handler(event, ctxAnswering({ behavior: "allow" }))).toBeUndefined(); + expect(event.input).toEqual({ command: "ls" }); + expect(await handler(event, ctxAnswering({ behavior: "deny", message: "no" }))).toMatchObject({ block: true }); + }); +}); diff --git a/src/tests/session-hooks.test.ts b/src/tests/session-hooks.test.ts index f2064449..28eccb20 100644 --- a/src/tests/session-hooks.test.ts +++ b/src/tests/session-hooks.test.ts @@ -30,6 +30,7 @@ import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { randomUUID } from "node:crypto"; +import { Database } from "bun:sqlite"; import { Store } from "../daemon/store.js"; import { TranscriptStore } from "../daemon/transcript.js"; import { Session, type AttachedClient } from "../daemon/session.js"; @@ -524,3 +525,133 @@ describe("Session hook integration", () => { expect(payload).toMatchObject({ event: "session_end", sessionId: session.id }); }); }); + +/** + * #367: a backend that only answers yes/no runs the input IT proposed, so a + * hook's rewrite would be shown (and approved) while the original ran. Such a + * call is refused instead — before it is shown or parked. + */ +describe("hook rewrite on a backend that can't apply it (#367)", () => { + function auditActions(): string[] { + const db = new Database(join(tmp, "codeoid.db"), { readonly: true }); + try { + return (db.query("SELECT action FROM audit_log ORDER BY id").all() as Array<{ action: string }>).map((r) => r.action); + } finally { + db.close(); + } + } + + function rewritingHooks(): HookBus { + return new HookBus([ + commandEntry(`printf '{"updatedInput":{"command":"echo SAFE"}}'`, { name: "rewriter", matcher: "^Bash$" }), + ]); + } + + it("refuses the call — never runs the original, never shows the rewrite", async () => { + const provider = new MockSessionProvider("mock-session", [bashToolTurn()]); + provider.appliesToolInputRewrite = false; + const session = makeSession(provider, rewritingHooks(), { mode: "autonomous" }); + const { client, received } = makeClient(); + session.attach(client); + + await session.send("run it", TEST_AUTH); + await waitForIdle(session); + + expect(provider.canUseToolResults).toHaveLength(1); + expect(provider.canUseToolResults[0]?.behavior).toBe("deny"); + expect(provider.canUseToolResults[0]?.message).toContain("rewrote"); + const refused = received.find((m) => m.type === "session.message" && m.metadata?.event === "hook.rewrite_refused"); + expect(refused).toBeDefined(); + // The rewrite never reached a display as if it would run. + expect(received.some((m) => m.type === "session.message" && m.metadata?.event === "hook.updated_input")).toBe(false); + expect(auditActions()).toContain("session.hook_rewrite_refused"); + }); + + it("in interactive mode nothing is parked for approval", async () => { + const provider = new MockSessionProvider("mock-session", [bashToolTurn()]); + provider.appliesToolInputRewrite = false; + const session = makeSession(provider, rewritingHooks(), { mode: "interactive" }); + const { client, received } = makeClient(); + session.attach(client); + + await session.send("run it", TEST_AUTH); + await waitForIdle(session); + + expect(provider.canUseToolResults[0]?.behavior).toBe("deny"); + expect(session.pendingToolApprovals()).toEqual([]); + // The announced call ends cancelled, with the reason — not left awaiting a yes. + const final = received + .filter((m) => m.type === "session.message.delta" && m.toolStateUpdate) + .map((m) => (m.type === "session.message.delta" ? m.toolStateUpdate : undefined)) + .at(-1) as { phase?: string; message?: string } | undefined; + expect(final?.phase).toBe("cancelled"); + expect(final?.message).toContain("can't run a rewritten input"); + }); + + it("a backend that applies rewrites is unchanged", async () => { + const provider = new MockSessionProvider("mock-session", [bashToolTurn()]); + const session = makeSession(provider, rewritingHooks(), { mode: "autonomous" }); + await session.send("run it", TEST_AUTH); + await waitForIdle(session); + expect(provider.canUseToolResults[0]).toMatchObject({ behavior: "allow", updatedInput: { command: "echo SAFE" } }); + }); + + it("an approver's form answers that the backend would drop: refused, not run without them", async () => { + const provider = new MockSessionProvider("mock-session", [ + [ + { + type: "tool_start", + toolId: "t1", + sdkToolUseId: "sdk-t1", + name: "AskUserQuestion", + input: { questions: [{ question: "Which scope?", header: "Scope", options: [{ label: "A" }, { label: "B" }] }] }, + approvalId: "a1", + patchableKeys: ["answers"], + }, + { type: "turn_done", result: mockResult({ providerId: "mock-session" }) }, + ] as ProviderEvent[], + ]); + provider.appliesToolInputRewrite = false; + const session = makeSession(provider, undefined, { mode: "interactive" }); + const { client, received } = makeClient(); + session.attach(client); + await session.send("ask", TEST_AUTH); + for (let i = 0; i < 200 && session.pendingToolApprovals().length === 0; i++) await new Promise((r) => setTimeout(r, 10)); + expect(session.pendingToolApprovals()).toHaveLength(1); + + session.approve("a1", true, TEST_AUTH, { answers: { "Which scope?": "A" } }); + await waitForIdle(session); + + expect(provider.canUseToolResults[0]?.behavior).toBe("deny"); + expect(provider.canUseToolResults[0]?.message).toContain("answers"); + const actions = auditActions(); + expect(actions).toContain("session.patch_refused"); + // The user's answer is audited (they did approve); the GATE never records + // the call as approved-and-running. + const db = new Database(join(tmp, "codeoid.db"), { readonly: true }); + const gateApproved = db + .query("SELECT count(*) AS n FROM audit_log WHERE action = 'session.approve' AND detail LIKE 'tool=%'") + .get() as { n: number }; + db.close(); + expect(gateApproved.n).toBe(0); + // The card ends cancelled — never left showing an approved, running call. + const phases = received + .filter((m) => m.type === "session.message.delta" && m.toolStateUpdate) + .map((m) => (m.type === "session.message.delta" ? (m.toolStateUpdate as { phase?: string }).phase : undefined)); + expect(phases).not.toContain("executing"); + expect(phases.at(-1)).toBe("cancelled"); + }); + + it("a hook that returns the input unchanged is not a rewrite — allowed even on such a backend", async () => { + const hooks = new HookBus([ + commandEntry(`printf '{"updatedInput":{"command":"rm -rf /tmp/x"}}'`, { name: "echoer", matcher: "^Bash$" }), + ]); + const provider = new MockSessionProvider("mock-session", [bashToolTurn()]); + provider.appliesToolInputRewrite = false; + const session = makeSession(provider, hooks, { mode: "autonomous" }); + await session.send("run it", TEST_AUTH); + await waitForIdle(session); + expect(provider.canUseToolResults[0]?.behavior).toBe("allow"); + expect(auditActions()).not.toContain("session.hook_rewrite_refused"); + }); +});