diff --git a/codex-cli/src/approvals.ts b/codex-cli/src/approvals.ts index cfd8111b..8a670b01 100644 --- a/codex-cli/src/approvals.ts +++ b/codex-cli/src/approvals.ts @@ -84,11 +84,6 @@ export function canAutoApprove( }; } - // In 'suggest' mode, all shell commands should require user permission - if (policy === "suggest") { - return { type: "ask-user" }; - } - const isSafe = isSafeCommand(command); if (isSafe != null) { const { reason, group } = isSafe; @@ -117,23 +112,23 @@ export function canAutoApprove( } catch (e) { // In practice, there seem to be syntactically valid shell commands that // shell-quote cannot parse, so we should not reject, but ask the user. - // We already checked for 'suggest' mode at the beginning of the function, - // so at this point we know policy is either 'auto-edit' or 'full-auto' - if (policy === "full-auto") { - // In full-auto, we still run the command automatically, but must - // restrict it to the sandbox. - return { - type: "auto-approve", - reason: "Full auto mode", - group: "Running commands", - runInSandbox: true, - }; - } else { - // In auto-edit mode, since we cannot reason about the command, we - // should ask the user. - return { - type: "ask-user", - }; + switch (policy) { + case "full-auto": + // In full-auto, we still run the command automatically, but must + // restrict it to the sandbox. + return { + type: "auto-approve", + reason: "Full auto mode", + group: "Running commands", + runInSandbox: true, + }; + case "suggest": + case "auto-edit": + // In all other modes, since we cannot reason about the command, we + // should ask the user. + return { + type: "ask-user", + }; } } @@ -143,9 +138,6 @@ export function canAutoApprove( // all operators belong to an allow‑list. If so, the entire expression is // considered auto‑approvable. - // We already checked for 'suggest' mode at the beginning of the function, - // so at this point we know policy is either 'auto-edit' or 'full-auto' - const shellSafe = isEntireShellExpressionSafe(bashCmd); if (shellSafe != null) { const { reason, group } = shellSafe; diff --git a/codex-cli/tests/approvals.test.ts b/codex-cli/tests/approvals.test.ts index 64ef4285..7cb0bd3d 100644 --- a/codex-cli/tests/approvals.test.ts +++ b/codex-cli/tests/approvals.test.ts @@ -10,33 +10,23 @@ describe("canAutoApprove()", () => { }; const writeablePaths: Array = []; - const check = ( - command: ReadonlyArray, - policy: "suggest" | "auto-edit" | "full-auto" = "suggest", - ): SafetyAssessment => canAutoApprove(command, policy, writeablePaths, env); + const check = (command: ReadonlyArray): SafetyAssessment => + canAutoApprove(command, "suggest", writeablePaths, env); - test("simple commands in suggest mode should require approval", () => { - // In suggest mode, all commands should require approval - expect(check(["ls"])).toEqual({ type: "ask-user" }); - expect(check(["cat", "file.txt"])).toEqual({ type: "ask-user" }); - expect(check(["pwd"])).toEqual({ type: "ask-user" }); - }); - - test("simple safe commands in auto-edit mode", () => { - // In auto-edit mode, safe commands should be auto-approved - expect(check(["ls"], "auto-edit")).toEqual({ + test("simple safe commands", () => { + expect(check(["ls"])).toEqual({ type: "auto-approve", reason: "List directory", group: "Searching", runInSandbox: false, }); - expect(check(["cat", "file.txt"], "auto-edit")).toEqual({ + expect(check(["cat", "file.txt"])).toEqual({ type: "auto-approve", reason: "View file contents", group: "Reading files", runInSandbox: false, }); - expect(check(["pwd"], "auto-edit")).toEqual({ + expect(check(["pwd"])).toEqual({ type: "auto-approve", reason: "Print working directory", group: "Navigating", @@ -44,30 +34,20 @@ describe("canAutoApprove()", () => { }); }); - test("bash commands in suggest mode should require approval", () => { - // In suggest mode, all bash commands should require approval - expect(check(["bash", "-lc", "ls"])).toEqual({ type: "ask-user" }); - expect(check(["bash", "-lc", "ls $HOME"])).toEqual({ type: "ask-user" }); + test("simple safe commands within a `bash -lc` call", () => { + expect(check(["bash", "-lc", "ls"])).toEqual({ + type: "auto-approve", + reason: "List directory", + group: "Searching", + runInSandbox: false, + }); + expect(check(["bash", "-lc", "ls $HOME"])).toEqual({ + type: "auto-approve", + reason: "List directory", + group: "Searching", + runInSandbox: false, + }); expect(check(["bash", "-lc", "git show ab9811cb90"])).toEqual({ - type: "ask-user", - }); - }); - - test("bash commands in auto-edit mode", () => { - // In auto-edit mode, safe bash commands should be auto-approved - expect(check(["bash", "-lc", "ls"], "auto-edit")).toEqual({ - type: "auto-approve", - reason: "List directory", - group: "Searching", - runInSandbox: false, - }); - expect(check(["bash", "-lc", "ls $HOME"], "auto-edit")).toEqual({ - type: "auto-approve", - reason: "List directory", - group: "Searching", - runInSandbox: false, - }); - expect(check(["bash", "-lc", "git show ab9811cb90"], "auto-edit")).toEqual({ type: "auto-approve", reason: "Git show", group: "Using git", @@ -76,23 +56,13 @@ describe("canAutoApprove()", () => { }); test("bash -lc commands with unsafe redirects", () => { - // In suggest mode, all commands should require approval expect(check(["bash", "-lc", "echo hello > file.txt"])).toEqual({ type: "ask-user", }); + // In theory, we could make our checker more sophisticated to auto-approve + // This previously required approval, but now that we consider safe + // operators like "&&" the entire expression can be auto‑approved. expect(check(["bash", "-lc", "ls && pwd"])).toEqual({ - type: "ask-user", - }); - - // In auto-edit mode, commands with redirects should still require approval - expect( - check(["bash", "-lc", "echo hello > file.txt"], "auto-edit"), - ).toEqual({ - type: "ask-user", - }); - - // In auto-edit mode, safe commands with safe operators should be auto-approved - expect(check(["bash", "-lc", "ls && pwd"], "auto-edit")).toEqual({ type: "auto-approve", reason: "List directory", group: "Searching", @@ -100,12 +70,8 @@ describe("canAutoApprove()", () => { }); }); - test("true command in suggest mode requires approval", () => { - expect(check(["true"])).toEqual({ type: "ask-user" }); - }); - - test("true command in auto-edit mode is auto-approved", () => { - expect(check(["true"], "auto-edit")).toEqual({ + test("true command is considered safe", () => { + expect(check(["true"])).toEqual({ type: "auto-approve", reason: "No‑op (true)", group: "Utility",