feat(commands): /disk-cleanup-merged-worktrees — reclaim disk, refuse the worktrees that still hold work - #59
feat(commands): /disk-cleanup-merged-worktrees — reclaim disk, refuse the worktrees that still hold work#59lapc506 wants to merge 2 commits into
Conversation
… the worktrees that still hold work The measurement that motivated it, on one real checkout: 60 worktrees, 20 GB under .claude/worktrees, 38 node_modules, the largest a single 1.6 GB. This tool deletes, so the value is entirely in what it REFUSES. Every refusal is a predicate in scripts/worktree-cleanup.mjs with an evidence string and a test, never a bullet point in prose: the main checkout, a locked worktree, uncommitted changes (untracked files included), unpushed commits, and anything unmerged. A branch that is AHEAD of its remote is not stale, it is unfinished, and that is the single most likely way to lose work. "Merged" is measured three ways because a squash merge leaves the branch neither an ancestor of the base nor patch-equivalent to it. An ancestry test alone therefore reports not-merged for work that certainly landed, and in a squash-merge repo that is the majority case. Losing gh downgrades a verdict to unverifiable, never to not-merged. The base is a SET, not one branch. Measuring only against origin/HEAD produced 41 false not-merged verdicts on the 60-worktree run, because that repo merges features into develop and promotes to main at release time. unverifiable is a third verdict and never collapses into "safe": a sibling process re-checking the worktree out mid-run, commits after the PR merged, a squash merge with no gh, an unreadable git status, a missing gitdir. Dry-run by default. node_modules reclaim is the default action and is fully reversible with one install; worktree removal is opt-in behind --worktrees and deletion behind --apply. --force is never passed unless the user asks for it in that invocation. Branch refs are never deleted, only directories. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![dojo-code-reviewer[bot]](https://avatars.githubusercontent.com/in/3092887?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Changes Requested
Changes requested — 1 blocker, 2 P2, 1 P3. Confidence: 1.00/5.00.
Walkthrough
main branch instead of following the standard GitFlow pathway (feature → develop → main). This warning is informational only and does not impact the code-based verdict.
Review Walkthrough
This PR introduces a robust git worktree cleanup command (/make-no-mistakes:disk-cleanup-merged-worktrees), script, and skill to reclaim disk space from merged, clean, and pushed worktrees, while safely refusing directories with uncommitted changes or unpushed work.
Files Reviewed
We reviewed scripts/worktree-cleanup.mjs (the classifier, measurement, and execution engine), skills/worktree-cleanup/SKILL.md (the doctrine), commands/disk-cleanup-merged-worktrees.md (the tool definition), and src/audit/worktree-cleanup.test.ts (unit and integration tests).
Safety Rationale
Once the timezone-offset date comparison bug is fixed to avoid potential data loss, the multi-layered verification tests and deterministic safety checks make the cleanup operation extremely safe against accidental data loss.
Changes requested — 1 blocker, 2 P2, 1 P3.
🔴 P1 — Blockers
scripts/worktree-cleanup.mjs:364— 🔴 P1 (blocker) — Timezone comparison bug in date string check.tip.stdoutis formatted using local committer date (%cI), which includes a local timezone offset (e.g.2026-07-01T01:00:00-05:00).pr.mergedAtis returned by the GitHub API in UTC (e.g.2026-07-01T03:00:00Z). Doing a direct lexicographical string comparison (tip.stdout > pr.mergedAt) is timezone-incorrect.
For example, if the local timezone is behind UTC (e.g. EST -05:00), a commit made after the merge can have a string representation that is lexicographically smaller than the UTC merge string (e.g. "2026-07-01T01:00:00-05:00" > "2026-07-01T03:00:00Z" evaluates to false even though the local time is 6:00 AM UTC, which is 3 hours after the merge). This would cause tipAfterMerge to be false, meaning the tool will fail to detect unpushed commits made after a merge, classify the worktree as safe for removal, and potentially destroy the user's unpushed work (data loss).
Use Date.parse() to safely perform timezone-aware comparison of the ISO 8601 strings.
[pass 1]
🟡 P2 — Major
scripts/worktree-cleanup.mjs:165— 🟡 P2 (major) — Sibling process re-checkout check alignment in classification. Aligning this with the simplified comparison logic ensures that we correctly report transitions to/from detached HEAD asbranch-changed-under-usand mark them asUNVERIFIABLE. Using a fallback label handlesnullbranches gracefully when printing the message.
[pass 1]
scripts/worktree-cleanup.mjs:313— 🟡 P2 (major) — Sibling process re-checkout check does not detect transitions from a branch to a detached HEAD. The current conditionf.liveBranch !== null && f.recordedBranch !== null && f.liveBranch !== f.recordedBranchrequiresf.liveBranch !== null. If a sibling process checks out a detached HEAD in that worktree in between the two checks,liveBranchwill be set tonull(becauselive.stdout === 'HEAD').
Since liveBranch is null, this check is skipped, and the script does not flag the worktree as branch-changed-under-us. Simplifying this to f.recordedBranch !== f.liveBranch handles all transitions correctly (including branch to detached, detached to branch, and branch to branch) and makes the logic much cleaner.
[pass 1]
🔵 P3 — Minor
src/audit/worktree-cleanup.test.ts:380— 🔵 P3 (minor) — Integration coverage of sibling detached re-checkout. Adding an integration test ensures that transitions to a detached HEAD are correctly identified asbranch-changed-under-usand never inadvertently cleaned up.
[pass 1]
Total findings: 1 security, 2 compliance, 1 business context (4 total)
| * Verdict precedence: any refusal wins; otherwise any ambiguity wins; | ||
| * otherwise, and only otherwise, `remove`. | ||
| */ | ||
| export function classify(f) { |
There was a problem hiding this comment.
🟡 P2 (major) — Sibling process re-checkout check alignment in classification. Aligning this with the simplified comparison logic ensures that we correctly report transitions to/from detached HEAD as branch-changed-under-us and mark them as UNVERIFIABLE. Using a fallback label handles null branches gracefully when printing the message.
[pass 1]
| missing: !existsSync(entry.path), prunableReason: entry.prunableReason, | ||
| detached: entry.detached, recordedBranch: entry.branch, liveBranch: null, | ||
| dirty: false, dirtyCount: 0, statusFailed: false, statusError: '', | ||
| hasUpstream: false, upstream: null, ahead: null, |
There was a problem hiding this comment.
🟡 P2 (major) — Sibling process re-checkout check does not detect transitions from a branch to a detached HEAD. The current condition f.liveBranch !== null && f.recordedBranch !== null && f.liveBranch !== f.recordedBranch requires f.liveBranch !== null. If a sibling process checks out a detached HEAD in that worktree in between the two checks, liveBranch will be set to null (because live.stdout === 'HEAD').
Since liveBranch is null, this check is skipped, and the script does not flag the worktree as branch-changed-under-us. Simplifying this to f.recordedBranch !== f.liveBranch handles all transitions correctly (including branch to detached, detached to branch, and branch to branch) and makes the logic much cleaner.
[pass 1]
| if (pr) { | ||
| f.mergedBy = 'pr'; | ||
| f.prNumber = pr.number; | ||
| f.mergedAt = pr.mergedAt; |
There was a problem hiding this comment.
🔴 P1 (blocker) — Timezone comparison bug in date string check. tip.stdout is formatted using local committer date (%cI), which includes a local timezone offset (e.g. 2026-07-01T01:00:00-05:00). pr.mergedAt is returned by the GitHub API in UTC (e.g. 2026-07-01T03:00:00Z). Doing a direct lexicographical string comparison (tip.stdout > pr.mergedAt) is timezone-incorrect.
For example, if the local timezone is behind UTC (e.g. EST -05:00), a commit made after the merge can have a string representation that is lexicographically smaller than the UTC merge string (e.g. "2026-07-01T01:00:00-05:00" > "2026-07-01T03:00:00Z" evaluates to false even though the local time is 6:00 AM UTC, which is 3 hours after the merge). This would cause tipAfterMerge to be false, meaning the tool will fail to detect unpushed commits made after a merge, classify the worktree as safe for removal, and potentially destroy the user's unpushed work (data loss).
Use Date.parse() to safely perform timezone-aware comparison of the ISO 8601 strings.
[pass 1]
| expect(reasons(r)).toContain('branch-changed-under-us'); | ||
| }); | ||
|
|
||
| it('finds node_modules per worktree without charging one for another', () => { |
There was a problem hiding this comment.
🔵 P3 (minor) — Integration coverage of sibling detached re-checkout. Adding an integration test ensures that transitions to a detached HEAD are correctly identified as branch-changed-under-us and never inadvertently cleaned up.
[pass 1]
# Conflicts: # .claude-plugin/marketplace.json # CHANGELOG.md
There was a problem hiding this comment.
💬 Review Comments
Comments — 0 blockers, 1 P2. Confidence: 3.80/5.00. NITs: 1 (shown).
Walkthrough
main directly. According to GitFlow guidelines, features should target develop first, then be merged into main via a release sync. This is a non-blocking informational warning, and the code verdict remains based solely on the technical quality and safety of the changes.
Walkthrough
This pull request introduces the /disk-cleanup-merged-worktrees command, the worktree-cleanup skill, and the supporting scripts/worktree-cleanup.mjs script to safely reclaim disk space from git worktrees. It automatically identifies and safely reclaims node_modules folders from live worktrees and removes merged, clean, and pushed worktrees (while preserving local branch refs).
Files Reviewed
I have thoroughly reviewed the core implementation in scripts/worktree-cleanup.mjs (the classification rules, git subprocess parsing, and safety bounds), the comprehensive suite in src/audit/worktree-cleanup.test.ts, the command specifications in commands/disk-cleanup-merged-worktrees.md, and the updated plugin configurations.
Safety Rationale
The design is extremely robust and conservative: it enforces rigorous dry-run bounds, prevents branch ref deletion, skips locked or active worktrees, and implements strong guard checks in assertRemovableNodeModules to guarantee that no deletions occur outside of valid node_modules paths in known worktrees.
Verdict
Commented — 0 blockers, 1 P2.
🟡 P2 — Major
scripts/worktree-cleanup.mjs:399— 🟡 P2 (major) — The strict ISO 8601 committer date format (%cI) output depends on the committer's local timezone offset (e.g.2026-07-01T12:00:00-04:00), whereas the GitHub PRmergedAttimestamp is returned in UTC (2026-07-01T16:00:00Z). Lexicographical string comparison of ISO 8601 strings with differing timezone offsets or formats can yield incorrect comparisons (e.g.'12'compared lexicographically against'16'). UsingDate.parse()converts both to UTC millisecond values, which guarantees timezone-independent comparison correctness.
[pass 1]
⚪ P4 — Nitpicks
scripts/worktree-cleanup.mjs:72— [NIT] ⚪ P4 (nit) — ThestatSyncfunction is imported from'node:fs'but is never used anywhere in the cleanup script. We can safely remove it from the import destructuring list.
[pass 1]
Total findings: 1 compliance, 1 nit (2 total)
| * did not run". | ||
| */ | ||
|
|
||
| import { execFileSync } from 'node:child_process'; |
There was a problem hiding this comment.
[NIT] ⚪ P4 (nit) — The statSync function is imported from 'node:fs' but is never used anywhere in the cleanup script. We can safely remove it from the import destructuring list.
[pass 1]
| * Find `node_modules` directories inside a worktree. | ||
| * | ||
| * Does not descend into a found `node_modules` (nested copies belong to their | ||
| * parent's total), into `.git`, or into ANY other worktree — worktrees live |
There was a problem hiding this comment.
🟡 P2 (major) — The strict ISO 8601 committer date format (%cI) output depends on the committer's local timezone offset (e.g. 2026-07-01T12:00:00-04:00), whereas the GitHub PR mergedAt timestamp is returned in UTC (2026-07-01T16:00:00Z). Lexicographical string comparison of ISO 8601 strings with differing timezone offsets or formats can yield incorrect comparisons (e.g. '12' compared lexicographically against '16'). Using Date.parse() converts both to UTC millisecond values, which guarantees timezone-independent comparison correctness.
[pass 1]
1 participant
What
/make-no-mistakes:disk-cleanup-merged-worktrees— reclaim disk from git worktrees without destroying work.Three files do the work:
commands/disk-cleanup-merged-worktrees.md(the command),skills/worktree-cleanup/SKILL.md(the doctrine), andscripts/worktree-cleanup.mjs(the measurement). The command delegates rather than instructing an agent to re-derive any of this withgit worktree list | grep.Why the logic is a program and not a checklist
This tool deletes. The value is entirely in what it REFUSES, and a refusal written as a bullet point runs only when the reader remembers it. Every refusal below is a predicate with a name, an evidence string, and a test that fails when the predicate is removed.
The asymmetry that shapes every decision: 20 GB of stale worktrees costs disk, which is recoverable by definition. One removed worktree holding the only copy of someone's commits costs the work. So the classifier is biased all the way to the safe side and accepts leaving space on the table to stay there.
main-checkoutgit worktree listlockedlockedin--porcelainoutputuncommittedgit status --porcelainnon-empty — untracked files includedunpushedgit rev-list --count <upstream>..HEADnot-merged"Merged" is measured three ways, and the third is the common one
ancestor(git merge-base --is-ancestor),cherry(git cherry, every line-), andpr(a GitHub PR withstate == MERGED).A squash merge collapses N commits into one commit with a new patch id, so the branch is neither an ancestor of the base nor patch-equivalent to it. An ancestry test alone reports
not-mergedfor work that certainly landed, and in a squash-merge repo that is the majority case. That is why the PR test exists, and why losingghdowngrades a verdict tounverifiablerather than tonot-merged.The base is resolved as a SET, not one branch. The first version took
origin/HEADand stopped; against a repo that merges features intodevelopand promotes tomainat release time, that produced 41 falsenot-mergedverdicts on the run below.--base Xnarrows it back to one.unverifiableis a third verdict and never collapses into "safe"Five ambiguous states are reported and skipped rather than resolved:
branch-changed-under-us(a sibling process re-checked the worktree out mid-run — every other measurement then describes a state that is gone),commits-after-merge(PR merged, branch tip newer thanmergedAt),merge-unmeasurable(no local evidence and nogh),status-unreadable,gitdir-missing.Two actions, deliberately separate
node_modulesreclaim is the DEFAULT. 38 dirs in the measured repo, reversible with one install, touches no tracked file in any worktree. It targets worktrees that are still alive — a dirty worktree is a normal target, because a build artifact directory is not the work.--worktrees, and deleting anything is opt-in behind--apply.--forceis never passed unless the user asks for it in that invocation, and the command says so in prose rather than offering it as the way past a refusal. Branch refs are never deleted —git worktree removetakes the directory only, so even a wrong removal is recoverable withgit worktree add.Verification
1. Dry run against
dojo-os's 60 worktreesThe two it would remove, each with its evidence — note that they came from different merge tests:
It correctly refuses at least one worktree with uncommitted changes (several, in fact — the largest holding 442 lines):
And at least one with unpushed commits — the case that would have destroyed work:
Ambiguity reported rather than resolved:
2. The
--applypath, end to end in a scratch repoAn apply path that has never run is not verified either. Three worktrees, all merged into
develop; one clean, one with an uncommitted file, one with a local-only commit.State afterwards — the two refused worktrees are untouched, and the removed one's branch ref survives:
A second
--applyis a clean no-op:TOTAL 0 B reclaimed across 0 action(s), same three refusals.3. Tests — 35 new, and mutation-checked
bun/npx vitest run: 95 passed (13 files), of which 35 are new. Ten build real git repositories with a real remote, a real merged branch, a real dirty worktree and a real unpushed branch, because a hand-built fact object can be wrong about what git actually reports.A suite that has only ever been green proves nothing, so the two load-bearing refusals were mutation-checked — disabling the
uncommittedandunpushedpredicates:Restored: 35 passed.
assertRemovableNodeModulesis likewise tested with paths it must reject, including a prefix collision (/repo-otheris not inside/repo).Notes
andres/ban-discard-stderr(v1.40.0) is open and unmerged whilemainsits at 1.38.0, so a number picked here either collides or leaves a hole. The entry sits under## [Unreleased]; whichever PR lands second takes the next number.run()helper withstdio: 'pipe'and no shell, so stderr is captured and surfaced. In this tool the found-nothing-versus-errored collapse is the difference between "no unpushed commits" and "the check did not run".Skills (10)with 10 table rows whileskills/shipped 11 (resolve-open-questionshad no row). Both now read 12.Created by Claude Opus 5 (1M context) on behalf of @lapc506