fix(worktree): match rebuildable ignored FILES on the leaf, not the whole path - #799
Open
griffinwork40 wants to merge 3 commits into
Open
fix(worktree): match rebuildable ignored FILES on the leaf, not the whole path#799griffinwork40 wants to merge 3 commits into
griffinwork40 wants to merge 3 commits into
Conversation
…hole path Post-merge review finding H1 on #770. `REBUILDABLE_FILE_PATTERNS` mixed two anchoring styles and the whole table was tested against the full normalized path, so four of its entries only ever matched at the repo root: .DS_Store -> opaque (reaped) src/.DS_Store -> protected (preserved, permanently) A nested copy matched no file pattern and no directory pattern, fell through to `protected`, and the tree was preserved as `stale-dirty` -- a warn-only verdict with no age escape, so the automatic sweep could never reclaim it again. `.DS_Store` is gitignored in this repo, so a single Finder visit to any subdirectory of a worktree was enough to make that worktree immortal. This fails in the SAFE direction -- it costs the sweep, it does not destroy data -- but unbounded worktree accumulation is exactly what the sweep exists to prevent. `worktree remove --force` was always still an escape, since the probe is gated behind `if (!force)`. Fixed structurally rather than by re-anchoring the four offending entries: the file table is now matched against the LEAF segment, via the `leafOf` helper already present in the module. Depth can no longer change the verdict for a machine-generated filename, and a future entry cannot reintroduce the bug by choosing the wrong anchor. The table's anchors are unified to `^` to match, and the invariant is named in-comment so the next reader sees the axis. The two directory tables deliberately keep matching the WHOLE path. Leaf matching those would classify a plain file named `dist` as build output, and would collapse the documented `logs/` asymmetry. That split is now stated as a contract on `classifyIgnoredEntry`. Verification: - new src/agent/worktree-ignored-patterns.test.ts (36 tests) pins depth invariance in both directions, including the counterweight cases: nested hand-authored files and nested sensitive leaves still protect, and a bare file named `dist` is still not build output - mutation-tested: reverting only the one-line matching change fails 13 of the 36, so the guard discriminates - pnpm test -- 723 files, 13893 passed, 2 skipped - pnpm lint (tsc --noEmit) -- clean Scope: H1 only. H2 from the same review is already tracked as #794; M1 (zero-entry expansion falls open) and M2 (`target/` enumeration cost) are untouched.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad0505edda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (normalized === '') return 'protected'; | ||
| if (isSensitiveLeaf(normalized)) return 'protected'; | ||
| if (REBUILDABLE_FILE_PATTERNS.some((re) => re.test(normalized))) return 'opaque'; | ||
| if (REBUILDABLE_FILE_PATTERNS.some((re) => re.test(leafOf(normalized)))) return 'opaque'; |
There was a problem hiding this comment.
When an ignore rule matches a directory named .DS_Store, .eslintcache, or debug.log, Git's traditional --ignored mode—the default documented by git status -h—emits entries such as !! src/.DS_Store/. Because leafOf strips the trailing slash, this new check classifies that directory as opaque, so the probe never expands it and the automatic forced worktree removal can irreversibly delete nested .env files or other local state. I reproduced all three directory names being reported with / and classified as opaque; file-pattern matching should reject directory entries before taking the leaf.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Updated ignored-entry classification so paths ending in
/cannot match generated-file patterns, while preserving whole-path policies for containing build and dependency directories. src/agent/worktree-ignored-patterns.tsL181-L205 - Added regression coverage for directories named
.DS_Store,.eslintcache,.stylelintcache, anddebug.log, plus coverage ensuring containing-directory policies still apply. src/agent/worktree-ignored-patterns.test.tsL126-L139 - Committed the changes as
8748e14(fix(worktree): exclude directories from file patterns). - Created the follow-up PR metadata with title “fix(worktree): exclude ignored directories from generated-file matching.”
Testing
- ✅
pnpm test src/agent/worktree-ignored-patterns.test.ts— 38 tests passed. - ✅
pnpm test src/agent/worktree-ignored-probe.test.ts— 78 tests passed. - ✅
pnpm lint - ✅
git diff --check - ✅
git status --short --branch— working tree clean. ⚠️ Test and lint commands emitted an engine warning because the environment uses Node 20 while the package requests Node 22 or newer; all checks still completed successfully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes finding H1 from the post-merge review of #770. #770 shipped in v5.83.3, so this is the fast follow-up that review called for.
The defect
REBUILDABLE_FILE_PATTERNSmixed two anchoring styles, and the whole table was tested against the full normalized path rather than the leaf. Four of its entries were therefore^-only and matched at the repo root and nowhere else:.DS_Storeopaque— reapedopaquesrc/.DS_Storeprotected— preserved foreveropaquea/b/.DS_Storeprotectedopaque.eslintcacheopaqueopaquepackages/app/.eslintcacheprotectedopaquedeep/.stylelintcacheprotectedopaqueSame filename, different depth, opposite verdict. A nested copy matched no file pattern and no directory pattern, so it fell through to
protected.Only
/\.tsbuildinfo$/was already depth-safe, because it is suffix-anchored rather than^-anchored — the mutation run below confirms this independently: reverting the fix fails every entry except that one.Why it persisted. The resulting verdict is
stale-dirty, which is warn-only and has no age escape, so the tree was never auto-reaped again..DS_Storeis gitignored in this repo (.gitignore:25), so one Finder visit to any subdirectory of a worktree was enough to make that worktree immortal.Blast radius. This fails in the safe direction — it costs the sweep, it does not destroy data, and
worktree remove --forcewas always still an escape because the probe is gated behindif (!force). What it defeats is the automatic reclamation, i.e. the unbounded accumulation the sweep exists to prevent.The fix
The review offered two shapes: re-anchor the four entries to
(?:^|\/), or match the table against the leaf. I took the leaf option, because it fixes the class rather than the four instances — after this, a future entry cannot reintroduce the bug by picking the wrong anchor, and the axis is no longer something each pattern has to remember to encode. It reusesleafOf, already in the module and already used byisSensitiveLeaf, so the two leaf-scoped tables are now symmetric.The table's anchors are unified to
^to match (against a leaf,(?:^|\/)and^are equivalent — a leaf holds no slash), so the mixed-style trap is gone from the source too.The directory tables deliberately keep matching the whole path, and that asymmetry is now written down as a contract on
classifyIgnoredEntry. Leaf-matching them would classify a plain file nameddistas build output, and would collapse thelogs/asymmetry #770 documented on purpose.Verification
src/agent/worktree-ignored-patterns.test.tspnpm test src/agent/worktree-ignored-probe.test.tspnpm test src/agent/worktree-sweep.test.tspnpm test src/agent/worktree-occupancy.test.tspnpm test(full)pnpm lint(tsc --noEmit)Mutation-tested, because a non-discriminating test was itself a finding on #770. Reverting only the one-line matching change (
leafOf(normalized)→normalized) fails 13 of the 36 new cases. The guard observes the fix rather than re-proving ambient behaviour.New coverage pins depth invariance in both directions, including the counterweight cases that would catch an over-reach:
src/notes.md,my-notes/dist-plan.md,src/.DS_Store.bak,src/eslintcache)src/.env,dist/app.env,logs/prod.key)dist,out,target,build,coverage,logs)logs/decisions.logasymmetry is unchanged'','/','./') still fails safe toprotectedThe policy module had no test file of its own — its only coverage was through the probe's wrapper. This adds one, which is also what keeps
worktree-ignored-probe.test.ts(294 lines) off the repo's 350-line ceiling.worktree-ignored-patterns.tsis 202 lines and the new test file is 141.Scope
H1 only. From the same review:
orphaned-dirbranch removes recursively with no dirty check, no probe, and no age gate) is already tracked as worktree sweep: the orphaned-dir path removes recursively with no dirty check, no ignored-file probe, and no age gate #794 and is pre-existing, not from fix(worktree): stop the sweep reaping live trees and discarding ignored files (#759) #770.target/in the inspectable tier converts a slow enumeration into permanent protection, via the same never-auto-reaped state as H1) are untouched here. M2 is arguably the same class of leak as H1 and is the natural next follow-up, but its fix is a policy call about which tiertarget/belongs to, not a mechanical one.