feat(worktree): seed the absent children of an existing seed dir (#235)#257
Open
Polloinfilzato wants to merge 1 commit into
Open
Conversation
WalkthroughChangesWorktree seeding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant provision_worktree
participant _copy_traversable
participant WorktreeFilesystem
participant LocalGitExclude
provision_worktree->>_copy_traversable: seed existing directory
_copy_traversable->>WorktreeFilesystem: copy absent children
_copy_traversable-->>provision_worktree: report copied or total no-op
provision_worktree->>LocalGitExclude: shield partially seeded path
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A `worktree_seed` entry naming a DIRECTORY was a total no-op whenever its destination existed — and it always does, because a worktree checks out the tracked children. The gitignored children that are absent, and would clobber nothing, were skipped along with them (bmad-code-org#230). bmad-code-org#233 made that skip visible; this makes it stop happening. `_copy_traversable` gains an opt-in `skip_existing` guard so the no-clobber property holds at FILE granularity, and returns whether it wrote anything. The guard is opt-in rather than baked in because `install_into --force-skills` rmtree's the destination precisely to overwrite it; only the seeding call passes it, so every other call site is byte-identical. Reporting follows the same rule as before, one level down: an entry that seeded even one child is applied configuration and is no longer reported skipped-whole, while an entry that still copied nothing stays reported. Per-child skips are not reported — the checkout is expected to carry its tracked children, the same routine-noise argument that excluded globs in bmad-code-org#233. A partially-seeded entry still gets its exclude pattern written, so the newly seeded children stay out of the unit's `git add -A`. `seed_globs` deliberately keeps its whole-entry behaviour: bmad-code-org#235 scopes this to `worktree_seed`. Fixes bmad-code-org#235.
Polloinfilzato
force-pushed
the
feat/seed-absent-children-of-existing-dir
branch
from
July 22, 2026 16:26
1524b45 to
6361515
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/install.py`:
- Around line 403-410: Update _copy_traversable’s directory branch to initialize
copied based on whether dst existed before mkdir, so creating a missing empty
directory returns True; retain child propagation for non-empty directories. Add
a regression test covering an empty child directory created during
provision_worktree and verify it is reported as applied rather than skipped.
- Around line 578-585: The _copy_traversable call for directory sources must
skip existing file targets without recursing. Before recursing, distinguish
destination types: treat an existing non-directory dst as a skipped no-op, and
invoke _copy_traversable only when dst is absent or dst.is_dir(), preserving
skip_existing no-clobber behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79007fc3-586d-4d5e-af10-87d922570307
📒 Files selected for processing (3)
docs/FEATURES.mdsrc/bmad_loop/install.pytests/test_install.py
Comment on lines
403
to
+410
| if src.is_dir(): | ||
| dst.mkdir(parents=True, exist_ok=True) | ||
| # `any(...)` would short-circuit and skip the remaining children. | ||
| copied = False | ||
| for child in src.iterdir(): | ||
| _copy_traversable(child, dst / child.name) | ||
| elif isinstance(src, Path): | ||
| if _copy_traversable(child, dst / child.name, skip_existing=skip_existing): | ||
| copied = True | ||
| return copied |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Count newly created empty directories as copied.
A missing empty child directory is created by mkdir, but copied remains False; provision_worktree then reports the entry as skipped despite modifying the worktree. Initialize copied from whether dst existed before mkdir, and add an empty-directory regression test.
Based on PR objectives, an entry that seeds a missing child must be treated as applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bmad_loop/install.py` around lines 403 - 410, Update _copy_traversable’s
directory branch to initialize copied based on whether dst existed before mkdir,
so creating a missing empty directory returns True; retain child propagation for
non-empty directories. Add a regression test covering an empty child directory
created during provision_worktree and verify it is reported as applied rather
than skipped.
Comment on lines
+578
to
+585
| if not src.is_dir(): | ||
| skipped.append(str(rel)) | ||
| continue | ||
| # A DIRECTORY whose destination exists is the case #230 reported: the | ||
| # checkout carries some tracked child, so the whole entry used to be a | ||
| # no-op — including the gitignored children that are absent and would | ||
| # clobber nothing. Recurse instead, copying only what is missing. | ||
| if not _copy_traversable(src, dst, skip_existing=True): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not recurse into an existing file target.
dst.exists() does not establish that dst is a directory. If the seed source is a directory but the worktree already has a file at that path, _copy_traversable calls dst.mkdir(..., exist_ok=True) and raises FileExistsError. Treat non-directory destinations as skipped no-ops; recurse only when dst.is_dir().
Proposed fix
if dst.exists():
- if not src.is_dir():
+ if not src.is_dir() or not dst.is_dir():
skipped.append(str(rel))
continueBased on PR objectives, recursive copying applies to existing destination directories while preserving no-clobber behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not src.is_dir(): | |
| skipped.append(str(rel)) | |
| continue | |
| # A DIRECTORY whose destination exists is the case #230 reported: the | |
| # checkout carries some tracked child, so the whole entry used to be a | |
| # no-op — including the gitignored children that are absent and would | |
| # clobber nothing. Recurse instead, copying only what is missing. | |
| if not _copy_traversable(src, dst, skip_existing=True): | |
| if not src.is_dir() or not dst.is_dir(): | |
| skipped.append(str(rel)) | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bmad_loop/install.py` around lines 578 - 585, The _copy_traversable call
for directory sources must skip existing file targets without recursing. Before
recursing, distinguish destination types: treat an existing non-directory dst as
a skipped no-op, and invoke _copy_traversable only when dst is absent or
dst.is_dir(), preserving skip_existing no-clobber behavior.
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.
What
worktree_seedentries naming a directory now seed the children the worktree is missing,instead of skipping the entry whole. Option B from #230, as scoped in #235.
Why
A worktree checks out tracked files, so a seed directory with any tracked child always exists —
and copy-when-absent then skipped the entry entirely, including the gitignored children that are
absent and would clobber nothing. #233 made that skip visible; this makes it stop happening.
How
_copy_traversablegains an opt-inskip_existingguard, so no-clobber holds at filegranularity, and returns whether it wrote anything.
Opt-in rather than baked into the helper because
install_into --force-skillsrmtrees thedestination precisely to overwrite it (
install.py:455); a guard in the helper would havesilently regressed it. Only the seeding call passes the flag — the other four call sites are
byte-identical, and
test_install_skills_forcestill pins the force path. The guard sits aheadof both write branches, so it holds for the zip-imported
Traversablesource too, not justshutil.copy2.Reporting follows the rule from #233 one level down:
the same routine-noise argument that excluded globs in fix: report worktree_seed entries skipped as no-ops #233;
patterns, so the children just written stay out of theunit's
git add -A. Excluding the whole dir is safe: an exclude does not untrack the trackedchildren that were already there.
test_provision_worktree_reports_seed_dir_skipped_wholecarried a# documents today's behaviourassertion; it is inverted and renamed here, which is the behaviour change #235 askedfor.
test_provision_worktree_seed_does_not_clobber_existingpasses unmodified.Testing
LC_ALL=C LANG=C).trunk fmt && trunk check --no-progressclean.install.pystashed, 5 of the 5 new tests fail (the6th,
reports_seed_dir_with_nothing_to_copy, passes either way by design — it pins theunchanged case).
total no-op still reported; exclude pattern written for a partial seed in a real git worktree;
skip_existingon the zipTraversablebranch; theFalsereturn on a total no-op.Note, not addressed here
seed_globshas the same whole-entry behaviour for a directory match (install.py:572). I leftit alone deliberately — #235 scopes this to
worktree_seed, and changing the glob path wouldalter behaviour for plugin-authored seeds that were not part of the report. Happy to extend it
in a follow-up if you'd rather the two paths share one semantic.
Also carrying forward your non-blocking note on #233: the skipped-entry signal is not specific to
hand-written
worktree_seedentries, since the seed list also carries adapter-profileseed_files. I have not repeated that claim here.Fixes #235.