fix: quick-dev self-review verifies AC coverage against parent requirements#2168
fix: quick-dev self-review verifies AC coverage against parent requirements#2168gabadi wants to merge 2 commits intobmad-code-org:mainfrom
Conversation
…ct requirements Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 Augment PR SummarySummary: Updates quick-dev planning guidance to prevent upstream (epic/PRD/story) requirements from being dropped during acceptance-criteria decomposition. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| 1. Investigate codebase. _Isolate deep exploration in sub-agents/tasks where available. To prevent context snowballing, instruct subagents to give you distilled summaries only._ | ||
| 2. Read `./spec-template.md` fully. Fill it out based on the intent and investigation, and write the result to `{wipFile}`. | ||
| 3. Self-review against READY FOR DEVELOPMENT standard. | ||
| 3. Self-review against READY FOR DEVELOPMENT standard. If the intent derives from a parent artifact (epic plan, PRD, story brief), verify that every requirement assigned to this scope has a corresponding AC — architectural mechanisms do not substitute for testable acceptance criteria. |
There was a problem hiding this comment.
This asks reviewers to ensure every parent-assigned requirement has a corresponding AC, but spec-template.md also says ACs should not duplicate I/O Matrix scenarios—requirements that are purely I/O-oriented may end up duplicated or unclear where they belong. Consider clarifying whether "corresponding" means represented either in the I/O Matrix or in ACs (vs always requiring an AC entry).
Severity: low
Other Locations
src/bmm-skills/4-implementation/bmad-quick-dev/spec-template.md:57
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes add validation constraints to the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
src/bmm-skills/4-implementation/bmad-quick-dev/spec-template.md (1)
57-58: Consider adding the architectural mechanisms caveat for consistency.The step-02-plan.md instruction (line 17) includes an important clarification: "architectural mechanisms do not substitute for testable acceptance criteria." This caveat addresses the core issue described in the PR objectives (epic annotations describing architectural mechanisms vs. explicit test expectations). Adding this same caveat to the template comment would provide consistent guidance at both the planning instruction and the template-filling points.
📝 Proposed addition for consistency
<!-- AC covers system-level behaviors not captured by the I/O Matrix. Do not duplicate I/O scenarios here. - If requirements were assigned from a parent artifact, every assigned requirement must have an AC here. --> + If requirements were assigned from a parent artifact, every assigned requirement must have an AC here. + Architectural mechanisms do not substitute for testable acceptance criteria. -->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-quick-dev/spec-template.md` around lines 57 - 58, Add the same architectural mechanisms caveat currently present in step-02-plan.md to the spec-template.md acceptance-criteria comment block: update the comment that begins "AC covers system-level behaviors not captured by the I/O Matrix..." to append a sentence such as "Note: architectural mechanisms do not substitute for testable acceptance criteria — ensure acceptance criteria state explicit, testable expectations rather than implementation or mechanism details." Ensure this exact caveat is present in the spec-template.md template comment so the guidance is consistent with step-02-plan.md.src/bmm-skills/4-implementation/bmad-quick-dev/step-02-plan.md (1)
17-17: Clarify what constitutes an "assigned requirement."The instruction says "every requirement assigned to this scope has a corresponding AC," but parent artifacts (epic plans, PRDs) contain multiple requirement categories: Functional Requirements, NonFunctional Requirements, Additional Requirements, and UX Design Requirements, plus an explicit FR Coverage Map that assigns specific FRs to stories.
Does "assigned requirement" mean:
- Only FRs listed in the FR Coverage Map?
- All requirement types (FR/NFR/Additional/UX) mentioned in the parent for this story's scope?
The PR objectives focus on FR Coverage Map ("create an epic with an FR coverage map assigning FRs to a story"), suggesting the primary concern is FRs explicitly assigned via the coverage map. Clarifying this scope would help agents distinguish between requirements that must have ACs vs. those that are contextual.
💡 Proposed clarification
-3. Self-review against READY FOR DEVELOPMENT standard. If the intent derives from a parent artifact (epic plan, PRD, story brief), verify that every requirement assigned to this scope has a corresponding AC — architectural mechanisms do not substitute for testable acceptance criteria. +3. Self-review against READY FOR DEVELOPMENT standard. If the intent derives from a parent artifact (epic plan, PRD, story brief), verify that every requirement explicitly assigned to this scope (e.g., in the parent's FR Coverage Map) has a corresponding AC — architectural mechanisms do not substitute for testable acceptance criteria.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-quick-dev/step-02-plan.md` at line 17, Clarify that "assigned requirement" in the READY FOR DEVELOPMENT checklist refers specifically to Functional Requirements (FRs) that are explicitly mapped to the story in the FR Coverage Map, not every requirement type in parent artifacts; update the sentence in step-02-plan.md to state that each FR listed in the FR Coverage Map for this story must have a corresponding, testable Acceptance Criterion (and add a short note that other requirement types—NonFunctional Requirements, Additional Requirements, UX Design Requirements—should be reviewed for applicability and captured as ACs only when they are explicitly within the story scope).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/bmm-skills/4-implementation/bmad-quick-dev/spec-template.md`:
- Around line 57-58: Add the same architectural mechanisms caveat currently
present in step-02-plan.md to the spec-template.md acceptance-criteria comment
block: update the comment that begins "AC covers system-level behaviors not
captured by the I/O Matrix..." to append a sentence such as "Note: architectural
mechanisms do not substitute for testable acceptance criteria — ensure
acceptance criteria state explicit, testable expectations rather than
implementation or mechanism details." Ensure this exact caveat is present in the
spec-template.md template comment so the guidance is consistent with
step-02-plan.md.
In `@src/bmm-skills/4-implementation/bmad-quick-dev/step-02-plan.md`:
- Line 17: Clarify that "assigned requirement" in the READY FOR DEVELOPMENT
checklist refers specifically to Functional Requirements (FRs) that are
explicitly mapped to the story in the FR Coverage Map, not every requirement
type in parent artifacts; update the sentence in step-02-plan.md to state that
each FR listed in the FR Coverage Map for this story must have a corresponding,
testable Acceptance Criterion (and add a short note that other requirement
types—NonFunctional Requirements, Additional Requirements, UX Design
Requirements—should be reviewed for applicability and captured as ACs only when
they are explicitly within the story scope).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 348a6e8f-e8b9-4c1e-9956-bed57d1d37e0
📒 Files selected for processing (2)
src/bmm-skills/4-implementation/bmad-quick-dev/spec-template.mdsrc/bmm-skills/4-implementation/bmad-quick-dev/step-02-plan.md
alexeyv
left a comment
alexeyv
left a comment
There was a problem hiding this comment.
Augment is right. An AC from an upstream document can end either in the ACs, or the I/O matrix section of the spec file. Not in both.
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
Agreeing with @alexeyv's feedback — the current wording says "every assigned requirement must have a corresponding AC" but requirements can legitimately be covered by the I/O Matrix instead.
Suggested fix: Change the wording to:
Every upstream-assigned requirement must be represented in either the Acceptance Criteria or the I/O Matrix — not both, and none omitted.
This preserves the intent (no requirement slips through) while respecting that I/O Matrix is a valid coverage mechanism. The template comment hint should get the same treatment.
Once that's addressed, this looks good to merge.
3 participants
What
Adds a general-purpose safeguard to
quick-devso that specs derived from parent artifacts (epics, PRDs) don't silently drop assigned requirements during AC decomposition.Why
When a story derives from an epic with requirement mappings,
quick-devcan omit ACs for assigned requirements — particularly when annotations describe architectural mechanisms rather than explicit test expectations. The self-review and adversarial review steps validate internal consistency but not completeness against upstream requirements.Fixes #2167
How
Both changes are one line each — no project-specific logic, preserves quick-dev's speed philosophy.
Testing
Validated against a real postmortem where FR18/FR19 were correctly assigned at the epic level but dropped during spec writing, causing a false PASS on traceability.