feat(bmad-auto-dev): rebuild as standalone machine-first skill#9
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
Walkthrough
Changesbmad-auto-dev Rebuild and Workflow Contract
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🤖 Augment PR SummarySummary: Rebuilds Changes:
Technical Notes: Updates engine/policy docs to reflect the skip-review behavior (no separate review session; dev does inline triple-review and finalizes to 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if not spec_path.is_file(): | ||
| return VerifyOutcome.retry(f"claimed spec file does not exist: {spec_path}") | ||
|
|
||
| workflow = rj.get("workflow") |
There was a problem hiding this comment.
verify_dev() currently allows sprint-status to be either 'review' or 'done' regardless of review_enabled (see the sprint = story_status(...) check later in the function), which can let a skip-review run pass dev verification even if sprint-status wasn’t advanced to 'done'. Consider making the expected sprint state conditional (and updating the error text) the same way expected is handled for the spec status.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Fixed in 9fbfaa5. verify_dev now derives the expected sprint state from review_enabled ('review' when a separate review session follows, 'done' in skip-review mode) instead of accepting both, and the error text reflects the conditional expectation. Added test_verify_dev_review_disabled_rejects_review_sprint covering the skip-review-but-sprint-left-at-'review' hole. (verify_dev_bundle is unchanged — bundles have no sprint-status entry.)
| f"dev result.json workflow is {workflow!r}, expected {DEV_WORKFLOW!r}" | ||
| ) | ||
|
|
||
| # With review disabled, the dev session runs its own internal review and |
There was a problem hiding this comment.
This comment says the dev session “runs its own internal review” when review is disabled, but the updated skill/docs describe skip-review mode as verification-only finalize (no adversarial review). Consider updating this comment to match the new behavior to avoid misleading future changes.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Won't fix — the comment is accurate. Skip-review mode is not verification-only: the dev session runs the inline three-layer adversarial review (step-04-review.md, titled "Inline Triple-Review (skip-review mode only)") before finalizing to done. This matches ReviewPolicy (policy.py), POLICY_TEMPLATE, engine.py:_skip_review_and_commit, and SKILL.md Rules. So "runs its own internal review and finalizes straight to done" is the correct description.
| - `escalations` = contents of any escalations raised this run (else empty). | ||
| - **Bundle mode only:** include `"dw_ids": [<the bundle's ids, verbatim>]`. | ||
|
|
||
| 6. **End the turn** with a one-line statement of what was implemented. Do not ask questions, offer next steps, or wait for anything. |
There was a problem hiding this comment.
Instruction 6 says to “End the turn”, but the subsequent “On Complete” block instructs running an additional command afterward, which is contradictory/unreachable as written. This also conflicts with the earlier statement that writing result.json is the last action if workflow.on_complete has side effects.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Fixed in 9fbfaa5. Reworded so the ordering is unambiguous: result.json is the last artifact written, and the On Complete hook is now an explicit numbered step (run on_complete if non-empty → then end the turn). Updated the step-05 header line and the matching SKILL.md contract line so they no longer claim result.json is the final action when on_complete has side effects.
| @@ -10,14 +10,14 @@ plus the orchestrator that invokes them. Standard BMAD installs are never | |||
| modified; the skills are automator-owned forks maintained against their upstream | |||
There was a problem hiding this comment.
The intro says the skills are “forks maintained against their upstream counterparts,” but this PR positions bmad-auto-dev as first-class/standalone and bmad-auto-resolve/bmad-auto-sweep as automator-native. Consider adjusting that sentence so it doesn’t imply all bundled skills are forks.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Fixed in 9fbfaa5. The intro no longer claims all skills are forks — it now says the skills are automator-owned, some forks of upstream (bmad-auto-review), others standalone or automator-native, consistent with the component table and the "Maintaining the forks" section below.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/automator/data/skills/bmad-auto-dev/SKILL.md (1)
14-15: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider rewording repeated sentence-initial "No" for clarity.
Static analysis flagged three successive sentences beginning with "No" (lines 14–15). While the repetition emphasizes the machine-first contract, rephrasing could improve readability without loss of meaning:
- No greeting. No questions. No menus. No editor. - No commit. No push. No remote ops. The orchestrator... + No greeting, questions, menus, or editor. + No commit, push, or remote operations — the orchestrator...This is optional; the current phrasing is emphatic and may be intentional for contract clarity.
🤖 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/automator/data/skills/bmad-auto-dev/SKILL.md` around lines 14 - 15, In the SKILL.md file, rephrase the bullet points on lines 14-15 to reduce repetition of sentence-initial "No" while maintaining the emphatic machine-first contract clarity. Consider restructuring at least one of the statements to convey the same restrictions (no greeting, questions, menus, editor, commit, push, or remote operations) with varied sentence construction, such as converting a "No" statement into an affirmative description of what is allowed or using different grammatical structures for the second bullet point.Source: Linters/SAST tools
🤖 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/automator/data/skills/bmad-auto-dev/SKILL.md`:
- Around line 14-15: In the SKILL.md file, rephrase the bullet points on lines
14-15 to reduce repetition of sentence-initial "No" while maintaining the
emphatic machine-first contract clarity. Consider restructuring at least one of
the statements to convey the same restrictions (no greeting, questions, menus,
editor, commit, push, or remote operations) with varied sentence construction,
such as converting a "No" statement into an affirmative description of what is
allowed or using different grammatical structures for the second bullet point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0489253-1dd5-4442-867f-b85192b64889
📒 Files selected for processing (23)
CHANGELOG.mdsrc/automator/data/skills/README.mdsrc/automator/data/skills/bmad-auto-dev/SKILL.mdsrc/automator/data/skills/bmad-auto-dev/automation-mode.mdsrc/automator/data/skills/bmad-auto-dev/customize.tomlsrc/automator/data/skills/bmad-auto-dev/step-01-clarify-and-route.mdsrc/automator/data/skills/bmad-auto-dev/step-01-resolve.mdsrc/automator/data/skills/bmad-auto-dev/step-02-plan.mdsrc/automator/data/skills/bmad-auto-dev/step-03-implement.mdsrc/automator/data/skills/bmad-auto-dev/step-04-finalize.mdsrc/automator/data/skills/bmad-auto-dev/step-04-review.mdsrc/automator/data/skills/bmad-auto-dev/step-05-present.mdsrc/automator/data/skills/bmad-auto-dev/step-auto-finalize.mdsrc/automator/data/skills/bmad-auto-dev/step-oneshot.mdsrc/automator/engine.pysrc/automator/policy.pysrc/automator/verify.pytests/conftest.pytests/test_engine.pytests/test_engine_worktree.pytests/test_generic_tmux.pytests/test_sweep.pytests/test_verify.py
💤 Files with no reviewable changes (6)
- src/automator/data/skills/bmad-auto-dev/step-05-present.md
- src/automator/data/skills/bmad-auto-dev/automation-mode.md
- src/automator/data/skills/bmad-auto-dev/step-04-review.md
- src/automator/data/skills/bmad-auto-dev/step-oneshot.md
- src/automator/data/skills/bmad-auto-dev/step-01-clarify-and-route.md
- src/automator/data/skills/bmad-auto-dev/step-auto-finalize.md
Replace the bmad-quick-dev fork (interactive workflow + an automation-mode.md decision table that existed only to override that interactivity) with a first-class machine-first skill: lean steps (resolve/plan/implement/review/ finalize), the orchestrator contract stated up front, no greeting/menus/HALTs. Preserves epic-context compilation, previous-story continuity, and the inline three-layer adversarial review: with review.enabled=false the dev session runs that inline triple-review itself before finalizing to done (a judge that did the planning is a better-informed judge); with review.enabled=true the orchestrator runs a separate fresh-context review session instead. Mirrors the upstream draft bmad-code-org/BMAD-METHOD#2498 (renamed bmad-dev-auto -> bmad-auto-dev to match the orchestrator's /bmad-auto-dev invocation). No engine change required. Also make the dev result.json `workflow` an enforced contract instead of a documented-but-ignored string: verify_dev/verify_dev_bundle reject a mismatch against verify.DEV_WORKFLOW ("auto-dev"), and the skill emits "auto-dev" rather than the misleading legacy "quick-dev". Review's "code-review" is documented as informational by design (verify_review is purely disk-derived and is never handed the result.json). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pbean
force-pushed
the
feat/bmad-auto-dev-standalone-skill
branch
from
c011230 to
1a622dd
Compare
…ordering Remediate review comments on PR #9: - verify_dev: the sprint-status gate accepted both 'review' and 'done' regardless of review_enabled, so a skip-review run that left the sprint at 'review' could pass dev verification. Make the expected sprint state conditional (mirrors the spec-status `expected`) and fix the hardcoded error text. Add a regression test + a docstring. (Augment, medium) - step-05-finalize.md / SKILL.md: "result.json is your last action" + "End the turn" contradicted the On Complete hook that runs afterward. Reword so result.json is the last *artifact* and the on_complete hook is an explicit numbered terminal step before ending the turn. (Augment, low) - skills/README.md: intro claimed all skills are upstream forks; bmad-auto-dev is standalone and resolve/sweep are automator-native. Soften the sentence to match the component table. (Augment, low) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
augment review |
| """Simulate a quick-dev bundle session (--dw-bundle): edits code, writes | ||
| the bundle spec, and (like step-auto-finalize bundle mode) marks the | ||
| """Simulate a bmad-auto-dev bundle session (--dw-bundle): edits code, writes | ||
| the bundle spec, and (like step-04-finalize bundle mode) marks the |
There was a problem hiding this comment.
This docstring references step-04-finalize, but that step file doesn’t exist in this skill anymore (the terminal step is step-05-finalize.md). Keeping the reference in sync would avoid confusion when using these helpers as documentation for the workflow.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Fixed in 45205ef. The docstring now references step-05-finalize (the terminal step that performs bundle ledger-marking) — there is no step-04-finalize; step 4 is step-04-review.md.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse "who" instead of "that" for person referent.
"Judge" refers to an agent; use "who" instead of "that".
✏️ Suggested fix
- better-informed); with `review.enabled = true` the orchestrator runs a separate fresh-context - review session instead. Mirrors the upstream draft bmad-code-org/BMAD-METHOD#2498 (renamed + better-informed); with `review.enabled = true` the orchestrator runs a separate fresh-context + review session instead. Mirrors the upstream draft bmad-code-org/BMAD-METHOD#2498 (renamedActually, the easier fix:
- better-informed); with `review.enabled = true` the orchestrator runs a separate fresh-context + better-informed); with `review.enabled = true` the orchestrator runs a separate fresh-contextLet me be more precise with the actual line content. The text is:
- a judge that did the planning is better-informed); with `review.enabled = true` + a judge who did the planning is better-informed); with `review.enabled = true`🤖 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 `@CHANGELOG.md` at line 18, In the CHANGELOG.md file, the text uses "that" to refer to "judge," which is an agent (person), but should use "who" instead. Find the sentence containing "a judge that did the planning" and replace "that" with "who" to correctly refer to the judge as a person. Use "who" for person and agent referents, and "that" for non-person referents.
🤖 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 `@CHANGELOG.md`:
- Line 18: In the CHANGELOG.md file, the text uses "that" to refer to "judge,"
which is an agent (person), but should use "who" instead. Find the sentence
containing "a judge that did the planning" and replace "that" with "who" to
correctly refer to the judge as a person. Use "who" for person and agent
referents, and "that" for non-person referents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9803e31d-c3bc-4978-8bb2-c2bd660f06a6
📒 Files selected for processing (24)
CHANGELOG.mdsrc/automator/data/skills/README.mdsrc/automator/data/skills/bmad-auto-dev/SKILL.mdsrc/automator/data/skills/bmad-auto-dev/automation-mode.mdsrc/automator/data/skills/bmad-auto-dev/customize.tomlsrc/automator/data/skills/bmad-auto-dev/spec-template.mdsrc/automator/data/skills/bmad-auto-dev/step-01-clarify-and-route.mdsrc/automator/data/skills/bmad-auto-dev/step-01-resolve.mdsrc/automator/data/skills/bmad-auto-dev/step-02-plan.mdsrc/automator/data/skills/bmad-auto-dev/step-03-implement.mdsrc/automator/data/skills/bmad-auto-dev/step-04-review.mdsrc/automator/data/skills/bmad-auto-dev/step-05-finalize.mdsrc/automator/data/skills/bmad-auto-dev/step-05-present.mdsrc/automator/data/skills/bmad-auto-dev/step-auto-finalize.mdsrc/automator/data/skills/bmad-auto-dev/step-oneshot.mdsrc/automator/engine.pysrc/automator/policy.pysrc/automator/verify.pytests/conftest.pytests/test_engine.pytests/test_engine_worktree.pytests/test_generic_tmux.pytests/test_sweep.pytests/test_verify.py
💤 Files with no reviewable changes (5)
- src/automator/data/skills/bmad-auto-dev/automation-mode.md
- src/automator/data/skills/bmad-auto-dev/step-05-present.md
- src/automator/data/skills/bmad-auto-dev/step-oneshot.md
- src/automator/data/skills/bmad-auto-dev/step-auto-finalize.md
- src/automator/data/skills/bmad-auto-dev/step-01-clarify-and-route.md
✅ Files skipped from review due to trivial changes (8)
- src/automator/engine.py
- src/automator/data/skills/bmad-auto-dev/step-05-finalize.md
- tests/conftest.py
- tests/test_engine_worktree.py
- tests/test_engine.py
- tests/test_sweep.py
- src/automator/policy.py
- src/automator/data/skills/bmad-auto-dev/customize.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_generic_tmux.py
- src/automator/verify.py
- tests/test_verify.py
- src/automator/data/skills/README.md
- src/automator/data/skills/bmad-auto-dev/step-02-plan.md
- conftest.py: bundle_dev_effect docstring referenced a nonexistent step-04-finalize; the terminal finalize step is step-05-finalize. (Augment) - CHANGELOG: "a judge that did the planning" -> "who" (person referent). (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eam #2500 Fold three refinements from bmad-code-org/BMAD-METHOD#2500 into the skill: - Spec Boundaries gain a frozen "Block If" tier enumerating decisions that cannot be made unattended. Triggering one in step-03 (implement) or step-04 (review) escalates CRITICAL (type: block-if); step-02 populates it during planning. Reuses the existing CRITICAL pause path — no engine change. - sync-sprint-status now writes a machine-readable `sprint-sync-skipped` frontmatter warning when a story key is absent (was log-only). - compile-epic-context uses a deterministic fallback sentence when planning artifacts are missing, and drops "Edit freely" from the regenerated banner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Closing — no longer valid. The bmad-dev-auto integration was substantially reworked (orchestrator-side review gate removed); the branch is being repurposed as release/0.6.5. |
1 participant
What
Rebuilds
bmad-auto-devfrom a near-clone fork ofbmad-quick-devinto a standalone, machine-first skill, and makes the devresult.jsonworkflowvalue an enforced contract.Adopts the direction of the upstream draft bmad-code-org/BMAD-METHOD#2498 (
feat(bmm): draft bmad-dev-auto skill), renamedbmad-dev-auto→bmad-auto-devto match the orchestrator's/bmad-auto-devinvocation, and fleshed out with the capabilities our orchestrator and sibling skills depend on.Why
The old
bmad-auto-devwas a hand-maintained fork ofbmad-quick-dev: a full interactive workflow (greeting, menus,step-05-present,step-oneshot) plus anautomation-mode.mddecision table whose only purpose was to override all that interactivity whenBMAD_AUTO_MODE=1. That meant upstreamdiff -rmerges to maintain, a SKILL.md that still read "Quick Dev Workflow / Greet the User," and anautomation-mode.mdlayer that existed only to suppress behavior we never wanted in automation.PR #2498 prototypes the cleaner model: a machine-first workflow with no interactive surface to override. This PR brings that model into
bmad-autowhile preserving everything the orchestrator actually relies on — including the inline triple-review.What changed
Skill rebuilt (
src/automator/data/skills/bmad-auto-dev/):step-01-resolve→step-02-plan→step-03-implement→step-04-review(inline triple-review) →step-05-finalize— replacing the dual-mode interactive step files and theautomation-mode.mdoverride layer.SKILL.mdstates the contract up front: Invocation (all four forms incl.--dw-bundle), Identity & I/O, escalation schema (CRITICAL/PREFERENCE), Result Schema. No greeting, no menus, no HALTs.step-01-clarify-and-route,step-05-present,step-oneshot,step-auto-finalize,automation-mode.md.review-loop-exceededcap); epic-context compilation + previous-story continuity for sprint stories;deferred-work-format.md(sibling-skill dependency forbmad-auto-review);spec-template.mdfrozen-block semantics (used bybmad-auto-resolve);sync-sprint-status.md.Review wiring (unchanged behavior, now machine-first): driven by the existing
review.enabledpolicy flag /$BMAD_AUTO_SKIP_REVIEW:review.enabled = true→ dev finalizes atin-review; the orchestrator runs a separate fresh-contextbmad-auto-reviewsession.review.enabled = false→ the dev session runs its own inline triple-review (step-04) and finalizes todone. As @alexeyv noted, a judging session that did the planning is a better-informed judge — so that inline path is retained, not dropped.workflowis now an enforced contract on the dev path (src/automator/verify.py):verify.DEV_WORKFLOW = "auto-dev";verify_dev/verify_dev_bundlenow reject a mismatchedworkflow(retryable) instead of ignoring the field. Previously this value was documented as a "machine contract" but only the sweep validators actually checked their workflow value — the dev path never did."auto-dev"rather than the misleading legacy"quick-dev"(a skill namedbmad-auto-devshould not reportquick-dev)."code-review"is left informational and documented as such:verify_reviewis purely disk-derived and is never handed theresult.json, so enforcing it there would require a signature change for marginal, redundant benefit. The README now states the per-skill enforcement status honestly.spec-template.md: reframed the<frozen-after-approval>reason as orchestrator-owned-intent (it's locked at approval; only a human renegotiation viabmad-auto-resolvemay change it), and dropped the interactive "Ask First → HALT and ask the user" boundary tier, which is meaningless in an unattended skill.Design decisions
bmad-quick-devfork. (bmad-auto-reviewremains abmad-code-reviewfork.)Testing
pytest: 954 passed (addedtest_verify_dev_wrong_workflowto lock the new contract; updated all dev-path fixtures to"auto-dev"). The only failures are pre-existing.claude//.agents/workspace-drift on otherbmad-auto-*skills — gitignored trees that are absent (skipped) in CI.trunk check(no filter): clean.bmad-auto-dev./bmad-auto-dev, and the on-disk contract (spec status, baseline, sprint-status, dw_ids) plus thereview.enabledskip-review wiring are unchanged.Follow-up (separate repo)
The same files should land upstream as bmad-code-org/BMAD-METHOD#2498, renamed
bmad-dev-auto→bmad-auto-devand extended with the epic-context + inline-review capabilities.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
bmad-auto-devdocs to formalize unattended “resolve → plan → implement → finalize” behavior and intent handling.Changed
auto-devinresult.json, with deterministic verification gates.Bug Fixes
sprint-sync-skippedwarning when a story key is missing.