diff --git a/src/permission/auto-shell-policy.test.ts b/src/permission/auto-shell-policy.test.ts index cc3140cf5..b2e8eae4b 100644 --- a/src/permission/auto-shell-policy.test.ts +++ b/src/permission/auto-shell-policy.test.ts @@ -96,3 +96,54 @@ describe("read-only git commands pass through the policy", () => { ).toBeUndefined(); }); }); + +describe("git worktree force spellings ask (CL-6824)", () => { + // Control: a contained non-force add is not flagged at all. + test("contained non-force add stays unflagged", () => { + expect( + autoShellRuleForCall(shellCall("git worktree add ./wt-plain")), + ).toBeUndefined(); + }); + + // Git has no --force= form — real git dies with + // "error: option `force' takes no value" (exit 129) — but the spelling + // still expresses force intent, so the policy asks rather than letting the + // --flag=value skip swallow it the way the old exact-match check did. + test("--force= spellings hit the worktree ask rule", () => { + for (const flag of ["--force=true", "--force=1", "--force="]) { + expect( + autoShellRuleForCall(shellCall(`git worktree add ${flag} ./wt-eq`)) + ?.name, + ).toBe("git-worktree"); + } + }); + + // Short -f takes no value either — real git dies with + // "error: unknown switch `='" for `-f=` and + // "error: unknown switch `'" for glued `-f` (exit 129 both) — + // but the spellings still express force intent, so the policy asks rather + // than letting the generic-flag skip swallow them. + test("-f= and glued -f spellings hit the worktree ask rule", () => { + for (const flag of ["-f=true", "-f=", "-ftrue", "-ff"]) { + expect( + autoShellRuleForCall(shellCall(`git worktree add ${flag} ./wt-short`)) + ?.name, + ).toBe("git-worktree"); + expect( + autoShellRuleForCall( + shellCall(`git worktree remove ${flag} ./wt-short`), + )?.name, + ).toBe("git-worktree"); + } + }); + + // The negation is not force: --no-force must not be caught by the prefix. + test("--no-force stays unflagged", () => { + expect( + autoShellRuleForCall(shellCall("git worktree add --no-force ./wt-no")), + ).toBeUndefined(); + expect( + autoShellRuleForCall(shellCall("git worktree remove --no-force ./wt-no")), + ).toBeUndefined(); + }); +}); diff --git a/src/permission/auto-shell-policy.ts b/src/permission/auto-shell-policy.ts index d05176bef..edf997708 100644 --- a/src/permission/auto-shell-policy.ts +++ b/src/permission/auto-shell-policy.ts @@ -346,8 +346,18 @@ const WORKTREE_PRUNE_FLAGS = new Set(["-n", "--dry-run", "-v", "--verbose"]); // Flags that take a following value on `git worktree add` (branch name, lock reason). const WORKTREE_ADD_VALUE_FLAGS = new Set(["-b", "-B", "--reason"]); -function isWorktreeForceFlag(arg: string): boolean { - return arg === "-f" || arg === "--force"; +export function isWorktreeForceFlag(arg: string): boolean { + // Git's --force takes no value (real git rejects --force= with + // "error: option `force' takes no value"), but the spelling still expresses + // force intent, so the policy treats it as force rather than letting the + // --flag=value path skip swallow it. + if (arg === "--force" || arg.startsWith("--force=")) return true; + // Short -f takes no value either (real git rejects `-f=` with + // "error: unknown switch `='" and glued `-f` with + // "error: unknown switch `'"); the same fail-closed reasoning applies. + // Any `-f`-prefixed token expresses force intent. `--no-force` negations are + // unaffected: they start with "--n", not "-f". + return arg.startsWith("-f"); } // True when the path is safe for unattended worktree add/remove: inside the diff --git a/src/permission/gate.test.ts b/src/permission/gate.test.ts index 1bcb68637..070239ae9 100644 --- a/src/permission/gate.test.ts +++ b/src/permission/gate.test.ts @@ -286,3 +286,192 @@ describe("standing grant covers a later git worktree command (CL-5638)", () => { expect(prompts).toBe(1); }); }); + +// CL-6824: when a standing grant covers a command but a pre-grant guard still +// forces an ask, the prompt carries PermissionRequest.notice naming the +// guard's reason. Matching semantics are unchanged — every case below still +// asks (and stays deniable); only the prompt gains the why. +describe("grant-mismatch asks carry the guard reason as a notice (CL-6824)", () => { + const root = mkdtempSync(join(tmpdir(), "gate-mismatch-notice-")); + const sessionCwd = join(root, "main"); + const git = (args: string[], cwd: string) => + execFileSync("git", args, { cwd, stdio: "ignore" }); + mkdirSync(sessionCwd); + initTemporaryGitRepo(sessionCwd, { initArgs: ["-q"] }); + writeFileSync(join(sessionCwd, "seed.txt"), "seed\n"); + git(["add", "."], sessionCwd); + git(["commit", "-qm", "seed"], sessionCwd); + + async function askWithGrants(command: string, approvals: Approval[]) { + const seen: PermissionRequest[] = []; + const gate = createPermissionGate({ + approvals, + interactive: true, + skipPermissions: false, + reactorGated: false, + cwd: sessionCwd, + rootsProvider: () => [], + requestApproval: async (request) => { + seen.push(request); + return { allow: false }; + }, + }); + const verdict = await gate.evaluate(shellCall(command)); + return { verdict, seen }; + } + + const worktreeGrant: Approval[] = [ + { tool: "run_shell", pattern: "git worktree *" }, + ]; + const catGrant: Approval[] = [{ tool: "run_shell", pattern: "cat *" }]; + + test("force worktree names --force in the notice", async () => { + const { verdict, seen } = await askWithGrants( + "git worktree add --force ../sib-force", + worktreeGrant, + ); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but it uses --force, so it still needs approval.", + ); + }); + + test("--force= is still force in the notice", async () => { + const { verdict, seen } = await askWithGrants( + "git worktree add --force=true ../sib-force-eq", + worktreeGrant, + ); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but it uses --force, so it still needs approval.", + ); + }); + + test("short -f= is still force in the notice", async () => { + const { verdict, seen } = await askWithGrants( + "git worktree add -f=true ../sib-force-short-eq", + worktreeGrant, + ); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but it uses --force, so it still needs approval.", + ); + }); + + test("glued -f is still force in the notice", async () => { + const { verdict, seen } = await askWithGrants( + "git worktree remove -ftrue ../sib-force-glued", + worktreeGrant, + ); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but it uses --force, so it still needs approval.", + ); + }); + + test("uncontained add destination names the approved locations", async () => { + // A direct child of tmpdir() is not a permitted sibling of sessionCwd + // (only direct children of root/ are), so the restricted guard trips. + const outside = join(tmpdir(), "gate-6824-outside"); + const { verdict, seen } = await askWithGrants( + `git worktree add ${outside}`, + worktreeGrant, + ); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but the worktree destination is outside the approved locations, so it still needs approval.", + ); + }); + + test("uncontained remove names the worktree, not a destination", async () => { + // `remove` names an existing worktree — there is no destination — so the + // notice drops the destination noun the `add` case uses. + const outside = join(tmpdir(), "gate-6824-outside-remove"); + const { verdict, seen } = await askWithGrants( + `git worktree remove ${outside}`, + worktreeGrant, + ); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but the worktree is outside the approved locations, so it still needs approval.", + ); + }); + + test("secret reference names the sensitive path", async () => { + const { verdict, seen } = await askWithGrants("cat .env", catGrant); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but it references a sensitive path, so it still needs approval.", + ); + // The secret ask still strips grant scopes; the notice survives it. + expect(seen[0]?.scopes).toEqual([]); + }); + + test("restricted target names the workspace", async () => { + const { verdict, seen } = await askWithGrants("cat /etc/passwd", catGrant); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBe( + "A standing grant matches this command, but it targets a path outside the workspace, so it still needs approval.", + ); + }); + + test("an ask with no matching grant carries no notice", async () => { + const { verdict, seen } = await askWithGrants( + "git worktree add --force ../sib-nogrant", + [], + ); + expect(verdict.allowed).toBe(false); + expect(seen).toHaveLength(1); + expect(seen[0]?.notice).toBeUndefined(); + }); + + test("a covered command still allows with no prompt and no notice", async () => { + const seen: PermissionRequest[] = []; + const gate = createPermissionGate({ + approvals: worktreeGrant, + interactive: true, + skipPermissions: false, + reactorGated: false, + cwd: sessionCwd, + rootsProvider: () => [], + requestApproval: async (request) => { + seen.push(request); + return { allow: false }; + }, + }); + const verdict = await gate.evaluate( + shellCall("git worktree add ../sib-plain -b br-plain"), + ); + expect(verdict.allowed).toBe(true); + expect(seen).toHaveLength(0); + }); + + test("a --no-force command still allows with no prompt and no notice", async () => { + const seen: PermissionRequest[] = []; + const gate = createPermissionGate({ + approvals: worktreeGrant, + interactive: true, + skipPermissions: false, + reactorGated: false, + cwd: sessionCwd, + rootsProvider: () => [], + requestApproval: async (request) => { + seen.push(request); + return { allow: false }; + }, + }); + const verdict = await gate.evaluate( + shellCall("git worktree add --no-force ../sib-noforce -b br-noforce"), + ); + expect(verdict.allowed).toBe(true); + expect(seen).toHaveLength(0); + }); +}); diff --git a/src/permission/gate.ts b/src/permission/gate.ts index a3810da64..2b0bbbec2 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -18,6 +18,7 @@ import { import { autoShellRuleForCall, safeWorktreeCommand, + isWorktreeForceFlag, } from "./auto-shell-policy.js"; import { commandReferencesSensitivePath } from "../plugins/secret-guard-plugin.js"; import { @@ -34,6 +35,7 @@ import { import { splitChainedCommand, isShellCommentOnly, + tokenize, stripCommentLines, } from "./command.js"; import { @@ -145,6 +147,45 @@ function segmentGuard( return undefined; } +// Classifies a guarded `git worktree add/remove` segment so a grant-mismatch +// notice can name the operative reason: a force flag, or a destination the +// containment authority did not approve. Returns undefined for anything else. +// Display-only refinement — the guard decision itself is unchanged. +function worktreeMismatchKind( + segment: string, +): "force" | "destination" | "worktree" | undefined { + const tokens = tokenize(segment); + if (tokens[0] !== "git" || tokens[1] !== "worktree") return undefined; + if (tokens[2] !== "add" && tokens[2] !== "remove") return undefined; + if (tokens.slice(3).some(isWorktreeForceFlag)) return "force"; + // `add` takes a destination for the new worktree; `remove` names an + // existing worktree, so only `add` gets the destination noun. + return tokens[2] === "remove" ? "worktree" : "destination"; +} + +// Explains a grant mismatch: a standing grant covers the segment, but the +// pre-grant guard still forced an ask. The wording names the guard's reason, +// never the grant — matching semantics are untouched. +function grantMismatchNotice( + segment: string, + kind: "secret" | "restricted", +): string { + if (kind === "secret") { + return "A standing grant matches this command, but it references a sensitive path, so it still needs approval."; + } + const worktreeKind = worktreeMismatchKind(segment); + if (worktreeKind === "force") { + return "A standing grant matches this command, but it uses --force, so it still needs approval."; + } + if (worktreeKind === "destination") { + return "A standing grant matches this command, but the worktree destination is outside the approved locations, so it still needs approval."; + } + if (worktreeKind === "worktree") { + return "A standing grant matches this command, but the worktree is outside the approved locations, so it still needs approval."; + } + return "A standing grant matches this command, but it targets a path outside the workspace, so it still needs approval."; +} + // Relative path tokens in a shell command resolve against the process cwd of the // agent that issued the call — not the session cwd that built the gate. Absolute // paths pass through unchanged so createPathRestriction still judges them against @@ -731,6 +772,7 @@ export function createPermissionGate( let needsOperator = false; let anySecret = false; + let mismatchNotice: string | undefined; for (const segment of segments) { // A secret-path reference or restricted target always requires the // operator, whether the segment would otherwise auto-allow or match @@ -747,6 +789,22 @@ export function createPermissionGate( if (guard !== undefined) { if (guard.kind === "secret") anySecret = true; needsOperator = true; + // A standing grant may still cover this segment even though the + // pre-grant guard forces an ask — record why so the prompt can say + // so. Matching semantics are untouched; this only annotates the ask. + if ( + mismatchNotice === undefined && + (await evaluateApprovals({ + tool: request.tool, + subject: segment, + approvals, + activeProviderModel, + requestCwd: effectiveCwd, + workspace: grantWorkspace(), + })) + ) { + mismatchNotice = grantMismatchNotice(segment, guard.kind); + } continue; } if ( @@ -788,9 +846,19 @@ export function createPermissionGate( // Secret-path shell must never mint a stored grant — even an exact match // would be misleading because future secret-path shell always re-asks. + // A grant mismatch carries the guard's reason on the prompt so the + // operator sees why the standing grant did not apply. const requestForOperator = anySecret - ? { ...request, scopes: [] } - : request; + ? { + ...request, + scopes: [], + ...(mismatchNotice !== undefined + ? { notice: mismatchNotice } + : null), + } + : mismatchNotice !== undefined + ? { ...request, notice: mismatchNotice } + : request; return { kind: "ask", request: requestForOperator,