From 8bf675c152e895abdbf3a27536f8100f4f9d1e4f Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:32:33 +0800 Subject: [PATCH 1/7] fix(worktrees): require human confirmation before clean-worktree.mjs deletes anything A worktree under .claude/worktrees/ was wiped mid-task five times today, including once through an explicit lock. The script's own merge-detection was checked against the fleet and did not misfire on the destroyed branches, so the gap was upstream of it: --remove was reachable from any agent session in any worktree with zero human checkpoint, since the user's global Claude Code defaultMode is "auto" and this repo's own settings.json auto-allowed every invocation of the script, destructive or not. --remove now refuses to run unless CLEAN_WORKTREE_CONFIRM=1 is set in the invoking shell. This is tool-agnostic by design: it protects against Codex and Antigravity sessions too, not just Claude Code, since none of them will spontaneously set an unusual env var without a human telling them to. Narrows the project's own allow-list to the list-only/dry-run forms of the command so the destructive form is no longer blanket-approved for Claude Code sessions specifically. Co-Authored-By: Claude Sonnet 5 --- .claude/settings.json | 6 +++++- scripts/clean-worktree.mjs | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/.claude/settings.json b/.claude/settings.json index 5e13c19446..84b4287df9 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -177,7 +177,11 @@ "Bash(gh auth status)", "Bash(curl -s http://localhost:*)", "Bash(node scripts/check-base-freshness.mjs:*)", - "Bash(node scripts/clean-worktree.mjs:*)", + "Bash(node scripts/clean-worktree.mjs --self-test)", + "Bash(node scripts/clean-worktree.mjs --merged)", + "Bash(node scripts/clean-worktree.mjs --merged --dry-run)", + "Bash(node scripts/clean-worktree.mjs --merged --squashed)", + "Bash(node scripts/clean-worktree.mjs --merged --squashed --dry-run)", "mcp__Claude_Browser__navigate", "mcp__Claude_Browser__get_page_text", "mcp__Claude_Browser__read_page", diff --git a/scripts/clean-worktree.mjs b/scripts/clean-worktree.mjs index 7ec8c72c2d..485ae2ebcd 100644 --- a/scripts/clean-worktree.mjs +++ b/scripts/clean-worktree.mjs @@ -439,6 +439,20 @@ export function parseArgs(argv) { if (remove && !merged) { throw new Error("--remove is only valid together with --merged. Run `--merged` alone to list candidates first."); } + // `--remove` is reachable from any agent session in any worktree on this machine (Claude + // Code, Codex, Antigravity/Gemini — only Claude Code's own permission config in + // .claude/settings.json can gate a Bash call before it runs, and that file governs nothing + // outside Claude Code). A worktree destroyed here has no local reflog of its own to recover + // from, so the actual deletion needs a second, tool-agnostic gate that only a human sets: + // CLEAN_WORKTREE_CONFIRM=1 in the invoking shell. No default flips this on. Listing + // candidates (`--merged` without `--remove`) is unaffected and needs no env var. + if (remove && process.env.CLEAN_WORKTREE_CONFIRM !== "1") { + throw new Error( + "--remove refused: set CLEAN_WORKTREE_CONFIRM=1 in your shell to confirm you (a human) reviewed the " + + "candidate list above and want these worktrees deleted. This gate exists because a worktree removed " + + "here has no reflog of its own — see AGENTS.md 'Worktree sweep destroys live work'.", + ); + } if (squashed && !merged) { throw new Error("--squashed is only valid together with --merged."); } @@ -691,9 +705,28 @@ export function selfTest() { if (!args3.merged || args3.remove) { throw new Error("selfTest failed: --merged must default to list-only (remove=false)"); } - const args4 = parseArgs(["--merged", "--remove"]); + let unconfirmedRemoveThrew = false; + const savedConfirm = process.env.CLEAN_WORKTREE_CONFIRM; + delete process.env.CLEAN_WORKTREE_CONFIRM; + try { + parseArgs(["--merged", "--remove"]); + } catch { + unconfirmedRemoveThrew = true; + } + if (!unconfirmedRemoveThrew) { + throw new Error("selfTest failed: --merged --remove without CLEAN_WORKTREE_CONFIRM=1 must refuse"); + } + + process.env.CLEAN_WORKTREE_CONFIRM = "1"; + let args4; + try { + args4 = parseArgs(["--merged", "--remove"]); + } finally { + if (savedConfirm === undefined) delete process.env.CLEAN_WORKTREE_CONFIRM; + else process.env.CLEAN_WORKTREE_CONFIRM = savedConfirm; + } if (!args4.merged || !args4.remove) { - throw new Error("selfTest failed: --merged --remove did not set both flags"); + throw new Error("selfTest failed: --merged --remove (confirmed) did not set both flags"); } let removeThrew = false; try { From d9f67213557eadda67e1cfc983e6948ecf57fe70 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:47:43 +0800 Subject: [PATCH 2/7] fix(worktrees): detect a broken .git link at session start instead of silently misdirecting When a worktree's .git file disappears, git does not error - it walks up the directory tree and silently adopts the next real repository it finds as the target. Every command after that point looks normal but is operating on the wrong checkout, on whatever branch that repo happens to be on. This nearly caused this very fix to land on an unrelated in-progress branch in the main checkout instead of its intended worktree. check-base-freshness.mjs already runs as a SessionStart hook and already has the machinery to surface an advisory finding into the session's injected context. Add one more check ahead of the existing freshness check: compare `git rev-parse --show-toplevel` against CLAUDE_PROJECT_DIR (the directory Claude Code was actually told the project lives in, which git's own resolution cannot see). A mismatch means the .git link broke sometime after the worktree was created, and the human-readable error plus the hook's JSON envelope both name the wrong root it climbed to, so a session starting in a broken worktree sees this in its own context instead of silently working against the wrong repository. Co-Authored-By: Claude Sonnet 5 --- scripts/check-base-freshness.mjs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/scripts/check-base-freshness.mjs b/scripts/check-base-freshness.mjs index 082ca359a5..8e498521cb 100644 --- a/scripts/check-base-freshness.mjs +++ b/scripts/check-base-freshness.mjs @@ -47,6 +47,7 @@ * --strict exit 1 when the base ref cannot be resolved (default: exit 0) */ import { execFileSync } from "node:child_process"; +import path from "node:path"; const threshold = Number.parseInt(process.env.STALE_BASE_THRESHOLD ?? "10", 10) || 10; const asJson = process.argv.includes("--json"); @@ -110,6 +111,32 @@ function finish(result) { process.exit(result.error && strict ? 1 : 0); } +// Worktree-identity tripwire. A worktree's `.git` file can vanish (observed repeatedly +// against `.claude/worktrees/*` this week — see AGENTS.md "Worktree sweep destroys live +// work") without any command erroring: git just walks up the directory tree, finds the +// next real `.git` above it, and silently treats THAT repository as the target from then +// on — wrong branch, wrong working tree, commands that look like they succeeded. Detect +// it the same way a human would: ask git where it thinks the repo root is, and compare +// that to where Claude Code was actually told the project lives. CLAUDE_PROJECT_DIR is +// exported for every hook invocation and is the one signal available here that names the +// INTENDED worktree independent of whatever `.git` link git happens to resolve. +const expectedRoot = process.env.CLAUDE_PROJECT_DIR; +if (expectedRoot) { + const resolvedToplevel = tryGit(["rev-parse", "--show-toplevel"]); + const normalize = (p) => (process.platform === "win32" ? path.resolve(p).toLowerCase() : path.resolve(p)); + if (resolvedToplevel && normalize(resolvedToplevel) !== normalize(expectedRoot)) { + finish({ + branch: "(unknown)", + error: + `this worktree's .git link is broken — git resolved the repo root as "${resolvedToplevel}" ` + + `instead of the expected "${expectedRoot}". Commands run here are silently operating on a ` + + `DIFFERENT checkout. Stop and recreate this worktree before doing any more work in it.`, + behind: 0, + ahead: 0, + }); + } +} + const branch = tryGit(["rev-parse", "--abbrev-ref", "HEAD"]) ?? "(unknown)"; if (process.env.BASE_FRESHNESS_NO_FETCH !== "1") { From a7cda2756473f2cc5a84f94c1de1b1ea0933a261 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:42:14 +0800 Subject: [PATCH 3/7] docs: record review ledger entry for PR #2240 Co-Authored-By: Claude Sonnet 5 --- ...4a1d9780e9e58d5b160d06bbdbaa8772c51e525108cf6ec2484.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/a8edc68de42114a1d9780e9e58d5b160d06bbdbaa8772c51e525108cf6ec2484.record.md diff --git a/docs/branch-review-records/a8edc68de42114a1d9780e9e58d5b160d06bbdbaa8772c51e525108cf6ec2484.record.md b/docs/branch-review-records/a8edc68de42114a1d9780e9e58d5b160d06bbdbaa8772c51e525108cf6ec2484.record.md new file mode 100644 index 0000000000..d3a3023bb6 --- /dev/null +++ b/docs/branch-review-records/a8edc68de42114a1d9780e9e58d5b160d06bbdbaa8772c51e525108cf6ec2484.record.md @@ -0,0 +1 @@ +| 2026-08-21 | claude/worktree-cleanup-guard | d9f67213557eadda67e1cfc983e6948ecf57fe70 | scripts/clean-worktree.mjs, scripts/check-base-freshness.mjs, .claude/settings.json | author-implemented; PR #2240 opened | self-test, lint, typecheck, format(unchanged) | From 48b83fe152bea55a460e1895652e43cbfd6fc975 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:55:36 +0800 Subject: [PATCH 4/7] =?UTF-8?q?fix(worktrees):=20address=20Codex=20review?= =?UTF-8?q?=20=E2=80=94=20dry-run=20preview=20and=20symlink=20false=20posi?= =?UTF-8?q?tives?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real bugs from automated review on PR #2240, both in yesterday's fix: - clean-worktree.mjs: the CLEAN_WORKTREE_CONFIRM gate fired before dry-run got a chance to win, so `--merged --remove --dry-run` - which deletes nothing - was refused anyway. Gate now only applies when a deletion can actually happen. - check-base-freshness.mjs: the worktree-identity tripwire compared lexical paths, so a CLAUDE_PROJECT_DIR reaching the checkout through a symlink or junction would report a false "broken .git link" even on a healthy worktree. Now resolves both sides through realpathSync.native before comparing, falling back to the lexical path if resolution fails (the check stays advisory-only and must never crash the SessionStart hook it runs in). Co-Authored-By: Claude Sonnet 5 --- scripts/check-base-freshness.mjs | 18 +++++++++++++++++- scripts/clean-worktree.mjs | 15 +++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/scripts/check-base-freshness.mjs b/scripts/check-base-freshness.mjs index 8e498521cb..5728909597 100644 --- a/scripts/check-base-freshness.mjs +++ b/scripts/check-base-freshness.mjs @@ -47,6 +47,7 @@ * --strict exit 1 when the base ref cannot be resolved (default: exit 0) */ import { execFileSync } from "node:child_process"; +import { realpathSync } from "node:fs"; import path from "node:path"; const threshold = Number.parseInt(process.env.STALE_BASE_THRESHOLD ?? "10", 10) || 10; @@ -123,7 +124,22 @@ function finish(result) { const expectedRoot = process.env.CLAUDE_PROJECT_DIR; if (expectedRoot) { const resolvedToplevel = tryGit(["rev-parse", "--show-toplevel"]); - const normalize = (p) => (process.platform === "win32" ? path.resolve(p).toLowerCase() : path.resolve(p)); + // realpath, not just path.resolve: CLAUDE_PROJECT_DIR can name the intended checkout + // through a symlink or junction (D: Dev Drive worktrees are reached that way in some + // setups) while `git rev-parse --show-toplevel` always returns the canonical path. Two + // spellings of the same real directory must not trip the tripwire. If either side can't be + // resolved (permissions, a path that vanished between the git call and this one), fall back + // to the lexical form rather than throwing — this check is advisory-only and must never take + // down the SessionStart hook it runs in. + const normalize = (p) => { + let resolved = path.resolve(p); + try { + resolved = realpathSync.native(resolved); + } catch { + // Leave `resolved` as the lexical path.resolve() form. + } + return process.platform === "win32" ? resolved.toLowerCase() : resolved; + }; if (resolvedToplevel && normalize(resolvedToplevel) !== normalize(expectedRoot)) { finish({ branch: "(unknown)", diff --git a/scripts/clean-worktree.mjs b/scripts/clean-worktree.mjs index 485ae2ebcd..3396f7e4c5 100644 --- a/scripts/clean-worktree.mjs +++ b/scripts/clean-worktree.mjs @@ -445,8 +445,11 @@ export function parseArgs(argv) { // outside Claude Code). A worktree destroyed here has no local reflog of its own to recover // from, so the actual deletion needs a second, tool-agnostic gate that only a human sets: // CLEAN_WORKTREE_CONFIRM=1 in the invoking shell. No default flips this on. Listing - // candidates (`--merged` without `--remove`) is unaffected and needs no env var. - if (remove && process.env.CLEAN_WORKTREE_CONFIRM !== "1") { + // candidates (`--merged` without `--remove`) is unaffected and needs no env var. `--dry-run` + // is exempt too: `--merged --remove --dry-run` can delete nothing (dry-run wins over remove + // in runMergedWorktreeReport below), so gating it here would only block the safe preflight + // preview an operator runs before setting the confirm var for real. + if (remove && !dryRun && process.env.CLEAN_WORKTREE_CONFIRM !== "1") { throw new Error( "--remove refused: set CLEAN_WORKTREE_CONFIRM=1 in your shell to confirm you (a human) reviewed the " + "candidate list above and want these worktrees deleted. This gate exists because a worktree removed " + @@ -717,6 +720,14 @@ export function selfTest() { throw new Error("selfTest failed: --merged --remove without CLEAN_WORKTREE_CONFIRM=1 must refuse"); } + // `--dry-run` can delete nothing, so it must stay usable as a preflight preview without the + // confirm var — this was the actual P2 Codex found: the gate above fired before dry-run got + // a chance to win. + const dryRunArgs = parseArgs(["--merged", "--remove", "--dry-run"]); + if (!dryRunArgs.merged || !dryRunArgs.remove || !dryRunArgs.dryRun) { + throw new Error("selfTest failed: --merged --remove --dry-run without CLEAN_WORKTREE_CONFIRM=1 must still parse"); + } + process.env.CLEAN_WORKTREE_CONFIRM = "1"; let args4; try { From a0a957c2993bdf78a24c3061c9e5bc2fe59f42bd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 15:20:43 +0000 Subject: [PATCH 5/7] fix(worktrees): exempt --dry-run from the removal confirmation gate The CLEAN_WORKTREE_CONFIRM guard added in this branch threw for any `--remove` invocation regardless of `--dry-run`, breaking the documented "dry-run wins over --remove" preview path and failing the pre-existing `parses valid combination of flags` test (which parses `--merged --squashed --remove --dry-run ...` and expects a plain parse result). Only gate the destructive path: `remove && !dryRun`. Also addresses a review comment on the same PR: check-base-freshness.mjs compared CLAUDE_PROJECT_DIR against `git rev-parse --show-toplevel` with plain path.resolve(), which does not resolve symlinks/junctions, so an aliased-but-healthy worktree would trip the "broken .git link" tripwire. Canonicalize both sides through realpathSync (falling back to path.resolve if realpath fails) before comparing. --- scripts/check-base-freshness.mjs | 17 ++++++++++++++++- scripts/clean-worktree.mjs | 8 ++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/scripts/check-base-freshness.mjs b/scripts/check-base-freshness.mjs index 8e498521cb..a967485188 100644 --- a/scripts/check-base-freshness.mjs +++ b/scripts/check-base-freshness.mjs @@ -47,6 +47,7 @@ * --strict exit 1 when the base ref cannot be resolved (default: exit 0) */ import { execFileSync } from "node:child_process"; +import { realpathSync } from "node:fs"; import path from "node:path"; const threshold = Number.parseInt(process.env.STALE_BASE_THRESHOLD ?? "10", 10) || 10; @@ -123,7 +124,21 @@ function finish(result) { const expectedRoot = process.env.CLAUDE_PROJECT_DIR; if (expectedRoot) { const resolvedToplevel = tryGit(["rev-parse", "--show-toplevel"]); - const normalize = (p) => (process.platform === "win32" ? path.resolve(p).toLowerCase() : path.resolve(p)); + // Canonicalize through realpath before comparing. `git rev-parse --show-toplevel` always + // returns the canonical filesystem path, but CLAUDE_PROJECT_DIR can name the same checkout + // through a symlink or junction (a container image's working-dir alias, a Dev Drive + // junction) — comparing the raw, uncanonicalized paths would then report a false "broken + // .git link" for a perfectly healthy worktree. realpath can fail (path deleted between + // hook invocation and here, permissions) — fall back to the lexical path rather than + // throwing out of an advisory tripwire. + const canonicalize = (p) => { + try { + return realpathSync(p); + } catch { + return path.resolve(p); + } + }; + const normalize = (p) => (process.platform === "win32" ? canonicalize(p).toLowerCase() : canonicalize(p)); if (resolvedToplevel && normalize(resolvedToplevel) !== normalize(expectedRoot)) { finish({ branch: "(unknown)", diff --git a/scripts/clean-worktree.mjs b/scripts/clean-worktree.mjs index 485ae2ebcd..4bdc3fc8e6 100644 --- a/scripts/clean-worktree.mjs +++ b/scripts/clean-worktree.mjs @@ -445,8 +445,12 @@ export function parseArgs(argv) { // outside Claude Code). A worktree destroyed here has no local reflog of its own to recover // from, so the actual deletion needs a second, tool-agnostic gate that only a human sets: // CLEAN_WORKTREE_CONFIRM=1 in the invoking shell. No default flips this on. Listing - // candidates (`--merged` without `--remove`) is unaffected and needs no env var. - if (remove && process.env.CLEAN_WORKTREE_CONFIRM !== "1") { + // candidates (`--merged` without `--remove`) is unaffected and needs no env var. `--dry-run` + // already documents that it "wins over --remove" (see printHelp below) and never deletes + // anything, so it must not be gated behind the same confirmation as the real deletion path — + // otherwise the safe preflight an operator runs specifically to review candidates before + // setting CLEAN_WORKTREE_CONFIRM=1 would itself refuse to run. + if (remove && !dryRun && process.env.CLEAN_WORKTREE_CONFIRM !== "1") { throw new Error( "--remove refused: set CLEAN_WORKTREE_CONFIRM=1 in your shell to confirm you (a human) reviewed the " + "candidate list above and want these worktrees deleted. This gate exists because a worktree removed " + From a57a3bd52fdf25e5479e1e867cfbb2c7bccc9bb1 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:30:25 +0800 Subject: [PATCH 6/7] fix(worktrees): close the programmatic bypass CodeRabbit found on PR #2240 Two real findings, both in the CLEAN_WORKTREE_CONFIRM gate: - Major/security: the confirm check lived only inside parseArgs, but runMergedWorktreeReport is exported and reachable directly by any programmatic caller with { remove: true, dryRun: false } - bypassing parseArgs, and the confirmation, entirely. Extracted the check into assertRemovalConfirmed() and call it from both parseArgs (fast CLI feedback) and runMergedWorktreeReport immediately before the deletion loop, so the actual boundary is covered regardless of caller. - Minor/stability: selfTest()'s finally block only covered the confirmed case, so a thrown assertion on the unconfirmed-refusal check upstream of it would leave CLEAN_WORKTREE_CONFIRM permanently deleted from process.env in a longer-lived process. Wrapped the whole confirm/parse cycle in one outer try/finally. Verified the boundary fix directly: importing the module and calling runMergedWorktreeReport({ remove: true, dryRun: false }) with the env var unset now lists candidates but refuses before the removal loop, instead of silently deleting. Co-Authored-By: Claude Sonnet 5 --- scripts/clean-worktree.mjs | 95 +++++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 37 deletions(-) diff --git a/scripts/clean-worktree.mjs b/scripts/clean-worktree.mjs index 81fc764e8d..81d05b7829 100644 --- a/scripts/clean-worktree.mjs +++ b/scripts/clean-worktree.mjs @@ -364,6 +364,31 @@ export function verifyWorktreeSafetyBeforeRemove( return { safe: true, confidence }; } +/** + * The actual confirmation gate. `--remove` is reachable from any agent session in any + * worktree on this machine (Claude Code, Codex, Antigravity/Gemini — only Claude Code's own + * permission config in .claude/settings.json can gate a Bash call before it runs, and that + * file governs nothing outside Claude Code). A worktree destroyed here has no local reflog of + * its own to recover from, so the actual deletion needs a second, tool-agnostic gate that only + * a human sets: CLEAN_WORKTREE_CONFIRM=1 in the invoking shell. No default flips this on. + * + * Called from BOTH `parseArgs` (so a CLI run without the confirm var fails fast, before it + * even lists candidates) AND `runMergedWorktreeReport` immediately before the deletion loop — + * the CLI is not the only caller: `runMergedWorktreeReport` is exported and any programmatic + * caller (a test, another script) can pass `{ remove: true, dryRun: false }` directly, bypassing + * `parseArgs` entirely. A single check living only in `parseArgs` would leave that path able to + * delete worktrees with no confirmation at all. + */ +function assertRemovalConfirmed(remove, dryRun) { + if (remove && !dryRun && process.env.CLEAN_WORKTREE_CONFIRM !== "1") { + throw new Error( + "--remove refused: set CLEAN_WORKTREE_CONFIRM=1 in your shell to confirm you (a human) reviewed the " + + "candidate list above and want these worktrees deleted. This gate exists because a worktree removed " + + "here has no reflog of its own — see AGENTS.md 'Worktree sweep destroys live work'.", + ); + } +} + /** * Parse CLI arguments for batch size and flags. * @@ -439,24 +464,11 @@ export function parseArgs(argv) { if (remove && !merged) { throw new Error("--remove is only valid together with --merged. Run `--merged` alone to list candidates first."); } - // `--remove` is reachable from any agent session in any worktree on this machine (Claude - // Code, Codex, Antigravity/Gemini — only Claude Code's own permission config in - // .claude/settings.json can gate a Bash call before it runs, and that file governs nothing - // outside Claude Code). A worktree destroyed here has no local reflog of its own to recover - // from, so the actual deletion needs a second, tool-agnostic gate that only a human sets: - // CLEAN_WORKTREE_CONFIRM=1 in the invoking shell. No default flips this on. Listing - // candidates (`--merged` without `--remove`) is unaffected and needs no env var. `--dry-run` - // already documents that it "wins over --remove" (see printHelp below) and never deletes - // anything, so it must not be gated behind the same confirmation as the real deletion path — - // otherwise the safe preflight an operator runs specifically to review candidates before - // setting CLEAN_WORKTREE_CONFIRM=1 would itself refuse to run. - if (remove && !dryRun && process.env.CLEAN_WORKTREE_CONFIRM !== "1") { - throw new Error( - "--remove refused: set CLEAN_WORKTREE_CONFIRM=1 in your shell to confirm you (a human) reviewed the " + - "candidate list above and want these worktrees deleted. This gate exists because a worktree removed " + - "here has no reflog of its own — see AGENTS.md 'Worktree sweep destroys live work'.", - ); - } + // Fail fast at the CLI so a run without the confirm var doesn't even list candidates. + // `--dry-run` documents that it "wins over --remove" (see printHelp below) and never + // deletes anything, so `assertRemovalConfirmed` exempts it — the safe preflight preview an + // operator runs before setting CLEAN_WORKTREE_CONFIRM=1 for real must stay usable. + assertRemovalConfirmed(remove, dryRun); if (squashed && !merged) { throw new Error("--squashed is only valid together with --merged."); } @@ -709,29 +721,33 @@ export function selfTest() { if (!args3.merged || args3.remove) { throw new Error("selfTest failed: --merged must default to list-only (remove=false)"); } - let unconfirmedRemoveThrew = false; + // One outer try/finally for the whole CLEAN_WORKTREE_CONFIRM cycle: if any assertion in + // here throws (including the negative one right below), the real env var must still be + // restored before selfTest() exits, or a caller running selfTest() inside a longer-lived + // process (not a one-shot CLI invocation) inherits a deleted CLEAN_WORKTREE_CONFIRM. const savedConfirm = process.env.CLEAN_WORKTREE_CONFIRM; - delete process.env.CLEAN_WORKTREE_CONFIRM; + let args4; try { - parseArgs(["--merged", "--remove"]); - } catch { - unconfirmedRemoveThrew = true; - } - if (!unconfirmedRemoveThrew) { - throw new Error("selfTest failed: --merged --remove without CLEAN_WORKTREE_CONFIRM=1 must refuse"); - } + delete process.env.CLEAN_WORKTREE_CONFIRM; + let unconfirmedRemoveThrew = false; + try { + parseArgs(["--merged", "--remove"]); + } catch { + unconfirmedRemoveThrew = true; + } + if (!unconfirmedRemoveThrew) { + throw new Error("selfTest failed: --merged --remove without CLEAN_WORKTREE_CONFIRM=1 must refuse"); + } - // `--dry-run` can delete nothing, so it must stay usable as a preflight preview without the - // confirm var — this was the actual P2 Codex found: the gate above fired before dry-run got - // a chance to win. - const dryRunArgs = parseArgs(["--merged", "--remove", "--dry-run"]); - if (!dryRunArgs.merged || !dryRunArgs.remove || !dryRunArgs.dryRun) { - throw new Error("selfTest failed: --merged --remove --dry-run without CLEAN_WORKTREE_CONFIRM=1 must still parse"); - } + // `--dry-run` can delete nothing, so it must stay usable as a preflight preview without + // the confirm var — this was the actual P2 Codex found: the gate above fired before + // dry-run got a chance to win. + const dryRunArgs = parseArgs(["--merged", "--remove", "--dry-run"]); + if (!dryRunArgs.merged || !dryRunArgs.remove || !dryRunArgs.dryRun) { + throw new Error("selfTest failed: --merged --remove --dry-run without CLEAN_WORKTREE_CONFIRM=1 must still parse"); + } - process.env.CLEAN_WORKTREE_CONFIRM = "1"; - let args4; - try { + process.env.CLEAN_WORKTREE_CONFIRM = "1"; args4 = parseArgs(["--merged", "--remove"]); } finally { if (savedConfirm === undefined) delete process.env.CLEAN_WORKTREE_CONFIRM; @@ -1088,6 +1104,11 @@ export function runMergedWorktreeReport(options = {}) { return; } + // Re-assert at the deletion boundary itself, not just at CLI parse time: this function is + // exported and a programmatic caller can reach here with `{ remove: true, dryRun: false }` + // without ever going through `parseArgs`. + assertRemovalConfirmed(remove, dryRun); + const batch = candidates.slice(0, batchSize); console.log( `\n[clean-worktree] Removing ${batch.length} of ${candidates.length} candidate(s) (batch size ${batchSize})...`, From 3b16f53c3ded0d46e6c0c875e4154670580d16c5 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:30:50 +0800 Subject: [PATCH 7/7] docs: record ledger entry with decisive gate output for the CodeRabbit-fix commit Co-Authored-By: Claude Sonnet 5 --- ...41f3d4d59e978c5c64eba136dbf114ced90cc88cac2c1f28c08.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/e246b1146921341f3d4d59e978c5c64eba136dbf114ced90cc88cac2c1f28c08.record.md diff --git a/docs/branch-review-records/e246b1146921341f3d4d59e978c5c64eba136dbf114ced90cc88cac2c1f28c08.record.md b/docs/branch-review-records/e246b1146921341f3d4d59e978c5c64eba136dbf114ced90cc88cac2c1f28c08.record.md new file mode 100644 index 0000000000..4dd66a1faf --- /dev/null +++ b/docs/branch-review-records/e246b1146921341f3d4d59e978c5c64eba136dbf114ced90cc88cac2c1f28c08.record.md @@ -0,0 +1 @@ +| 2026-08-21 | claude/worktree-cleanup-guard | a57a3bd52fdf25e5479e1e867cfbb2c7bccc9bb1 | scripts/clean-worktree.mjs, scripts/check-base-freshness.mjs | author-implemented; addressed CodeRabbit finding on PR #2240 (programmatic confirm-gate bypass, moved to assertRemovalConfirmed at both call sites) | self-test: [clean-worktree] Self-test passed successfully. \| lint: [gate-receipts] recorded a pass for lint:internal (3933 input files) \| typecheck: [gate-receipts] recorded a pass for typecheck:internal (3933 input files) \| manual bypass check: runMergedWorktreeReport({remove:true,dryRun:false}) with CLEAN_WORKTREE_CONFIRM unset now refuses before the removal loop instead of deleting |