fix: resolve the bmad-build-auto rename end to end (6A+6B of #433) - #436
Conversation
BMAD-METHOD#2651 renamed the dev primitive bmad-dev-auto -> bmad-build-auto and left a forwarding shim under the old name: a lone SKILL.md whose customization migration gate is interactive, so an unattended session dispatched into it HALTs having written nothing to disk. Resolve the primitive per skill tree instead of hardcoding one era, and never accept the shim. resolve_dev_primitive prefers bmad-build-auto (SKILL.md alone), falls back to bmad-dev-auto only when marker-complete -- which is exactly what the shim is not, so DEV_PRIMITIVE_MARKERS doubles as the shim detector -- else None, meaning fail the preflight. missing_base_skills splits three ways: base-incomplete (resolved but truncated), the new base-shim (nothing resolved, a legacy SKILL.md present), base-missing (nothing under either name). resolve_review_layers now resolves the name internally rather than reading a hardcoded bmad-dev-auto/customize.toml and step-04-review.md. On a renamed project both were absent, so it returned None and both callers silently degraded to the static catalog -- worktree_flow then seeding a worktree with catalog reviewers instead of the ones the project configured. Silent degradation, not a loud FAIL. _customize_overrides reads the RESOLVED name only: upstream keys on the skill dir, so at run time the legacy override IS ignored, and dual-reading would make the preflight resolve layers the session never applies. The orphaned file is reported as skills.customize-legacy -- a warning, since these gates have no severity filter and no --force, and a false FAIL pauses every run behind an inapplicable fix. BASE_SKILLS names both eras, which is what carries the primitive into isolation worktrees across the rename: the layer resolution returns review skills only, so a primitive the catalog did not name was silently left behind. Also aligns a live main-side defect: cli._require_base_skills gated runsetup.ROLES (dev/review/triage) while worktree provisioning only ever carried dev+review, so `[adapter.triage] name = "gemini"` under a claude dev/review pair refused runs until the whole bmm module was installed in a tree no session dispatches into. The new cli._skill_trees and WorktreeFlow.worktree_profiles both read install.DEV_PRIMITIVE_ROLES, so the gated set and the provisioned set cannot drift.
…le dry runs (#433) Every session prompt hardcoded `/bmad-dev-auto`, which post-rename dispatches the forwarding shim and HALTs an unattended session on its interactive migration gate. `Engine._dev_skill(role)` resolves the name from the adapter's own skill tree, memoized per tree so a run mixing `.claude/skills` and `.agents/skills` gets the right era per role. Threaded through the generic-dev legs, the review prompt, all three sweep bundle legs, and stories folder+id dispatch. `devcontract.FALLBACK_RESULT_PREFIX` becomes the `FALLBACK_RESULT_PREFIXES` tuple, matched unconditionally against both eras: a result marker is named after whichever skill wrote it, so a resume across an upstream upgrade must still read one back. The workflow completion marker is produced from the workflow's OWN role, whose tree can sit at a different era than dev's. `--dry-run` on run, stories and sweep all return before their skill preflight, so a broken install got a plausible-looking schedule. They now print the preflight problems to stderr under a "NOT runnable as-is" banner. Exit code stays 0 and stdout is untouched — a dry run is a diagnostic, and rc 0 has always meant "the preview rendered". The banner filters on `severity == "problem"`, unlike the shipped 0.9.1 one: `missing_base_skills` can return warnings that `_require_base_skills` prints and steps over, so an unfiltered banner would claim a run aborts when it does not. `policy.dev.skill` stays `bmad-dev-auto` and `DEV_SKILLS` stays one element — it is the adapter discriminator, not the invoked name. Only the `PolicyError` text changed. Tests: prompt threading across engine/sweep/stories, the per-role marker producer, the `FALLBACK_RESULT_PREFIXES` <-> `DEV_PRIMITIVE_*` subset invariant, a post-rename sandbox E2E row, and a two-sided pin that `cli._skill_trees` and `WorktreeFlow.worktree_profiles` stay one decision -- previously asserted only in a comment.
![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: 52 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 (3)
WalkthroughThe change adds compatibility with the ChangesDevelopment primitive compatibility
Runtime and documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 2
🧹 Nitpick comments (3)
src/bmad_loop/install.py (1)
410-435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the loop variable to stop shadowing the resolved primitive.
Line 410 binds
skillto the resolved dev-primitive name. Line 435 rebindsskillto each review-skill name inside the layer loop. Today this is harmless: the loop runs only in theif layers:branch, which returns at line 439, so the step-04 read at line 449 still sees the primitive. The safety depends entirely on that early return. A later edit that readsskillafter the loop would silently use a review-skill name and probe the wrong directory.♻️ Proposed rename
- for bucket, skills in ((required, hard), (advisory, soft)): - for skill in skills: - ids = bucket.setdefault(skill, []) + for bucket, skills in ((required, hard), (advisory, soft)): + for invoked_skill in skills: + ids = bucket.setdefault(invoked_skill, []) if layer_id not in ids: ids.append(layer_id)🤖 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 410 - 435, Rename the inner `skill` loop variable used when populating `required` and `advisory` in the layer-processing block, preserving the outer `skill` binding from `dev_primitive_or_default(project, tree)` for subsequent logic.src/bmad_loop/sweep.py (1)
1207-1223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the docstring to stop naming one era.
The three prompt legs now resolve the primitive from disk. The docstring at line 1191 still reads "the generic bmad-dev-auto dev skill".
Engine._generic_dev_promptandStoriesEngine._stories_dev_prompthad their equivalent wording generalized in this change, so this seam now reads inconsistently against its two siblings.♻️ Proposed docstring change
- """Bundle invocation for the generic bmad-dev-auto dev skill: the self-contained - intent.md (intent + verbatim ledger entries) is handed over as freeform - intent. The orchestrator owns the deferred-work ledger — the skill is told + """Bundle invocation for the generic dev primitive (disk-resolved — see + ``Engine._dev_skill``): the self-contained intent.md (intent + verbatim + ledger entries) is handed over as freeform + intent. The orchestrator owns the deferred-work ledger — the skill is told not to edit it — and records resolution itself in `_post_dev_state_sync`.🤖 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/sweep.py` around lines 1207 - 1223, Update the docstring for the prompt-building method containing the deferred-work branches to describe resolving the development skill generically, rather than naming “bmad-dev-auto.” Keep the behavior and prompt strings unchanged, matching the generalized wording used by Engine._generic_dev_prompt and StoriesEngine._stories_dev_prompt.tests/test_install.py (1)
69-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the primitive markers from the catalog too.
The docstring states the catalog is derived rather than restated. The primitive entry still restates
DEV_PRIMITIVE_MARKERSinstead of reusing the legacy entry ofDEV_BASE_SKILLS. If those two ever diverge,_era_catalogproduces a scaffold that no longer matches the legacy catalog it re-keys.♻️ Proposed refactor
return { - primitive: DEV_PRIMITIVE_MARKERS, + primitive: DEV_BASE_SKILLS[DEV_PRIMITIVE_LEGACY], **{k: v for k, v in DEV_BASE_SKILLS.items() if k != DEV_PRIMITIVE_LEGACY}, }🤖 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 69 - 79, Update _era_catalog to derive the re-keyed primitive entry from DEV_BASE_SKILLS[DEV_PRIMITIVE_LEGACY] instead of using DEV_PRIMITIVE_MARKERS directly, while preserving the existing filtering and re-keying 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.
Inline comments:
In `@CHANGELOG.md`:
- Around line 3005-3006: Update the [Unreleased] comparison link in the
changelog to use v0.9.1 as its starting tag while retaining HEAD as the
endpoint.
In `@tests/test_frontmatter.py`:
- Around line 68-75: Update the documentation around
test_runs.test_rearm_restore_mode_sets_in_review_strips_arr_and_latches and the
test_stories_e2e sprint/sweep intent-gap restore tests to describe the status as
the unquoted value status: in-review. Remove the incorrect status: done wording
and avoid suggesting that the value’s quotes are preserved.
---
Nitpick comments:
In `@src/bmad_loop/install.py`:
- Around line 410-435: Rename the inner `skill` loop variable used when
populating `required` and `advisory` in the layer-processing block, preserving
the outer `skill` binding from `dev_primitive_or_default(project, tree)` for
subsequent logic.
In `@src/bmad_loop/sweep.py`:
- Around line 1207-1223: Update the docstring for the prompt-building method
containing the deferred-work branches to describe resolving the development
skill generically, rather than naming “bmad-dev-auto.” Keep the behavior and
prompt strings unchanged, matching the generalized wording used by
Engine._generic_dev_prompt and StoriesEngine._stories_dev_prompt.
In `@tests/test_install.py`:
- Around line 69-79: Update _era_catalog to derive the re-keyed primitive entry
from DEV_BASE_SKILLS[DEV_PRIMITIVE_LEGACY] instead of using
DEV_PRIMITIVE_MARKERS directly, while preserving the existing filtering and
re-keying 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: dca0cf6b-60ca-4728-9eec-b6bad7505177
📒 Files selected for processing (19)
CHANGELOG.mdsrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/devcontract.pysrc/bmad_loop/engine.pysrc/bmad_loop/install.pysrc/bmad_loop/policy.pysrc/bmad_loop/stories_engine.pysrc/bmad_loop/sweep.pysrc/bmad_loop/worktree_flow.pytests/conftest.pytests/test_cli.pytests/test_devcontract.pytests/test_engine.pytests/test_frontmatter.pytests/test_install.pytests/test_stories_e2e.pytests/test_stories_engine.pytests/test_sweep.py
…layers The layer loop rebinds `skill` to each invoked review skill, shadowing the dev-primitive name the step-04 fallback reads as a directory. Harmless only because the layer branch returns first — rename the outer binding to `primitive`, matching `_review_findings`, so a later edit cannot silently probe a review skill's directory for step-04-review.md. Also generalize two docstrings that still named one era: sweep's bundle invocation, and the frontmatter non-preservation pin — whose downstream consumers read back `in-review` as well as `done`, so the property, not one value, is what it must state.
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.
|
Triaging the three nitpicks from the review body (they carry no inline thread, so replying here). Two accepted, one rejected — all in
Worth noting this PR introduced the hazard: before the rename thread, that line was a module constant with no local to shadow. Fixed by renaming the outer binding to
DEV_BASE_SKILLS = {
DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS,
...
}so There is also a direction argument for keeping it as-is. Separately, for whoever picks up the next sub-phase: Greptile is not reviewing this PR. It posted a review object at the head sha whose body is a trial-credit-limit notice ( |
|
@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: 88fd54dbfd
ℹ️ 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".
| adapter = self.adapters.get(role) | ||
| tree = getattr(getattr(adapter, "profile", None), "skill_tree", None) | ||
| if tree not in self._dev_skill_cache: | ||
| self._dev_skill_cache[tree] = dev_primitive_or_default(self.paths.project, tree) |
There was a problem hiding this comment.
When an isolated run is paused on a legacy-era worktree and the main checkout is upgraded to bmad-build-auto before resume, the preflight passes against the upgraded checkout, but reopen_unit reuses the old worktree without provisioning it again. This lookup nevertheless resolves against self.paths.project, caches the new name, and dispatches /bmad-build-auto inside a worktree that only contains bmad-dev-auto, causing an Unknown-command stall and potentially discarding the resumed work; resolve against the active workspace or refresh the reopened worktree instead.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted and fixed in 2a3da76 — with one correction and one addition.
Confirmed, and the address is exact. provision_worktree has a single runtime call site (worktree_flow.py:505, the fresh-mount path inside run_isolated); reopen_unit (worktree_flow.py:746) only is_dir()-checks and re-absolutizes spec_file. All three reopen arms (engine.py:814/844/865, mirrored at sweep.py:637/652) set self.workspace = unit.workspace and dispatch, and sessions run at cwd=self.workspace.root (engine.py:2845). The resume preflight is rooted at the main checkout (cli.py:1313/1323), so it passes. There was corroboration sitting in the code already: at engine.py:2836-2838 the result-marker path is self.workspace.paths... while the skill name inside that same expression came from the main-rooted lookup.
Correction — nothing is discarded. The failure is a bounded burn, not data loss: no Stop event means verify.py never runs (gated on completed, engine.py:1220), decide_dev burns an attempt, and at max_dev_attempts=2 the story DEFERs. Defer under isolation touches no files, keep_failed defaults True, and _rollback_or_pause no-ops on a clean tree. The cost is ~2 wasted sessions per story across the sprint, not lost work.
Addition — the obvious fix reproduces the bug. _dev_skill_cache is built once (engine.py:278) and never cleared, keyed on tree alone. Switching only the resolution root leaves _finish_inflight caching the reopened legacy worktree's bmad-dev-auto, which _loop then serves to the fresh worktrees it mounts afterwards — provisioned from the upgraded checkout. So the key moves to (workspace project root, tree) as well.
Both halves are pinned by test_dev_prompt_resolves_in_the_reopened_worktree_not_the_main_checkout, and separately ablated against the whole suite (3929 collected on every row): reverting the resolution root fails its first assert; reverting the memo key alone fails its second — the behaviour assert, not the memo-shape one. Three pre-existing memo-shape assertions moved to the new key rather than being weakened.
Worth noting for the rest of this forward-port: _dev_skill is byte-identical to origin/release/0.9.x, docstring included, so this is a latent defect in the shipped 0.9.1 rather than something the port introduced. The false premise was in that docstring's last paragraph, which asserted the worktree "can only carry the name resolved here" — true for a fresh unit, false across a resume. It has been replaced with the reopen path spelled out.
| resolved_new = any( | ||
| resolve_dev_primitive(project, tree) == DEV_PRIMITIVE_NEW for tree in dict.fromkeys(trees) | ||
| ) |
There was a problem hiding this comment.
When one active tree resolves to bmad-build-auto and another still resolves to marker-complete bmad-dev-auto, this any(...) marks _bmad/custom/bmad-dev-auto*.toml as globally orphaned even though the legacy tree still applies it. The emitted remediation tells the operator to rename the file, which can silently remove the legacy tree's customization; only call it orphaned when no active tree resolves to the legacy primitive, or tell mixed-era projects to copy rather than rename it.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted and fixed in 2a3da76, taking your second remedy (copy, not suppress). Measured rather than reasoned, because the two remedies point opposite ways.
The harm reproduces. On a mixed-era project — .claude/skills on bmad-build-auto, .agents/skills on a marker-complete bmad-dev-auto, one _bmad/custom/bmad-dev-auto.toml — following the emitted advice literally:
.agents/skills (legacy) |
.claude/skills (new) |
|
|---|---|---|
| before | bmad-house-reviewer ← the override applies |
bmad-review ← shipped default |
| after "rename" | bmad-review ← customization lost |
bmad-house-reviewer |
The rename moves the customization between trees rather than migrating it, exactly as you said.
But your first remedy would be the opposite error. That same table shows the new tree genuinely running unstyled (bmad-review, not the operator's bmad-house-reviewer) — so suppressing the finding whenever a legacy tree survives would hide a real degradation for as long as the migration lasts, which is precisely what this warning exists to surface. The fix keeps the finding and fixes the claim: on a mixed-era project it names both trees, says the file still applies in the legacy one, and advises copy — noting that renaming would drop it from the legacy tree.
This PR owned the contradiction on both sides. It added test_validate_reports_a_different_primitive_era_in_each_tree, which asserts a mixed-era project validates green with dev_primitive == ["bmad-build-auto", "bmad-dev-auto"] and documents that "each tree is driven under its OWN name" — and the neighbouring test_legacy_customize_does_not_warn[tree-still-resolves-legacy] already states the principle: "the tree still resolves to the LEGACY primitive, so the legacy override is the one that applies — warning about it would be exactly backwards." The any(...) made the two-tree case do that.
legacy_trees rides in detail on the mixed branch only, so the all-new dict stays byte-identical and test_orphaned_legacy_customize_warns_once's exact-match assert stays a live oracle instead of churn. Pinned by test_mixed_era_orphan_says_copy_because_the_legacy_tree_still_applies_it and ablated against the whole suite: disabling the branch reddens it and the message reverts to the destructive "rename" advice. The docstring also said "the tree resolved to the NEW name" (singular) while the code was any() — corrected.
Like the P1 finding, this is byte-identical to shipped release/0.9.x, so it is a latent defect in 0.9.1 rather than a port regression.
#433) Codex round 2 on #436. Both findings are latent in the shipped 0.9.1 and were ported faithfully — neither is a port regression. P1 — `Engine._dev_skill` resolved against the main checkout. Only `run_isolated`'s fresh-mount path provisions a worktree; `reopen_unit` re-mounts an existing one untouched, so a checkout upgraded across a pause resolved `bmad-build-auto` into a worktree carrying only `bmad-dev-auto`. The session runs with `cwd=self.workspace.root`, so it HALTs on an unknown command having written nothing for verify to read, and burns its dev attempts through to DEFER. Resolve against the workspace — and key the memo on it too: one Engine drives every unit of a run, so a tree-only key merely moves the bug, serving the reopened worktree's answer to the fresh worktrees mounted after it. P2 — `dev_primitive_warnings` gated on `any(tree resolves NEW)`, so a project mid-upgrade had `_bmad/custom/bmad-dev-auto.toml` called orphaned while another tree still resolved the legacy primitive and still applied it. Following the emitted "rename" advice moves the customization between trees instead of migrating it. Keep the warning — the new-era tree really is running unstyled, which is the degradation it exists to surface — but say copy, name both trees, and carry `legacy_trees` in `detail` on that branch only, leaving the all-new detail dict byte-identical so its exact-match test stays an oracle. Ablations run against the whole suite, 3929 collected on every row: - resolution root reverted -> reopened-worktree test fails on its first assert - memo key reverted alone -> it fails on the SECOND assert (the behaviour claim) - mixed-era branch disabled -> copy-advice test fails, message reverts to "rename"
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a3da762ef
ℹ️ 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".
|
|
||
| ### Fixed | ||
|
|
||
| - **The upstream `bmad-dev-auto` → `bmad-build-auto` rename no longer breaks a project (#393).** |
There was a problem hiding this comment.
This single Unreleased entry spans twenty lines and combines primitive resolution, resume behavior, mixed-era overrides, dry-run diagnostics, and triage gating into one narrative paragraph. Split these operator-visible changes into short, imperative bullets so the entry complies with the repository's requirement that changelog entries be terse and scannable.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred, not rejected — tracked as #438, with the measurement that made me not act on it here.
AGENTS.md:66 does say terse/scannable/imperative, so the rule you cited is real. But measured against the file it governs, the ## [Unreleased] bullets run 3–57 lines: 28 in the 3–9 range, 15 at 10–15, 6 at 16–25, and four at 33/35/53/57. The entry you flagged is ~20 lines — mid-distribution, with about ten longer entries sitting unremarked beside it.
So splitting this one entry into imperative bullets would make it an outlier against ~50 narrative neighbours, and would rewrite text already shipped in 9618b69 earlier in this same PR. That is churn dressed as compliance.
The real defect is that the rule is unenforceable as written: with no stated limit, "too long" cannot be adjudicated when the surrounding entries are 2–3× longer. #438 puts the three options (tighten the file to the rule / reword the rule to the practice / require a terse bold lead sentence and allow the body) and asks for a concrete limit or an explicit statement that there is none.
Your other finding this round — the stale bmad-dev-auto references in README and FEATURES — was accepted and fixed in 8a95866; see that thread.
|
|
||
| ### Fixed | ||
|
|
||
| - **The upstream `bmad-dev-auto` → `bmad-build-auto` rename no longer breaks a project (#393).** |
There was a problem hiding this comment.
For users on the renamed BMAD release, the public instructions still require and directly invoke only bmad-dev-auto (for example README.md:43 and README.md:168, plus docs/FEATURES.md:43 and docs/FEATURES.md:125). That old name can now be the interactive forwarding shim this change explicitly refuses, so following the documented prerequisites or command examples can lead users to install or invoke the unusable path. Update the README and behavior reference to describe disk resolution, prefer bmad-build-auto, and identify marker-complete bmad-dev-auto as legacy compatibility.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted and fixed in 8a95866 — and it was a better catch than its P2 grade suggests.
All four addresses check out. README.md:43 is the load-bearing one: it listed bmad-dev-auto as a prerequisite, so a user on a renamed BMAD release was being pointed at exactly the forwarding shim this PR now refuses by name.
This is not a port artefact, which is what makes it worth having. git diff --numstat v0.9.0..origin/release/0.9.x -- README.md docs/FEATURES.md is 1/1 on each, and both hunks touch only the #414 refusal — the shipped 0.9.1 never updated these files for the rename either. No sub-phase of the forward-port owned it (6E covers these two files only for #414), so the whole program would have shipped the gap.
Two deviations from your suggestion, both deliberate:
Wider than the four spots you named. There are 25 bmad-dev-auto mentions across the two files. I fixed the prescriptive ones — prerequisites, the skills table row, install instructions, both stories folder+id dispatch examples, the "new enough for folder+id dispatch" requirement, and the validate descriptions — plus the flow-diagram dispatch lines, since those illustrate the command a current install actually shows. Narrative references to upstream skill behaviour (what the skill does on an intent gap, how followup_review_recommended is computed) are left as-is: they describe the upstream skill, not what the reader should install or expect to be invoked, and rewriting all 25 would be churn with a real chance of introducing errors.
The era split is stated once, authoritatively, rather than everywhere. README.md:43 and docs/FEATURES.md:43 now carry the full statement — bmad-build-auto, or a complete bmad-dev-auto on pre-rename releases, resolved from disk per skill tree and invoked under the name it resolves to, with the bare shim refused. Elsewhere the docs say "the dev primitive". That mirrors how these files already handle the other upstream era split (the standalone review hunters vs the merged bmad-review).
Your other finding this round — splitting the [Unreleased] entry into imperative bullets — is deferred rather than rejected, and tracked as #438 with the measurement: AGENTS.md:66 does say terse/scannable, but Unreleased bullets currently run 3–57 lines with four above 30, so restructuring this one entry would have made it an outlier against ~50 neighbours and rewritten text already shipped earlier in this PR. The rule needs a concrete limit before it can be applied to a single entry.
Codex round 3 on #436. The prerequisites and examples named only `bmad-dev-auto`, which on a renamed install is the forwarding shim this PR now refuses by name — so following the documented prerequisite led users straight at the path the product rejects. Prescriptive spots only: prerequisites, the skills table, install instructions, the stories folder+id dispatch examples, the version requirement, and the `validate` descriptions. Narrative references to upstream skill behaviour are left alone now that the era split is stated authoritatively at README's prerequisite and FEATURES' dev-stage bullet. Flow-diagram dispatch lines updated too, since they illustrate the command a current install actually shows. Not a port artefact: `git diff v0.9.0..origin/release/0.9.x -- README.md docs/FEATURES.md` is 1/1 on each and touches only the #414 refusal, so the shipped 0.9.1 has this same gap. No Phase 6 sub-phase owned it — 6E covers these two files only for #414.
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: 3
🤖 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 `@README.md`:
- Around line 203-210: Update the compatibility-sensitive examples in README.md
at lines 203-210 and 246 to avoid hard-coding only bmad-build-auto: use the
resolved primitive-name placeholder or explicitly document both bmad-build-auto
and legacy bmad-dev-auto for the DEV workflow and bundle execution examples.
- Line 43: Document the Verification-Gap review-layer prerequisite from
docs/FEATURES.md in both README.md:43-43 and the prerequisite table at
README.md:278-283. Update the introductory prerequisite list to include the
required skill or clearly mark the list as version-specific, and add the
corresponding Verification-Gap skill to the table; keep the existing
version-compatibility guidance intact.
- Line 43: The resolver documentation incorrectly attributes forwarding-shim
rejection to its name. In README.md lines 43-43 and docs/FEATURES.md lines
43-43, update the corresponding BMAD prerequisite wording to state that the
legacy path is accepted only when required markers are complete and the
forwarding shim is rejected because those markers are missing; make no other
changes.
🪄 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: 2a7118f1-4e10-489a-af06-98110ba5c5cc
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/engine.pysrc/bmad_loop/install.pysrc/bmad_loop/sweep.pytests/test_engine.pytests/test_frontmatter.pytests/test_install.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/bmad_loop/sweep.py
- tests/test_frontmatter.py
- tests/test_engine.py
- src/bmad_loop/engine.py
- CHANGELOG.md
- src/bmad_loop/install.py
CodeRabbit round 4 on #436. README, FEATURES and the CHANGELOG entry all said the forwarding shim is "refused by name". The resolver does the opposite: `resolve_dev_primitive` accepts a marker-complete `bmad-dev-auto` and refuses only an install missing `DEV_PRIMITIVE_MARKERS`, so the discriminator is completeness and the legacy name is fully supported. README's own sentence contradicted itself — "a complete `bmad-dev-auto`" accepted, then the same name "refused by name". The bot named two sites; the CHANGELOG entry added in 9618b69 carried the same claim and is fixed too. The CHANGELOG now also records that a truncated install is byte-identical on disk to the shim, which is why `skills.base-shim` names both causes.
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.
Round 4 triage (CodeRabbit @
|
| # | Finding | Verdict |
|---|---|---|
| 1 | Shim rejection described as by-name rather than by marker completeness | Fixed — b9f11e5 |
| 2 | Verification-Gap missing from README prerequisites | Rejected — the edit would document a skill the preflight never requires |
| 3 | Flow-diagram examples hard-code bmad-build-auto |
Rejected — prescriptive lines carry the caveat; diagrams are deliberately concrete |
The one that was real
resolve_dev_primitive accepts a marker-complete bmad-dev-auto and refuses only an install missing DEV_PRIMITIVE_MARKERS. The legacy name is fully supported; completeness is the entire discriminator. Three docs said "refused by name", and the README sentence contradicted itself within one line — offering "a complete bmad-dev-auto" as supported, then declaring the same name refused.
The bot named two sites; the phrase had three. CHANGELOG.md:266 — added by 9618b69 on this PR, directly beneath its own correct "a marker-complete bmad-dev-auto accepted" clause — carried it too. Fixed there as well, and the entry now also records that a truncated install is byte-identical on disk to the shim, which is why skills.base-shim names both causes rather than claiming to know which one it found.
Why the other two were declined
Verification-Gap is in the copy-if-present BASE_SKILLS superset under an explicit never validated comment, not in _REVIEW_LAYER_SKILLS. README:43 and the table document the static fallback set the preflight enforces; FEATURES:55 documents the default layers a #2550-era customize.toml declares, which resolve_review_layers reads per project. Different sets, deliberately. Adding it to the prerequisite table would send users to install something validate never asks for.
Hard-coded examples — README.md:169 (stories dispatch) already carries "spelled with whichever primitive name resolves on disk" because it is the dispatch contract. The flow diagrams are pictures of one run, next to a hard-coded story ref and a literal claude; the era split is stated authoritatively at :43 and :276.
Docs-only change: trunk check --no-fix clean, no test asserts on the phrasing.
1 participant
Forward-port of the 0.9.1 hotfix to
main, sub-phases 6A + 6B. Part of #433.Fixes #393
mainstill hardcodesbmad-dev-autoeverywhere. Since BMAD-METHOD#2651 renamed the devprimitive to
bmad-build-auto, a project on bmm >= 6.10.1 failsvalidateoutright, and — worse— a project left with the forwarding shim gets a preflight that passes and a session that HALTs
on the shim's interactive migration gate having written nothing to disk. This is a
re-implementation against
main's seams, not a cherry-pick:provision_worktreelives inworktree_flow.pyhere, worktree excludes are worktree-scoped (#384/#385),_copy_traversablealready merges, and pyright is a CI gate that never ran on any of this code.
What lands
6A — resolution seam.
install.resolve_dev_primitive/dev_primitive_or_default/_is_dev_primitive_shim/dev_primitive_warnings, replacing theDEV_PRIMITIVE_SKILLliteralwith
DEV_PRIMITIVE_NEW/LEGACY/MARKERS/ROLES.bmad-build-autois preferred; amarker-complete
bmad-dev-autois accepted; the shim is refused by its own check id(
skills.base-shim) rather than resolving. The resolved name is threaded through_merged_review_layers/resolve_review_layersandworktree_flow.py's provisioning call.That thread is the non-obvious half.
resolve_review_layersreadsproject/<tree>/bmad-dev-auto/customize.tomlandstep-04-review.md; on a renamed project bothare absent, so it returned
Noneand degraded silently to the static catalog — and the samecall in
worktree_flow.pythen seeded worktrees with the wrong reviewers instead of the ones theproject configured. Silent degradation, not a loud FAIL, which is why it could not be deferred.
6B — prompt threading + dry-run banner.
Engine._dev_skill(role)with a per-skill-tree memo;every prompt site in
engine.py,sweep.py(all three bundle legs) andstories_engine.pynowspells the resolved name.
devcontract.FALLBACK_RESULT_PREFIXbecomes theFALLBACK_RESULT_PREFIXEStuple matched viastr.startswith(tuple), so a result marker writtenby either era is read back.
_warn_preflight_would_aborton all three dry-run entry points.A live
maindefect fixed on the waycli._require_base_skillsderived its trees fromrunsetup.ROLES— three roles — whileWorktreeFlow.worktree_profilesprovisions two.maingates a triage tree it neverprovisions, so
[adapter.triage] name = "gemini"beside a claude dev/review pair demanded thewhole bmm module in
.agents/skillsbefore a run could start: a hard FAIL, on a gate with no--force, over a tree no session dispatches these skills into. Both sides now readinstall.DEV_PRIMITIVE_ROLES.Deviations from the shipped 0.9.1, all deliberate
_warn_preflight_would_abortfiltersif p.severity == "problem"; 0.9.1 does not. Onmain,missing_base_skillsreally can return warnings —skills.customize-unreadableandskills.review-layer-unresolved— which_require_base_skillsprints and then steps over.Unfiltered, the banner claims "the real command aborts at preflight" for a project that runs
fine. Ablating the filter reddens exactly one test, and its output is the false alarm itself.
[Unreleased]still comparesv0.9.0...HEAD.mainis not a descendant ofv0.9.1, soretargeting the link would render the entire hotfix as removed. Left as-is deliberately.
An
[Unreleased]entry for this PR, which the plan assigned to the program's close-out.PR 1 is a ship-worthy release point on its own — it is the fix users are waiting for — and a
release cut here would otherwise ship the rename fix unmentioned. The close-out can
consolidate.
Also:
_warn_preflight_would_aborttakesproject: Path, notpaths. 0.9.x needspathsonlyfor the #414 isolation conflict, which is a later sub-phase; that sub-phase widens the signature
when it adds the reader.
Kept deliberately
policy.dev.skillstays the literal"bmad-dev-auto"andDEV_SKILLSstays a one-element set —it is the adapter discriminator read by
engine.pyandrunsetup.py, not the invoked name.Those two call sites are byte-identical to
main. Only thePolicyErrortext changed, to say theinvoked name is disk-resolved.
engine.py's completion-marker site resolvesself._dev_skill(role)— the workflow's own role,not the dev default — because a workflow runs on its own adapter, whose tree can be a different
tree at a different era.
test_workflow_marker_is_named_for_the_workflows_own_role_treepins it,and asserts the two trees genuinely resolve to different eras so it is not reading a coincidence.
test_skill_trees_covers_review_even_when_review_is_disabledis the #424 deferral pin and isdeliberately kept — it exists to make a future narrowing fail.
Gates
pytest3899 passed / 24 skipped,pyright0 errors / 0 warnings / 0 informations(delta 0 vs the
origin/mainbaseline), fulltrunk check --no-fixclean. The sandbox E2E ranfor real on tmux 3.7b — 9 passed, nothing skipped.
The local baseline additionally carries 4 pre-existing failures in
tests/test_module_skills_sync.py:.claude/skillsand.agents/skillsare untracked dev-boxinstalls that have drifted from
src/bmad_loop/data/skills/. Measured identical on a cleanorigin/main; that test skips in CI. Collected total 3927 throughout.Ablations
Each was a real source edit run against the whole suite, then hand-reverted and md5-verified
byte-identical against a pristine copy. Every row re-collected 3927, so the baseline did not
drift mid-round.
bmad-build-auto-result-fromFALLBACK_RESULT_PREFIXES_warn_preflight_would_abortcallscli._skill_treesback atrunsetup.ROLESseverity == "problem"filterworktree_profilesat 3 rolesKeyError)Two of these changed the shipped work rather than just confirming it:
FALLBACK_RESULT_PREFIXEShas threeconsumers, but
is_frontmatter_candidateandengine._record_dev_specwere both pinned on thelegacy spelling alone — the era a current project is least likely to be on. Widened both to
parametrize over both eras; the ablation now reddens 4.
test_gated_trees_and_provisioned_trees_stay_one_decisionexists.Constraint 1 — that the gated tree set and the provisioned profile set are one decision — was
asserted in a comment and enforced by nothing. Reverting either side used to escape: the
cliside was caught only by the triage tests, and theworktree_flowside only asKeyErrorsfrom fixture shape, which would redden for any third role whether or not the sets agreed.
Summary by CodeRabbit
bmad-build-autoskill while continuing to support existingbmad-dev-autoinstallations.