fix: report worktree_seed entries skipped as no-ops#233
Merged
pbean merged 1 commit intoJul 21, 2026
Merged
Conversation
provision_worktree returns the seed_files entries that named a real source but were skipped because the destination already existed, and the engine journals them as worktree-seed-skipped. Copy-when-absent is right for a file the checkout legitimately carries. For a DIRECTORY entry it is skipped whole the moment any child is tracked — and a worktree checks out tracked files, so `worktree_seed = ["_bmad"]` where _bmad/custom is tracked copies nothing at all, including the children that are absent and would clobber nothing. Nothing reported it: the local `seeded` list only feeds the git-exclude patterns. Reporting rather than printing because provision_worktree is quiet by contract (it runs under the TUI), so the journal is where the signal belongs. Glob-expanded matches are deliberately excluded: a plugin's glob is expected to hit paths the checkout already carries, so that skip is routine. Behaviour is unchanged — nothing new is copied. Fixes bmad-code-org#230
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughWorktree provisioning now reports seed entries skipped because destinations already exist. Isolated runs capture these entries and append a ChangesWorktree seed reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Engine._run_isolated
participant provision_worktree
participant Journal
Engine._run_isolated->>provision_worktree: provision worktree seeds
provision_worktree-->>Engine._run_isolated: return skipped entries
Engine._run_isolated->>Journal: append worktree-seed-skipped event
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 |
pbean
approved these changes
Jul 21, 2026
pbean
left a comment
pbean
left a comment
Collaborator
There was a problem hiding this comment.
Validated line-by-line against main plus a sweep over every consumer surface — approving.
What I checked beyond the diff:
- The return-type change is inert everywhere else:
_run_isolatedis the only production caller, and no existing test consumes the return. - The new journal kind can't break a consumer: there is no closed event-name enum anywhere —
diagnosehistograms kinds via an openCounter, the TUI journal view falls back to dim styling for unknown kinds, and the CLI resume readers only special-case their own kinds. No test pins an exact journal-kind sequence. Indiagnose,entriesroutes throughsanitize.scrub_jsonand relative seed paths don't trip the leak guard. - No false-positive path: the one thing that could have made this event misleading is a re-provision of an already-seeded worktree reporting every entry as skipped. That can't happen — resume reconstructs the workspace via
_reopen_unitwithout provisioning, andopen_unit_workspacealways mounts fresh (git worktree addrefuses an existing path; that failure defers withworktree-open-failed). Every reported skip is genuine. - Cross-platform: a pure
pathlibbranch split, and reporting the TOML-authored relative entries (rather than OS-native paths) is the right call. Full suite green locally (2694 passed) and on CI's Linux + real-Windows legs.
Two non-blocking notes:
- The signal isn't quite "specific to hand-written
worktree_seedentries": the seed list the engine passes also carries adapter-profileseed_files(underscm.seed_adapter_defaults, e.g..mcp.jsonfrom the claude profile) and plugin seeds — so a project that tracks one of those files journals a skip for it once per story. Benign and honest; just noting the commit message's claim only holds vs. globs. Tagging entry origin could be a future refinement if this ever gets noisy. - Nothing load-bearing enumerates journal kinds, so no doc change is required.
docs/tui-guide.md's journal section is the natural home for a one-liner if you want one — equally happy to take that as a follow-up.
On your two offers: filing both as follow-up issues — one for the option-B shape from #230 (per-child copy-when-absent), which is yours if you'd like it; and one for the locale failure you hit. On the latter, it's better than you thought: the English match lives in production, not the test — safe_rollback's preserve-restore tolerates the benign checkout failure by substring-matching "did not match", so under a localized git that benign no-op raises GitError in the rollback path. Good catch.
Thanks for an unusually well-argued issue + PR pair.
This was referenced Jul 21, 2026
Merged
Merged
dracic
pushed a commit
to dracic/bmad-auto
that referenced
this pull request
Jul 22, 2026
…atching English (bmad-code-org#236) safe_rollback tolerated a benign `git checkout <snapshot> -- <preserve-dir>` no-op (a preserve dir holding only untracked files) by substring-matching git's English "pathspec ... did not match" message. `_run_git` ran git with the inherited environment, so under a localized git (e.g. LANG=it_IT.UTF-8) the message is translated, the substring never matches, and the benign no-op raises GitError — turning a resolvable re-drive into the rollback-pause path. Force LC_ALL=C at the sole git spawn point (_run_git) so every message the orchestrator inspects stays stable English, making the whole class of locale-fragile parsing impossible rather than patching the one instance. The locale is merged last so it wins over both the inherited environment and the explicit env the two _git_env callers pass (their throwaway GIT_INDEX_FILE / synthetic commit identity vars are preserved by the spread). Regression coverage spies on the actual subprocess env rather than the parent LANG, so the guarantee holds even on a CI box with no non-English locale catalogs installed (where a plain LANG=it_IT test would emit English anyway and pass without the fix). Refs bmad-code-org#233.
Polloinfilzato
added a commit
to Polloinfilzato/bmad-loop
that referenced
this pull request
Jul 22, 2026
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.
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
2 participants
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
provision_worktreenow returns theseed_filesentries it skipped because the destinationalready existed, and the engine journals them as
worktree-seed-skipped.No behaviour change — nothing new is copied.
Why
Fixes #230.
Copy-when-absent is right for a file the checkout legitimately carries, and
test_provision_worktree_seed_does_not_clobber_existingstates the reason: no diff is mergedback.
For a directory entry the same rule is over-broad. A worktree checks out tracked files, so a
seed directory with any tracked child already exists — and the entry is then skipped whole,
including the children that are absent and would clobber nothing.
worktree_seed = ["_bmad"]with
_bmad/customtracked is the natural value to write, and it copies nothing at all.Nothing reported it: the local
seededlist only feeds the git-exclude patterns, so the skipleft no trace in stdout, the journal, or
validate. The configuration reads as applied and isnot. In my case the isolated sessions re-created the missing tree themselves, once per story, so
every run stayed green while paying for the repair.
How
provision_worktreegained alist[str]return: entries whose source existed but whosedestination did too.
worktree-seed-skipped, alongsideworktree-opened.runs inside the engine loop under a TUI, so a
printwould corrupt the display — the journalis where this signal belongs, next to
worktree-open-failedandsprint-status-unknown-keys.not src.exists()anddst.exists()are now separate branches. A missing source is adifferent case (already covered by its own test) and is not a no-op worth reporting.
seed_globspattern is expected tohit paths the checkout already carries, so that skip is routine rather than a misconfiguration.
The signal stays specific to hand-written
worktree_seedentries.I stopped at reporting rather than the alternative in #230 (recursing into an existing directory
with per-child copy-when-absent). That one changes behaviour and
_copy_traversable's filebranch would need a per-child
dst.exists()guard to keep the no-clobber property — happy tofollow up with it if you'd prefer that shape.
Testing
Three tests added in
tests/test_install.py:the case from the issue, pinned as current behaviour;
Existing seeding tests pass unchanged, including
test_provision_worktree_seed_does_not_clobber_existing, so the no-clobber guarantee is intact.uv run pytest -q→ 2678 passed, 23 skippedtrunk fmtandtrunk check→ no issuesOne unrelated note:
tests/test_engine.py::test_rollback_or_pause_resolved_auto_recoversfailson my machine because it matches git's English
"did not match"while my git is localised. Itfails the same way on a clean
mainand passes underLC_ALL=C, so it is untouched by thischange — but the suite isn't locale-independent. Happy to open that separately if it's useful.
Written with AI assistance, reviewed line by line before submitting.
Summary by CodeRabbit
Bug Fixes
Tests