Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions src/permission/auto-shell-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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=<value> 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=<value> 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=<value>` and
// "error: unknown switch `<char>'" for glued `-f<val>` (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=<value> and glued -f<val> 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();
});
});
14 changes: 12 additions & 2 deletions src/permission/auto-shell-policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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=<value> 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=<value>` with
// "error: unknown switch `='" and glued `-f<val>` with
// "error: unknown switch `<char>'"); 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
Expand Down
189 changes: 189 additions & 0 deletions src/permission/gate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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=<value> 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=<value> 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<val> 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);
});
});
72 changes: 70 additions & 2 deletions src/permission/gate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
import {
autoShellRuleForCall,
safeWorktreeCommand,
isWorktreeForceFlag,
} from "./auto-shell-policy.js";
import { commandReferencesSensitivePath } from "../plugins/secret-guard-plugin.js";
import {
Expand All @@ -34,6 +35,7 @@ import {
import {
splitChainedCommand,
isShellCommentOnly,
tokenize,
stripCommentLines,
} from "./command.js";
import {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 (
Expand Down Expand Up @@ -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,
Expand Down
Loading