Resolve the dev primitive skill dynamically (bmad-build-auto / bmad-dev-auto) - #444
Resolve the dev primitive skill dynamically (bmad-build-auto / bmad-dev-auto)#444ron332 wants to merge 1 commit into
Conversation
…ev-auto) The BMad Method (bmm) module renamed the inner dev primitive skill from bmad-dev-auto to bmad-build-auto, leaving bmad-dev-auto as a permanent one-file forwarding shim. bmad-loop still hardcoded bmad-dev-auto as the primitive everywhere (validate's marker check, review-layer resolution, customize-override paths, the stories-dispatch probe, and worktree provisioning's copy list), so `bmad-loop validate` reported a correctly installed project as `skills.base-incomplete`, and review-layer resolution was reading the forwarding shim's SKILL.md instead of the real step-04-review.md/customize.toml — a functional bug in live dev/review sessions, not just a validate false positive. Add resolve_primitive_skill(), the single place that decides which skill is a project's real primitive per tree: prefers bmad-build-auto whenever it's installed there at all, falling back to bmad-dev-auto only when build-auto is absent (pre-rename bmm installs). Every consumer now resolves through this function instead of a hardcoded name, so they can never disagree about which skill is authoritative for a given project.
WalkthroughThe installer now resolves ChangesPrimitive resolution and provisioning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Installer
participant WorktreeFlow
participant ProjectTree
CLI->>Installer: validate installed skills
Installer->>ProjectTree: resolve primitive for tree
ProjectTree-->>Installer: bmad-build-auto or bmad-dev-auto
Installer-->>CLI: primitive-specific validation result
WorktreeFlow->>Installer: request base skills for tree
Installer-->>WorktreeFlow: resolved primitive and review skills
WorktreeFlow->>ProjectTree: copy required skills into worktree
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bmad_loop/install.py (1)
154-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
base_skills_for_treefromBASE_SKILLSinstead of duplicating the composition.
base_skills_for_treehand-lists the same four fallback skills (bmad-review-adversarial-general,bmad-review-edge-case-hunter,bmad-review-verification-gap,MERGED_REVIEW_SKILL) thatBASE_SKILLSalready expresses viaDEV_BASE_SKILLS. A future change to that composition (add/remove a fallback skill) now has two places to update. Miss one, and the static catalog and the dynamic per-tree resolution silently diverge.Build the dynamic dict from
BASE_SKILLSdirectly, swapping in the resolved primitive key, to keep one source of truth for the skill list.♻️ Proposed refactor to derive from a single source of truth
def base_skills_for_tree(project: Path, tree: str) -> dict[str, tuple[str, ...]]: """:data:`BASE_SKILLS`, but with the dev primitive resolved for THIS project/tree. Same composition — the primitive plus the fallback review hunters, the merged reviewer, and the pre-consolidation verification-gap forwarder — except the primitive entry names whichever skill :func:`resolve_primitive_skill` actually finds installed, instead of always the legacy name. Worktree provisioning uses this (not the static ``BASE_SKILLS``) so an isolated run copies the SAME primitive the main repo's preflight validated — a project on `bmad-build-auto` must not have its worktree seeded with a `bmad-dev-auto` that was never there. """ primitive = resolve_primitive_skill(project, tree) - return { - primitive: PRIMITIVE_MARKERS, - "bmad-review-adversarial-general": (), - "bmad-review-edge-case-hunter": (), - "bmad-review-verification-gap": (), - MERGED_REVIEW_SKILL: (), - } + skills = dict(BASE_SKILLS) + if primitive != LEGACY_PRIMITIVE_SKILL: + del skills[LEGACY_PRIMITIVE_SKILL] + skills[primitive] = PRIMITIVE_MARKERS + return skills🤖 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 154 - 191, Update base_skills_for_tree to derive its returned mapping from BASE_SKILLS, replacing the primitive entry with the key resolved by resolve_primitive_skill while preserving PRIMITIVE_MARKERS for that entry. Do not hand-list the fallback skills; retain their existing marker values and ensure the dynamic result reflects any future changes to BASE_SKILLS.
🤖 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 154-191: Update base_skills_for_tree to derive its returned
mapping from BASE_SKILLS, replacing the primitive entry with the key resolved by
resolve_primitive_skill while preserving PRIMITIVE_MARKERS for that entry. Do
not hand-list the fallback skills; retain their existing marker values and
ensure the dynamic result reflects any future changes to BASE_SKILLS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4a749e3-82e4-4345-9f5d-23f8ac81bb9a
📒 Files selected for processing (4)
src/bmad_loop/cli.pysrc/bmad_loop/install.pysrc/bmad_loop/worktree_flow.pytests/test_install.py
Greptile SummaryThis PR fixes a functional regression caused by the
Confidence Score: 4/5Safe to merge; the core fix is correct and well-tested, with no regressions in the full 3816-test suite. The implementation logic is sound — resolve_primitive_skill correctly uses SKILL.md existence as the signal, every consumer is updated consistently, and backward-compatible exports are preserved. The minor concerns are: DEV_PRIMITIVE_SKILL is a previously-public constant dropped without a compat alias; ReviewResolution's docstring still names bmad-dev-auto after the rest of the module was updated; and missing_stories_support for the bmad-build-auto topology has no explicit test despite being a changed code path. None of these affect correctness of the fix itself. Files Needing Attention: src/bmad_loop/install.py — the dropped DEV_PRIMITIVE_SKILL export and stale ReviewResolution docstring. tests/test_install.py — missing missing_stories_support coverage for the new topology.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/install.py | Core of the fix — adds resolve_primitive_skill, base_skills_for_tree, and _customize_overrides(primitive); threads the resolved primitive through all consumers. DEV_PRIMITIVE_SKILL is dropped without a compat alias; ReviewResolution docstring retains a stale bmad-dev-auto reference. |
| src/bmad_loop/worktree_flow.py | Swaps BASE_SKILLS for base_skills_for_tree(repo_root, tree) in the worktree copy-list floor; resolve_primitive_skill is called twice for the same tree, a minor redundancy the PR's passthrough mechanism could avoid. |
| src/bmad_loop/cli.py | Documentation-only changes: user-facing messages and docstrings updated from hardcoded bmad-dev-auto to generic 'dev primitive' phrasing; no logic changes. |
| tests/test_install.py | Adds _install_build_auto, TestResolvePrimitiveSkill (5 cases), and explicit bmad-build-auto tests for missing_base_skills, resolve_review_layers, customize-override precedence, base_skills_for_tree, and provision_worktree; missing_stories_support against the new topology is not covered. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["project / tree"] --> B["resolve_primitive_skill(project, tree)"]
B -->|"bmad-build-auto/SKILL.md exists"| C["PRIMITIVE_SKILL (bmad-build-auto)"]
B -->|"absent — pre-rename or no install"| D["LEGACY_PRIMITIVE_SKILL (bmad-dev-auto)"]
C & D --> E["primitive"]
E --> F["missing_base_skills"]
E --> G["resolve_review_layers"]
E --> H["missing_stories_support"]
E --> I["_customize_overrides"]
E --> J["base_skills_for_tree"]
J --> K["provision_worktree"]
Comments Outside Diff (2)
-
src/bmad_loop/install.py, line 231-232 (link)Stale
bmad-dev-autoreference inReviewResolutiondocstringThe class-level docstring on
ReviewResolutionstill says "Which skills the installedbmad-dev-auto's review step actually invokes." — every other comparable string in the module was updated to say "dev primitive" or "resolved primitive". A contributor reading the NamedTuple in isolation gets a misleading impression that it is always tied to the legacy name, which contradicts the whole point of this PR.Prompt To Fix With AI
This is a comment left during a code review. Path: src/bmad_loop/install.py Line: 231-232 Comment: **Stale `bmad-dev-auto` reference in `ReviewResolution` docstring** The class-level docstring on `ReviewResolution` still says "Which skills the installed `bmad-dev-auto`'s review step actually invokes." — every other comparable string in the module was updated to say "dev primitive" or "resolved primitive". A contributor reading the NamedTuple in isolation gets a misleading impression that it is always tied to the legacy name, which contradicts the whole point of this PR. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
tests/test_install.py, line 535-543 (link)missing_stories_supporttest for thebmad-build-autotopologymissing_stories_supportwas updated in this PR to resolve the primitive dynamically viaresolve_primitive_skill, but the new tests only covermissing_base_skills,resolve_review_layers,base_skills_for_tree, andprovision_worktreeagainst abmad-build-autoproject. The stories probe path is only exercised viaSTORIES_PROBE_SKILL(which still resolves to"bmad-dev-auto", the pre-rename path). Adding a counterpart test that installsbmad-build-autoand callsmissing_stories_supportwould mirror the coverage pattern used for all other functions in this change.Prompt To Fix With AI
This is a comment left during a code review. Path: tests/test_install.py Line: 535-543 Comment: **No `missing_stories_support` test for the `bmad-build-auto` topology** `missing_stories_support` was updated in this PR to resolve the primitive dynamically via `resolve_primitive_skill`, but the new tests only cover `missing_base_skills`, `resolve_review_layers`, `base_skills_for_tree`, and `provision_worktree` against a `bmad-build-auto` project. The stories probe path is only exercised via `STORIES_PROBE_SKILL` (which still resolves to `"bmad-dev-auto"`, the pre-rename path). Adding a counterpart test that installs `bmad-build-auto` and calls `missing_stories_support` would mirror the coverage pattern used for all other functions in this change. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Prompt To Fix All With AI
### Issue 1
src/bmad_loop/install.py:231-232
**Stale `bmad-dev-auto` reference in `ReviewResolution` docstring**
The class-level docstring on `ReviewResolution` still says "Which skills the installed `bmad-dev-auto`'s review step actually invokes." — every other comparable string in the module was updated to say "dev primitive" or "resolved primitive". A contributor reading the NamedTuple in isolation gets a misleading impression that it is always tied to the legacy name, which contradicts the whole point of this PR.
### Issue 2
src/bmad_loop/install.py:167
**`DEV_PRIMITIVE_SKILL` removed without a backward-compat alias**
The PR explicitly preserves `DEV_BASE_SKILLS`, `BASE_SKILLS`, and `STORIES_PROBE_SKILL` for backward compat with existing call sites, but the equally-public `DEV_PRIMITIVE_SKILL = "bmad-dev-auto"` constant is silently dropped. Any downstream code that does `from bmad_loop.install import DEV_PRIMITIVE_SKILL` (outside the test files updated in this PR) will get an `ImportError`. If no external callers are known, adding a one-line alias `DEV_PRIMITIVE_SKILL = LEGACY_PRIMITIVE_SKILL` alongside the other preserved exports would close the risk at zero cost.
### Issue 3
src/bmad_loop/worktree_flow.py:234-236
**`resolve_primitive_skill` called twice for the same project/tree**
`base_skills_for_tree(repo_root, tree)` and `resolve_review_layers(repo_root, tree)` each independently call `resolve_primitive_skill(repo_root, tree)`, resulting in two identical filesystem probes for the same `SKILL.md`. The optional `primitive` parameter added to `resolve_review_layers` in this very PR exists for exactly this use-case — `missing_base_skills` already takes advantage of it to pass the resolved primitive through to `_review_findings`. Passing the resolved primitive here too would make the pattern consistent across all call sites.
### Issue 4
tests/test_install.py:535-543
**No `missing_stories_support` test for the `bmad-build-auto` topology**
`missing_stories_support` was updated in this PR to resolve the primitive dynamically via `resolve_primitive_skill`, but the new tests only cover `missing_base_skills`, `resolve_review_layers`, `base_skills_for_tree`, and `provision_worktree` against a `bmad-build-auto` project. The stories probe path is only exercised via `STORIES_PROBE_SKILL` (which still resolves to `"bmad-dev-auto"`, the pre-rename path). Adding a counterpart test that installs `bmad-build-auto` and calls `missing_stories_support` would mirror the coverage pattern used for all other functions in this change.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Resolve the dev primitive skill dynamica..." | Re-trigger Greptile
| # primitive name for back-compat with existing call sites and tests that treat it | ||
| # as a fixed catalog; `base_skills_for_tree` is the per-project/tree equivalent | ||
| # that resolves the primitive dynamically, and is what worktree provisioning uses. | ||
| BASE_SKILLS = {**DEV_BASE_SKILLS, "bmad-review-verification-gap": (), MERGED_REVIEW_SKILL: ()} |
There was a problem hiding this comment.
DEV_PRIMITIVE_SKILL removed without a backward-compat alias
The PR explicitly preserves DEV_BASE_SKILLS, BASE_SKILLS, and STORIES_PROBE_SKILL for backward compat with existing call sites, but the equally-public DEV_PRIMITIVE_SKILL = "bmad-dev-auto" constant is silently dropped. Any downstream code that does from bmad_loop.install import DEV_PRIMITIVE_SKILL (outside the test files updated in this PR) will get an ImportError. If no external callers are known, adding a one-line alias DEV_PRIMITIVE_SKILL = LEGACY_PRIMITIVE_SKILL alongside the other preserved exports would close the risk at zero cost.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/bmad_loop/install.py
Line: 167
Comment:
**`DEV_PRIMITIVE_SKILL` removed without a backward-compat alias**
The PR explicitly preserves `DEV_BASE_SKILLS`, `BASE_SKILLS`, and `STORIES_PROBE_SKILL` for backward compat with existing call sites, but the equally-public `DEV_PRIMITIVE_SKILL = "bmad-dev-auto"` constant is silently dropped. Any downstream code that does `from bmad_loop.install import DEV_PRIMITIVE_SKILL` (outside the test files updated in this PR) will get an `ImportError`. If no external callers are known, adding a one-line alias `DEV_PRIMITIVE_SKILL = LEGACY_PRIMITIVE_SKILL` alongside the other preserved exports would close the risk at zero cost.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| resolved = resolve_review_layers(repo_root, tree) | ||
| required = dict.fromkeys((*BASE_SKILLS, *(resolved.skills() if resolved else ()))) | ||
| required = dict.fromkeys( | ||
| (*base_skills_for_tree(repo_root, tree), *(resolved.skills() if resolved else ())) |
There was a problem hiding this comment.
resolve_primitive_skill called twice for the same project/tree
base_skills_for_tree(repo_root, tree) and resolve_review_layers(repo_root, tree) each independently call resolve_primitive_skill(repo_root, tree), resulting in two identical filesystem probes for the same SKILL.md. The optional primitive parameter added to resolve_review_layers in this very PR exists for exactly this use-case — missing_base_skills already takes advantage of it to pass the resolved primitive through to _review_findings. Passing the resolved primitive here too would make the pattern consistent across all call sites.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/bmad_loop/worktree_flow.py
Line: 234-236
Comment:
**`resolve_primitive_skill` called twice for the same project/tree**
`base_skills_for_tree(repo_root, tree)` and `resolve_review_layers(repo_root, tree)` each independently call `resolve_primitive_skill(repo_root, tree)`, resulting in two identical filesystem probes for the same `SKILL.md`. The optional `primitive` parameter added to `resolve_review_layers` in this very PR exists for exactly this use-case — `missing_base_skills` already takes advantage of it to pass the resolved primitive through to `_review_findings`. Passing the resolved primitive here too would make the pattern consistent across all call sites.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary
bmad-dev-auto→bmad-build-auto, leavingbmad-dev-autoinstalled as a permanent one-file forwarding shim (nostep-04-review.md/customize.tomlof its own).bmad-loopstill hardcodedbmad-dev-autoas the primitive everywhere: thevalidatemarker check, review-layer resolution,_bmad/custom/override paths, the stories-dispatch probe, and worktree provisioning's copy list. This madebmad-loop validatereport a correctly-installed post-rename project asskills.base-incomplete.step-04-review.md/customize.toml— a functional bug that would affect live dev/review sessions on any post-rename bmm install, not justvalidateoutput.resolve_primitive_skill(project, tree)as the single place that decides which skill is a project's real dev primitive: prefersbmad-build-autowhenever it's installed there at all (existence, not completeness — a half-upgraded install still resolves tobmad-build-autoso the marker gap is reported against the right name), falling back tobmad-dev-autoonly whenbmad-build-autois entirely absent (pre-rename bmm installs).missing_base_skills,resolve_review_layers,_merged_review_layers,missing_stories_support, the_bmad/custom/override filenames, andprovision_worktree's copy list via the newbase_skills_for_tree) now resolves through that one function instead of a hardcoded literal, so they can never disagree about which skill is authoritative for a given project/tree.DEV_BASE_SKILLS,BASE_SKILLS,STORIES_PROBE_SKILLare kept exported under their legacy values for backward compatibility with existing call sites.Test plan
pytest tests/test_install.py— 234 passed (2 pre-existing/unrelated failures onmain, macOS/APFS undecodable-filename tests, verified viagit stash)pytest tests/— 3816 passed, 48 skipped, 1 pre-existing/unrelated failure (same macOS/APFS issue), reproduced identically before and after this changeruff check/ruff format --checkclean on all touched filespyrightclean (0 errors) oninstall.py,worktree_flow.py,cli.pyTestResolvePrimitiveSkill(5 cases) plus coverage formissing_base_skills,resolve_review_layers, customize-override precedence,base_skills_for_tree, andprovision_worktreeagainst abmad-build-autoprojectbmad-loop validategoes from a falseFAIL: skills.base-incompleteto a clean passSummary by CodeRabbit
New Features
Documentation
Tests