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 7 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]
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