Repository navigation
fix(#4941): reap-orphans names .claude/worktrees residue git has forgotten, and reports what it scanned - #5233
behruznassre wants to merge 70 commits into
Conversation
…has forgotten, and reports what it scanned
reapOrphanWorktrees discovered candidates only from .git/worktrees/, so a
harness worktree whose teardown did not complete — no .git file, admin
entry already pruned — was invisible, and the sweep answered
{ok:true, reaped:0, entries:[]} with the directory still on disk.
A second discovery source reads <repo>/.claude/worktrees/ and NAMES each
child that is a real directory (never a symlink), has no .git entry, is
not in `git worktree list`, and is older than the existing mtime guard —
as a `skipped` / `unregistered_residue` row, which also reaches the
existing "N orphan(s) skipped" warning. It never deletes: a missing .git
proves nothing about ownership or cleanliness (a user's own directory, a
dirty worktree whose metadata was removed, a directory another session is
about to `git worktree add` into all look the same), so disposal stays an
operator decision. The discovery root must resolve inside the checkout.
The CLI output gains `scan`, naming which sources were actually read, so
reaped:0 is distinguishable from "did not look". The admin-dir path and
its fail-closed bail-outs are unchanged.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Full local suite on the PR head (Node 24.19.0, macOS, Both failures reproduce identically on a clean
|
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#4941): reap-orphans names .claude/worktrees residue git has forgotten, and reports what it scanned
Head reviewed: c87a758
Summary
Adds a report-only second discovery source (findUnregisteredResidue) to reapOrphanWorktrees and a scan field to worktree.reap-orphans output. Residue is named as skipped / unregistered_residue; nothing is deleted. Root cause addressed: candidates were derived only from .git/worktrees/, and an absent admin dir returned an all-clear.
Classification & gate compliance
- Track: bug fix; #4941 is open with
confirmed-bug.Fixes #4941present. Fix template used. Not a draft. Title conventional, branchfix/4941-...valid. - Changeset present, type
Fixed,pr: 5233. Body lacks the required bold lead (finding m1). - Docs gate:
Fixedis exempt. CHANGELOG.md untouched. No INVENTORY surface touched. - Single concern. One merge commit of
nextin history. - CI (fresh, head above): 44 check-runs, none failing or pending; 0 action_required runs; Stryker jobs SKIPPED. behind_by 0. mergeStateStatus BLOCKED / REVIEW_REQUIRED (review pending).
Root-cause verdict
Root-cause fix. The reaper's only discovery source was the admin dir, which is gone for the target class. The new source plus scan also removes the "could not look" = "nothing there" conflation. Report-only is consistent with the issue's stated minimum.
Functional checklist (issue #4941)
- Backstop names the directory: met (no-admin-dir, beside-live, CLI rows).
reaped: 0distinguishable from did-not-look: met (scan,scan: nullon throw).- Misleading
rollbackPartialWorktreecomment corrected: met. - Safety: lstat (symlink never a candidate),
.gitprobe ENOENT-specific, discovery root realpath-contained, git calls bounded, no deletion. Fail-closed rows verified by reading.
Blast radius
codegraph_impact on reapOrphanWorktrees: 4 affected symbols (itself, cmdWorktreeReapOrphans, two test files). Low risk. New export reapOrphanWorktreesWithScan; existing signature unchanged; CLI JSON change is additive.
Co-change completeness
Generated conformance-tier lists updated for the new tier file. No docs/INVENTORY/CONTEXT.md change required for a Fixed change. Workflows that call the command (quick.md, quick-batch.md, executor-isolation-dispatch.md) discard stderr and ignore content; no edits needed.
CodeGraph Evidence
codegraph_impacttargetreapOrphanWorktrees: 4 affected symbols, tests + cmd wrapper only.codegraph_callerstargetcmdWorktreeReapOrphans: test file only (CLI dispatch is dynamic).codegraph_nodetargetreapOrphanWorktrees(src/worktree-safety.cts:2485): confirms base signature(repoRoot, deps) => ReapResult[]retained by the wrapper.
Memtrace unavailable: monthly query quota exhausted (20972/20933, resets 2026-10-20)
Findings
Minor (judgment):
- m1
.changeset/plucky-eagles-chatter.md:5- body must open with the bold lead per.changeset/README.md:29(**<user-visible change>** — <symptom>.). Fix: bold the first clause. - m2
tests/worktree-safety-reap.test.cjs(#4941 block) - mtime guard has noguardMs-1 / guardMs / guardMs+1rows (only "now" and STALE). Add them usingreapMtimeGuardMs+nowMsdeps. - m3 same file -
absent,unreadable,not_main_checkoutstatuses and theresidue_unreadablereason (non-ENOENT readdir/lstat) are part of the documentedscancontract and untested; Stryker was skipped on this head so there is no mutation evidence. - m4
src/worktree-safety.cts(cmdWorktreeReapOrphans warning) - stderr says "run with DEBUG=1 for details" but no DEBUG path exists in this module for these rows, and the three workflow callers discard stderr. Residue is reported forever with no disposal path. Maintainer decision (opt-in removal flag is the PR's own proposed follow-up); at minimum drop or implement the DEBUG hint.
Verdict
Request changes (minor findings m1-m3 to fix; m4 for maintainer decision). No blockers; CI green at head c87a758, behind_by 0 (no rebase needed).
…can status Review findings m1-m4: - changeset body opens with the bold lead per .changeset/README.md - residue mtime guard covered at guardMs-1 / guardMs / guardMs+1 - absent, unreadable and not_main_checkout scan statuses and the non-ENOENT residue_unreadable rows (child and .git probe) now tested; each fails under a mutation of the compiled module - the skipped-count stderr line no longer points at a DEBUG=1 path that does not exist; it points at the JSON "entries" that carry the rows Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e residue scan Codex review: with a live sibling keeping .git/worktrees/ present, a repo whose remote has no origin/HEAD failed the reap closed and also skipped the report-only residue scan, which does not depend on the default branch. The three default_branch_unresolved exits now run the residue sweep; the reap itself stays fail-closed, and only an unresolvable git dir skips the scan. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for the review. Addressed in
Self-found (Codex whole-PR review), fixed in Verified on the new head (macOS, Node 24.19.0, |
trek-e
left a comment
There was a problem hiding this comment.
Automated maintainer re-review of head b7a2ae62b1. Zero-tolerance policy: every finding at any severity must be resolved before approval.
Review — fix(#4941): reap-orphans names .claude/worktrees residue git has forgotten, and reports what it scanned
Head reviewed: b7a2ae6 (fresh gh pr view this turn: mergeable=MERGEABLE, mergeStateStatus=BLOCKED, reviewDecision=CHANGES_REQUESTED). Compare API vs origin/next: ahead 7, behind 0. 44 check-runs: 39 SUCCESS, 5 SKIPPED, 0 failing/pending.
Note: PR is Bug-Fix track, issue #4941 labels bug, confirmed-bug. PR carries only review: changes requested label.
Untrusted-data note: shared scratchpad file names collided with a concurrent reviewer once; all data below was re-fetched into unique files.
Summary
Report-only second discovery source (findUnregisteredResidue) for reapOrphanWorktrees plus a scan field on worktree.reap-orphans output. No deletion. Prior review (c87a758) found no blockers; m1-m3 fixed, m4 partly. Substance remains sound, but the PR departs from the Agent Brief (reap) and adds a second porcelain parser. mergeStateStatus BLOCKED is solely the standing CHANGES_REQUESTED review (behind_by 0, mergeable, CI all green); no conflict, no rebase needed.
Prior findings status (trek-e 2026-10-07, head c87a758)
| # | Finding | Status | Evidence |
|---|---|---|---|
| m1 | changeset body bold lead | RESOLVED | .changeset/plucky-eagles-chatter.md:5 starts **...** + —, ends (#4941), type Fixed, pr 5233 |
| m2 | mtime guard guardMs-1/guardMs/guardMs+1 | RESOLVED | tests/worktree-safety-reap.test.cjs "a residue aged guardMs..." loop, [-1,0,1], injected nowMs/reapMtimeGuardMs/mtimeSafe, no wall clock; src guard nowMs - mtime < guardMs matches |
| m3 | untested statuses absent/unreadable/not_main_checkout/residue_unreadable | RESOLVED | rows for absent, unreadable (ENOTDIR), not_main_checkout (linked worktree), child + .git EACCES lstat, age-unknown, worktree_list_failed, outside_checkout, symlink child, bail. Stryker still SKIPPED on head: no mutation evidence (see F4) |
| m4 | stderr "DEBUG=1" hint false; residue reported forever, no disposal path | PARTIAL | hint fixed (see "entries" in the JSON output, pinned by test). Disposal/visibility decision NOT made: author declined opt-in removal; callers quick.md, quick-batch.md, executor-isolation-dispatch.md still discard stderr and ignore content, so residue is invisible in automated runs (F1) |
Acceptance checklist (issue #4941 "Expected" + Agent Brief, verbatim intent)
- Second discovery source reading
.claude/worktrees/, child not ingit worktree listand no.git: MET (src findUnregisteredResidue; tests no-admin, beside-live). - Preserve admin-dir path and lock/PID/ancestry guards unchanged: MET (admin loop untouched; only early
return resultsreplaced bysweepResidue/bail). - "treat ... as reapable residue", guarded delete, no rm -rf: NOT MET by design. PR names only, deletes nothing. Issue "Expected" allows "names the directory", but the Agent Brief says reapable and the measured cost (21 orphans / 531 MB, each with node_modules) is not reduced. Needs maintainer ruling in chat (F1).
reaped: 0distinguishable from did-not-look: MET (scan{admin_dir,residue_dir};scan:nullon throw; tests).- Failing-first regression test for no-.git/no-admin-entry residue: MET in form (PR body reports per-mutation red; not executed here, no local test runs permitted).
- Correct rollbackPartialWorktree doc comment: MET (src ~2168-2176), wording awkward (see N1).
Findings
Blocker: none.
Major
- F1 [judgment] src/worktree-safety.cts findUnregisteredResidue / .changeset: Agent Brief and issue title require the backstop to reap; PR is report-only and the three auto-run callers discard stderr and ignore stdout content, so the residue is neither disposed nor surfaced anywhere an operator sees it; 531 MB-class growth persists. This was prior m4 "maintainer decision" and is still undecided. Fix: maintainer confirms report-only in chat, or add the opt-in removal verb / surface entries to callers. Do not treat as resolved by the author's follow-up offer.
- F2 [judgment] src/worktree-safety.cts findUnregisteredResidue (git worktree list block, ~L2810-2818 of head): hand-parses
git worktree list --porcelain(line.startsWith('worktree ')) and issues a secondworktree listcall, while the same function already builds this index at step 3 (canonicalToListed, ~L2551-2567) andparseWorktreeEntries/parseWorktreePorcelainexist as the module's parser (CONTEXT.md:268). One owner per concept: this is a third parser beside two existing ones; also a new parser with no fast-check property (CLAUDE.md "Parsers ... must include at least one fast-check"). Note step 5worktree pruneruns between the two listings, so the stale step-3 index cannot be reused as-is; fix by routing throughparseWorktreeEntrieson the fresh listing (removes the hand parser) or add a discriminating fast-check property.
Minor
- F3 [judgment] findUnregisteredResidue returns rows in
readdirSyncorder (filesystem dependent);entriesJSON is nondeterministic across macOS/Linux/Windows. Fix:names.sort(). - F4 [judgment] No mutation evidence: Stryker SKIPPED on head (prior m3 note); author reports manual mutation reds only. RULESET: Stryker 80% threshold. Surface for maintainer; not executed here.
- F5 [judgment]
admin_dir/residue_dirstatus vocabulary and the new reasonsresidue_unreadable,residue_age_unknown,worktree_list_failed,unregistered_residueare documented only in code comments; CLI JSON contract has no doc/CONTEXT.md entry (CONTEXT.md:268 does not list reapOrphanWorktrees at all).Fixedtype is docs-exempt; add one CONTEXT.md sentence if the maintainer treatsscanas new vocabulary. - F6 [judgment]
not_main_checkoutis also returned for a main checkout using--separate-git-dir/ non-.gitbasename (srcpath.basename(gitDirPath) !== '.git'); status label is then misleading. Fix: document or resolve top viarev-parse --show-toplevel.
Nit
- N1 [judgment] rollbackPartialWorktree comment sentence "Anything else ... is not reached by that sweep. The result is intentionally ignored and a throw is" has a line break mid-clause (~L2171-2175); reflow.
Security / robustness (checked, no finding)
- No deletion anywhere in the new path; cannot touch a live worktree: candidates require lstat-directory (symlink/junction excluded),
.gitENOENT, not in freshworktree list(realpath-normalized both sides), mtime guard, discovery root realpath contained in checkout top (isContainedIn). Failures fail to namedskippedrows, never a clean verdict. Output rows are data only (JSON); stderr prints counts only (no injection surface).execGitis the bounded default (DEFAULT_GIT_TIMEOUT_MS); no argv growth; fs reads are single readdir/lstat per child. Path scope is<top>/.claude/worktreesonly. - TOCTOU irrelevant for report-only.
Test quality
Behavioral (real git repos, faulty-deps injection), asserts on-disk survival (fs.existsSync, file contents) not only JSON, boundary triple present, no wall-clock (nowMs injected; deadOwnerDeps stubs mtimeSafe), no source-grep. Controls assert specific verdicts. Gaps: no fast-check (F2); residue_dir unreadable via realpath failure branch and worktree list partial-registration race untested; Windows junction rows only verified by CI matrix (green).
Blast radius
codegraph_impact reapOrphanWorktrees: 4 affected symbols (itself, cmdWorktreeReapOrphans, tests/worktree-safety-reap.test.cjs, tests/worktree-safety.test.cjs). codegraph_callers cmdWorktreeReapOrphans: test file only (CLI dispatch dynamic). Export surface: additive reapOrphanWorktreesWithScan; reapOrphanWorktrees signature unchanged; CLI JSON gains scan (additive). Risk low. codegraph index is main-checkout (not PR head): lower bound. Memtrace quota exhausted, not run.
Co-change gaps
- Changeset present (Fixed, pr 5233), CHANGELOG untouched, no docs/ required (Fixed), no references/workflows files added (INVENTORY n/a), no shipped workflow growth (no Emitted-Drift-Ack-Growth). Conformance-tier generated lists updated for tests/worktree-safety-reap.test.cjs (both lists). CONTEXT.md: see F5.
- PR format: branch fix/4941-reap-unregistered-residue OK, title conventional,
Fixes #4941, not draft, Fix template used. History has 3Merge branch 'next'commits (c87a758, 629f9f7, b7a2ae6); no conventional-title violation on authored commits.
Behind-by / rebase
behind_by 0 vs origin/next; MERGEABLE; BLOCKED = CHANGES_REQUESTED review decision only (no failing/pending check, no action_required runs). No rebase required.
Verdict
Changes requested. Prior m1-m3 resolved, m4 partial. Open: F1 (Major, maintainer ruling: report-only vs reap / visibility), F2 (Major, parallel porcelain parser + no property test), F3-F6 minor, N1 nit. CI green at b7a2ae6.
Memtrace Evidence
Memtrace quota was exhausted for this review run, so the graph-backed checks were NOT executed; this is stated rather than reported as passed:
get_impact: not run (quota). Blast radius above comes from codegraph where present.get_symbol_context: not run (quota). Callers/callees were read via codegraph where present.recall_decision: not run (quota). The recorded-decision gate was NOT evaluated; this is not a finding that no decision governs the change.find_code_review_issues: not run (quota). The deterministic AST/YAML/cross-module pass did not execute; findings above are judgment-sourced unless marked deterministic.
Findings above Nit are open, so no further CI is being spent on this head. Address them; the fix push re-triggers CI once.
Review round on PR open-gsd#5233: - name each unregistered_residue path on stderr; quick, quick-batch and executor-isolation-dispatch now keep stderr and discard the JSON they never read, so the residue reaches the operator (F1, option c) - route both worktree listings through parseWorktreeEntries instead of hand-parsed porcelain (F2) - sort residue names so entries are filesystem-independent (F3) - a --separate-git-dir main checkout is asked of git instead of being labeled not_main_checkout; git failure there is top_unresolved (F6) - document the verb's scan vocabulary in CONTEXT.md (F5); reflow the rollbackPartialWorktree comment (N1) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex review (three minors): - residue paths are JSON-quoted on stderr; a directory name with a newline or escape sequence could forge a [gsd] line or drive the terminal (the JSON keeps the exact name) - the `<top>/.git` shortcut also requires the cwd inside `<top>`, so a --separate-git-dir that is itself named `.git` scans the checkout, not the metadata directory's parent - a residue location that could not be read is reported on stderr, and the skipped-orphans hint names the command, since the workflow callers discard the JSON Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… the git dir Codex round 2: the `<top>/.git` fast path still picked the wrong checkout for separate metadata in the checkout's parent (/project/.git beside /project/checkout) and missed its residue. The git dir's location says nothing reliable about the checkout's, so the shortcut is removed: the residue scan always asks git for the common dir and the top (the path both review rounds already covered). The admin-dir argv pin now includes those two calls. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…sweep The startup orphan sweep's call line changes from `2>/dev/null` to `>/dev/null` identically in all three callers so the residue paths on stderr reach the operator. Row 48 read the quick-batch.md copy as quick-batch phase work and then charged the quick.md copy. Recognize exactly that line, old or new form, as a mechanical sweep, alongside the existing open-gsd#3730/open-gsd#2529/open-gsd#4834/open-gsd#4780 carve-outs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed the 2026-10-09 review. Head is now F1 (Major), residue neither disposed of nor shown to anyone: kept report-only. It now reaches the operator.
F2 (Major), third porcelain parser: both F3: residue names are sorted, so F4, no mutation evidence: the Stryker job had nothing to run because F5: CONTEXT.md's Worktree Safety Policy Module entry now documents F6, N1: the comment is reflowed. Codex reviewed the whole PR twice, and both rounds' findings are fixed here:
The row-48 carve-out came from the full suite after the second round, so no Codex round has seen it. Gates on this head: |
trek-e
left a comment
There was a problem hiding this comment.
Review of head 7ba53cf77f47 — changes requested
Summary. Adds report-only discovery of .claude/worktrees/ residue that git has forgotten (findUnregisteredResidue) and a scan field on worktree.reap-orphans. This round adds residue paths on stderr (JSON-quoted), changes three workflow callers from 2>/dev/null to >/dev/null, routes both porcelain reads through parseWorktreeEntries, sorts residue names, updates CONTEXT.md, locates the checkout through git, adds top_unresolved, and adds a row-48 carve-out.
Prior findings
| # | Status | Evidence |
|---|---|---|
| F1 | Not resolved | Visibility is real; disposal is still absent, and the new stderr text introduces a hazard (M2). See Major 1. |
| F2 | Resolved | Both porcelain reads go through parseWorktreeEntries(stdout.replace(/\r\n/g,'\n')) (src ~2589, ~2828); no hand parser left. |
| F3 | Resolved (cannot fail on macOS) | fs.readdirSync(residueDir).sort(). |
| F4 | Resolved; claim verified | COVERED in scripts/mutation-matrix.cjs lists 14 modules, none of them worktree-safety; the Stryker jobs are SKIPPED. |
| F5 | Resolved | CONTEXT.md:268 documents reapOrphanWorktreesWithScan, the residue rows and the admin_dir / residue_dir vocabulary. |
| F6 | Resolved | --git-common-dir and --show-toplevel, top_unresolved, rows for --separate-git-dir, .git-named metadata elsewhere and metadata in the parent. |
| N1 | Resolved | Comment reflowed. |
Classification and gates. #4941 carries bug + confirmed-bug; bug-fix track. Fixes #4941, fix template, conventional title, branch fix/4941-reap-unregistered-residue. Changeset .changeset/plucky-eagles-chatter.md is type Fixed, pr: 5233, bold lead, ends (#4941). CHANGELOG.md untouched; Fixed is docs-exempt. No references/ or workflows/ files added. No Emitted-Drift-Ack-Growth trailer is needed: the three shipped workflow edits each shrink by one byte (2>/dev/null → >/dev/null) and no emitted-drift check is red.
Acceptance criteria, #4941
- Issue "Expected": "the backstop names the directory — or, at minimum, does not answer
ok: true, reaped: 0when it did not look." — Met. - Agent Brief 1: "Give
reapOrphanWorktreesa second discovery source … treat any child directory that is not present ingit worktree listoutput and has no.gitfile as reapable residue" — Not met (nothing is reaped). - Agent Brief 2: preserve the
.git/worktrees/admin path and its guards unchanged — Met (admin loop untouched). - Agent Brief 3: "Do not add an
rm -rffallback … usefs.rmSync(path, {recursive: true})… or an equivalent guarded delete." — Not met (disposal removed entirely). - Agent Brief 4: make
reaped: 0distinguishable from "did not look" — Met (scan,scan: nullon throw). - Agent Brief 5: failing-first regression test — Met in form. I did not re-execute it (local
node --testis not permitted). - Agent Brief 6: update the misleading
rollbackPartialWorktreecomment — Met.
Findings
Major 1 — Agent Brief items 1 and 3 are unmet: residue is reported but never disposed of [judgment; maintainer ruled: guarded removal is required]
src/worktree-safety.cts:2906 and .changeset/plucky-eagles-chatter.md ("Nothing is deleted"). The measured cost in the issue (21 orphans, 531 MB, each with node_modules) is unchanged: the operator is told about the directory on every run and has no supported way to dispose of it. Please add the guarded removal the Agent Brief describes: fs.rmSync(path, {recursive: true}) or an equivalent that does not traverse reparse points or junctions, behind the same discovery guards (lstat-directory with symlinks and junctions excluded, no .git entry, absent from a fresh git worktree list, the mtime guard, discovery-root containment through isContainedIn). Keep the report-only rows for anything that fails a guard, and extend the tests (on-disk survival for guarded cases, removal for eligible ones, plus the mtime boundary at guardMs-1 / guardMs / guardMs+1).
Major 2 — the new stderr text tells the reader to "delete" with no safe method [fact for the text; severity is judgment]
src/worktree-safety.cts:2906: "delete once nothing in it is needed". These directories typically hold a full node_modules. The issue and agents/gsd-code-fixer.md forbid rm -rf because on Windows it follows reparse points. The three automatic callers (quick.md:159, quick-batch.md:117, executor-isolation-dispatch.md:59) now route that sentence into an LLM agent's transcript, which is the party most likely to act on it with rm -rf. Once Major 1 is addressed, replace the imperative with the actual verb or outcome; do not leave an unqualified "delete".
Minor 1 — attacker-controlled directory names reach agent context [judgment, prompt-injection surface]
A committed .claude/worktrees/<text>/file in a cloned repo is, after the 5-minute REAP_MTIME_GUARD_MS (src:2481), now printed to stderr by three auto-run workflows that previously discarded stderr. JSON quoting stops newline and ESC forging but not instruction-like text, and the printed list has no cap on count or length (residue.map(...), src:2908). Cap the number of paths (first N plus "and M more") and consider printing only the repo-relative name.
Minor 2 — JSON.stringify leaves control and bidi characters raw [fact]
It does not escape U+007F, the C1 controls U+0080–U+009F (checked: c2 9b survives), or bidi overrides such as U+202E (e2 80 ae survives). The test covers only \n and ESC. 8-bit CSI is honored by some terminals, and an RLO can spoof a path shown to an operator. Escape [\u007f-\u009f\u202a-\u202e\u2066-\u2069\u2028\u2029] after JSON.stringify and extend the test.
Minor 3 — a tautological warning outside a git repo [fact by code reading; not executed because the built module is absent in my worktree]
bail('git_dir_unresolved') yields residue_dir: 'not_scanned', which is not in the exclusion list at src:2912, so running the verb outside a git repo prints .claude/worktrees was not scanned (not_scanned; admin dir: git_dir_unresolved) on every call (silent before). Exclude git_dir_unresolved from the warning or reword it.
Minor 4 — unclassifiable residue is invisible to automated runs [fact]
residue_unreadable, residue_age_unknown and worktree_list_failed are counted in the generic "N orphan(s) skipped" line (src:2916-2919) but never named on stderr, while the JSON is now discarded by the callers. Name them (quoted and capped) like unregistered_residue.
Minor 5 — the row-48 carve-out also admits a new old-form line [fact]
tests/gsd-quick-batch-quick-regression.test.cjs:122: the exact-line regex ^gsd_run query worktree\.reap-orphans (?:2)?>\/dev\/null \|\| true$ applies to added and removed lines, so a brand-new old-form line anywhere in quick.md also passes row 48. Restrict it to a - old-form / + new-form pair (as isWorkstreamForwardingSweepLine does with added and removed bodies), and add the #4941 carve-out to the function docstring (lines 75-94 list only #2529 and #4834). No external review has seen this carve-out until now.
Nit 1. On Windows, JSON.stringify doubles backslashes in the printed path (C:\\Users\\…), so copy-paste yields wrong paths.
Nit 2. The "the three workflow callers" row asserts on shipped prose with includes / regex; consistent with existing rows and exempt from the lint rule, but source-grep in character.
Nit 3. The sort row cannot fail on macOS (the author acknowledges this); fs.readdirSync is called directly, so there is no injection seam, unlike readDirSafe.
Security and robustness. No deletion exists in the current path; candidates require lstat-directory, .git ENOENT, absence from a fresh worktree list (realpath-normalized), the mtime guard and realpath containment. Git calls are bounded (DEFAULT_GIT_TIMEOUT_MS). No argv growth. Every unreadable branch fails closed with a named row. The remaining concerns are the output surface (Major 2, Minors 1 and 2); they become more serious once Major 1 adds real deletion, so the guards above matter.
Not verified. I did not execute any test; the per-row "went red under mutation" claims are the author's.
CI at this head. All required lanes SUCCESS: Ubuntu full and shards, macOS and Windows conformance, smoke, lint-tests, docs-lint, changeset-lint, security, coverage gate, Required tests. Six CANCELLED check runs at 19:00:44–19:00:52Z (PR mergeability / mergeability, validate-target, Pull request template format, validate-title, Issue link required, default-flip-documentation) each have a same-named SUCCESS run completed 19:00:50–19:01:08Z. SKIPPED: Stryker ×2, emitted-baseline artifact, test (inert CI), auto-merge. No failing, pending or action_required runs. Not the reason for this verdict.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run. The recorded-decision check was not evaluated. The index is of the main checkout, not this PR head, so it is a lower bound.
codegraph_impact reapOrphanWorktrees: 4 symbols (reapOrphanWorktreesatsrc/worktree-safety.cts:2485,cmdWorktreeReapOrphansat :2742,tests/worktree-safety-reap.test.cjs,tests/worktree-safety.test.cjs).codegraph_callers reapOrphanWorktrees:cmdWorktreeReapOrphans(src/worktree-safety.cts:2742) and the two test files. The signature is unchanged and the new exportreapOrphanWorktreesWithScanis additive.- Finding: the code blast radius is small. The meaningful exposure is in what the three shipped workflows now put in agent transcripts (Major 2, Minors 1 and 2).
Verdict
Changes requested. Blocker 0, Major 2, Minor 5, Nit 3.
trek-e
left a comment
There was a problem hiding this comment.
Review of head 7ba53cf77f47 — changes requested
Summary. This is the full pass on the same head; the earlier review (01:51Z) is not edited. All three Skill-pass sub-agents and the isolated pass agree with it and add one Major it did not have: a sweep run from a linked worktree is silent, which is the all-clear the PR itself says must never happen. Two earlier Minors (control and bidi characters, uncapped output) are rated Major by the isolated pass; both ratings are kept. Blocker 0, Major 5, Minor 8, Nit 5.
Classification and gates. #4941 carries bug + confirmed-bug; bug-fix track. Fixes #4941, fix template, conventional title, branch fix/4941-reap-unregistered-residue. Changeset .changeset/plucky-eagles-chatter.md: type Fixed, pr: 5233, bold lead, ends (#4941); CHANGELOG.md untouched. The three shipped workflow edits each shrink by one byte, so no Emitted-Drift-Ack-Growth trailer is needed; node scripts/benchmark-compact-content.cjs --check reports the baseline up to date. No references/ or workflows/ file added.
Acceptance, issue #4941 (verbatim)
- Expected: "the backstop names the directory — or, at minimum, does not answer
ok: true, reaped: 0when it did not look." — Met for the registered-checkout case; not met from a linked worktree (Major 5). - Agent Brief: "Give
reapOrphanWorktreesa second discovery source … treat any child directory that is not present ingit worktree listoutput and has no.gitfile as reapable residue" — Not met: nothing is reaped. - "Preserve the existing
.git/worktrees/-admin-dir path and its lock/PID/ancestry guards unchanged; this is additive" — Met. - "Do not add an
rm -rffallback … usefs.rmSync(path, {recursive: true})… or an equivalent guarded delete." — Not met: disposal removed entirely. - "make
reaped: 0distinguishable from 'did not look'" — Partial: thescanfield does it, butadmin_dir: 'unlisted'still merges "missing" and "unreadable" (Minor 7). - Failing-first regression test, and the
rollbackPartialWorktreecomment — Met in form.
Findings
Major 1 — residue is named but never disposed of [judgment; the maintainer ruled that guarded removal is required, carried] src/worktree-safety.cts (findUnregisteredResidue, report-only by design) and the changeset ("Nothing is deleted"). The issue's measured cost (21 orphans, 531 MB, each with node_modules) is unchanged and the residue is reported on every sweep forever. Add the guarded removal the Brief describes: fs.rmSync(path, {recursive: true}) or an equivalent that does not traverse reparse points or junctions, behind the discovery guards (lstat-directory with symlinks and junctions excluded, no .git entry, absent from a fresh git worktree list, the mtime guard, containment through isContainedIn); keep the report-only rows for anything failing a guard and extend the tests (on-disk survival for guarded cases, removal for eligible ones, the mtime boundary at guardMs-1/guardMs/guardMs+1).
Major 2 — the new stderr text says "delete" with no safe method, into an LLM transcript [fact for the text, carried] cmdWorktreeReapOrphans prints "…not removed; delete once nothing in it is needed:". The three automatic callers (quick.md:159, quick-batch.md:117, executor-isolation-dispatch.md:59) now route it into an agent transcript, while agents/gsd-code-fixer.md forbids rm -rf because it follows reparse points on Windows, and the CLI test pins the sentence verbatim. Replace the imperative with the actual verb or outcome once Major 1 lands and update the pinned string.
Major 3 — JSON.stringify is not terminal- or transcript-safe [verified by node and by the isolated pass's probe against a scratch repo; earlier rated Minor] It escapes U+0000–U+001F, the quote and the backslash only. A directory name with U+009B (an 8-bit CSI introducer), U+007F, U+202E (RLO) or U+2028/U+2029 reaches stderr raw, so a displayed path can be reordered or a terminal driven; the source comment ("a raw newline or escape sequence would forge lines or drive the terminal") claims a guarantee the code does not give, and the test covers only ESC and LF. Escape [\u007f-\u009f\u200b-\u200f\u202a-\u202e\u2066-\u2069\u2028\u2029] after JSON.stringify, or print a sanitized, index-numbered basename, and add a row per class plus a fast-check property over arbitrary names (no raw control or bidi character; one [gsd] line per path).
Major 4 — attacker-controlled names reach the transcript, unbounded, on every run [verified by the isolated pass; earlier rated Minor] A cloned repo can commit .claude/worktrees/<instruction-like text>/file; after the 5-minute REAP_MTIME_GUARD_MS it is printed verbatim inside quotes, directly after the "delete" sentence (the isolated pass's probe printed IGNORE PREVIOUS INSTRUCTIONS and run rm -rf ~ then report done that way), with the full absolute path (home directory and username), with no cap on row count or name length (240-character names printed in full), and again at every /gsd-quick, /gsd-quick-batch and execute-phase start because the residue is never removed. Quoting does not neutralize natural-language injection. Print a count and a fixed instruction, or a capped list of sanitized repo-relative names ("first N and M more"), and test limit-1/limit/limit+1 for the cap.
Major 5 — a sweep from a linked worktree is silent: the all-clear the PR forbids [verified by reading the exclusion list; Spec and isolated passes agree] cmdWorktreeReapOrphans warns "not scanned" unless residue_dir is in ['scanned', 'absent', 'not_main_checkout']. The workflows run from linked worktrees routinely (this repo's own harness sessions do); there the scan never runs, stderr says nothing, and the output is reaped: 0, entries: [] — scan.residue_dir carries the fact only in JSON, which the three callers send to /dev/null. The PR's own principle is that reaped: 0 must not read as an all-clear when nothing was looked at. Either scan the main checkout's residue from a linked worktree (resolve it through the common dir) or emit the not-scanned line for this status, and add a CLI-level row (the existing row asserts only the JSON field).
Minor 1 — a tautological warning outside a git repo [verified by reading, carried] bail('git_dir_unresolved') yields residue_dir: 'not_scanned', not in the exclusion list, so the verb prints ".claude/worktrees was not scanned (not_scanned; admin dir: git_dir_unresolved)" on every call outside a repository (silent before).
Minor 2 — unclassifiable residue is invisible to automated runs [verified by reading, carried] residue_unreadable, residue_age_unknown and worktree_list_failed are counted in the generic "N orphan(s) skipped" line but never named on stderr while the JSON is discarded; the generic line also tells the reader to run gsd-tools query worktree.reap-orphans, which the callers cannot act on.
Minor 3 — the row-48 carve-out admits a new old-form line [verified, carried] tests/gsd-quick-batch-quick-regression.test.cjs:122: the exact-line regex applies to added and removed bodies, so a brand-new 2>/dev/null line anywhere in quick.md also passes; pair removed-old with added-new as isWorkstreamForwardingSweepLine does, and add #4941 to the docstring (lines 75-94).
Minor 4 — removing 2>/dev/null widens stderr to everything gsd_run emits [isolated pass; security pass 3/10] Runtime build notices, Node warnings and the unquoted [gsd] worktree.reap-orphans failed: <err.message> line (which can embed paths) now reach the transcript on each run, though only the residue lines were intended. Route residue to a dedicated channel, or at minimum say so in the changeset.
Minor 5 — the issue's second symptom is untouched [Spec and isolated passes; judgment on scope] The issue observes worktree.cleanup-wave returning branch_mismatch with stderr: "" for this residue; the diff leaves the wave-cleanup region unchanged, so an operator with the residue in a manifest still gets a misleading empty-stderr block.
Minor 6 — the tests cannot detect Majors 3-5 [verified from the diff] The quoting test uses one hostile name with ESC and LF only and skips on win32; no fast-check property for the escaping and bounded-output contract; no cap rows; "the three workflow callers keep stderr" greps the shipped prose for 2>/dev/null, asserting the text of the fix rather than that stderr reaches anything.
Minor 7 — admin_dir: 'unlisted' merges "missing" and "unreadable" [Spec pass; the code comment admits it] The Brief asks reaped: 0 to be distinguishable from "did not look"; the two causes still read the same.
Minor 8 — Windows path comparison [Standards pass, judgment] registered and canonical are compared as raw realpathSync.native strings; drive-letter case and \\?\ prefixes are not normalized on either side, and the canonicalOrResolved fallback can disagree with realpathSync.native when realpath fails for only one side; nothing exercises this on win32.
Nit 1 [verified] On Windows JSON.stringify doubles backslashes in the printed path (C:\\Users\\…), so copy-paste yields the wrong path.
Nit 2 [judgment] The sort row cannot fail on macOS (fs.readdirSync is called directly, no injection seam).
Nit 3 [judgment] The callers row asserts on shipped .md prose with includes/regex; outside local/no-source-grep but source-grep in character (a behavioral check would run the shipped line against a stub that writes to stderr).
Nit 4 [verified by reading] parseWorktreeEntries trims the path, so a registered worktree whose path has leading or trailing whitespace will not match its realpath and could be named as residue; the refactor now routes both passes through it.
Nit 5 [judgment] ReapScan.admin_dir/residue_dir are string, not unions (an enum-locked test is impossible); bail is defined before gitDirPath is assigned (temporal coupling); two near-identical exports (reapOrphanWorktrees and the WithScan wrapper); the new scan JSON field has no reference-doc entry.
Considered, not findings: no deletion path exists, so traversal and TOCTOU are moot (lstat excludes symlink and junction children, the .git probe is ENOENT-specific, discovery is realpath-contained through isContainedIn); the mtime guard being spoofable only changes whether a line prints; all new git calls are bounded argv arrays; the issue's wording lets "names the directory" be the minimum, but the maintainer ruled that guarded removal is required, and I do not re-rule it here.
Passes run
| Pass | Result |
|---|---|
/diagnose |
run; the isolated pass's transpiled-module probe against a scratch repo with hostile names, and the stderr read |
/root-cause-analysis |
run; the early return conflating "nothing there" with "could not look" is fixed, but the linked-worktree case reintroduces it and disposal is still absent |
/qa-test-architect |
run; Minors 3 and 6, Nits 2 and 3 |
/skills-from-the-artificer (bugfix-review) |
run; block below |
/code-review |
run as two parallel sub-agents: Standards (Minors 6 and 8, Nits 3 and 5) and Spec (Major 5, Minors 5 and 7, acceptance) |
/security-review |
run: identification sub-agent found no additional issue at 80% (path disclosure 3/10, widened stderr 3/10); the injection and control-character findings above are from the isolated pass and my node check |
| Isolated adversarial pass | run by a fresh agent with no prior context; found Majors 3, 4, 5 and Minors 4, 5, Nit 4 |
Not run (Memtrace is unavailable): the Deterministic Review Pass (detect_changes, find_cross_module_issues, find_code_review_issues, find_yaml_rule_matches, review_github_pr), preflight_check, risk tiering (find_central_symbols, find_bridge_symbols), the recorded-decision check (recall_decision, governing_rules) and the co-change gate. None is reported clean. I did not execute any test (local node --test is not permitted); Stryker is SKIPPED and worktree-safety is not in the COVERED matrix, so no mutation evidence exists for src/worktree-safety.cts.
Laws that apply
Subject: PR #5233 adds report-only discovery of unregistered .claude/worktrees/ residue and a scan field to worktree reap-orphans, and lets its stderr reach three auto-run workflows' agent transcripts.
- kerckhoffs-principle — fired: the safety of the output channel rests on directory names being benign; a committed
.claude/worktrees/<text>/makes the name the attacker's input, and quoting does not hold once the mechanism is public (cmdWorktreeReapOrphans). Action: Majors 3 and 4. - leaky-abstractions — fired:
JSON.stringifyis a JSON escaper, not a terminal or LLM-transcript escaper, and the PR leans on it as one. Action: Major 3. - hyrums-law — fired: three callers that discarded stderr now ingest it, so every message in the verb (including the unquoted
failed:line and the generic skipped count) becomes an interface. Action: Minors 2 and 4. - postels-law — fired: input (directory names) is accepted as-is and normalized only at the printer by an escaper that is too narrow; normalization belongs at one edge with a bounded, sanitized form. Action: Majors 3 and 4.
Considered and cleared: knuths-optimization-principle (no optimization), kernighans-law (the residue function is long but linear).
CI. 33 checks SUCCESS and 5 SKIPPED (Stryker ×2, Publish emitted-baseline artifact, test (inert CI), auto-merge); six CANCELLED runs from 19:00:44-52Z each have a newer same-name SUCCESS, so none survives newest-wins; nothing failing, pending or awaiting approval. The branch is BEHIND next by one disjoint commit (a rebase is a CI condition, not a finding). I am not approving: the Majors need a push.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted and the server disconnected, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run, and the recorded-decision check was not evaluated.
codegraph_impact reapOrphanWorktrees(earlier pass on this head): 4 symbols (reapOrphanWorktreesatsrc/worktree-safety.cts:2485,cmdWorktreeReapOrphansat :2742,tests/worktree-safety-reap.test.cjs,tests/worktree-safety.test.cjs);codegraph_callers:cmdWorktreeReapOrphansand the two test files. The signature is unchanged andreapOrphanWorktreesWithScanis additive. The index is the main checkout, so branch-new symbols are absent.- Finding: the code blast radius is small; the meaningful exposure is what three shipped workflows now put into agent transcripts (Majors 2-4) and the silent linked-worktree case (Major 5).
Verdict
Changes requested. Blocker 0, Major 5, Minor 8, Nit 5.
…d keep the transcript safe Answers the 2026-10-10 review of open-gsd#5233; the maintainer ruled that guarded removal is required. - Major 1: a `.claude/worktrees/agent-*` child is REMOVED (fs.rmSync, recursive — it unlinks a nested symlink or junction, never follows it) when it is a real directory with no `.git` entry, absent from `git worktree list`, older than the mtime guard, holds no git-tracked file, and resolves inside the checkout; any other child is reported with the guard that kept it and left on disk. - Majors 2-4: stderr (read by an agent: the callers keep it) names removed directories only, escaped by formatDiagnosticToken, cut to 64 code points, at most five; kept ones are counted by reason, never named; the failure message is escaped too. - Major 5: a sweep from a linked worktree says on stderr that it did not look; outside a git repository nothing is said (Minor 1). - Minors: wave cleanup blocks a directory with no `.git` entry as `worktree_unregistered` instead of `branch_mismatch` with an empty stderr; `admin_dir` tells `absent` from `unreadable`; Windows path comparison folds case and the `\\?\` prefix; the row-48 carve-out admits only the paired 2>/dev/null -> >/dev/null swap; the shipped caller lines are run, not grepped; scan statuses are typed unions; parseWorktreeEntries keeps edge whitespace in a path; CLI-TOOLS.md documents the sweep. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… a verified path Codex whole-PR review (round 1 on this head) reproduced five ways the guarded removal could delete something that is not harness residue. Each now has a guard and a row whose would-be victim survives: - `.claude` and `.claude/worktrees` must be real directories (not links; `residue_dir: aliased`), re-checked right before each removal, and the removed path is built from that verified chain (parent-alias swap; an alias to tracked content inside the checkout); - a harness branch must exist for the child (`agent-<id>` or `worktree-agent-<id>`) — positive evidence it was this repo's worktree; - no repository may lie inside it: no `.git` at any depth, no bare layout (a bare repo, a nested repo with uncommitted work); - the tracked-file check runs per child with a literal, case-insensitive pathspec (a case-only rename; a backslash in a POSIX name); - wave cleanup resolves a relative entry path against plan.repoRoot; - docs say which children get a row and which do not. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ode_modules, remove only the path resolved at removal Codex whole-PR review, round 2, reproduced three more deletion paths: - the top came from `--show-toplevel`, which the subprocess seam trims, so a checkout whose path ends in a space swept a same-named sibling repository; it now comes from `--show-cdup` (`../` steps survive trimming); - the repository walk skipped node_modules, so a package checked out from a local repository (with uncommitted work) was deleted; node_modules is walked, under a larger bound; - a `.claude/worktrees` swapped for a link after the final checks redirected the path-based removal; the child is now resolved at removal time, must sit directly in the verified `.claude/worktrees`, and that resolved path is the one removed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks — this round answers the 05:06Z review (it supersedes the 01:51Z one). Head: Major 1 — guarded removal. Done, as ruled. A
Removal is A Codex whole-PR pass on my first cut of the removal found five ways it could delete something that is not residue. All five were reproduced in its sandbox, and each now has a guard and a row whose would-be victim survives on disk:
A second Codex round on the result reproduced three more paths, now fixed with rows. (1) The checkout top came from The harness-branch marker is what turns "no Major 2 — "delete" into a transcript. The imperative is gone. A removed directory is reported as removed ( Major 3 — terminal/transcript safety. Names go through Major 4 — attacker-reachable names, unbounded, every run. Only removed directories are named. They are capped at five ( Major 5 — silent from a linked worktree. Stderr now says Minor 1 — outside a git repository ( Nits — 1: only names are printed now, not full paths, so no doubled backslashes. 2: the residue dir is listed through the injectable One existing row changed for a reason worth naming. Verification. Mutation. |
…t-injection scan does not flag The security job's scan flagged the literal phrase in a test fixture; the row only needs a name that addresses a reader, which is never printed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…an a literal exec-bit mode Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re: review 5638711345. Pushed Minor 1 — Minor 2 — a partial removal is not "left in place". A Minor 3 — what the guards cannot see. I took your listing option. A removed row now carries Minor 4 — Self-test report. Nit 5. Nit 6. Verification: the 9 new or changed rows fail on |
…ecGit drops inherited GIT_DIR / GIT_WORK_TREE open-gsd#5747 (open-gsd#5548) made every execGit spawn drop the inherited repo-location variables, so an inherited GIT_DIR or GIT_WORK_TREE can no longer point the sweep's git calls at another repository: the sweep scans this checkout and its own index keeps tracked work. The two tests that expected git_mismatch for those variables now assert that outcome, and fail if the scrub is removed. core.worktree is config, not env, and still yields git_mismatch. The stderr reason, CLI-TOOLS.md, CONTEXT.md and the source comments stop naming the env variables as a cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Update for the red CI after the Cause. #5747 (#5548) made every Fix (6aa0483), test and wording only. No behavior change in
Gate on the head: Codex, whole PR against the merge-base: no blocker or should-fix findings. Two accuracy nits:
The PR's own |
trek-e
left a comment
There was a problem hiding this comment.
Review - fix(#4941): reap-orphans names .claude/worktrees residue git has forgotten, and reports what it scanned
Head reviewed: 6aa0483 (live gh pr view: mergeable=MERGEABLE, mergeStateStatus=BLOCKED, not draft). Whole PR reviewed. Delta since my last review (2a7c727): b1e2df6 (aliased for a link in the chain, residue_remove_incomplete, contents, lstatState), 478f483, merges of next, and 6aa0483 (git_mismatch names only core.worktree after #5548). Findings 1, 2, 3, 5 and 6 of the previous review are addressed in code; findings below are new at this head.
Summary
Second discovery source (findUnregisteredResidue) with guarded removal, a scan report, stderr kept in the three callers, worktree_unregistered in wave cleanup. The removal path has one reproduced data-loss path: the sweep's own git worktree prune runs before the registration list is read, so a worktree git still lists (admin entry present, .git file gone) is treated as unregistered and deleted with its uncommitted files.
Classification & gate compliance
- Bug-fix track. #4941 is OPEN with
bug+confirmed-bug;Fixes #4941; conventional title; branchfix/4941-reap-unregistered-residue. Competing PRs for #4941: only #5233. RULESET.CONTRIB.CLASSIFY.fix=requires confirmed/confirmed-bug before implementation- met.- Changeset
.changeset/plucky-eagles-chatter.md(Fixed,pr: 5233); docs/CLI-TOOLS.md and CONTEXT.md updated; workflow edits are2>/dev/null->>/dev/null(one byte shorter each). - Merge state: GitHub reports mergeable=MERGEABLE, mergeStateStatus=BLOCKED.
git merge-tree --write-tree origin/next <head>is clean (tree 45ff3c82fe1d); the branch is 2 commits behindnext(not a finding under the merge queue). - CI at this head (newest run per name): all green except
Self-test report(fail_suite_not_passed, finding 2) andReviewed or maintainer(waits for a review). 0 runsaction_required. Windows and macOS conformance lanes green. No CI was released and no rebase was pushed.
Must-Have Acceptance Checklist (issue #4941 + the Agent Brief comment of 2026-09-24)
- Second discovery source over
.claude/worktrees/: met. - Child not in
git worktree listand with no.gitentry (brief: "both conditions together cannot match a live worktree"): NOT met in the sweep path, see finding 1. - Existing
.git/worktreespath and its guards preserved: met (admin loop unchanged apart from early returns now routed throughsweepResidue). - No
rm -rf; guardedfs.rmSync: met in design; finding 1 and 3 are removal-gate gaps. reaped: 0distinguishable from "did not look" (scan): met in the JSON; the stderr that the callers now keep stays silent for two admin-dir states, finding 4.- Failing-first regression test in
tests/worktree-safety-reap.test.cjs: met for the residue shapes; no row for finding 1's shape. rollbackPartialWorktreedoc comment corrected: met (src hunk at 2221).- Issue's second symptom (
branch_mismatchwith empty stderr): met (worktree_unregistered).
Root-cause verdict
Root-cause fix for the stated gap: residue is now discovered from the filesystem and scan separates "scanned/absent/unreadable/aliased/...". The fix site is on the failing path (cmdWorktreeReapOrphans -> reapOrphanWorktrees, get_symbol_context caller edge). /root-cause-analysis has no installed skill; the root-cause comparison was done manually. get_timeline on reapOrphanWorktrees (branch next): 3 versions, one AST hash (not repeatedly patched).
Blast radius and risk tier
get_impact (node 7597a7ef, branch next, both, depth 5): risk HIGH on the graph, 33 affected, 1 direct upstream caller, 0 cross-repo; preflight_check (upstream view): LOW, 1 affected. reapOrphanWorktrees cyclomatic 46 / cognitive 81 (preflight_check). find_central_symbols (top 100) and find_bridge_symbols (top 10) on next: no src/worktree-safety.cts symbol appears; tests/worktree-safety.test.cjs closures rank as bridges. PR-added symbols are not in the base graph, so graph answers describe the pre-PR callers.
Co-change completeness
get_cochange_context/preflight_check co-change returned only archived changeset files (no replayed pairs for the file), so unproven by that tool. The diff carries the changeset, docs/CLI-TOOLS.md, CONTEXT.md, the two generated conformance tier lists and the three workflow files; Docs Required, docs-lint, changeset-lint, Required tests are green.
Memtrace Evidence
Graph state: repo gsd-core, indexed branch next; list_indexed_repositories shows graph_head 0ce32128b, 234076 nodes, 1144113 edges.
get_impact(targetreapOrphanWorktrees, node_id 7597a7ef-340b-561c-adbc-68c63958cb9c, branch next, both, depth 5): risk HIGH, total_affected 33, 40+ affected processes including CmdWorktreeReapOrphans, CmdWorktreeCleanupWave, ExecuteWorktreeWaveCleanupPlan,cross_repo_affected: [].get_symbol_context(same node): 1 caller (cmdWorktreeReapOrphans), calleesexecGitandgitResultOk, communityReasonWorktreePath, processWorktree-safetyCmdWorktreeReapOrphans; 16 omitted reference edges.recall_decision("worktree reap-orphans residue removal guarded deletion .claude/worktrees rm -rf prohibition junction", top_k 5): 5 decisions, all low-score (decay 0.4) about using codegraph with a worktree path, none about reaping or deletion guards.governing_rulesonsrc/worktree-safety.ctsreturned only ADR-443 (reasoning-effort cascade), which does not govern this change. Result: no recorded decision proven for this change; this is not a clean bill, and no ADR indocs/adrnames #4941 or residue removal.find_code_review_issues(online mode, max 30): 0 issues,_graph_state: ready, over a re-typed excerpt of the diff (the wave-cleanup hunk), because the tool takes inline text and the full diff is 154,543 bytes. Full-diff coverage comes frommemtrace code-review --diff pr5233.diff --repo-id gsd-core --repo-root <PR checkout> --review-mode online --min-severity low --max-comments 30 --graph-mode strict --json: commentCount 0, sourceCounts ast 2 / cross_module 0 / online 0 / yaml 0, graphState ready (the 2 raw ast hits were filtered by the ranking policy).review_github_pr(post:false, same options): commentCount 0, sourceCounts ast 2, graphState ready.find_cross_module_issuesandfind_yaml_rule_matchesran over theisPresentWithoutGitEntryhunk only: 0 issues (cross-module reportedinvalid unified difffor the excerpt's hunk counts, so it did not evaluate; treat as not run).find_ast_review_issues: skipped, no.pyin the diff.detect_changeson the four source/test files: all matched, file-level only (it returned 0 added/removed lines for a file list).- Deterministic tools found nothing; findings 1-8 come from the isolated adversarial pass (reproductions) and my own re-run of its repro, so a clean deterministic result here is not evidence of absence.
Findings
Attribution: [D] = reproduced by code run, [J] = judgment.
Blocker
- [D] The "absent from
git worktree list" guard is defeated by the sweep's own prune (src/worktree-safety.cts:2805execGit(['worktree','prune'])runs, thenreturn sweepResidue('scanned'); the registration list is read afterwards,:2954worktree list --porcelain -z). A worktree git still lists (admin entry present,.gitfile gone, reportedprunable) is pruned by step 5, then found "unregistered" and deleted with its uncommitted files. Repro (isolated passt6.js, re-run by me):git worktree add -b agent-r .claude/worktrees/agent-r, delete<wt>/.git, adddirty.txt->reaped / unregistered_residue,contents: ["a.txt","dirty.txt"], directory gone; control with only that prune stubbed ->entries: [], directory kept. Before this PR that state lost only git metadata; now the working files are destroyed, against docs/CLI-TOOLS.md, CONTEXT.md and the PR body ("not ingit worktree list"). Fix: read the registration list before step 5 (or scan before pruning) and treat aprunableentry as registered; add a row for a registered worktree whose.gitwas removed.
Major
2. [D] CI check Self-test report is red at this head: fail_suite_not_passed ("the full suite must have passed, with at least one passing test and no failures"). The PR comment of 2026-10-28 reports 9 failing tests in the full suite (decisions #4939, effort-local-install #4988, runtime-launcher-parity K2, host-integration #2627), stated to fail on a clean next too. A red required-style gate is not mine to reinterpret; it stays red until the report records a passing suite.
Minor
3. [D] The sweep deletes the directory it runs from (findUnregisteredResidue, no cwd guard). From <repo>/.claude/worktrees/agent-y2 (no .git) git resolves the enclosing checkout, not_main_checkout does not fire, and the candidate containing the cwd is renamed and removed (repro t1.js, agent-y2 and agent-y/sub). Skip with a row when comparablePath(repoRoot) is the candidate or inside it.
4. [D] Two admin-dir outcomes stay silent on the stderr the three callers now keep: default_branch_unresolved (remote without origin/HEAD and a stale .git/worktrees/dead) and unreadable (mode 000) give reaped: 0 and empty stderr (repro t4.js). docs/CLI-TOOLS.md says reaped: 0 never stands for "did not look"; that holds only for the JSON the callers discard. Emit a not-scanned line for both.
5. [D] A candidate renamed away by another sweep during the ls-files/walk window yields a residue_unverified (or residue_unreadable) row and a "left in place" count for a directory that no longer exists (repro t2.js race). Only the rename failure is treated as "taken by another sweep".
6. [D] The mtime guard reads only the candidate directory's own mtime; a file written deep in the tree leaves it unchanged (repro t2.js deep-active: aged top mtime, a/b/c/live.txt just written -> reaped). repositoryInside already visits every entry and could carry the newest mtime.
7. [J] The removal evidence proves "untracked, orphaned, aged", not "harness-created": a user directory .claude/worktrees/agent-notes/NOTES.md plus any branch named agent-notes is reaped (repro t2.js), as is an ignored-by-git one; the stranded .gsd-reap-<16 hex> path skips name and branch entirely. The contents stderr line is the only trace.
8. [J] The walk (RESIDUE_WALK_LIMIT 1,000,000, synchronous) is the real TOCTOU window: after it only the top-level .git is rechecked before the rename, so a nested repository created during the walk is not seen (reasoned from the diff, not reproduced; needs a concurrent writer).
Nit
9. [J] aliased source comment says "nothing is read", but a link to a readable directory is listed before the chain check rejects it (the author noted this on 2026-10-28).
10. [J] GIT_INDEX_FILE: undefined on the ls-files call is redundant with the #5548 scrub (the author asked whether to keep it).
11. [J] Names cut at 64 code points can print identically for long siblings sharing a prefix; contents is read before the walk and can differ from what was deleted; case-insensitive filesystems with a differently-cased .claude fail closed (residue_unreadable), making the feature a silent no-op there; the "not swept" line repeats on every run from a linked worktree.
Laws that apply
Subject: PR #5233 adds a guarded second discovery source with removal, a scan report and kept stderr to worktree.reap-orphans.
- postels-law - fired: the guard that must be strict is evaluated after the sweep mutated the state it reads (
:2805before:2954), andreaped: 0stays lenient where two admin states print nothing (default_branch_unresolved,unreadable). Action: findings 1 and 4. - hyrums-law - fired:
2>/dev/null->>/dev/nullinquick.md,quick-batch.md,executor-isolation-dispatch.mdmakes runtime-build notices and Node warnings observable in every transcript, andscan,contentsand the stderr lines are new output others can parse. Action: none in this review; the transcript change is recorded here as observable output. - kernighans-law - fired:
findUnregisteredResidueand its helpers (~240+ lines of nested guards, a rename-reserve protocol, a ctime rule) make the ordering bug in finding 1 invisible to a reader. Action: findings 1, 5. - leaky-abstractions - fired: Windows ctime-on-rename, junction handling and path-case spelling differ; handled at
identity()/ctimeSafeand pinned by real-rename rows on the three lanes (green). Case-insensitive path comparison is the residual leak (finding 11). Action: none beyond finding 11. - goodharts-law - fired: the Self-test report gate (
fail_suite_not_passed) is the number being steered toward green; the report records a failing suite. Action: finding 2.
Considered and cleared: kerckhoffs-principle (safety does not rest on the.gsd-reap-*name being unguessable), knuths-optimization-principle (the walk limit is a bound, not a speed claim).
Action items: 1, 2, 3, 4, 5, 6, 7.
Verdict
Changes requested: 1 Blocker, 1 Major, 6 Minor, 3 Nit. CI runs are not released or re-run by this review.
|
Addendum to the review at head 6aa0483: two scope findings. (1) CONTRIBUTING.md:201: "Scope matches the approved issue — if your PR does more than what the issue describes, the extra changes will be asked to be removed or moved to a new issue". The Agent Brief did not ask for the change of 2>/dev/null to >/dev/null in quick.md, quick-batch.md and executor-isolation-dispatch.md. Remove it from this PR; the design goes in an ADR, preferably as a sub-issue, first. (2) Maintainer position: the guarded-removal engine (rename-to-.gsd-reap protocol, ctime rule, 1M-entry walk, contents listing) needs an ADR defining the residue-removal guard standard, accepted before this PR merges; no ADR in docs/adr defines it today. |
trek-e review 5691418415: - Blocker 1: a worktree whose `.git` file was deleted, with uncommitted files, was removed once a prune ended its registration — the sweep's own, and (Codex review) `/gsd-progress`'s pruneOrphanedWorktrees, wave cleanup, git gc. Registration cannot protect it, so removal now also needs content proof: the directory matches a harness branch tip with no modified, deleted or untracked-unignored file (`git worktree remove`'s standard without --force), compared in a throwaway index (`residue_has_work`; `residue_unverified` when git cannot compare). The listing is also read before the sweep's prune. - Minor 3: a candidate holding the sweep's repoRoot or the process cwd is kept as `residue_contains_cwd`. - Minor 4: `.git/worktrees` not swept (`default_branch_unresolved`, `unreadable`) is said on stderr. - Minor 5: a candidate another sweep removed mid-check gets no row — only when `.claude/worktrees` is still the directory this sweep verified. - Minor 6: every entry inside a candidate must be past the mtime guard. - Minor 7: a harness id is `a` + lower-case hex (`agent-a78683ac`), so a user's `agent-notes` with a same-named branch is kept. - Nits 9/10: `aliased` comment; the GIT_INDEX_FILE override is dropped (execGit scrubs it, open-gsd#5548). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…of the issue's scope) trek-e addendum 6485710283: the Agent Brief did not ask for the `2>/dev/null` -> `>/dev/null` change in quick.md, quick-batch.md and executor-isolation-dispatch.md. Restored to `next`, with the gsd-quick-batch-quick-regression carve-out and the stderr-passthrough test that pinned it; the changeset no longer describes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s work One holding `*` hid every untracked file, itself included, from the --exclude-standard listing, so the content check read the residue as clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-index cleanup cannot abort the sweep Codex review: git does not look inside a gitlink's directory without the submodule's .git, so a residue whose submodule pointer was lost had its files there removed (reproduced with real git). A failed scratch-index removal threw out of finally and the CLI then reported reaped: 0 for a sweep that had removed directories. Docs name the untracked-.gitignore and gitlink rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review and the addendum. Pushed Blocker 1: fixed by a content check, not by prune ordering. I tried what you suggested first: read the listing before step 5 (still done), then hold the sweep's own prune while git lists an on-disk Removal now also requires that the directory hold no work git lacks. It must match the tip of one of its harness branches with no modified, deleted, or untracked-and-unignored file. That is the standard
A failed check is Major 2 (Self-test): still red, honestly. The full suite ran on the head in a Minors:
Nits:
Finding 8 (walk-to-rename window): not acted on. It is listed in the ADR as an accepted limit for you to rule on. Addendum (1), scope: the three workflow edits are reverted to Addendum (2), ADR: opened #5812 as a chore ADR issue. I could not attach it as a sub-issue of #4941 (no permission); the body names #4941 as its parent. Per CONTRIBUTING I will write Verification:
|
…ing file the harness did not create Maintainer ruling on open-gsd#5812 (ADR item 4): matching a harness branch tip is not enough when an ignored `.env*` or `.secrets` file (or a file in a directory of that name) or a `.planning/` file is in the directory, since git cannot restore it. Such a candidate, a stranded `.gsd-reap-*` one included, is kept as `residue_has_user_data`, unless the file is a byte-identical copy of the main checkout's file at the same path that resolves outside the candidate (what Claude Code's `.worktreeinclude` makes). The files are listed against an empty throwaway index, so the answer does not depend on which branch matched. Other ignored files stay disposable. Also: the stderr escaping row now exercises a real removal (its directory name failed the harness-name check, so nothing was removed or printed). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
#5812 was approved with a ruling on its open question, so I pushed The ruling: an ignored secret or user-data file the harness did not create keeps the directory. The new reason is
Rows:
Each guard's rows go red under a targeted mutation of the compiled module. Also fixed: the stderr escaping row never removed anything. Its directory name failed the harness-name check before the removal path, so the escaped "held" output was never exercised. The hostile name now rides in a file inside a real residue, and the row asserts the removal and the escaped stderr. ADR mapping: every guard in this PR maps to a decision in ADR-5812. Verification:
Noted, not acted on (Codex; pre-existing, not changed by this PR): the registered-worktree path in |
…l guard standard (open-gsd#5833) States when the sweep may delete an unregistered .claude/worktrees directory, as the prerequisite for open-gsd#5233 (fix open-gsd#4941): positive evidence, the no-work-git-lacks content proof, disqualifiers, the maintainer's ruling on ignored secrets and user data, the removal protocol, the reporting contract and the accepted limits. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…leading-space secret is kept execGit trims stdout. residueHoldsUserData compares each name in its -z listing with the main checkout, and git lists ` config/.env` before `config/.env`. Trimmed, the unique file read as the harness copy and the residue was removed with it (Codex review, reproduced with real git). execGit gains an opt-in rawStdout; only this listing uses it, so every other caller is unchanged. The test delegate now runs through the production seam, so fault-injected rows get the throwaway GIT_INDEX_FILE and the same trimming as production (it used to drop env). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…l, not its argument list execGit now passes rawStdout to _spawnResult, so the old end anchor _spawnResult(result, 'git') no longer matched. The anchor still binds the execGit spawn site. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ADR-5812 merged (#5833), so I ran a whole-PR review against it before your re-review. It found one data-loss path in the user-data guard, now fixed. Head is Fixed: leading whitespace in an ignored file name (Major, data loss).
Fixed: the fault-injection delegate dropped
Also:
Validation:
The fix was not given a second review round. Noted, not acted on:
None of these causes a wrong result. |
next gained open-gsd#5832 and open-gsd#5835, which grow agents/gsd-code-reviewer.md with two different ack reasons; measured from this branch's old merge-base they conflict in emitted-attribution. Merging moves the base past them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rees, not just outside the candidate isHarnessCopy accepted a main-checkout source anywhere outside the candidate. One reached through a link into another directory under .claude/worktrees can be removed by the same sweep, so the "copy" would not survive (Codex review). The containment root is now .claude/worktrees, the parent of the verified candidate; ADR-5812 decision 4, docs/CLI-TOOLS.md and CONTEXT.md say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up push, head
|
Fix PR
Linked Issue
Fixes #4941
What was broken
worktree.reap-orphansdiscovered candidates only from.git/worktrees/. The most common Claude Code residue — a.claude/worktrees/agent-*directory whose harness teardown did not complete, so it has no.gitfile, and whose admin entrygit worktree prunehas since deleted — was invisible to it by construction. The sweep answered{"ok": true, "reaped": 0, "entries": []}with the directory,node_modulesand all, still on disk. An absent admin directory and an empty one also reported identically.Reproduced on current
nextwith the issue's script, plus a live harness worktree beside the residue: stillreaped: 0, so the gap is not only the no-admin-dir case — residue is missed whenever its own admin entry is gone.What this fix does
A second, additive discovery source,
findUnregisteredResidue, reads<main checkout>/.claude/worktrees/and removes a child only on positive evidence that it is this repository's harness teardown residue, through a path verified to stay inside the checkout:.git(a directory, or a gitfile'sgitdir:) leads to the git dir git is using, so acore.worktreecannot substitute another work tree's registrations, branches or index (residue_dir: git_mismatchotherwise — nothing is removed); an inheritedGIT_DIR/GIT_WORK_TREEnever reaches git, sinceexecGitdrops it (bug(git): execGit inherits GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE, so cwd can select the wrong repository #5548), so the sweep reads this checkout's own index;.claudeand.claude/worktreesare real directories under the checkout's canonical top, not links (residue_dir: aliasedotherwise — nothing is processed or removed); re-checked right before each removal, and the removed path is built from that verified chain;lstat) with no.gitentry (ENOENT), absent fromgit worktree list(read before the sweep's own prune), holding neither the sweep's root nor the process cwd (residue_contains_cwd), and older than the lock path'sREAP_MTIME_GUARD_MS— and so is every entry inside it;agent-<id>with the id shape Claude Code uses,a+ lower-case hex (agent-a78683ac,agent-a29ecaf07c8ddbe07;residue_not_harness_namedotherwise, so a user'sagent-notesis kept), and a harness branch exists for it —agent-<id>orworktree-agent-<id>(residue_no_harness_branchotherwise);residue_tracked; fails closed), read from the checkout's own index (an inheritedGIT_INDEX_FILEis unset for these calls);git worktree list --porcelain -z), so a path holding a newline is read whole;.git(any case) at any depth and no bare layout; links are not followed,node_modulesis walked too, the walk is bounded (residue_contains_repository/residue_unverified);residue_has_work;residue_unverifiedwhen git cannot compare): it matches the tip of one of its harness branches with no modified, deleted, or untracked file git does not ignore, no untracked.gitignore, and nothing at a submodule (gitlink) path — the standardgit worktree removeapplies without--force, checked byresidueHoldsWorkin a throwaway index (read-tree,update-index --refresh,diff-files,ls-files --others --exclude-standard). Ignored files (node_modules, build output) are disposable by that standard.residue_has_user_data; maintainer ruling on chore(#4941): ADR — residue-removal guard standard for worktree.reap-orphans #5812): an ignored.env*or.secretsfile, a file in a directory of that name, or a file under.planning/(ignored whenplanning.commit_docsisfalse) keeps the directory, a stranded.gsd-reap-*one included. The exception is a file with the same bytes at the same path in the main checkout that resolves outside the directory: that is a copy Claude Code's.worktreeincludemade, and it survives the removal. The files are listed byresidueHoldsUserDataagainst an empty throwaway index, so the answer does not depend on which branch matched.A candidate that fails a check is reported with
status: "skipped"and that reason and left on disk; a child that is not a candidate (a link, one with a.gitentry, one git lists, a young or empty one) gets no row. Removal renames the directory to an unguessable.gsd-reap-*name inside the verified directory, re-verifies it there, and only then runsfs.rmSync(path, { recursive: true }), which unlinks a nested symlink or junction rather than following it. A removed directory is{status: "reaped", reason: "unregistered_residue", contents}and counts inreaped, wherecontentslists the names it held at the top level (the checks prove harness residue, not that nothing in it was wanted). A rename that fails isresidue_remove_failed(nothing was deleted); a removal that throws isresidue_remove_incomplete, since part of the directory may already be gone; when the rename back fails too, the row names the.gsd-reap-*path that still holds the files. The.gsd-reap-<16 hex>name is reserved for the sweep: a directory left under it (the process died between the rename and the removal) is reclaimed on a later sweep without a harness name or branch, still subject to every other guard, and only once its ctime (which the sweep's rename sets, unlike its mtime) is past the five-minute guard, so a concurrent sweep never takes another's removal in flight. Rows come back in name order. Only a main checkout sweeps; from a linked worktree the scan reportsnot_main_checkout, and stderr says so.Why removal is safe enough now
The maintainer ruled that guarded removal is required (review of 2026-10-10), and that the guard standard needs an ADR before this merges (6485710283). That ADR is ADR-5812 in #5833 (Closes #5812). The maintainer approved #5812 with a ruling on its open question, and this PR implements the ruling (
residue_has_user_data). Every guard here maps to a decision in that ADR. Review 5691418415's Blocker showed that registration cannot protect work: anygit worktree pruneends it (this sweep's,/gsd-progress'spruneOrphanedWorktrees, wave cleanup, git gc). The content check above is what protects a worktree that lost its.gitfile with work in it. Every data-loss path found against a deleting draft has a guard and a row whose would-be victim survives: a user's own directory (notagent--named, no harness branch), committed content (the tracked check, case-insensitive and literal), another repository inside (the repository walk), a symlinked or swapped.claude/worktrees(the verified real-directory chain), and a worktree being created (the mtime guard, and the empty-directory skip:git worktree addrefuses a non-empty target — measured). Twelve of these came from three Codex whole-PR passes, which reproduced each one in a sandbox before the fix; the last four (a removal-time parent swap,.GITcase aliasing, a newline in the checkout path, an NFC/NFD rename) have not had a fourth pass.What stderr says
The three workflow callers are unchanged (
2>/dev/null, as onnext). The CLI's stderr is written for a reader that a directory name must not be able to address:"agent-x" (held "a", "b")) — the record of what was deleted. Every name goes throughio.formatDiagnosticToken, which escapes control, invisible and bidi characters and U+2028/U+2029 as\uXXXX; names are cut to 64 code points, and at most five are shown per list (… and N more);failed: …) is escaped too;.claude/worktreesthat was not swept is said (not_main_checkout,unreadable,aliased,top_unresolved,git_mismatch), and so is a.git/worktreesnot swept (default_branch_unresolved,unreadable); outside a git repository nothing is said;N orphan(s) skippedline counts only the admin-dir path's rows.scanThe CLI output gains
scan, which names what was actually read, soreaped: 0is distinguishable from "did not look":admin_dir:scanned/absent/unreadable/git_dir_unresolved/default_branch_unresolvedresidue_dir:scanned/absent/unreadable/aliased(a.claudeor.claude/worktreeslink, dangling or not) /not_main_checkout/top_unresolved/git_mismatch/not_scannedWhether the cwd is in the main checkout, and where that checkout's top is, are asked of git (
--git-common-dir,--show-cdup) rather than read off the git dir's location, which--separate-git-dircan put anywhere under any name. When the reaper throws,scanisnull. The residue scan runs where the admin-dir scan used to return early for a missing.git/worktrees/, and after the admin loop; the existing lock / PID / ancestry path and its fail-closed bail-outs are unchanged. Both are typed unions and documented indocs/CLI-TOOLS.mdand CONTEXT.md.The issue's second symptom
Wave cleanup ran
git -C <residue> rev-parse --abbrev-ref HEAD; with no.gitthere, git walked up and answered the enclosing repository's branch, and the entry blocked asbranch_mismatchwith an empty stderr. An entry whose directory exists with no.gitentry now blocks asworktree_unregistered, with a stderr saying why. The #4415 absent-directory path is unchanged.The
rollbackPartialWorktreedoc comment, which called the reaper "a genuine directory-scan backstop", now says what it covers and what it does not.Root cause
reapOrphanWorktreesderived its whole candidate set from git's admin directory, and the residue class is defined by its admin entry being gone.if (!entries) return results;then collapsed "nothing there" and "could not look" into the same all-clear.Testing
How I verified the fix
The issue's reproduction (no admin dir; and again with a live harness worktree beside the residue) against a build of this branch: the residue is removed, the live worktree is untouched,
reaped: 1,scansays both locations were read, and stderr names the removedagent-t1.Every new row is fail-first against the previous head
7ba53cf77(report-only), compiled module and all. Two Codex whole-PR rounds this update, each with sandboxed reproductions: round 1 found five deletion paths against the first removal cut (a swapped or aliased.claude/worktrees, bare and nested repositories, a case-only rename, a backslash name) and a relative-path bug in wave cleanup; round 2 found three more (a whitespace-ending checkout top, a repository undernode_modules, a late parent swap). Every one has a row whose would-be victim survives; each fails on the commit before its fix. Round 2's fixes were not given a third Codex pass. A residual window remains between resolving the target and unlinking it, usable only by a process that can already write the checkout.npm run lint:ci— exit 0tests/worktree-safety*.test.cjs+tests/prune-orphaned-worktrees.test.cjs582/582 (worktree-safety-reap139/139) — greenFull suite on
4ab24c656: 46,394 tests, 46,291 pass, 3 fail, none in this PR's code:runtime-launcher-parityK2 andhost-integration(orchestrator-worktree argv shape) fail identically on a cleannextcheckout on this Mac;verification-statusfeat(completion): thread convention through the completion-path membership seam (epic #612) #4142 fails only from this machine's~/.gsd/defaults.jsonstderr warning (passes with HOME sandboxed).lint:ciexits 0. The branch includes a merge ofnextat7a4b76f68.Review 5563200267: a reserved
.gsd-reap-<16 hex>name changed inside the guard window, by its ctime, is left for a later sweep, so a concurrent sweep never takes another's removal in flight (rows at guardMs-1/guardMs/guardMs+1 by ctime, unreadable ctime, a real rename, and anagent-<id>residue that a fresh ctime does not hold back); rows for a young reserved name and for the path reported when a rename that did not land cannot be renamed back. Two Codex whole-PR rounds: round 1 showed an earlier cut ("gone after a failed removal means reaped") could reportreapedwhile another sweep still held the files, so it was replaced by the ctime guard; round 2 found nothing blocking. Rename-advances-ctime: see the next item.Review 5593733152: a real-rename row proves the platform's rename advances the directory's ctime against the filesystem's own clock (a probe write stamped past the ctime, then the rename), then sweeps with an injected
nowMsat which the creation ctime would be exactly guardMs old: left alone by the real ctime. It is in the platform conformance tier and passed on every CI lane, includingwindows-latest(NTFS) and macOS (APFS), on6a80e6415. A rename to the reserved name that fails because a concurrent sweep took the candidate gets no row; one that fails with the candidate in place, or after.claude/worktreesitself moved, is stillresidue_remove_failed. A fast-check property covers the reserved-name boundary. Two Codex whole-PR rounds: round 1 found the moved-parent case (fixed, red without the fix); round 2's remaining minor is in the reply.Review 5608091528: the parent's
dev/inois recorded when the candidate is verified, and a failed rename counts as "another sweep took it" only while.claude/worktreesis still that same directory. Row: the parent is moved aside and a fresh directory is created in its place →residue_remove_failed(red without the identity check). The stranded-name property cleans up in afinally. Codex whole-PR round 1 found a.claudethe sweep cannot traverse scanned asabsent(existsSyncis false on EACCES); only ENOENT isabsentnow, with a mode-000 row (red before the fix). Round 2: no findings.Withdrawn review 5622668874, Nit 1: the admin-dir side still used
existsSync(false on EACCES/ENOTDIR) to choose betweenabsentandunreadable. Both sides now shareisAbsent(only ENOENT is absent). Row: a git dir under a mode-000 parent scans asadmin_dir: "unreadable"(red on838b1e1dd). Codex whole-PR round: no blocker/major; one minor noted, not acted on (a symlinked.claudewhose target has noworktreesscansabsent, a dangling.claude/worktreeslink scansunreadable, rather thanaliased; diagnostics only, nothing is deleted or hidden).Review 5638711345: Minor 1, a listing failure under a linked
.claudeor.claude/worktrees(dangling, or leading to noworktrees) isaliased, three rows with the stderr line. Minor 2, a removal that throws isresidue_remove_incompletewith its own stderr line; rows for an injected throw and for a real removal stopped by a no-write subdirectory. Minor 3, a removed row carriescontentsand stderr names them (untracked-work row, cap and escaping row). Nit 5,scan: nulldocumented. Nit 6, onelstatStatebacks every lstat/ENOENT test. All nine new or changed rows fail on2a7c72746and pass on the head.lint:ciexit 0; full suite onb1e2df63746,670 pass, 3 fail (the same three environment-only failures as before); Codex whole-PR round: no findings.Review 5691418415 + 6485710283 (this update, head
291380c63): Blocker 1 — a registered worktree with its.gitfile deleted and an uncommitted file is kept on every sweep, including after an externalgit worktree prune(residue_has_work); reproduced with real git before and after. Holding the sweep's own prune was tried first and dropped: Codex showed/gsd-progressand wave cleanup prune too. Rows for an edited, a deleted and an untracked file, an untracked self-ignoring.gitignore, content at a gitlink path, an ignored file (still removed), work matching the other harness-branch spelling (still removed), and git unable to compare (residue_unverified). Minor 3 cwd rows (the candidate, a directory inside it, the process cwd); Minor 4 stderr rows; Minor 5 a candidate removed mid-check gets no row, and only while.claude/worktreesis the directory the sweep verified; Minor 6 a young entry deep in the tree and an unreadable entry mtime; Minor 7agent-notes,agent-a123456,agent-A0000001,agent-a0000001x,agent-b0000001with matching branches are kept. Every new guard row fails on6aa0483f1(compiled) and passes on the head. Fixture names changed to the harness id shape; fixtures that must be removable now ignore their extra files in.git/info/exclude. Scope: the three workflow edits and their regression-test carve-out are reverted.lint:ciexit 0;worktree-safety-reap167/167,worktree-safety439/439. Two Codex whole-PR rounds: round 1's Blocker (other prune callers) led to the content check; round 2 found the gitlink blind spot (reproduced with a real submodule, fixed in291380c63) and an unguarded scratch-index cleanup (fixed).291380c63itself has not had a Codex pass.chore(#4941): ADR — residue-removal guard standard for worktree.reap-orphans #5812 approval and ruling (head
9ac2a6567): the ruling's rows are an ignored.env, a nested.env.local,.secrets, a file inside an ignored.env/directory, and an ignored planning file, each kept asresidue_has_user_datawith its bytes intact. Also a same-length.envthat differs from the main checkout's (kept), a byte-identical copy (removed, main copy intact), a "copy" reached through a junction into the residue (kept), a stranded.gsd-reap-*holding an.env(kept), and git unable to list ignored files (residue_unverified). Separate mutations of the compiled module turn each guard's rows red: the verdict off, stranded skipped, byte compare always true, the.env*/**pathspec dropped, the outside-the-residue check off. The stderr escaping row now exercises a real removal; its old fixture name failed the harness-name check, so nothing had been removed or printed. One Codex whole-PR round, run on both this PR and docs(#5812): ADR — residue-removal guard standard for worktree.reap-orphans #5833, found the junction alias and the stranded bypass, both fixed here. The fixes were not re-reviewed by Codex.lint:ciexit 0 on9ac2a6567;worktree-safety-reap177/177. The full suite ran on4c3cc0949, which differs from the head only by a behavior-identical swap toisContainedIn(the lint asks for it) and a CONTEXT.md wording change.After ADR-5812 merged (this update, head
36374d06f):execGittrims stdout.residueHoldsUserDatareads each name from a-zlisting, and git listsconfig/.envbeforeconfig/.env. Trimmed, a unique ignored secret at a path starting with a space read as the harness copy ofconfig/.env, and the residue was removed. Codex reproduced this with real git, and so did the new row.execGitgains an opt-inrawStdout. Only this listing uses it, so every other caller is unchanged.residue_has_user_data, and one row in the seam tests pins exact versus trimmed output.env, so fault-injected rows compared against the fixture's own index rather than the throwaway one.windowsHidesource anchor forexecGitnow ends at the call, not its argument list.CONTEXT.mdnow cites ADR-5812 by name.rawStdoutfrom the compiled module turns the new reap row red, and the seam row goes red without it.worktree-safety-reappasses 178/178.lint:ciexit 0 and the full suite (48,732 tests, 48,628 pass, 2 fail: the same two environment-only failures) on the head.02c8a58ac):next: CI's emitted-attribution measured from the old merge-base, which made enhance(#4100): add a necessity dimension to the code-review rubric #5832 and enhance(#4101): add a CI/CD pipeline reliability rubric to code-review #5835's two different ack reasons foragents/gsd-code-reviewer.mdcollide. Neither file is in this PR..claude/worktreescould be removed by the same sweep. The containment root is now.claude/worktrees, not the candidate. ADR-5812 decision 4,docs/CLI-TOOLS.mdandCONTEXT.mdsay so, and a new row goes red with the old root.worktree-safety-reappasses 179/179.lint:ciexit 0 and the full suite on the head: 48,956 tests, 48,852 pass, 2 fail (the same two environment-only failures, K2 fix(launcher): a globally-installed gsd_run on PATH shadows a local install's gsd-tools, so the git.allow_default_branch_commits override is not honored #4834 and the feat(#2584): Phase 3 — scheduler consumer + isolation adapters (proven on Codex) #2627 orchestrator-worktree argv shape).Regression test added?
tests/worktree-safety-reap.test.cjs's#4941 regressionblock checks the disk in every row: removal for eligible residue (including beside a live worktree, under--separate-git-dir, with an unresolvable default branch, and for either harness-branch spelling); survival for each guard (notagent-*, no harness branch, tracked — including a case-only rename and a backslash name —, a bare or nested repository, failedls-files, failed listing, failed removal, young, empty, age unknown, still registered, non-ENOENTlstat, symlinked child, a linked.claude/worktreesinside or outside the checkout, a swap to an outside link mid-sweep and after the final checks, a repository undernode_modules, a checkout whose path ends in a space beside a same-named sibling, linked worktree); the mtime boundary atguardMs-1/guardMs/guardMs+1; and a junction inside a residue whose target survives. The CLI rows cover the removed-name line, the kept-by-reason line (an instruction-shaped name is never printed), every escaped character class (plus a fast-check property over arbitrary names), the five-name cap (4/5/6) and the 64-code-point cut (63/64/65), the linked-worktree and unreadable not-swept lines, silence outside a repository, and the escaped failure message. Wave cleanup of a residue entry blocks asworktree_unregistered, for absolute and repoRoot-relative manifest paths. Existing rows changed: the reaper-throws row expects the escaped message; the admin-dir-probe row now counts the residue scan's own read; the#2596SUMMARY-parity and#4758relative-path rows' stand-in worktrees get the.gitfile a real worktree has.Platforms tested
node:24-bookwormcontainer)The new junction rows make
worktree-safety-reap.test.cjsa conformance-tier file — both generated tier lists are regenerated here — so the junction handling runs on real Windows in CI, which I could not do locally.Runtimes tested
Self-test report
The full suite is reported as it ran on macOS: two failures, both environment-only on this machine and outside this PR's code (K2 #4834;
#2627orchestrator-worktree argv shape), each previously confirmed to fail on a cleannextcheckout here. CI on the queue commit is authoritative.{ "schema": 1, "head_sha": "02c8a58acc92f9fc1c9c043488eef1d0a77edb36", "tool": "npm-test", "failing_first": { "test": "tests/worktree-safety-reap.test.cjs", "fails_on_base": true, "passes_on_head": true }, "commands": [ "node --test tests/worktree-safety-reap.test.cjs", "npm run lint:ci", "npm test" ], "full_suite": { "command": "npm test", "result": "failed", "passed": 48852, "failed": 2 }, "environment": { "os": "macOS (Darwin 27.0.0, arm64)", "node": "v24.19.0" }, "output_excerpt": "tests 48956, pass 48852, fail 2 on 02c8a58ac (the head, which includes a merge of next). The 2 failures are environment-only on this machine: runtime-launcher-parity K2 (#4834) and the #2627 dispatch-isolation orchestrator-worktree argv shape. lint:ci exits 0; tests/worktree-safety-reap.test.cjs 179/179." }Checklist
Fixes #NNN— PR will be auto-closed if missingconfirmed-buglabelnpm test), except two environment-only failures that fail the same way on a cleannexton this Mac (see the Self-test report).changeset/fragment added if this is a user-facing fix (npm run changeset -- --type Fixed --pr <NNN> --body "...") — orno-changeloglabel appliedBreaking changes
worktree.reap-orphansnow deletes.claude/worktrees/agent-*directories that pass every guard above, and its output gains ascanfield and the residue rows; existing fields are unchanged. The three workflow callers are unchanged. Wave cleanup reportsworktree_unregisteredwhere it reportedbranch_mismatchfor a directory with no.gitentry.parseWorktreeEntrieskeeps leading/trailing whitespace in a listed path (it used to trim it), stripping only a CR.🤖 Generated with Claude Code