feat(commands): /disk-cleanup — one command, four stages, ascending risk - #59
feat(commands): /disk-cleanup — one command, four stages, ascending risk#59lapc506 wants to merge 8 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]
The version-bump collision every release produces. Both conflicts were `marketplace.json` + `CHANGELOG.md` and nothing else; `package.json` and `plugin.json` auto-merged. Resolution: - **Version: main's, in all four files.** 1.43.0, not the branch's 1.41.0 and not a number picked here. Measured before resolving: `git show origin/main:package.json | grep version` -> 1.43.0. The branch had already merged main once at 1.41.0; main has since released 1.42.0 and 1.43.0. A feature branch that picks its own number either collides with an open release PR or leaves a hole, so it takes what is on main and whichever change lands next takes the following one. - **CHANGELOG: this branch's `[Unreleased]` entry re-applied ABOVE main's released sections.** The entry's leading note was rewritten because it had gone stale — it named `andres/ban-discard-stderr` as open and main as being at 1.38.0, and both statements were false by the time of this merge. `[Unreleased]` now compares from v1.43.0 rather than v1.38.0. Counts corrected to the measured post-merge values, because the merge is what made them wrong: the README tables carry 40 command rows and 13 skill rows (`ls commands/*.md | wc -l` = 40, `ls skills/*/SKILL.md | wc -l` = 13) while the headers still read 38 and 12, and `marketplace.json` still described "38 commands, 12 auto-activating skills". Main's own count was already one behind before this merge (38 stated, 39 on disk); that is recorded here rather than silently absorbed. Suite after the merge: 95 tests across 13 files, all passing. Created by Claude Opus 5 on behalf of @lapc506
… in it
Global Constraint 1 names three independent keep-reasons plus UNVERIFIABLE.
The shipped classifier implemented three of the four: dirty tree, commits
absent from the base, and UNVERIFIABLE. **A merge or rebase in progress had no
check at all**, and the assumption that the dirty check covers it is false.
Measured, on a repository built for the question rather than argued from the
code:
two branches add the SAME file with the SAME content. `git merge --no-commit`
auto-merges cleanly, the resulting tree is byte-identical to HEAD's, and
`git status --porcelain` returns ZERO lines while MERGE_HEAD exists.
Against that worktree the classifier returned:
dirty: false midOperation: undefined
VERDICT: remove
findings: []
and `git worktree remove` then took the directory — **exit 0, no output, no
refusal of its own.** Git offers no protection here. The whole in-progress
merge was gone. That is the exact shape the reference implementation in dojo-os
guards against and the reason its comment says the state "lives in .git and not
in the file list".
What is added:
- `detectStoppedOperation()` reads the worktree's OWN git dir via
`git rev-parse --absolute-git-dir` — a linked worktree keeps these files in
`.git/worktrees/<name>`, not in the shared dir, so this must be resolved per
worktree. Covers MERGE_HEAD, rebase-merge, rebase-apply, CHERRY_PICK_HEAD,
REVERT_HEAD and BISECT_LOG. Cherry-pick and revert are the same class of
git-dir-resident state with the same destroy path; covering merge and rebase
alone would leave `git cherry-pick`, which conflicts constantly, as a live
hole.
- It returns `{ unmeasurable: true }` when the git dir cannot be resolved,
rather than "nothing in progress". This is the reason it is not one
`existsSync` call: with the location unknown all six probes return absent,
and six absent probes read exactly like a clean worktree — which is the
answer that authorises deletion.
- `classify()` gains `mid-operation` (REFUSE) and `mid-operation-unmeasurable`
(UNVERIFIABLE), placed BEFORE the `statusFailed` early return so the stronger
verdict survives an unreadable status, and BEFORE `dirty` so the report leads
with the stopped operation. When both fire the stopped operation is the
EXPLANATION for the dirty files, and "17 uncommitted changes" sends the
reader to `git stash` when the answer is `git merge --abort`.
Tests: 35 -> 46 in this file, 95 -> 106 across the suite. Six pure cases and
five integration cases against real git, including the clean-mid-merge repro
above, a stopped rebase, a stopped cherry-pick, and two positive controls — the
same worktree returning to REMOVE after `git merge --abort`, and
`detectStoppedOperation` giving three DIFFERENT answers for clean / stopped /
off-repo.
The fixture asserts its own premise, because it failed without one. With
identical content, author, parent and message, the two side commits hash to the
SAME commit whenever both land inside one second — then `feat/b` IS `feat/a`,
the merge reports "already up to date", and the fixture silently stops testing
anything. On its first run it passed one test and failed three purely according
to which side of a second boundary the commits fell on. The messages now
differ and `expect(sha('feat/a')).not.toBe(sha('feat/b'))` guards it.
Mutation controls, per Global Constraint 4 — tests going red when each
predicate is disabled: uncommitted 4, mid-operation 7, unpushed 3, not-merged
3, UNVERIFIABLE-collapsed-to-REMOVE 9, unmeasurable-reported-as-clean 1. Six
mutations, six different failure sets; unmutated and post-restore controls both
green, restored file byte-identical to the backup.
`tsc --noEmit` error count unchanged at 15 (all pre-existing); `npm run build`
green.
Created by Claude Opus 5 on behalf of @lapc506
…g the repo with none
Scope item 3 of the brief: confirm each repo's OWN base is resolved rather than
one being assumed. `resolveBases()` already did this; nothing here changes the
resolver. What was missing was a control that can fail, and a measurement.
Verified by RUNNING the resolver read-only over the 23 git checkouts under
~/Documentos/GitHub/dojocoding rather than reading its code:
21 resolve `main` first 1 resolves `develop` first
7 resolve a two-element set 1 resolves NONE
`dojo-infra-gitops` resolves `[main]`, which is the case the plan names — a
hardcoded `develop` would find no base there. `dojo-os` resolves
`[main, develop]`: `origin/HEAD` is `main` while its PRs target `develop`, so
the SET is what saves it. It tries `main`, finds no evidence, and lands the
verdict on `develop`.
The third shape was not anticipated and is the interesting one. `openclaw`
carries **3155 remote-tracking refs and not one** of
origin/{HEAD,main,develop,master,trunk} — every branch is
`origin/dojo/v<date>-fixes`. The resolver returns an empty set and `main()`
exits 2 asking for `--base`. That is the correct behaviour and it was untested:
an empty base set is exactly the input that must not become "nothing to compare
against, therefore nothing is unmerged", which is the reasoning the reference
implementation in dojo-os calls out explicitly.
Three tests added, each able to fail:
- a main-only repo resolves `main`, does NOT contain `develop`, and the
resolved base is then USED — a branch merged into main classifies REMOVE;
- a repo with none of the conventional names resolves `[]` with the honest
`how` string, while `--base` still narrows to an explicit name;
- the mirror control: the same branch measured against a base it never reached
reports mergedBy null and REFUSE, so a resolver that silently widened its set
could not pass both.
Note on the brief's figures: it states 6 of 13 repos base on `main` and 7 on
`develop`. Measured against remote-tracking refs the split is 21/1, because
those are different questions — the brief counts each repo's PR-target policy,
this counts what `origin/*` actually carries. The tool reads the refs, so the
refs are what its tests assert.
49 tests in this file, 109 across the suite. `tsc --noEmit` unchanged at 15.
Created by Claude Opus 5 on behalf of @lapc506
There was a problem hiding this comment.
✅ Approved
Approved — 0 blockers, 1 P3. Confidence: 4.80/5.00.
Walkthrough
main branch directly. According to the repository's GitFlow practices, feature branches are expected to target develop first (i.e. feature → develop → main). This is a non-blocking informational warning, and the technical review verdict below is based solely on the code quality.
Walkthrough
This PR introduces the /make-no-mistakes:disk-cleanup-merged-worktrees command, the worktree-cleanup skill, and an accompanying NodeJS script (scripts/worktree-cleanup.mjs) to reclaim disk space from git worktrees. It runs a deterministic classifier that checks for uncommitted changes, unpushed commits, lock states, stopped operations (merge, rebase, cherry-pick, revert, bisect), and three independent kinds of merge evidence (ancestry, patch-equivalence, and PR merge state) to safely determine if a worktree can be removed.
Reviewed Files and Areas
- Core Logic:
scripts/worktree-cleanup.mjs(reviewed the classification logic, git command executions, andnode_modulesdetection). - Tests:
src/audit/worktree-cleanup.test.ts(reviewed pure classification tests and real-git integration tests). - Documentation & Manifests:
commands/disk-cleanup-merged-worktrees.md,skills/worktree-cleanup/SKILL.md,README.md,CHANGELOG.md, and.claude-plugin/marketplace.json.
Safety Rationale
Safety is guaranteed because the tool operates in dry-run mode by default, separates the safer node_modules cleanup from the destructive --worktrees removal, and treats any ambiguous or unmeasurable states as UNVERIFIABLE to explicitly refuse deletion.
Approved — 0 blockers, 1 P3.
🔵 P3 — Minor
scripts/worktree-cleanup.mjs:416— 🔵 P3 (minor) — Theducommand is not natively available on Windows environments (outside of Git Bash, MSYS2, or WSL). Under standard Windows Command Prompt or PowerShell,dirByteswill return0bytes and report an error, making the size of recoverable disk space unreadable. Adding a note in the README or a brief runtime check/fallback helps clarify Windows environment requirements.
[pass 1]
Total findings: 1 business context (1 total)
| if (up.ok && up.stdout) { | ||
| f.hasUpstream = true; | ||
| f.upstream = up.stdout; | ||
| const ahead = git(['rev-list', '--count', `${up.stdout}..HEAD`], entry.path); |
There was a problem hiding this comment.
🔵 P3 (minor) — The du command is not natively available on Windows environments (outside of Git Bash, MSYS2, or WSL). Under standard Windows Command Prompt or PowerShell, dirBytes will return 0 bytes and report an error, making the size of recoverable disk space unreadable. Adding a note in the README or a brief runtime check/fallback helps clarify Windows environment requirements.
[pass 1]
…and not `any` PR #65 (`ci: run the TypeScript suite`, MERGEABLE) adds a workflow running `npx tsc --noEmit` and `npm test` on every PR with **no `paths:` filter**. Its own changelog reports tsc "failing on three errors in `src/cli.ts`" — measured against `main`, where `src/audit/worktree-cleanup.test.ts` does not exist yet. This PR introduces that file carrying **9 errors of its own**, so whichever of the two lands second goes red on a gate the other one built. Fixed here rather than left as a merge-order trap. The fix is a declaration file, not a suppression. `scripts/worktree-cleanup.mjs` stays plain ESM because a slash command invokes it as `node scripts/...` with no build step available; `scripts/worktree-cleanup.d.mts` sits beside it and TypeScript resolves it automatically for the `.mjs` import. It is worth more than the error count. The suite was `any` throughout, and every case is `{ ...clean, oneField: x }` — an excess-property check does not reach inside a spread, so under `any` a MISSPELLED field name is not an error, it is a fact the classifier never reads. The test then passes while asserting nothing whatsoever about the guard named in its own title. That is precisely the "regression test that still passes while proving nothing" this suite was written to avoid, present in the suite itself. Typing `clean` as `WorktreeFacts` surfaced it immediately: `mergedBy: 'pr'` widened to `string` and failed against the `'ancestor' | 'cherry' | 'pr' | null` union, in every one of the spread cases. `ahead` and `midOperation` now carry their `null`-means-unmeasured meaning in the type rather than only in a comment. Measured: `tsc --noEmit` on worktree-cleanup files 9 errors -> 0. Repo total 15 -> 6, and all 6 remaining are the pre-existing `src/cli.ts` ones that PR #65 fixes — so after either merge order the gate is green. `npm run build` green, 109 tests passing. Created by Claude Opus 5 on behalf of @lapc506
There was a problem hiding this comment.
💬 Review Comments
Comments — 0 blockers, 1 P2. Confidence: 3.80/5.00.
Walkthrough
⚠️ Governance Warning: This PR targetsmaindirectly. GitFlow expects development to progress via feature → develop → main. Since this is an informative governance note, the code assessment remains independent of this warning.
Walkthrough
This PR introduces the /disk-cleanup-merged-worktrees command, the corresponding worktree-cleanup skill, and a node script (worktree-cleanup.mjs) to reclaim disk space from git worktrees. It implements robust criteria (refusing dirty/unpushed/mid-operation worktrees and verifying merged state via ancestry, patch-equivalence, and GitHub PRs) to safely delete stale worktrees and node_modules without destroying active work.
Reviewed Areas
I reviewed commands/disk-cleanup-merged-worktrees.md, skills/worktree-cleanup/SKILL.md, scripts/worktree-cleanup.mjs, scripts/worktree-cleanup.d.mts, and src/audit/worktree-cleanup.test.ts.
Safety Rationale
The cleanup logic is safe to merge because it defaults to a non-destructive dry run, treats any unverifiable/ambiguous state as unremovable, never deletes branches or runs pruning on its own, and has extremely rigorous integration tests and mutation verification.
Approved — 0 blockers, 1 P2, 0 P3.
🟡 P2 — Major
scripts/worktree-cleanup.mjs:478— 🟡 P2 (major) — Lexicographical comparison of local commit timezone vs PR mergedAt UTC
Intip.stdout > pr.mergedAt,tip.stdoutis a local ISO 8601 string with a timezone offset (e.g.,-05:00), whereaspr.mergedAtis UTC (Z). Lexicographical comparison (>) is timezone-unaware and will produce incorrect results when the local timezone has a negative offset, failing to detect late commits added after the merge.
UseDate.parse()to compare chronologically.
[pass 1]
Total findings: 1 compliance (1 total)
| * 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) — Lexicographical comparison of local commit timezone vs PR mergedAt UTC
In tip.stdout > pr.mergedAt, tip.stdout is a local ISO 8601 string with a timezone offset (e.g., -05:00), whereas pr.mergedAt is UTC (Z). Lexicographical comparison (>) is timezone-unaware and will produce incorrect results when the local timezone has a negative offset, failing to detect late commits added after the merge.
Use Date.parse() to compare chronologically.
[pass 1]
Folds the docker/node_modules/volumes command in with the worktree classifier it delegates to, so the toolkit ships ONE disk-reclaim entry point instead of two doors onto subsets of the same job. Measured 2026-08-11 on a 468 GB volume that had hit 100% full: docker images returned 48.3 GB, worktree node_modules 11 GB, volumes ~1 GB, and worktree removal cleared 8 of 69 while keeping 61. `docker system df` predicted 18 GB where 48.3 GB came back — it does not count shared layers — so every stage measures free space before and after and no stage prints a prediction. Stages 2 and 3 delegate to scripts/worktree-cleanup.mjs rather than reimplementing it. A shorter copy of a destructive classifier reads as equivalent, drifts in silence, and the drift surfaces as deleted work. RETIRES commands/disk-cleanup-merged-worktrees.md. The classifier and its skill stay and are unchanged; only the second slash command goes. Once /disk-cleanup delegates to the same classifier, a second command means the reader must decide which to reach for and the answer is always the one that also does docker. README table, header count and the command doc's Related section all updated: 40 rows, 40 files, header 40. THREE OUTCOMES, never two. `already clean` means an --apply run removed nothing because there was nothing to remove. `UNAVAILABLE` means the stage could not run. A dry run says so and reports the classifier's recoverable figure. Keying the report only on the before/after delta collapsed the third into the first, because in a dry run that delta is zero BY CONSTRUCTION: the default invocation printed "276 MB recoverable" and "already clean" two lines apart, in the command whose entire purpose is refusing that confusion, and its protocol tells the reader to relay `already clean` verbatim. Two test defects fixed alongside it, both of which had been passing for the wrong reason: - The UNAVAILABLE case relied on worktree-cleanup.mjs being absent from the checkout, so it went red the moment the classifier shipped beside it. The absence is now materialized in a temp dir. A test whose premise the repo can revoke tests the repo's contents, not the guard. - Nothing covered the delegate being PRESENT in a dry run — the axis the defect above lives on. Added, against a stub delegate rather than the real classifier, so the assertion does not depend on `gh` or on the host's worktrees. Verified: shellcheck clean on both scripts; suite 19/19. Mutation control — removing the dry-run branch turns exactly the two new assertions red (17/2) and leaves the other seventeen green, so the failure set is specific rather than uniform. Created by Claude Code on behalf of @lapc506 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191NZw1kufQfsAVPocTH6ju
lapc506
changed the title
There was a problem hiding this comment.
✅ Approved
Approved — 0 blockers, 1 P3. Confidence: 4.80/5.00.
Walkthrough
Governance Notice
main branch directly. Per standard GitFlow practices, feature PRs are typically expected to target the develop branch first, followed by a release/promotion to main. Since this is an administrative alignment, the code review verdict below is based solely on the quality, safety, and functionality of the code change itself.
Walkthrough
This PR adds a highly disciplined disk space reclamation tool (/disk-cleanup) alongside a deterministic worktree cleanup classifier (scripts/worktree-cleanup.mjs / skills/worktree-cleanup/SKILL.md). The tool reclaims space in ascending order of risk (unused Docker images, then worktree node_modules, then safe-to-remove merged worktrees) while robustly refusing to delete any worktrees that contain uncommitted changes, unpushed commits, active/stopped operations (merge/rebase/cherry-pick), or locked statuses.
Files Reviewed
- Commands & Skills:
commands/disk-cleanup.md,skills/worktree-cleanup/SKILL.md - Shell & JS Scripts:
scripts/disk-cleanup.sh,scripts/worktree-cleanup.mjs - Typings & Tests:
scripts/worktree-cleanup.d.mts,scripts/test-disk-cleanup.sh,src/audit/worktree-cleanup.test.ts - Project Metadata:
README.md,CHANGELOG.md,.claude-plugin/marketplace.json
Safety Rationale
The implementation is exceptionally safe: it relies on robust, non-destructive defaults (reclaiming node_modules instead of deleting worktrees, dry-running by default), explicitly avoids destructive shell-invocation patterns by utilizing execFileSync arguments arrays directly, lists (but refuses to auto-delete) Docker volumes containing state, and utilizes a multi-layered verification system (pure + integration tests) ensuring every protective guard behaves exactly as intended.
Approved — 0 blockers, 1 P3.
🔵 P3 — Minor
scripts/worktree-cleanup.mjs:429— 🔵 P3 (minor) — Passing--baseat the very end of the argument list without a branch name results ino.basebeing set toundefined(sinceargv[i + 1]is undefined) rather than throwing an error. A check for a missing value would prevent silent fallbacks to conventional bases when the user intended to specify a branch.
[pass 1]
Total findings: 1 business context (1 total)
| f.mergedBase = base; | ||
| break; | ||
| } | ||
| if (!branch) continue; |
There was a problem hiding this comment.
🔵 P3 (minor) — Passing --base at the very end of the argument list without a branch name results in o.base being set to undefined (since argv[i + 1] is undefined) rather than throwing an error. A check for a missing value would prevent silent fallbacks to conventional bases when the user intended to specify a branch.
[pass 1]
1 participant
Ships one disk-reclaim command, not two.
What changed since this PR was opened
It originally added
/disk-cleanup-merged-worktrees— the worktree classifier plus a slash command of its own. A second change then built/disk-cleanup, covering the docker stages that nothing in this repo touched. Landing both would have left the toolkit with two commands where one is a strict subset of the other, so they are folded together here and the second command is retired before it ever shipped.The classifier and its skill are unchanged. Only
commands/disk-cleanup-merged-worktrees.mdgoes.The command
Four stages, ascending order of risk, stopping when the target is met. Measured 2026-08-11 on a 468 GB volume that had reached 100% full:
docker image prune -anode_modulesdocker system dfunder-reports and is never used as the answer. It predicted 18 GB where 48.3 GB came back, because it does not count shared layers. A command built on that prediction would have understated its largest safe reclaim by two thirds and moved on to riskier stages to cover a shortfall that did not exist. Every stage measures free space before and after; no stage prints a prediction.Stages 2 and 3 delegate to
scripts/worktree-cleanup.mjs. A shorter copy of a destructive classifier reads as equivalent, drifts in silence, and the drift surfaces as deleted work. The classifier resolves the base as a SET, tests "merged" three ways — ancestry alone reportsnot-mergedfor every squash merge — and treatsunverifiableas its own verdict rather than folding it into "safe".Two guards have no flag behind them at all: branch refs are never deleted (
--branchesand--forceare rejected, not passed through), and volumes are listed, never removed, with or without--apply, because that stage has no--applypath.Three outcomes, never two — and one of them was missing
already cleanmeans an--applyrun removed nothing because there was nothing to remove.UNAVAILABLEmeans the stage could not run. A dry run says it is a dry run and reports the classifier's recoverable figure.Keying the report only on the before/after delta collapsed the third into the first, because in a dry run that delta is zero by construction. The default invocation printed, two lines apart:
In the command whose entire purpose is refusing exactly that confusion — and whose protocol instructs the reader to relay
already cleanverbatim. Fixed, with the reasoning in the code rather than only here, since that comment is what the next reader will find.Stage 1 was never affected: it takes its own
--applybranch and returns before reaching the shared reporter.Two tests that were passing for the wrong reason
UNAVAILABLEcase relied onworktree-cleanup.mjsbeing absent from the checkout. It went red the moment the classifier shipped beside it — correctly. The absence is now materialized in a temp dir. A test whose premise the repository can revoke is testing the repository's contents, not the guard.ghor on the host's worktrees.Verification
shellcheckon both scriptsscripts/test-disk-cleanup.shvitest src/audit/worktree-cleanup.test.tsMutation control, because a suite that cannot fail proves nothing: removing the dry-run branch turns exactly the two new assertions red (17 / 2) and leaves the other seventeen green. A specific failure set rather than a uniform one — uniformity across variants would have meant the control was broken, not that the guards agreed.
Note on the branch name
The head branch is still
andres/disk-cleanup-merged-worktrees, from when that was the deliverable. Renaming it would break existing review links, so it stays; the shipped command is/disk-cleanup.Created by Claude Code on behalf of @lapc506
🤖 Generated with Claude Code
https://claude.ai/code/session_0191NZw1kufQfsAVPocTH6ju