diff --git a/.claude/settings.json b/.claude/settings.json index 851940f5bb..3acfcbdfd0 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -212,7 +212,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)", "Bash(tasklist)", "Bash(tasklist /v)", "mcp__Claude_Browser__navigate", 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) | 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 | diff --git a/scripts/check-base-freshness.mjs b/scripts/check-base-freshness.mjs index 082ca359a5..a967485188 100644 --- a/scripts/check-base-freshness.mjs +++ b/scripts/check-base-freshness.mjs @@ -47,6 +47,8 @@ * --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; const asJson = process.argv.includes("--json"); @@ -110,6 +112,46 @@ 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"]); + // 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)", + 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") { diff --git a/scripts/clean-worktree.mjs b/scripts/clean-worktree.mjs index 7ec8c72c2d..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,6 +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."); } + // 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."); } @@ -691,9 +721,40 @@ 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"]); + // 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; + let args4; + try { + 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"); + } + + process.env.CLEAN_WORKTREE_CONFIRM = "1"; + 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 { @@ -1043,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})...`,