fix(validate): accept merged bmad-review, drop unreleased verification-gap requirement (#260)#283
Conversation
…n-gap requirement (#260) `missing_base_skills` required `bmad-review-verification-gap`, which no tagged BMAD-METHOD release ships: it is absent from v6.10.0 (whose step-04 invokes only adversarial-general + edge-case-hunter by name), and post-6.10.0 it exists only as a thin forwarder to the merged `bmad-review`. So `validate` — and the shared run/resume/sweep preflight — could not pass on any real install, and the message misdiagnosed it as "install the BMad Method (bmm) module" on projects where bmm was installed. - DEV_BASE_SKILLS drops verification-gap; it stays in BASE_SKILLS so a hand-installed forwarder is still copied into a worktree, but is never preflighted. - A tree carrying the merged `bmad-review` skill satisfies the review layers on its own (checked per tree), so the standalone hunters are skipped there. - base-missing messages now name the actual remedy: install/update bmm, BMAD-METHOD >= 6.10.0. Check ids, severities, and the preflight's abort-on-any-problem are unchanged.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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)
Walkthrough
ChangesDerived review-layer validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant cmd_validate
participant missing_base_skills
participant resolve_review_layers
participant provision_worktree
User->>cmd_validate: run validate
cmd_validate->>missing_base_skills: check base skills
missing_base_skills->>resolve_review_layers: derive configured review skills
resolve_review_layers-->>missing_base_skills: required and advisory findings
missing_base_skills-->>cmd_validate: validation result
cmd_validate->>provision_worktree: prepare isolated worktree
provision_worktree->>resolve_review_layers: resolve project layers
resolve_review_layers-->>provision_worktree: skill set and customization
provision_worktree-->>User: isolated worktree ready
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@docs/FEATURES.md`:
- Line 178: Update the earlier review-stage description for bmad-loop validate
to reflect the v6.10.0 two-layer topology: require only
bmad-review-adversarial-general and bmad-review-edge-case-hunter, or the merged
bmad-review skill when it supersedes them, and remove
bmad-review-verification-gap as a required prerequisite.
🪄 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: f7f9d156-dbfe-4405-9460-6c8d38211c4f
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mddocs/FEATURES.mddocs/setup-guide.mdsrc/bmad_loop/cli.pysrc/bmad_loop/install.pytests/test_install.py
|
Correction to the topology summary above: post-consolidation From
This does not change the fix: three of the four layers resolve to the single merged Re-verified the core premise across tags: no tag from v6.3.0 through v6.10.0 ships a standalone Known gap, pre-existing and out of scope here: a project whose |
…dev-auto (#260) A fixed catalog of review skills is wrong for at least one real topology no matter how it is written, because which reviewers the review step invokes is a property of the installed bmad-dev-auto, not of this package. Read it instead: - customize.toml [[workflow.review_layers]] when present — each layer's instruction names the skill it hands off to. Honors layers disabled with an empty instruction, and _bmad/custom/bmad-dev-auto.toml overrides (arrays of tables merge by id). - otherwise the reviewers step-04-review.md names inline (the v6.10.0 shape). - if neither can be read, fall back to the previous static requirement, which both mainstream topologies satisfy. This closes the gap where a project whose configured layers invoke a skill it does not have passed the preflight and then failed on every dev run — reported as a new `skills.review-layer-missing` problem. It also restores protection the previous commit dropped: a project whose layers name bmad-review-verification-gap requires it again, derived from its own config rather than demanded of everyone. The regex is deliberately narrow (backticked, bmad- prefixed): a false match here would be a false FAIL, which is the bug #260 reported.
|
Pushed The preflight no longer holds projects to a fixed catalog of review skills. Which reviewers the review step invokes is a property of the installed
Missing derived skills are reported as a new This also corrects something the first commit got wrong. Dropping Verified against the real upstream files, not fixtures —
Seven new tests cover the layer-driven, pre-consolidation, disabled-layer, project-override, and unreadable-config paths. Full suite 2938 passed / 7 skipped; |
Comprehensive review findingsThe central fix is directionally correct for both official BMAD topologies: the v6.10.0 two-hunter layout and current High severity
Medium severity
Recommended remediation plan
Verification performedReviewed head
The green suite confirms the two official happy-path topologies, but it does not exercise the runtime-parity and customization cases above. |
…oves (#283 review) The #260 fix made the preflight derive review skills from the installed bmad-dev-auto. That made it smarter than the provisioner: validate read project overrides and accepted skills provision_worktree never copied, resolving a layer set an isolated run does not. Resolver, against BMAD-METHOD src/scripts/resolve_customization.py: - port the array-of-tables merge exactly (keyed merge only when every combined item shares an identifier, code before id; append otherwise). Keying on id alone dropped base layers one way and kept superseded ones the other. - an unparseable override no longer abandons derivation: the resolver skips it and carries on, so we do too, reporting skills.customize-unreadable. - a workflow key of the wrong TOML shape no longer raises AttributeError out of validate/run/resume/sweep. Findings: - skills.review-layers-empty when every layer is disabled (step-04 HALTs blocked with 'no active review layers'). - skills.review-layer-unresolved warnings for what cannot be decided statically: when-gated layers, and handoff phrasings this check cannot confirm. Warnings never block. - validate and _require_base_skills now branch on severity, so an advisory can no longer abort a run. Parity: provision_worktree copies the review skills the project's own layers name, and seeds _bmad/custom/ (whose *.user.toml the upstream installer gitignores) so the worktree resolves what was validated. 21 new tests. Suite 2959 passed / 7 skipped; trunk check --no-fix and pyright 1.1.411 clean.
Review remediation — verdicts and fixes (
|
| # | Finding | Verdict | Fix |
|---|---|---|---|
| 1 | Dynamic reviewers not copied into worktrees | Confirmed — gap introduced by d1f39f2 |
provision_worktree copies the union of BASE_SKILLS and the skills the tree's own layers name |
| 2 | Personal overrides validated but not seeded | Confirmed, broader than reported | _bmad/custom/ seeded into each worktree |
| 3 | Override merge diverges from BMAD's resolver | Confirmed, understated — 3 divergences | Ported the real algorithm |
| 4 | All layers disabled passes preflight | Confirmed verbatim upstream | New skills.review-layers-empty problem |
| 5 | Wrong-shape TOML crashes preflight | Confirmed, reproduced | _layers_of guards each hop |
| 6 | Detection depends on one prose phrase | Confirmed — remedy changed | Warning, not problem |
| 7 | No topology/override/worktree test matrix | Confirmed | 21 new tests |
Corrections
F3 was understated. The resolver is src/scripts/resolve_customization.py. _detect_keyed_merge_field runs on the combined array, requires every item to carry the same key, and checks code before id; anything else plain-concatenates. Keying on id alone diverges three ways, not one:
- mixed/key-less arrays — upstream appends the whole override, we replaced by id → dropped a base layer the run keeps (the reported case, a false PASS);
- all-
codearrays — upstream replaces bycode, we appended both → required a skill the run never invokes (a false FAIL, the opposite direction); - items carrying both keys — upstream keys on
code, we keyed onid.
The merge is now a line-for-line port, cited by path in the docstring.
F2 is broader, and its stated mechanism is only half of it. The upstream installer writes _bmad/custom/.gitignore = *.user.toml (tools/installer/core/installer.js:687), so on a stock install only the personal override is untracked — the team bmad-dev-auto.toml is tracked and present in a worktree. But projects routinely gitignore _bmad/ wholesale (this repo does — .gitignore:12), in which case a worktree has no _bmad at all and resolves base layers only, ignoring both override layers validate just read. Seeding the directory covers both.
F6's proposed remedy would have re-created the bug this PR fixes. Turning an unrecognized backticked bmad-* reference into a problem means a prose mention, or an override handing off to something that is not a skill, blocks every run — #260's exact failure mode. It is now a warning. (The concern itself is real: upstream already writes use the `bmad-code-review` skill elsewhere, which the narrow regex misses.)
Malformed override handling also diverged, which the findings implied but did not state. _read_toml returning None made derivation bail to the static catalog; upstream warns and treats the file as empty, still resolving every other layer. We now match upstream and report skills.customize-unreadable.
Two findings not in the review
A1 — when-gated layers were hard-required. step-04-review.md skips "every layer whose when condition (if present) does not hold". The preflight ignored when and hard-required the layer's skill, so an override that gates a layer produced a false FAIL. The condition is model-evaluated against the diff and undecidable statically → advisory.
A2 — severity was not honoured, which blocked the whole remedy. cli.py:504 aborted the run when missing_base_skills returned anything at all. Any new warning would have hardened into a hard run abort, so this had to land first. _require_base_skills now partitions by severity (warnings print, only problems abort), and cmd_validate gates its ok line on problems rather than emptiness.
On the recommended remediation plan
Steps 1–6 are implemented. One deliberate deviation: step 5 asked for explicit findings on unresolvable skill references, which is step 6's remedy — those emit as warnings, per above.
Deliberately out of scope: seeding all of _bmad/ (the resolver script, bmm/config.yaml). That is the separate concern of the still-open #155. _bmad/custom/ alone is sufficient for layer-resolution parity, because the skill's documented fallback when resolve_customization.py is unavailable reads those same three TOMLs by hand (bmad-dev-auto/SKILL.md:81-89).
Verification
- Full suite 2959 passed, 7 skipped (was 2938 — +21 tests).
trunk check --no-fixclean;uvx pyright@1.1.411— 0 errors, 0 warnings.- New tests were checked to actually discriminate the old behavior, not merely pass. One draft case (mixed
code/idarrays) resolved identically under both implementations and was replaced with the case that does discriminate: an override carrying one id-less item, where the old id-keyed merge silently dropped a base layer the run still executes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bmad_loop/install.py (1)
431-432: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale docstring cross-reference.
missing_base_skillsdocuments that review skills are read via:func:_configured_review_skills``, but no such function exists — the derivation is done byresolve_review_layers/`_review_findings`. Point the reference at the real symbol so a reader (or a Sphinx build) isn't sent chasing a name that isn't defined.📝 Suggested docstring fix
- The review skills are read from the installed bmad-dev-auto itself - (:func:`_configured_review_skills`) so the preflight requires what this project + The review skills are read from the installed bmad-dev-auto itself + (:func:`resolve_review_layers`) so the preflight requires what this project will really invoke: a tree whose configured layers call the merged🤖 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 431 - 432, Update the missing_base_skills docstring to replace the nonexistent _configured_review_skills cross-reference with the actual review-skill derivation symbol, resolve_review_layers or _review_findings, using the symbol that performs the documented behavior.
🤖 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 `@src/bmad_loop/install.py`:
- Around line 431-432: Update the missing_base_skills docstring to replace the
nonexistent _configured_review_skills cross-reference with the actual
review-skill derivation symbol, resolve_review_layers or _review_findings, using
the symbol that performs the documented behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0138f9c-a8c1-4f1b-a6ad-3f444670287b
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mddocs/FEATURES.mddocs/setup-guide.mdsrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/install.pysrc/bmad_loop/worktree_flow.pytests/test_cli.pytests/test_install.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/FEATURES.md
- README.md
…w_layers (#283 review)
1 participant
Closes #260.
The bug
bmad-loop validate— and the sharedrun/resume/sweeppreflight — hard-failed unlessbmad-review-verification-gapwas present in every active CLI skill tree, and told the user to "install the BMad Method (bmm) module" on projects where bmm was installed. On a stock latest-release install the loop could not start at all.Upstream topology (verified against
bmad-code-org/BMAD-METHOD, 2026-07-23)bmad-dev-auto(with itscustomize.toml),bmad-review-adversarial-general, andbmad-review-edge-case-hunter. It does not shipbmad-review-verification-gap, and itsstep-04-review.mdinvokes only those two hunters by name — never verification-gap.main(unreleased) ships the mergedbmad-reviewskill, which provides every review lens itself; the three hunter IDs are thin forwarders to it, and step-04 is layer-driven ({workflow.review_layers}), naming no hunter.So the required skill was unsatisfiable in every real topology, and its absence causes no runtime failure on the only release that satisfies the loop's other requirements.
The fix
DEV_BASE_SKILLS(the preflight catalog) dropsbmad-review-verification-gap. It stays inBASE_SKILLSso a hand-installed forwarder is still carried into a worktree copy-if-present — it is simply never validated.MERGED_REVIEW_SKILL/_REVIEW_LAYER_SKILLS: a tree carryingbmad-review/SKILL.mdsatisfies the review layers on its own, so the standalone hunters are skipped there. Computed per tree, since a project can have a post-consolidation.claudetree and a pre-merge.agentsone side by side.bmad-dev-autostays hard-required — the merged reviewer never substitutes for the dev primitive.skills.base-missingmessages are rewritten to be actionable and to stop claiming bmm is uninstalled: they name the remedy (install or update bmm, BMAD-METHOD >= 6.10.0) and mention the consolidatedbmad-reviewalternative.Deliberately unchanged: check ids (
skills.base-missing/skills.base-incomplete),"problem"severity,_require_base_skills' abort-on-any-problem,checks.py, andworktree_flow.py. No init-time vendoring of upstream skills.Behavior, on real skill trees
bmad-review, no standalone hunters)bmad-review-edge-case-hunter, nobmad-review)Docs
docs/setup-guide.mdnow states the minimum BMAD-METHOD: v6.10.0 for sprint mode and explains the merged-bmad-reviewpath;README.mdanddocs/FEATURES.mdno longer claimvalidatepreflights "all four upstream skills" or list verification-gap as a prerequisite. CHANGELOG entry under[Unreleased] → Fixed.Tests
tests/test_install.py: existing absent/detail tests updated for the 3-skill catalog and the new wording, and the verification-gap sub-case inverted (removing it from a complete tree must now pass). Three new tests: mergedbmad-reviewsatisfies the review layers (and still doesn't substitute forbmad-dev-auto), the v6.10.0 shape validates with neither escape hatch present, and a genuinely missing hunter is still reported with a message that does not falsely claim bmm is uninstalled.Full suite: 2931 passed, 7 skipped.
trunk check --no-fix: clean.Summary by CodeRabbit