2e14b4df17
* fix(#4415): treat an absent worktree as removed, not as a branch mismatch Claude Code removes a subagent's worktree the moment the subagent finishes with a clean tree. A gsd-executor that committed everything — SUMMARY.md included, under `commit_docs: true` — is exactly that case, so by the time the orchestrator reaches wave cleanup the directory is routinely gone while the branch it left behind is intact and mergeable. `git -C <gone> rev-parse --abbrev-ref HEAD` fails, and nothing distinguished that filesystem failure from a real branch disagreement: both reached the same `if`, so the entry blocked `branch_mismatch`, NOTHING merged, and the branch was left dangling. When the directory instead vanished after the merge landed, `git worktree remove` failed "is not a working tree" and the entry blocked `worktree_remove_failed`, leaving the branch undeleted and the operator to run `git worktree prune` + `git branch -D` + `rm -rf` by hand every wave. Disambiguated at the point of failure rather than ahead of it. A SUCCESSFUL in-worktree read still decides identity exactly as before — a present worktree on the wrong branch blocks, unchanged — and only a FAILED read consults the filesystem. Two reads can fail, and they are not the same path: * The branch read fails with the directory absent. There is no checkout for identity to come from, so it falls back to `refs/heads/<branch>` read from repoRoot; a missing ref still blocks, so an absent worktree never becomes a silent pass. The SUMMARY rescue and the dirty check are then skipped. * The branch read succeeded and the later `status` read fails with the directory now absent — the harness removed it while the repoRoot-side base, deletion and scope checks ran. Identity was already established from the checkout and the rescue has already run; only the dirty decision is skipped. Without this, a mid-entry removal still blocked `worktree_dirty` with nothing merged: the same bug, one window later. Skipping those reads is not a claim that the worktree was clean. This code cannot tell who removed the directory, and a forced or manual `rm -rf` of a DIRTY worktree would already have destroyed an uncommitted SUMMARY before cleanup ran. The narrow thing that is true either way is that a missing source cannot be read. The two reads also fail differently: the default SUMMARY finder catches the unreadable directory and returns no files, while `git -C <gone> status` errors — and that error is what surfaced as `worktree_dirty`. A rescue that genuinely FAILS still blocks, since a copy that errored part-way can mean an uncommitted SUMMARY was really lost. Teardown prunes the stale .git/worktrees admin entry rather than removing a path that is not there, re-reading presence instead of reusing the branch-step answer since the harness can act in between. For an entry accepted as ABSENT it prunes ONLY and never issues `worktree remove --force`: that entry was merged without the rescue and dirty checks, so force-removing a checkout recreated at that path would delete contents that never passed either one — strictly worse than the bug being fixed. A genuine prune failure still reports `worktree_remove_failed`, and a blocked teardown still withholds the branch delete. `git worktree prune` is repository-wide maintenance, not an entry-scoped operation. The presence probe resolves `worktree_path` against repoRoot, the way git does. `normalizeCleanupManifestEntry` takes the path from the manifest verbatim, so it can be relative, and every git call passes it as `-C <path>` with `cwd: plan.repoRoot`; a bare `fs.existsSync` would have resolved it against the PROCESS working directory instead. Those differ whenever cleanup runs from elsewhere, reachable today through gsd-tools' `--cwd` override, and the mismatch reads both ways: a present checkout reported absent — skipping the dirty check that would have blocked it — or an absent one reported present. An earlier cut resolved presence UP FRONT, before the branch read. That broke 52 existing tests: every cleanup-wave test uses a fake path that does not exist on disk and injects no `existsSync`, so all of them re-routed down the absent branch. Disambiguating at the point of failure leaves those tests reading as they did. Three rows still needed their premise stated — each stubs a git failure against a worktree that is genuinely present — and now inject `existsSync: () => true`. No assertion in any of the three changed. Fourteen rows added. Every early row held presence CONSTANT and so could not reach the windows that matter, since the bug is caused by a directory that changes state WHILE cleanup runs: removal after the branch read, a present worktree whose status fails (which must still block), removal between the clean status read and teardown, a reappeared checkout at teardown, #2852 isolation of a blocked absent entry from the entries after it, and relative-path resolution. Verified: ran the issue's own reproduction verbatim against a build of this branch — `merged_removed`, merge commit present, branch deleted, no prunable entry in `git worktree list`. The same reproduction against a build at the merge-base returns blocked/branch_mismatch, no merge, branch present, `wt1 ... prunable`. Five of the first eight rows go red against the true merge-base file; the three that stay green are the safety-preservation rows. The rows added after each review round go red against the commit that round reviewed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRaNKCDUEacHudVDwvat8X * chore(#4415): add changeset Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRaNKCDUEacHudVDwvat8X * fix(#4415): build the probe-path expectation with path.resolve, not path.join The row asserting that the presence probe resolves a relative `worktree_path` against repoRoot failed on windows-latest while the code under test was correct. On win32 `path.resolve` prepends the current drive to a drive-less absolute path (`/repo/main` -> `D:\repo\main`) and `path.join` does not, so a join-built expectation disagrees with correct behavior: expected: '\repo\main\.claude\worktrees\agent-a1' actual: 'D:\repo\main\.claude\worktrees\agent-a1' `path.resolve` is what the fix must use — it is how git resolves `-C <path>` against `cwd: plan.repoRoot` — so the expectation moves to resolve as well. Two `notEqual` rows keep that from being circular: the probe must receive neither the raw relative path nor a process-cwd resolution. Verified by mutation — dropping the repoRoot anchoring in `worktreeExists` turns the row red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRaNKCDUEacHudVDwvat8X * fix(#4415): confirm absence before skipping the rescue and dirty checks `fs.existsSync` answers false for a genuinely missing path AND for one it merely cannot traverse — EACCES on a parent directory, an unreachable mount. Verified: with a parent at mode 000, `existsSync` returns false while `statSync` throws EACCES. That distinction carries weight here, because "absent" is what lets an entry skip the SUMMARY rescue and the dirty check. An unreadable-but-present worktree read as absent, so cleanup merged over uncommitted work that the dirty check exists to refuse — and it contradicted this code's own comment that a present checkout whose git read fails stays blocked. Before this PR a failed git read blocked unconditionally, so treating unreadable as present is not a new safety rule; it is the one that was already there. The default probe becomes `statSync`, which reports WHY it failed. Only ENOENT is absence; anything else reads as present and blocks. An injected probe stays authoritative, so tests state presence directly with no hidden dependency on the real filesystem, and may throw to state that a path is unreadable. Two rows added: an unreadable worktree still blocks as branch_mismatch with no merge and no teardown, and a confirmed-ENOENT probe still takes the absent path. Verified by mutation — reverting the discrimination to the permissive `return false` turns the unreadable row RED while the ENOENT row stays green, which is what distinguishes discrimination from over-blocking. The mutation was confirmed to reach the compiled artifact the test loads. Also from this round: the row named for a checkout that "reappeared" never modeled reappearance (production probes presence once, at identification), so it is renamed to the unconditional contract it does prove; the comment crediting the notEqual rows with removing circularity is narrowed to what they actually establish; and the changeset now says only a confirmed absence takes the new path. Found by Codex full-PR review (round 3) before pushing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL * fix(#4415): source identity from git's registration, removal from the errno Maintainer review rejected the premise this fix rested on. It held that once the worktree directory is gone there is no checkout to read, so identity must fall back to `refs/heads/<branch>`. Git does not lose the binding — measured, after `rm -rf`: worktree /path/to/wt branch refs/heads/feat-x prunable gitdir file points to non-existent location The ref fallback weakened identity from "the checkout registered at this path is on this branch" to "a branch by this name exists", which let a foreign sibling branch merge. Identity now comes from `git worktree list --porcelain`, so the #3677 swap control keeps its teeth on the absent path; the new swap row is what would have caught this, and dropping the branch conjunct turns only that row red. Two defects in the first cut of the porcelain rework, both measured rather than reasoned about: `prunable` is not a removal test. With a parent directory at mode 000, git prints `prunable gitdir file points to non-existent location` for a checkout that is STILL THERE — it cannot traverse the parent, so it reports the gitdir file as missing. Treating prunable as "removed" would skip the rescue and dirty checks and merge over uncommitted work in an unreadable worktree, reintroducing the review's Major finding by another route. Each source now answers only what it can prove: porcelain for identity, `statSync`'s errno for removal. Only ENOENT is removal; EACCES/EIO blocks, as it did before this PR. `git worktree prune` is repository-wide. Measured: two removed worktrees plus ONE prune leaves neither registration behind. Reading the list per entry therefore let the first absent entry's teardown erase the identity evidence of every entry after it, merging one worktree per wave and blocking the rest as branch_mismatch — worse than the bug being fixed, since a wave of parallel executors is the normal case. The identity read is now a snapshot, captured lazily on the first entry that needs it and reused for the wave, which is both pre-prune and off the happy path. The `existsSync` probe and its dep locals are deleted; the filesystem is consulted only for the errno. The comment calling repository-wide prune "Harmless" was wrong under the new identity rule and says so now. Tests: identity and removal are stated on their own axes rather than through one present/absent boolean. Added the absent-path #3677 swap row, the two-absent-entry prune row, a bare `prunable` marker row, and a fail-safe row for an unreadable worktree list. Three mutations each kill exactly the intended rows, verified against the compiled artifact the tests load. One fixture that still stated presence through the removed `existsSync` seam was passing for the wrong reason and now states both axes. Verified: lint:ci exit 0; full suite 24/24 chunks, 37,164 tests, 0 failures; tests/worktree-safety.test.cjs 422/422. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL * fix(#4415): re-confirm absence before teardown, and prove the porcelain claim against real git Maintainer review, Major. Presence was classified once, at identification, and everything between that point and teardown — the base, deletion and scope gates, and the merge itself — is a window in which a worktree can reappear. The defence was "prune only, and a live checkout would make `branch -D` fail visibly", which holds only while prune's own staleness check is not fooled by the same filesystem-visibility gap that produced the false absence one call earlier. If it is, prune clears the admin entry, `branch -D` then SUCCEEDS, and a live, unreviewed, un-rescued worktree loses its branch. That asymmetry is the argument for the fix: the bug this PR set out to repair only ever BLOCKED, while this path could DESTROY state. Absence is now re-confirmed with `confirmedGone()` immediately before teardown — no new subprocess, just the statSync already in hand — and a reappeared directory blocks as `worktree_remove_failed` instead of reaching prune or the branch delete. The review was also right that the gap was known and unverified: the existing row said so in its own comment ("it does NOT model the reappearance transition itself"). It is modelled now, by a stat that answers "gone" at identification and "present" at teardown. Mutation-verified: removing the re-confirmation turns ONLY the new row red while the old "prune, never force-remove" row stays green, which is exactly why that row could not have caught this. Minor, same review: the #4415 block was entirely mock-based, so the factual claim the identity mechanism rests on was asserted in comments and measured out of band but never proved executably. Two real-git rows now prove it — that git keeps the path -> branch binding after the checkout is deleted and marks the entry prunable, and that it ALSO reports prunable for an unreadable worktree that is still there, which is why removal is confirmed by errno rather than by prunable. The second row skips as root, where mode 000 does not deny traversal. Minor 2 (rescueSummaryArtifacts resolving worktree_path against process.cwd() while the new code resolves against plan.repoRoot) is pre-existing and not reachable through the CLI's same-cwd invocation; left for a follow-up issue rather than widened into this PR. Verified: lint:ci exit 0; full suite 27/27 chunks, 37,739 tests, 0 failures, against the true merge-base; tests/worktree-safety.test.cjs 425/425. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL * test(#4415): make the real-git rows platform-correct The Windows conformance shard caught both rows on their first push, and both failures were mine, not the code's. Path separators: git reports porcelain paths with FORWARD slashes on every platform, while `path.join` yields backslashes on win32, so `includes()` compared separator styles rather than paths and the registration assertions failed. Both sides are normalised before comparison now. Premise setup: the unreadable-worktree row establishes "git cannot traverse the parent" with mode 000, which win32 does not honour for directory traversal at all — the row would have asserted `prunable` against a perfectly readable worktree and failed for a reason unrelated to the behaviour under test. It now skips on win32 for the same reason it already skipped as root, with both reasons stated together. Verified: lint:ci exit 0; tests/worktree-safety.test.cjs 425/425 locally. The Windows shard is the real check for the separator fix, since macOS cannot reproduce it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL * fix(#4415): warn when an entry is accepted as absent, giving prunable its consumer Maintainer review round 3, both Medium findings — they close together, as the review noted. The absent path reported `merged_removed`/`ok` indistinguishably from an ordinary merge. This code cannot tell "the harness cleanly removed a finished executor" from "an operator or an external process removed this path": git keeps the path -> branch registration and `statSync` reports ENOENT in both cases. Before this path existed every anomalous absence blocked loudly, so accepting the routine case silently took the operator's only signal away from the case that is not routine. The module already carries an advisory channel for a materially less risky condition — scope conformance, a few lines below — so withholding one here was inconsistent with its own pattern. `WAVE_CLEANUP_WARNING.ACCEPTED_ABSENT_WORKTREE` is now emitted at both acceptance sites, carrying git's own `prunable` reason. Advisory, never a gate: the entry still merges. That also gives `WorktreeEntry.prunable` a consumer. It was parsed, documented as "worth surfacing to an operator", and then never read — the errno rework made it unused for the predicate and the parsing stayed behind. Quoting git's reason here is what it was for. The bare-marker test was vacuous, as the review said: it asserted `merged_removed`, which is driven by `confirmedGone` and the branch match, not by the bare-marker parsing it claimed to cover, so a regression in that parsing would not have reddened it. It now asserts the parsed value reaches the warning. A bare `prunable` line normalises to the literal 'prunable' — a truthiness signal, not a reason — so the warning reports null there rather than quoting a marker back at an operator as though git had said something. `WAVE_CLEANUP_WARNING`'s locked code set is updated deliberately, with the reason recorded in the test: the lock exists so a new advisory code is a decision rather than something that appears because a branch needed one. Verified: mutation — suppressing the warning at both sites turns both new rows red; lint:ci exit 0; full suite 27/27 chunks, 38,245 tests, 0 failures; tests/worktree-safety.test.cjs 426/426. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
624 B
624 B
type, pr
| type | pr |
|---|---|
| Fixed | 4612 |
worktree cleanup-wave no longer blocks an entry whose worktree directory the harness already removed: the branch merges and teardown prunes the stale admin entry instead of failing. Identity still comes from git's own worktree registration, so a path registered to a different branch blocks exactly as before, and removal must be confirmed by an ENOENT — a worktree that merely cannot be read blocks rather than being treated as removed. Every entry in a wave is evaluated against the registration as it stood before any teardown pruned it, so one removed worktree no longer strands the rest.