Reclaim the agent git worktrees stranded inside live workspace clones - #442
Reclaim the agent git worktrees stranded inside live workspace clones#442blooop wants to merge 8 commits into
Conversation
git worktree list is the authority for locked and prunable; the join to a directory on disk is by its place inside the clone, because the registered path is a container path that resolves to nothing on a host. The dirty check goes through the clone's admin directory for the worktree, which is the only side that resolves here, and excludes .claude/worktrees so a worktree holding a nested one does not read dirty forever. Reachability asks the sibling bare cache first: the clone is never fetched into, so its remote-tracking refs are as of clone time and asking them alone reports pushed-and-merged branches as unpushed.
The sweep goes into the plan the user already answers, so there is one dry run and not two. Its bytes are counted only for clones the run is keeping, because a clone that is going already accounts for everything inside it. --force-worktrees is its own flag. --force already means 'past a clone holding work nowhere else' and people type it; letting it reach a locked or dirty worktree would widen it into permission to remove one somebody may be working in. The acting pass removes the directory and only then runs git worktree prune, so an interrupted run leaves the prunable state the next run already handles. It re-classifies every directory immediately before removing it: a container running git worktree add is not a participant in devlaunch's repo lock.
The figure was invisible on the host that filled up, and it was 82% of the cache. A part of the clone's number rather than an addition: the worktrees are inside it. The object store is not in the attribution, because a linked worktree shares the clone's and the clone's objects are hardlinked from the bare next door.
The README names --force-worktrees because every flag dl offers has to appear there; the rules and the two things the report is careful about live in docs/cleanup.md, which is where depth goes. Only the tripwire file moved. prune_plan takes one Insisted instead of a bare Insistence, and SizeCell::Measured carries the clone's disk with its worktree share attributed; the promised api tier is byte-identical.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThis PR extends Sequence diagram for race-resistant worktree pruningsequenceDiagram
participant User
participant Prune as dl --prune
participant Git
participant FS as Host filesystem
participant Container
User->>Prune: Confirm prune plan
Prune->>Git: worktree list --porcelain
Git-->>Prune: Registration state
Prune->>FS: Scan .claude/worktrees recursively
Prune->>Git: status and commit reachability checks
Prune-->>User: Report removable and kept worktrees
Container->>Git: Re-register worktree
Prune->>Git: worktree list --porcelain again
Git-->>Prune: Updated registration state
alt Still removable
Prune->>FS: Remove worktree directory
Prune->>Git: worktree prune
else Registration or objection changed
Prune-->>User: Withhold worktree
end
Flow diagram for safe agent worktree reclamationflowchart TD
A["dl --prune"] --> B["Classify clone"]
B -->|clone being removed| C["Remove clone"]
B -->|clone being kept| D["Recursively scan .claude/worktrees"]
D --> E["Confirm linked Git worktree"]
E -->|plain directory or symlink| F["Leave untouched"]
E -->|linked worktree| G["Read Git registration"]
G --> H{"Forgotten or prunable?"}
H -->|no: held| F
H -->|yes| I["Check lock, dirt, and commit reachability"]
I --> J{"Objection and no --force-worktrees?"}
J -->|yes| F
J -->|no| K["Reclassify immediately before removal"]
K --> L{"Still removable?"}
L -->|no| F
L -->|yes| M["Remove directory"]
M --> N["Run git worktree prune when safe"]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed at b812411f8593cfc70dbdb907881dfccaabab90bb against merge-base 449cae2f. Preflight: all 14 checks green, diff non-empty. Spec taken from #426's body and the sharpening comment (id 5409723748), which is the governing one.
Two axes below, run independently and not merged.
Spec
Checked against the sharpened spec, in the code. Verified: reachability asks the sibling .bare first with the clone as fallback and no fetch anywhere (agent_worktrees.rs:337, git.rs:445), and the "as of the last fetch" sentence is printed once per sweep including under -y. Removal order is directory-then-prune (:1041-1053). The per-candidate re-check exists, iterates only clone.removing, so the approved set can shrink and never grow. Discovery recurses, confirms via the .git gitfile tail rather than the path shape (linked_worktree_name:255), and skips symlinks; the removal itself reuses the symlink-refusing remove_tree_as_far_as_it_goes. Sizes are an attribution and never an addition, and exclusive_usage bills a file only when every link to it is inside the tree, so the hardlinked object store is out. Nothing derives a path from anything but clone, so a scratch XDG_CACHE_HOME run stays harmless. Dirty, untracked, and detached-and-ahead are all protected and tested. --force-worktrees is refused outside --prune.
S1. The git worktree prune holdback reads the plan, not the re-check, and uncommitted work is destroyed on the next run. (blocker)
The guard is the right guard. It is asked of the wrong data. reclaim gates on clone.metadata_may_be_pruned() (agent_worktrees.rs:1049), and that method inspects only self.keeping from the plan (:672). A candidate the second pass demotes to Keep goes into report.withheld (:1032) and is never consulted.
Reproduced against rust/target/release/dl on lifecycle_scenario.py --agent-worktrees, with the container's write landing while the [y/N] question was on screen, which is the window the module itself calls "not a rare race":
Are you sure? [y/N] Removed 0 clone director(ies) -- 8.0 KiB.
Removed 1 agent worktree(s) -- 8.0 KiB.
Left .../.claude/worktrees/agent-unsaved: holds 1 uncommitted change(s) (notes.md)
-- add --force-worktrees to remove it anyway. That was not so when the plan above was printed.
The re-check did its job. But no "Did not run git worktree prune" line followed, because the plan's keeping was empty, so the prune ran and took agent-unsaved's registration with it. .git/worktrees/ is then gone entirely. The very next run:
- removing .../.claude/worktrees/agent-unsaved (12.0 KiB): git has already forgotten it
Removed 1 agent worktree(s) -- 12.0 KiB.
notes.md is gone. No flag was typed at either run. Forgotten is the one arm that carries no probe at all (:296-306) and an Unopposed promotion, which is what turns a demotion into a deletion.
Preconditions are ordinary rather than exotic: two prunable worktrees in one clone, one of which the container touches while the question is open. On a host every container-registered worktree reads prunable, so dirt is the main protection there, and this is the path that discards it.
The gate needs to see what this pass withheld, not only what the plan kept.
S2. The clone-removal figures now include bytes from clones that are being kept. (major)
PrunePlan::freed() and PruneReport::freed() chain the worktree total in (lifecycle.rs:2073, :2386), but render still spends them on clone-directory sentences (render.rs:1386, :1611). Observed on --agent-worktrees --prunable:
Removing 1 that nothing references -- 128.0 KiB:
- .../devlaunch-gone-nobody (120.0 KiB)
and, on a run that removed no clone at all, Removed 0 clone director(ies) -- 8.0 KiB. followed by Removed 1 agent worktree(s) -- 8.0 KiB. The same bytes are stated twice and the first statement attributes them to directories nothing references. Scaled to the reference host this is 104 GB folded into the headline number somebody says yes to.
S3. --prune never reaches "Nothing to prune" again once a registration lingers. (minor)
Correct behaviour, reported as work. After a held-back prune, the removed worktree's registration stays, so registrations_with_nothing_here is 1 forever, so nothing_to_do() is false forever. Verified: runs 2 and 3 print the full section, ask [y/N], and do nothing. Worth making a bytes-free registration count not by itself constitute something to do.
S4. The dirty-check pathspec masks tracked work, and non-worktree content under .claude/worktrees/ is neither seen nor protected. (minor)
The exclusion is justified and I confirmed the motivating case: without it, a worktree holding a nested one reads ?? .claude/ forever; with it the nested worktree's own dirt is still seen when it is the candidate. Two residues. A tracked file modified under .claude/worktrees/ is silent ( M .claude/worktrees/keep.md without the exclusion, nothing with it) — contrived, since the harness's directory is normally ignored, but it is real uncommitted work. More reachable: a plain directory sitting under a worktree's .claude/worktrees/ is excluded from the dirty check and skipped by the sweep, since it is not a linked worktree, so it neither blocks nor is reported, and it goes when the parent worktree does. The doc comment's justification ("those nested directories are what this sweep reasons about separately") is true only of confirmed worktrees.
S5. The container-path vs deleted distinction the spec asks for is not made. (minor)
"Distinguish directory absent because it was a container path that does not resolve here from directory absent because it was deleted."
Both collapse into registrations_with_nothing_here (:648). Substantively fine, since the suffix join is what finds the bytes behind a container path, and the doc comment argues the case. Flagged because the ask is literally unmet.
S6. Nits
linked_worktree_nameaccepts a gitfile tail of.git/worktrees/.., which makesadminthe clone's own.git. Fails safe (reads dirty, kept), but it is trust placed in file content.docs/cleanup.mdis worth one sentence saying that reclaiming an orphan clone containing agent worktrees takes two runs: run 1 keeps it (the clone-level probe has no pathspec exclusion, so the worktrees read as uncommitted work) and sweeps the worktrees, run 2 reclaims the clone with no flag.
On the changed test premise
No existing test was altered: git diff shows zero deletions in rust/dl/tests/lifecycle.rs. The Insistence::Insisted sits in a new test, a_worktree_inside_a_clone_that_is_going_is_not_swept_separately, whose comment states why. The premise is accurate and the call is right: removing the clone really would destroy what those worktrees hold, so the clone-level probe is honest to count them, and the situation converges over two runs. Nothing is buried. Excluding .claude/worktrees/ at clone level too would have been the defect.
Standards
Repo standards: test_docs_prose.py, test_readme_cli_doc.py, test_public_api_snapshots_doc.py and test_bench_doc.py all pass (62 tests). --force-worktrees is named in the README. docs/cleanup.md pre-dates the PR and is already in the Docs table. cargo clippy --locked --all-targets -- -D warnings and cargo fmt --check are clean.
Snapshot discipline holds. git diff -- '*api.txt' is empty, so the frozen tier is byte-identical (#251 §7). public-api.rest.txt is +200/-2 and the two removed rows are exactly the claimed pair: prune_plan taking Insisted in place of a bare Insistence, and SizeCell::Measured carrying CloneDisk. No other removals, no reordering, and the additions sit in generator order. Both moves are intended consequences of this change.
N1. The README carries rationale the repo says belongs in docs/. (minor)
CLAUDE.md: "A new paragraph of design rationale belongs in the docs page for its topic, not in the README." README.md:235-238 argues why it is not --force, and :338-343 restates the 82% / 104 GB measurement. Both are near-verbatim in docs/cleanup.md. Naming the flag is guard-required; the argument is not.
N2. Dead and over-wide public surface. (minor, Speculative Generality)
CloneWorktrees::freed() (agent_worktrees.rs:653) has no caller anywhere: core, dl, unit tests, or dl/tests/. Four more rows would be covered by pub(crate): agent_worktrees::bytes_in (sole caller listing.rs:827, same crate), WorktreeSweep::removing()/keeping() (in-crate tests only; dl uses the CloneWorktrees versions), and Insisted::nothing(). CloneDisk::measured is the one that genuinely needs pub and says so.
N3. Nits
- Shotgun Surgery with no guard:
WORKTREES_DIR(agent_worktrees.rs:91) and the literal":!.claude/worktrees"pathspec (git.rs:420), whose own comment says "The two have to move together". The repo's habit elsewhere is to pin such a pair with a test (flows::provision::lending_contract); nothing binds these. commands.rsextractsinsistence(bool), but the identical inline ternary still stands atcommands.rs:527-531.canonical(&clones.repo_manager().bare_dir(o, r).to_string_lossy())now appears twice (lifecycle.rs:2196,:2513).- README says "104 GB";
docs/cleanup.mdand the module header say "104.5 GB".
The read_side.rs flake
Nothing in this diff can plausibly cause it. The file is untouched, and the only read-path surface it changes is the disk object, whose new worktrees key appears solely when a clone has a .claude/worktrees/ — which scenario.py never builds, and which would fail that byte-for-byte JSON pin deterministically rather than one run in eight. The read path added here is a read_dir and a walk, both deterministic.
The plausible mechanism is load, not logic: the diff adds five binary-boundary tests that each build a python world and run real git worktree add plus git push, and #401 documents exactly that shape making the timing-sensitive binary tests flake on a loaded cargo test --workspace while passing alone. Same family as lock_wait.rs. Worth a line on #401 rather than a change here.
Verdict
Request changes.
Blocking:
- S1 — the prune holdback is computed from the plan's
keepingrather than from what the acting pass withheld, so a candidate the re-check correctly refuses is demoted toForgottenand deleted, with its uncommitted work, by the next run. Reproduced end to end with no flag typed. This is the hazard the guard was written for, reached by the race the module documents.
Should fix before merge, not strictly blocking:
- S2 — worktree bytes are folded into the clone-directory totals, so the plan's headline figure describes directories that are not going.
Everything else above is minor or a nit. The design work here is genuinely good: the four-arm classification, the Insisted pair instead of two booleans, the bare-first reachability, the ordering, and the honesty about what a lock does and does not mean are all right, and the test suite reaches most of the hard cases. S1 is one line of data flow away from the guarantee the rest of the module already earns.
The review on #442 reproduced a data loss with no flag typed at either run. `reclaim` asked `clone.metadata_may_be_pruned()`, which folds the *plan's* `keeping` and nothing else. A candidate the acting pass re-classified and withheld -- a worktree written into while the `[y/N]` question was on screen, which is the window this module already documents as not rare -- landed in `report.withheld`, which the gate never saw. So no holdback fired, `git worktree prune` ran, and it took the withheld worktree's registration with it. Next run the directory read `Forgotten`, the one arm with no probe, and went outright. Of the two directions the review offered, the first is what decided it: **the holdback has to be computed from what the acting pass did, not from what the plan predicted.** A prediction and an outcome that can disagree is the same shape as the plan-wide `force` boolean `PrunePlan`'s doc comment records as having caused a bug here already, and fixing the one call site would have left the shape. So the answer is a value that gets folded, `MetadataGate`, seeded from the plan's keeps and fed every outcome the acting pass reaches; `CloneWorktrees::metadata_gate` is the plan's fold of the same rule and reads as the forecast it is. There is one rule and two folds of it, where there were two rules that could disagree. The second direction is taken as far as it goes, because fixing only the gate leaves the blind deletion one bug away from returning. `Forgotten` now carries an `Unsaved` and is probed wherever there is an admin directory to probe through: reaching that arm with one present means git dropped the name or this module's suffix join missed, and no wrong classification should cost somebody an afternoon. With the admin directory gone there is genuinely no index and no HEAD, so nothing can be asked, and that stays the limit rather than becoming a claim. The `..` tail in a gitfile went the same way: without a guard it did not fail safe as the review supposed, it removed the directory as forgotten. The rest of the review, in one pass because it is all the same code: - **S2.** Worktree bytes were chained into `PrunePlan::freed`/`PruneReport::freed` and then spent on clone sentences: 128.0 KiB claimed over a 120.0 KiB directory, and `Removed 0 clone director(ies) -- 8.0 KiB.` over a run that removed no clone. Both are now `clones_freed`, the worktree section keeps its own figure, and a boundary test holds the headline to the rows under it. - **S3.** A registration with nothing behind it no longer constitutes work by itself. It frees nothing, so a run clears it and the run after has nothing to say -- where before `--prune` asked the question and no-opped forever. - **S4.** The `:!.claude/worktrees` pathspec excluded the place rather than the thing, hiding a tracked file modified there and, worse, plain content under a candidate's `.claude/worktrees/` that the sweep also skips: neither reported nor protected, and gone with its parent. git is now asked without a pathspec and untracked entries are dropped only where everything under them is a confirmed worktree. The motivating case -- a worktree holding a nested one reading dirty forever -- is still covered, by its own test. - **S5.** `registrations_with_nothing_here` tells a container path that never resolved here apart from a path in this clone with nothing at it, which is what the sharpened spec asked for and one number said neither of. - **N1.** The README's two paragraphs of rationale go to `docs/cleanup.md`, which already carried both nearly verbatim. The flag stays named. - **N2.** `CloneWorktrees::freed` had no caller and is gone; `bytes_in` is `pub(crate)`; `WorktreeSweep::removing`/`keeping` and `Insisted::nothing` are `#[cfg(test)]`, which is what they were for. `public-api.api.txt` is byte-identical. `public-api.rest.txt` removes ten rows -- `CloneWorktrees::{freed, metadata_may_be_pruned}`, the old `usize` signature of `registrations_with_nothing_here`, `PrunePlan::freed`, `PruneReport::freed`, `bytes_in`, `WorktreeSweep::{removing, keeping}`, and `Insisted::nothing` with its impl header -- and adds `MetadataGate`, `RegistrationsWithNothingHere` and the two `clones_freed` renames. Left alone: `dl/tests/read_side.rs`, which the review established this diff cannot flake, and #401 documents the load shape that does. Closes #426
Fixed at S1 (blocker) — fixed, and it was realReproduced as a failing test before anything was changed: Direction 1 decided it. The bug is not one wrong call site, it is that a prediction and an outcome could disagree and only one of them was reachable from the gate — the same shape Direction 2 is taken as far as it can go, because you are right that fixing only the gate leaves the second run's blind deletion one bug away. The second run is pinned too, in the same test: with the registration intact the worktree reads prunable-and-dirty rather than forgotten, and One correction to S6 while I was there. It does not fail safe. A gitfile tail of S2 (major) — fixed
Minors
Checks
Snapshot.
Added:
|
Closing the loop on S1 with your own reproduction rather than only the unit test, since your report ended at "reproduced end to end". Built Run 1 — the holdback fires, which is the line that was missing: Note the first line as well: Run 2 — the run that took the work. It no longer sees a forgotten directory, because the registration is still there: Three of the fixes show up in those seven lines: the holdback answering to the acting pass (S1), the clone sentence carrying clone bytes (S2), and run 2 reaching And the plan for the same world, for S2's other half — the headline is a total over the row under it again, where it read |
# Conflicts: # rust/devlaunch-core/public-api.rest.txt
One thing that was not a review finding but was blocking the review from being re-run: the branch had gone conflicting with
Re-ran after the merge: One thing I deliberately did not do: add a |
The plan's clone total and its worktree total are two claims about two sets of directories, which is the point of separating them; docs/cleanup.md described the --ls attribution and not this.
The forgotten arm now has a probe wherever one is possible, and `worktree_status` enumerates the four cases instead of catching two of them in a wildcard. The last one to gain a probe is a registration whose admin directory a concurrent prune took between the listing and the look: no index and no HEAD, so nothing can be asked of the working tree, but the registration still names a head and the commits can be asked about. Narrow, and the arm it lands on is the arm that deletes, which is the whole reason to bother. No public surface moves: both new functions are private.
One more push,
The third row is new since my earlier comment. It is a narrow window, since The only refactor in it is pulling the reachability half of
Net effect on the finding: the arm the review identified as "the one arm with no probe at all" now has a probe in every case where a probe is possible, and the one remaining case is documented as a limit with the reason it is one, rather than being a wildcard nobody reads. |
All checks green at Summary of the four commits on top of the review's
Both blocking findings are fixed with a failing test written first and verified red, and S1 is additionally reproduced end to end through the binary with your own |
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Second review, fresh context. Reviewed at 9b6180b4 against merge-base 2aa9602b. Spec taken from #426's body and the sharpening comment (id 5409723748). Preflight: 15 check runs green at the full head SHA, diff non-empty. I wrote neither the code nor the first review and took neither account on trust: every claim below is either reproduced or refuted here.
Locally: cargo test --workspace green, cargo clippy --locked --all-targets -- -D warnings clean, cargo fmt --check clean, pytest test/ 413 passed.
Spec
The blocker is closed, by reproduction
Ran the first review's own recipe: lifecycle_scenario.py --agent-worktrees --prunable, the fixture note removed so both worktrees read collectable at plan time, then the write into agent-unsaved landing while Are you sure? [y/N] was actually on screen, answered y. Two consecutive runs against target/release/dl at this SHA.
Run 1 withholds and, this time, says so:
Removed 1 clone director(ies) -- 120.0 KiB.
Removed 1 agent worktree(s) -- 8.0 KiB.
Left .../.claude/worktrees/agent-unsaved: holds 1 uncommitted change(s) (notes.md)
-- add --force-worktrees to remove it anyway. That was not so when the plan above was printed.
Did not run git worktree prune in .../devlaunch-main-legacy: a worktree there is being kept,
and the registration is what goes on protecting it.
git worktree list still names agent-unsaved after run 1. Run 2 reaches Nothing to prune., and notes.md reads back an afternoon nobody else has. No flag typed at either run. S1 is fixed.
The "verified red first" claim holds. I reverted one line in a scratch checkout -- the gate = gate.and_keeping(&because) fold in reclaim -- and a_worktree_that_went_dirty_while_the_question_was_open_keeps_its_registration fails at exactly the quoted assertion, with the same message:
assertion `left == right` failed: the prune has to answer to what this pass withheld,
not to what the plan predicted
left: []
right: [".../repos/o/r/r-live-aa"]
The seed is sound. MetadataGate::default() is open and and_keeping only ever ORs true in, so the fold is monotone: neither pass can reopen a gate the other closed, and the plan's seed can only be more conservative than the outcomes, never less. A plan keep the act would have removed costs a held-back prune, which is the harmless direction.
worktree_status is genuinely exhaustive. Four tuple arms over (Option<&Registration>, Option<&Path>), no catch-all -- a fifth situation would stop the build. And each arm's probe is the right one, not merely present: (Some, Some) asks dirt and reachability; (Some, None) asks the commits alone, which is all a vanished admin directory leaves askable; (None, Some) asks dirt, which is correct because a branch ref lives in the shared repository and survives the directory; (None, None) is #426's category 1 verbatim.
The S6 gitfile guard is complete, not just complete for the tail it was found with. I drove linked_worktree_name over eight shapes:
/workspaces/x/.git/worktrees/. -> None
/workspaces/x/.git/worktrees/.. -> None
/workspaces/x/.git/worktrees/../name -> None
/workspaces/x/.git/worktrees/name/../other -> None
/workspaces/x/.git//worktrees//name -> Some("name")
/workspaces/x/.git/worktrees/name/ -> Some("name")
/workspaces/x/.git/worktrees/./name -> Some("name")
.git/worktrees/name -> Some("name")
The four rejections are right and the four acceptances are right: Path::components folds ., doubled separators and a trailing separator away before the tail is read, so those are the same name and not a bypass. A tail naming a different worktree's admin directory in the same clone also fails safe -- the borrowed index reports the whole directory as changes, so it lands in keeping as Objected, which I confirmed. A symlinked admin directory either resolves to the right repository or makes git refuse, and a refusal is CouldNotTell, which objects.
Who was right about S6: the fixer
The first review said the .git/worktrees/.. tail "fails safe (reads dirty, kept)". It does not, and the first review's own S1 paragraph says why without noticing: at b812411 the Forgotten arm carried no probe at all. let (Some(registration), Some(admin)) = (registered, admin) else { return Forgotten { usage } } -- so an unregistered directory with a .. tail (which is the natural shape for a hand-written or clobbered gitfile) took the else, landed on the deleting arm, and went. The "reads dirty, kept" analysis holds only for the sub-case where the directory is also genuinely registered, and that is not the sub-case the tail produces. The fixer is right, its failing run was real, and 9b6180b's guard closes it.
Confirmed closed
S2 -- verified in the binary's own output: Removing 1 that nothing references -- 120.0 KiB over a single 120.0 KiB row, Agent git worktrees inside the clones above -- 16.0 KiB as its own figure, and Removed 1 clone director(ies) -- 120.0 KiB. / Removed 1 agent worktree(s) -- 8.0 KiB. as two sentences about two things. S3 -- run 2 reaches Nothing to prune. rather than prompting and no-opping. S4 -- both residues; the pathspec is gone from Git::worktree_dirt and dirt_in drops an untracked entry only when everything under it is a confirmed worktree, with the motivating nested-in-kept case still green. S5 -- the container-path sentence prints. N1, N2 -- closed (see Standards).
T1. A worktree nested inside one that is going is destroyed, unreported, with its work. (blocker)
#426, Ask 2: "
lockedworktrees are never removed implicitly. Require an explicit opt-in flag, and say how many were skipped and why."
#426, Ask 3: "Refuse -- or require the same explicit flag -- for any worktree whose branch has commits not reachable from the default branch or any remote ref."
sweep_clone descends into a worktree only when it is keeping it (agent_worktrees.rs:1145), and dirt_in drops an untracked entry when everything under it is a confirmed linked worktree. Put together, a nested worktree inside a directory that is going is invisible to both halves: it does not object through its parent's dirty check, and it is never classified on its own. It is removed with the parent, unreported, with no flag typed. Three probes, all against this SHA, in the module's own test harness (outer worktree finished, pushed and clean; container paths, so both read prunable, which is what every worktree on a host reads):
nested holds an uncommitted note -> removing: [agent-outer] keeping: []
nested holds an unpushed commit -> removing: [agent-outer]
nested is `git worktree lock`ed -> removing: [agent-outer]
The third line is Ask 2 broken outright: a locked worktree removed implicitly, not reported, not counted, no flag. The first two are Ask 3 and the sharpened spec's "Uncommitted work" paragraph. This is not exotic on the host in the ticket: nesting is how one clone reached 55 GB, on a host every container-registered worktree reads prunable, so an outer worktree's removability turns entirely on whether its own working tree is clean and its own commits are pushed. A finished, pushed outer task with a live nested session inside it is the ordinary shape, and the parent's byte figure includes the child's, so it presents as the biggest win in the plan.
I am raising this against the fix rather than against the body because the fix is what rests its case on it. docs/cleanup.md:283-286 now argues the exclusion is safe because nested worktrees "are what this sweep reasons about separately", and dirt_in's doc comment says the dropped set "is exactly the set this sweep reasons about separately". For a nested worktree inside a going one, neither sentence is true. S4 in the first review flagged the sibling case -- non-worktree content under a candidate's .claude/worktrees/ that "goes when the parent worktree does" -- and the fix closed the smaller half of it while the half holding the .pixi gigabytes and the live sessions stayed open. a_worktree_inside_one_that_is_going_is_not_reported_twice pins the behaviour with a deliberately clean inner worktree, so the suite does not see it.
Shape of a fix, since the naive one is wrong: descend into a worktree that is going as well, classify children first, and make a parent removable only if every nested worktree under it is. Reporting stays parent-only, so nothing is counted twice; what changes is that the parent's decision becomes conjunctive over its children.
T2. git worktree prune's blast radius is wider than the gate's domain. (major)
The gate is fed from two places and only two: the plan's fold over clone.keeping, and reclaim's fold over the outcomes for clone.removing. Both range over directories the plan enumerated. git worktree prune ranges over the whole clone. A worktree the container registers after the plan is taken is in neither set, so nothing closes the gate, the prune drops its registration and its admin directory, and the next run meets (None, None) -- the arm with no probe -- and removes it outright.
Reproduced at the reclaim seam at this SHA. Plan taken with one collectable worktree; the container then runs git worktree add and writes a note, as it would while the question is on screen:
run 1 report: removed [agent-finished], withheld [], metadata_held_back []
git worktree list after run 1: (agent-fresh is gone)
run 2 removing: [agent-fresh] seen_as: Forgotten
fresh dir still there: false
note still there: false
Same loss as S1, same "no flag typed at either run", reached through the neighbouring door. The module's guarantee that "the approved set can shrink and never grow" is true of the removals and does not cover the prune, and MetadataGate's "answers to outcomes rather than to predictions" is true only over the enumerated candidates. I do not think this is unclosable from here: reclaim already re-reads ClonePicture, so before pruning it can ask whether every registration whose directory is still present in the clone is accounted for by the removed, withheld or plan-kept sets, and close the gate when one is not. Nested registrations inside a removed worktree do not block that test, because their directories are gone by then.
T3. A refused removal is the one acting-pass outcome fed to neither the removal nor the gate. (minor)
agent_worktrees.rs:1352: Removal::WhatItCould | Removal::Nothing pushes to report.refused and folds nothing. The directory is still there and its registration is still there, and if a sibling removal in the same clone succeeded, or the clone has a dangling registration, the prune runs and takes it. Live loss needs work to arrive in a directory the filesystem would not let dl delete, so this is narrower than T2 -- but reclaim's doc and 913df8c's message both say the gate is fed every outcome, and this is the one that is not.
T4. The (None, Some(admin)) arm could ask the commits too. (nit)
dirt_only is right for the branch case, because the branch ref outlives the directory. It is not right for a detached head: the commit is reachable only from the admin directory's HEAD and reflog, which is exactly what a later prune removes. The admin directory is there and holds a HEAD, so it can be asked. Contrived to reach -- it needs a registration whose gitdir no longer names a .claude/worktrees/ path -- and it is the same arm 9b6180b has just been through.
Not defects
The a_worktree_inside_a_clone_that_is_going_is_not_swept_separately premise is right and the two-run convergence is honest, as the first review found. --prune stays inside repos_dir; nothing here derives a path from anything but clone. Sizes are an attribution and exclusive_usage bills a file only when every link to it is inside the tree, so the hardlinked object store stays out.
Standards
Repo standards are met. cargo clippy --locked --all-targets -- -D warnings and cargo fmt --check clean; pytest test/ 413 passed, including test_docs_prose, test_readme_cli_doc, test_public_api_snapshots_doc and test_bench_doc. No em or en dash in README.md or any page under docs/.
N1 closed. Both rationale paragraphs are out of README.md; --force-worktrees is still named at README.md:235, so test_readme_cli_doc is satisfied, and the argument now lives at docs/cleanup.md:283-288, 333-360.
N2 closed, and the new surface is tight. CloneWorktrees::freed is gone. bytes_in is pub(crate) with one caller; WorktreeSweep::removing/keeping and Insisted::nothing are #[cfg(test)] pub(crate), which is the better call than plain pub(crate) since plain would have been dead in a shipping build. Every symbol 913df8c/9b6180b add has a non-test caller: metadata_gate/MetadataGate::open and RegistrationsWithNothingHere's accessors all land in render.rs, and all seven new private functions are called. The WORKTREES_DIR / ":!.claude/worktrees" pair the first review flagged as unguarded Shotgun Surgery is genuinely gone rather than moved: the pathspec exists nowhere in Rust except a doc comment in git.rs:535 explaining its removal, and WORKTREES_DIR is now the sole spelling, used four times inside the module that owns it.
The snapshot resolution lost nothing. I checked this as a three-way rather than a two-way, because "took main's copy and regenerated" is the shape that drops rows silently. Main's delta across the merge window (449cae2..2aa9602) is 26 removals and 62 additions; the merge (913df8c -> 7596524) applied exactly those 26 and 62, and the sets are identical. All 62 of main's added rows are present at HEAD; none of its 26 removed rows came back. The four rows removed relative to current origin/main are all branch-side and all deliberate: SizeCell::Measured now carries CloneDisk, PrunePlan::freed and PruneReport::freed are renamed clones_freed (S2), and prune_plan takes Insisted in place of a bare Insistence. Nothing unexplained. Five further rows that differ are main-only additions from PR #433, which landed after this branch's merge and are not ancestors of HEAD -- not losses. I also ran CI's own generator (scripts/public-api-snapshots.sh) in the container: all three snapshots byte-identical to what is checked in. git diff origin/main HEAD -- '*api.txt' is empty, so the promised tier is untouched, as claimed.
One presentational note: the fix comment's table lists ten removed rows, which is the count against the pre-merge branch tip and not against main. Against main it is four. The claim is not wrong, but a reader checking it will not get ten.
The CHANGELOG omission is right for this repo. Established from evidence, not assumption: of the last 25 first-parent merges to main, two touched CHANGELOG.md, and none of the eight wayfinder/devlaunch-* PRs did. No test, hook, CI job, CONTRIBUTING or CLAUDE.md rule requires an entry; grep -il changelog test/ .github/ docs/ CLAUDE.md pyproject.toml hits only publish.yml and one archival document. publish.yml:72-85 extracts ## [$version] at release time and fails only there, and b878d42 ("Release 0.14.0") is where [Unreleased] gets curated. So the file is maintained per release, not per PR, and declining to invent release notes for somebody else's feature in a review-fix pass was the correct call. Worth passing on rather than acting on: history holds two retroactive-backfill commits (3982989, 4e200b7) whose messages regret a miss, and a whole new module plus a new flag is an ### Added somebody will want at 0.15.0.
N4. An unreachable arm pointing the deleting way. (minor)
agent_worktrees.rs:475 and :524: Losses::of([commits]) over a one-element array can never be None, since NonEmpty::of returns None only for an empty iterator -- so None => Unsaved::NothingToLose is dead in both. In a module whose stated discipline is that every uncertainty fails towards keeping, the dead arm to leave lying around is not the one that reads as a licence to delete. NonEmpty::one (workspace_state.rs:270) already exists and says the intent.
N5. Three copies of one collapse. (nit, Duplicated Code)
unsaved_in (:402), unsaved_without_an_admin_dir (:465) and dirt_only (:520) are the same Result<Option<Loss>, Unsaved> -> Unsaved fold three times over, differing only in which probes they ask. One helper taking the probes as an iterator would leave one rule where there are now three that have to agree.
N6. The skipped N3 nits
Still present, as stated: the duplicated ternary at commands.rs:527-531, the repeated canonical(bare_dir(..)) in lifecycle.rs, and docs/cleanup.md saying 104.5 GB at :252 and 104 GB at :373 (the README paragraph that disagreed is gone with N1, but the docs page now disagrees with itself). Fine to leave; the call to leave them was reasonable.
Verdict
Request changes. (Posted as a comment: GitHub refuses --request-changes on a PR authored by the same account.)
- Standards: pass. Every claimed Standards fix is genuinely closed, the snapshot resolution lost nothing and the generator agrees byte-for-byte, and the CHANGELOG omission is correct for this repo's actual convention. N4 to N6 are minors and nits.
- Spec: fail. T1 blocks.
Blocking:
- T1 -- a worktree nested inside one that is going is removed with it, unreported and with no flag typed, taking uncommitted work, unpushed commits, or a lock. #426 Ask 2 says a locked worktree is never removed implicitly; this removes one and does not even print it. Reproduced three ways. The fix's own justification, in
dirt_in's doc comment and atdocs/cleanup.md:283-286, is the sentence that is false here.
Should fix before merge, not strictly blocking:
- T2 -- the metadata prune reaches registrations neither pass enumerated, so a worktree the container registers after the plan loses its registration and is deleted outright by the next run. Reproduced end to end. Same loss as S1 through the neighbouring door, and closable with the picture
reclaimalready re-reads. - T3 -- a refused removal feeds neither the removal nor the gate, which is one hop from T2 and contradicts the stated invariant.
Everything the first review raised is closed, and closed properly: the blocker with a test verified red at the quoted assertion and reproduced through the binary, the minors each with their own pin, and the one place the first review was wrong -- S6 -- corrected with evidence rather than argued around. The MetadataGate fold is the right shape and the four-arm worktree_status is a real improvement over the wildcard. What T1 and T2 have in common is the boundary of the enumeration: both passes reason about the directories the sweep listed, and both the parent-child relation and the metadata prune reach past that list. That is one idea to fix, not two.
|
Superseded by #525, and closed unmerged. Not because the work was wrong. Three review rounds produced three correct fixes and the same What #525 carries over from here, unchanged in substance:
What was replaced, and why:
T3 needs no fix in the new shape: a refused or partial removal forgets nothing, because The measurements on #426 and its sharpened spec stay the record everything is checked against, |
The unit is a site and everything nested inside it, decided bottom-up and conjunctively, so a worktree nested inside one being removed cannot be lost with it: the collectable arm of a verdict is reachable only when every nested site handed one back, and the caller passes no child list. That is T1 from PR #442's second review, made unrepresentable rather than guarded. What decides a site is a verdict rather than a boolean: Collectable(Proof) | Stands(NonEmpty<Reason>), where the proof is a private-field witness only a probe that answered can mint, and reasons accumulate up the subtree, so a site that is both dirty and locked reports both and a parent's line names the child. A lock is an unproved, never a loss. The metadata operation is `git worktree remove <the path git printed>`, per registration by name, and the clone-wide `git worktree prune` is deleted rather than gated: its domain is a readdir at act time, so a registration created after the plan was printed was inside its blast radius and no plan could name it. `Recorded`'s only constructor parses a listing, so a name the pass did not read cannot be an argument. That is T2. Ownership is a join against this clone's own listing, never the gitfile tail: a live worktree of another repository, nested in one of ours, used to be offered for removal unopposed under a reason that was false. The clone is the root of the same forest, which closes the clone-level guard's blindness to nested worktrees. `Unsaved` survives as the wire flattening.
The unit is a site and everything nested inside it, decided bottom-up and conjunctively, so a worktree nested inside one being removed cannot be lost with it: the collectable arm of a verdict is reachable only when every nested site handed one back, and the caller passes no child list. That is T1 from PR #442's second review, made unrepresentable rather than guarded. What decides a site is a verdict rather than a boolean: Collectable(Proof) | Stands(NonEmpty<Reason>), where the proof is a private-field witness only a probe that answered can mint, and reasons accumulate up the subtree, so a site that is both dirty and locked reports both and a parent's line names the child. A lock is an unproved, never a loss. The metadata operation is `git worktree remove <the path git printed>`, per registration by name, and the clone-wide `git worktree prune` is deleted rather than gated: its domain is a readdir at act time, so a registration created after the plan was printed was inside its blast radius and no plan could name it. `Recorded`'s only constructor parses a listing, so a name the pass did not read cannot be an argument. That is T2. Ownership is a join against this clone's own listing, never the gitfile tail: a live worktree of another repository, nested in one of ours, used to be offered for removal unopposed under a reason that was false. The clone is the root of the same forest, which closes the clone-level guard's blindness to nested worktrees. `Unsaved` survives as the wire flattening.
The unit is a site and everything nested inside it, decided bottom-up and conjunctively, so a worktree nested inside one being removed cannot be lost with it: the collectable arm of a verdict is reachable only when every nested site handed one back, and the caller passes no child list. That is T1 from PR #442's second review, made unrepresentable rather than guarded. What decides a site is a verdict rather than a boolean: Collectable(Proof) | Stands(NonEmpty<Reason>), where the proof is a private-field witness only a probe that answered can mint, and reasons accumulate up the subtree, so a site that is both dirty and locked reports both and a parent's line names the child. A lock is an unproved, never a loss. The metadata operation is `git worktree remove <the path git printed>`, per registration by name, and the clone-wide `git worktree prune` is deleted rather than gated: its domain is a readdir at act time, so a registration created after the plan was printed was inside its blast radius and no plan could name it. `Recorded`'s only constructor parses a listing, so a name the pass did not read cannot be an argument. That is T2. Ownership is a join against this clone's own listing, never the gitfile tail: a live worktree of another repository, nested in one of ours, used to be offered for removal unopposed under a reason that was false. The clone is the root of the same forest, which closes the clone-level guard's blindness to nested worktrees. `Unsaved` survives as the wire flattening.
The unit is a site and everything nested inside it, decided bottom-up and conjunctively, so a worktree nested inside one being removed cannot be lost with it: the collectable arm of a verdict is reachable only when every nested site handed one back, and the caller passes no child list. That is T1 from PR #442's second review, made unrepresentable rather than guarded. What decides a site is a verdict rather than a boolean: Collectable(Proof) | Stands(NonEmpty<Reason>), where the proof is a private-field witness only a probe that answered can mint, and reasons accumulate up the subtree, so a site that is both dirty and locked reports both and a parent's line names the child. A lock is an unproved, never a loss. The metadata operation is `git worktree remove <the path git printed>`, per registration by name, and the clone-wide `git worktree prune` is deleted rather than gated: its domain is a readdir at act time, so a registration created after the plan was printed was inside its blast radius and no plan could name it. Ownership is a join against this clone's own listing, never the gitfile tail: a live worktree of another repository, nested in one of ours, used to be offered for removal unopposed under a reason that was false. The clone is the root of the same forest, which closes the clone-level guard's blindness to nested worktrees, and it takes #522's `BareCache` and passes it to the probe underneath rather than deciding the tag question for itself. A site's own reachability probe gives no tag account: it names one revision and asks the clone about it, where the tag question is about which of a clone's refs the mirror does not have, so `by_tags: None` is the honest answer. `Unsaved` survives as the wire flattening.
The unit is a site and everything nested inside it, decided bottom-up and conjunctively, so a worktree nested inside one being removed cannot be lost with it: the collectable arm of a verdict is reachable only when every nested site handed one back, and the caller passes no child list. That is T1 from PR #442's second review, made unrepresentable rather than guarded. What decides a site is a verdict rather than a boolean: Collectable(Proof) | Stands(NonEmpty<Reason>), where the proof is a private-field witness only a probe that answered can mint, and reasons accumulate up the subtree, so a site that is both dirty and locked reports both and a parent's line names the child. A lock is an unproved, never a loss. The metadata operation is `git worktree remove <the path git printed>`, per registration by name, and the clone-wide `git worktree prune` is deleted rather than gated: its domain is a readdir at act time, so a registration created after the plan was printed was inside its blast radius and no plan could name it. Ownership is a join against this clone's own listing, never the gitfile tail: a live worktree of another repository, nested in one of ours, used to be offered for removal unopposed under a reason that was false. The clone is the root of the same forest, which closes the clone-level guard's blindness to nested worktrees, and it takes #522's `BareCache` and passes it to the probe underneath rather than deciding the tag question for itself. A site's own reachability probe gives no tag account: it names one revision and asks the clone about it, where the tag question is about which of a clone's refs the mirror does not have, so `by_tags: None` is the honest answer. `Unsaved` survives as the wire flattening.
1 participant
Closes #426
An agent harness working inside a devcontainer makes its own git worktrees under
<clone>/.claude/worktrees/<name>/, one per task, and nothing ever collected them. On the hostin #426 that was 72 directories, 104.5 GB, about 82% of everything under
repos/, with one cloneholding 55 GB on its own. Every one of them sat inside a clone belonging to a live devpod
workspace, so
--prune's orphan rule not only missed them, it must never fire there: firingwould delete a live workspace's checkout.
So this is a second rule, and it runs only over the clones the first one is keeping. A clone that
is going already accounts for everything inside it, which is what keeps the bytes from being
counted twice.
What it does
--prunesweeps.claude/worktrees/inside the clones it keeps, classifying each directoryfour ways: git has forgotten it, git says the registration can go, git is holding it locked, or
git still holds it and does not offer it up. The first two go by default. The last is always
kept, which is what stops a run inside a container from collecting its own live worktrees.
--force-worktreesis the one flag that carries a worktree past a lock, past uncommitted work,or past commits nothing else reaches. Deliberately not
--force: that word already means "pasta clone holding work nowhere else" and people type it, so widening it would turn it into
permission to remove a worktree somebody may be working in. There is a test for exactly that.
path.
-ystill skips the question.git worktree pruneruns in the clone after the directory goes, so a run interruptedbetween the two leaves a registration whose directory is gone, which is precisely the prunable
state the next run handles. It is held back in a clone where a worktree is kept for what it
holds, because the prune is all-or-nothing and would drop that worktree's registration too,
turning it into a forgotten directory the next run removes outright.
dl --ls --sizenames how much of a clone's figure is worktrees, in the table and indisk.worktreesin the JSON. It was invisible there on the host that filled up.The things that took the work
of unstaged edits reads as finished if you only ask about commits. The dirty check goes through
--git-dir=<clone>/.git/worktrees/<name>with--work-tree=<directory>, because theworktree's own
.gitgitfile names a container path that resolves to nothing on a host, and.claude/worktrees/is excluded from it so a worktree holding a nested one does not read dirtyforever.
.bareand then has itsremote repointed at the forge with no fetch of its own, so its
refs/remotes/origin/*is as ofclone time. Asking it alone reports pushed-and-merged branches as unpushed, which would keep
every byte forever. The bare is asked first, and the report says the answer is as of the last
fetch. No network call was added.
that is how one clone reached 55 GB. The scan recurses, but only into worktrees it is keeping.
.gitgitfile naming a.git/worktrees/<name>admin directory, rather than by the path shape. A plain directory sitting there is not
devlaunch's to delete, and symlinks are stepped over so a removal cannot walk out of the cache.
git worktree addis not a participant in devlaunch's repository lock. Every directory is classified again
immediately before it goes, so the approved set can shrink between the report and the act and
can never grow. There is a test that re-registers a worktree between the two.
leaves one behind, so every line of the report says the fact it rests on and no more.
Not in scope
The 18 duplicated
.pixi/envs/defaultcopies are why the figure is 104 GB rather than about 10,and they cannot be pointed at the shared cache: only the pixi download cache is shared, because
installed environments bake absolute paths. Removing the worktree is how those bytes come back,
which is what this does. Recorded in
docs/cleanup.mdso nobody chases it.Checks
cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --checkand the Python doc guards are green locally. Thepublic-api.rest.txtsnapshot isregenerated: two rows moved,
prune_plantaking oneInsistedinstead of a bareInsistence,and
SizeCell::Measuredcarrying the clone's disk with its worktree share attributed. Thepromised
api.txttier is byte-identical.🤖 Generated with Claude Code
Summary by Sourcery
Reclaim finished agent Git worktrees from live workspace clones while retaining protected work and reporting their disk usage.
New Features:
--pruneto reclaim collectable agent Git worktrees nested inside clones that remain in use.--force-worktreesoption for removing worktrees protected by locks, local changes, or unreachable commits.dl --ls --sizetable and JSON output.Bug Fixes:
Enhancements:
Documentation:
Tests: