fix(worktree): seed and verify isolated BMAD surfaces - #449
Conversation
![greptile-apps[bot]](https://cdn.github.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughWorktree provisioning now seeds BMAD content, renderer files, central configuration, and required skills. It verifies delivery, protects seeded paths, journals worktree events, and escalates incomplete required surfaces before adapter dispatch. ChangesWorktree provisioning and isolated execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorktreeFlow
participant Provisioning
participant Journal
participant AdapterSession
WorktreeFlow->>Provisioning: create worktree and seed required surfaces
Provisioning-->>WorktreeFlow: return completeness and delivery results
WorktreeFlow->>Journal: record worktree-opened and undelivered seeds
alt required support is incomplete
WorktreeFlow->>Journal: record escalation
else required support is complete
WorktreeFlow->>AdapterSession: dispatch isolated story
end
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/bmad_loop/worktree_flow.py (1)
814-856: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
escalate_unitfor the two new escalation blocks.Both blocks repeat the exact body of
escalate_unit(lines 1036-1048): setPhase.ESCALATED, journalstory-escalated, callgates.notifywith the same title and resume suffix,_save, then_pause. Three copies of the same sequence will drift.escalate_unitalready raises through_pause, so the call sites keep their control flow.♻️ Proposed refactor
- task.phase = Phase.ESCALATED - self.journal.append("story-escalated", story_key=task.story_key, reason=reason) - gates.notify( - self.policy, - self.run_dir, - f"CRITICAL escalation: {task.story_key}", - f"{reason} — resolve, then `bmad-loop resume {self.state.run_id}`", - ) - self._save() - self._pause(reason, task.story_key) + self.escalate_unit(task, reason) # always raises RunPausedApply the same replacement to the renderer-surface block.
🤖 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/worktree_flow.py` around lines 814 - 856, Replace the duplicated escalation sequences in the absent_skills and short_surface blocks with calls to the existing escalate_unit helper. Pass each block’s computed reason and task.story_key so the helper preserves the existing phase update, journaling, notification, save, and pause behavior, leaving the surrounding detection logic unchanged.tests/test_install.py (1)
6693-6708: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the
projectfixture for theprovision_worktreetests.These tests call
provision_worktreeagainst a plaintmp_pathdirectory that is not a git repository. The shield write then takes the silent skip arm of_worktree_local_exclude, so the exclude behavior is never exercised for the non-monkeypatched cases. The coding guidelines ask tests to use the conftest sandbox fixtures, especiallyproject, instead of hand-rolled temporary repositories.As per coding guidelines: "Use conftest sandbox fixtures, especially
project, rather than hand-rolling temporary repositories or touching the template repository."Also applies to: 6782-6796
🤖 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 `@tests/test_install.py` around lines 6693 - 6708, Update the affected provision_worktree tests, including test_render_shield_is_omitted_when_root_shield_subsumes_it and the related case near the referenced lines, to use the conftest project fixture instead of constructing repositories with tmp_path and _write_worktree_renderer_surface. Pass the fixture’s repository/worktree paths into provision_worktree while preserving each test’s existing assertions and setup behavior.Source: Coding guidelines
🤖 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 `@tests/test_stories_e2e.py`:
- Around line 679-683: The .gitignore updates must ensure appended patterns
start on a new line. In tests/test_stories_e2e.py lines 679-683, normalize the
existing text to end with a newline before appending BMAD_SCRIPTS_SEED_REL and
CENTRAL_CONFIG_REL; in tests/test_engine_worktree.py lines 194-199, update
ignore_before_commit to add a newline to existing only when it is non-empty and
lacks one.
---
Nitpick comments:
In `@src/bmad_loop/worktree_flow.py`:
- Around line 814-856: Replace the duplicated escalation sequences in the
absent_skills and short_surface blocks with calls to the existing escalate_unit
helper. Pass each block’s computed reason and task.story_key so the helper
preserves the existing phase update, journaling, notification, save, and pause
behavior, leaving the surrounding detection logic unchanged.
In `@tests/test_install.py`:
- Around line 6693-6708: Update the affected provision_worktree tests, including
test_render_shield_is_omitted_when_root_shield_subsumes_it and the related case
near the referenced lines, to use the conftest project fixture instead of
constructing repositories with tmp_path and _write_worktree_renderer_surface.
Pass the fixture’s repository/worktree paths into provision_worktree while
preserving each test’s existing assertions and setup 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 Plus
Run ID: 24796a27-45fc-4ca7-ade8-e26691fb736c
📒 Files selected for processing (4)
src/bmad_loop/worktree_flow.pytests/test_engine_worktree.pytests/test_install.pytests/test_stories_e2e.py
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
![chatgpt-codex-connector[bot]](https://cdn.github.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: 3e0f8e59ac
ℹ️ 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 contained(dst, worktree) and (_is_file(dst) or _is_dir(dst)): | ||
| continue |
There was a problem hiding this comment.
When a seed_files or glob entry names a directory and provisioning copies at least one child but refuses another (for example, a child symlink resolving outside the repo), the destination directory now exists, so this condition marks the entire entry delivered. Consequently run_isolated emits no worktree-seed-dropped event even though the session is missing part of the requested config—the exact partial-copy case this new result-side check needs to expose.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0fb7c23. worktree_seed_undelivered now verifies destination type and containment for every usable source descendant, including directory visits, so a partial literal or glob directory seed is reported as worktree-seed-dropped. Added regression coverage for both seed_files and seed_globs; both witnesses failed on 3e0f8e5 and pass on the new head. Fresh CI is green.
| dst_scripts = worktree.joinpath(*BMAD_SCRIPTS_SEED_REL.split("/")) | ||
| return any( | ||
| (_is_file(src) or _is_dir(src)) and not _is_file(dst_scripts.joinpath(*rel.split("/"))) | ||
| for rel, src in _walk_traversable_files(scripts) | ||
| ) |
There was a problem hiding this comment.
For a renderer-backed project, this walks every file under _bmad/scripts, not just the required renderer unit. If an unrelated script—or config_utils.py when render_skill.py no longer imports it—is symlinked outside the repo or otherwise cannot be copied, the required renderer entry/config can still be fully usable, but this returns true and run_isolated CRITICAL-escalates before dispatch. The check should follow the same content-keyed required-file predicate used by renderer preflight rather than treating every sibling script as mandatory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0fb7c23. _bmad_scripts_seed_incomplete now checks only RENDERER_SCRIPT_UNIT_REL entries accepted by _renderer_unit_required, matching the preflight’s content-keyed contract. Added regression coverage for an unused config_utils.py and an unrelated sibling script; both witnesses failed on 3e0f8e5 and pass on the new head. Fresh CI is green.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fb7c231ac
ℹ️ 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".
| def _required_worktree_skills(repo_root: Path, tree: str) -> tuple[str, ...]: | ||
| """Every upstream skill the dev/review sessions in ``tree`` may invoke.""" | ||
| resolved = resolve_review_layers(repo_root, tree) | ||
| return tuple(dict.fromkeys((*BASE_SKILLS, *(resolved.skills() if resolved else ())))) |
There was a problem hiding this comment.
Because BASE_SKILLS is explicitly a copy-if-present superset, including obsolete standalone reviewers and the merged reviewer, returning all of it here also feeds those inactive skills into base_skills_seed_incomplete and the fatal pre-dispatch gate. For example, a modern project whose resolved layers invoke only bmad-review but retain an old bmad-review-edge-case-hunter symlink to a shared install will pass preflight, fail to copy only that unused skill, and then be escalated even though the session never invokes it. Keep the broad set for best-effort copying, but construct the fatal completeness set from the resolved primitive and actually required review layers rather than every present catalog entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e263866. Provisioning now uses the broad BASE_SKILLS plus resolved required/advisory set only for best-effort copying, while base_skills_seed_incomplete gates the selected primitive plus resolved.required. Unknown resolution mirrors missing_base_skills exactly: a present merged reviewer is required, otherwise the standalone fallback reviewers are required. Added modern obsolete-symlink, advisory-copy/nonfatal, and merged/standalone fallback regressions; the affected witnesses failed on 0fb7c23 and pass on the new head. Fresh CI run 30850459542 is green across Linux and Windows.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_install.py`:
- Around line 6751-6771: Add the existing POSIX/symlink skip marker before
test_base_skills_seed_incomplete_ignores_inactive_catalog_symlink, and apply the
same marker to
test_base_skills_seed_incomplete_preserves_unknown_review_fallback, so both
symlink-dependent catalogue tests are skipped on Windows while retaining their
current behavior on supported platforms.
🪄 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 Plus
Run ID: 451bf4ec-6840-43ea-8688-019c13aaea3f
📒 Files selected for processing (4)
src/bmad_loop/worktree_flow.pytests/test_engine_worktree.pytests/test_install.pytests/test_stories_e2e.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_stories_e2e.py
- tests/test_engine_worktree.py
- src/bmad_loop/worktree_flow.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e263866d8e
ℹ️ 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 (_is_file(src) or _is_dir(src)) | ||
| and not _is_file(worktree_skill.joinpath(*rel.split("/"))) |
There was a problem hiding this comment.
When an otherwise required skill contains an unreferenced auxiliary file or symlink that provisioning cannot copy—for example, a README symlinked to a shared install—this predicate marks the skill incomplete and run_isolated CRITICAL-pauses before dispatch. missing_base_skills only makes the review skill's SKILL.md and the primitive's defined markers and renderer sources mandatory, so absence of an unrelated descendant does not prove the session will stall; restrict the fatal completeness check to that same required-file contract while leaving other descendants best-effort.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8f3c1c0. base_skills_seed_incomplete now gates only the active contract: SKILL.md for the selected primitive and required reviewers, plus defined primitive markers and renderer snapshot sources. Unreferenced descendants remain best-effort. Stories mode separately re-runs missing_stories_support against the mounted worktree so missing or stale folder+id routers still pause. Added regressions for unreferenced auxiliary symlinks and required marker/snapshot omissions. Clean full suite: 4036 passed, 30 skipped.
| if _is_file(src): | ||
| return _is_file(dst) |
There was a problem hiding this comment.
When a seed points through an out-of-repo symlink but the worktree already contains a file at that path, provisioning refuses the escaped source, yet this arm reports delivery solely because the destination is a file. For example, a tracked stale config in the worktree suppresses worktree-seed-dropped even though the main checkout's current shared config was never copied, so the session silently uses different configuration; reject escaped sources before accepting destination existence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8f3c1c0. Delivery now rejects a top-level source outside repo_root before accepting any stale destination, and recursive directory verification applies the same containment rule to every visited file and directory. Regressions cover a stale top-level destination plus nested escaped file and directory sources. Clean full suite: 4036 passed, 30 skipped.
| if seeded_bmad: | ||
| skipped = [rel for rel in skipped if not _is_under_bmad(rel)] |
There was a problem hiding this comment.
_bmad no-op seed reports
When internal BMAD seeding copies any file, this removes every skipped user-authored seed beneath _bmad, even if the copied file is unrelated. For example, a tracked _bmad/config.toml seed is a genuine no-op, but if ignored _bmad/scripts files are seeded afterward its entry disappears and run_isolated emits no worktree-seed-skipped event; because the internal no-clobber copy cannot have filled an already occupied skipped entry, retain these reports and remove only the reserved internal sentinels handled immediately below.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8f3c1c0. The broad _bmad skipped-entry filter was removed; provisioning now suppresses only the reserved renderer sentinels handled by the dedicated result checks. The regression confirms an unrelated user-seed no-op remains reported even when internal BMAD seeding successfully lands a sibling. Clean full suite: 4036 passed, 30 skipped.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_stories_engine.py (1)
1109-1111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the per-parameter finding to keep the two branches distinguishable.
STORIES_PROBE_FILEappears in the pause reason for bothmissingandstale. The assertion therefore passes even if the two shapes collapse into one finding, which is the exact distinction this parametrization exists to hold. Assert the stale marker fragment in thestalebranch.Also confirm the negative assertion
adapter.sessions == []fails when the stories gate is removed, as the guidelines require for refusal tests.♻️ Proposed change to pin each finding shape
assert summary.paused and adapter.sessions == [] - assert STORIES_PROBE_FILE in (engine.state.paused_reason or "") + reason = engine.state.paused_reason or "" + assert STORIES_PROBE_FILE in reason + # the tracked-but-stale worktree copy is reported by MARKER, the untracked one + # by absence — a collapse of the two shapes must not pass this test. + assert (STORIES_PROBE_TEXT in reason) is (checkout_probe == "stale") assert Path(engine.state.tasks["1"].worktree_path).is_dir()As per coding guidelines: "For tests asserting that something is refused or absent, remove the gating code and verify the test fails before trusting the negative assertion."
🤖 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 `@tests/test_stories_engine.py` around lines 1109 - 1111, The parametrized pause-reason test must distinguish the missing and stale branches instead of asserting only the shared STORIES_PROBE_FILE marker. Update the stale case around the paused-reason assertions to require its stale-specific marker, while preserving the missing-case expectation; additionally verify the refusal assertion adapter.sessions == [] is meaningful by removing the stories gate and confirming that the test fails without it.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/test_stories_engine.py`:
- Around line 1109-1111: The parametrized pause-reason test must distinguish the
missing and stale branches instead of asserting only the shared
STORIES_PROBE_FILE marker. Update the stale case around the paused-reason
assertions to require its stale-specific marker, while preserving the
missing-case expectation; additionally verify the refusal assertion
adapter.sessions == [] is meaningful by removing the stories gate and confirming
that the test fails without it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b0987e9e-59e6-4692-98bb-c6a3a9683066
📒 Files selected for processing (3)
src/bmad_loop/worktree_flow.pytests/test_install.pytests/test_stories_engine.py
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
_bmad/surface into isolated worktrees, per file and without clobbering checkout content, while excluding generated render outputWorktreeFlow.run_isolated; CRITICAL-escalate only deterministic session stalls, while journaling arbitrary undelivered seeds asworktree-seed-droppedworktree-openedimmediately after assigning the mount path so a provisioning escalation leaves an inspectable location in the journalReference: #433 (Phase 6H).
D-H1 render shield
The render shield is worktree-local and transient. It is added without consulting whether
worktree/_bmadexists, so protection does not depend on provisioning order. The only suppression is structural: when/_bmadis already present in the exclude patterns, Git prunes that directory before descending, so a separate/_bmad/render/line can never be consulted. Omitting that inert line keeps the private exclude precise.Known gap
Wheel-provided
MODULE_SKILLSstill have no result-side completeness predicate. This remains explicit rather than being folded into the CRITICAL upstream-skill gate: story worktrees have a determinate dev/review consumer contract for upstream skills, but there is no equivalent required story-session consumer contract for the bundled operator/triage surface. Reusing the deterministic-stall gate would risk refusing healthy runs.Validation
uv run pytest tests/test_install.py -q: 329 passeduv run pytest tests/test_engine_worktree.py -q: 56 passeduv run pytest tests/test_stories_e2e.py -q: 14 passed.claude/skillsand.agents/skillsuv run pyright: 0 errors, warnings, or informationtrunk check --no-fix: cleangit diff --check: cleanSummary by CodeRabbit
New Features
Bug Fixes