fix(dev-agent): fix review findings subsection and step mismatch in workflow#2189
fix(dev-agent): fix review findings subsection and step mismatch in workflow#2189cantecim wants to merge 7 commits intobmad-code-org:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Dev Agent workflow text was updated to allow editing the "Review Follow-ups (AI)" subsection in the Dev Agent Record and to change the completion-control rule so Step 8 (instead of Step 6) determines whether to schedule a next session or pause review. The code-review presentation now writes follow-up items to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bmm-skills/4-implementation/bmad-dev-story/workflow.md (1)
311-333:⚠️ Potential issue | 🟠 MajorMajor: Missing implementation for marking Review Findings checkboxes as resolved.
While line 8 grants permission to modify Review Findings, Step 8 (lines 311–334) only marks items in two locations:
- Line 326: Tasks/Subtasks → Review Follow-ups (AI)
- Lines 329–330: Senior Developer Review (AI) → Action Items
However, it does not mark the corresponding checkboxes in the Review Findings section itself. Review Findings entries use markdown checkboxes (- [ ] unchecked, - [x] resolved) per the code-review workflow format. After resolving a review finding, the workflow should check off its checkbox in the Review Findings section to maintain consistency across all three tracking locations.
Add logic to Step 8 to find and mark the corresponding Review Findings checkbox as [x] alongside the existing Action Items update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-dev-story/workflow.md` around lines 311 - 333, Step 8 currently marks Review Follow-ups and the Senior Developer Review action item but misses updating the Review Findings checklist; add an action to locate the matching Review Findings entry (match by description/severity or related AC/file) and toggle its markdown checkbox from "- [ ]" to "- [x]" when resolving review follow-ups, similar to the existing actions that mark the Tasks/Subtasks checkbox and the "Senior Developer Review (AI) → Action Items" checkbox so all three tracking locations are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bmm-skills/4-implementation/bmad-dev-story/workflow.md`:
- Line 8: The story workflow currently claims write access to the "### Review
Findings" subsection which conflicts with the bmad-code-review workflow (see
bmad-code-review steps/step-04-present.md) that also writes that subsection;
update the story workflow (the allowed-modifications list in the workflow.md) to
remove "Review Findings" from writable areas and instead add a new subsection
name such as "Review Findings Status" that this dev-story can write; ensure any
automation that reads the original "Review Findings" still treats it as
read-only and update references to "Review Findings" in
src/bmm-skills/4-implementation/bmad-dev-story/workflow.md to point to the new
"Review Findings Status" when marking completion so only code-review owns the
actual findings while dev-story only updates status.
---
Outside diff comments:
In `@src/bmm-skills/4-implementation/bmad-dev-story/workflow.md`:
- Around line 311-333: Step 8 currently marks Review Follow-ups and the Senior
Developer Review action item but misses updating the Review Findings checklist;
add an action to locate the matching Review Findings entry (match by
description/severity or related AC/file) and toggle its markdown checkbox from
"- [ ]" to "- [x]" when resolving review follow-ups, similar to the existing
actions that mark the Tasks/Subtasks checkbox and the "Senior Developer Review
(AI) → Action Items" checkbox so all three tracking locations are updated
consistently.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5f39064-72f3-43d7-987a-cfd84dac348c
📒 Files selected for processing (1)
src/bmm-skills/4-implementation/bmad-dev-story/workflow.md
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
The intent is right — dev-story should be able to mark review findings as resolved. However there's a design concern to address:
Write-ownership conflict: bmad-code-review (step-04-present.md) creates and writes to "Review Findings." Granting dev-story full write access to the same section creates a conflict with no coordination protocol. Two workflows claiming write access to the same section is a recipe for overwrites.
Missing implementation: Step 8 of the dev-story workflow doesn't contain logic to actually mark Review Findings checkboxes as completed. The permission grant alone is inert without corresponding workflow instructions.
Suggestion: Consider one of these approaches:
- Narrow the permission to checkbox-toggling only (not content modification) and add Step 8 logic for marking findings done
- Add a "Review Findings Status" sub-section that dev-story owns, separate from the findings themselves that code-review owns
- Use a different mechanism like appending resolution notes rather than modifying the original findings
This would be a more robust solution than the quick permission grant. Happy to help work through the design if useful.
You are right on the approaches. This is very interesting, Claude models and GPT were already only marking items under Review Findings completed in dev-story skill run. I'm not 100% sure how this behavior is inferred from the workflow, but I see, "Mark task checkbox [x] in "Tasks/Subtasks → Review Follow-ups (AI)" section" instruction must lead to it. In this current approach, marking review findings completed is actually complete and done in dev-story skill, however, code-review skill uses the Review Findings section to write those to the story artifact. I have thought the same as your second suggestion, as briefly explained in the "How". To achieve this:
In this case, dev-story can not change any decision-needed finding to a concrete action (in reality, it can because of the same model's very own mental model) What do you think about this exact approach? Any objections/blind spots? I'll look into it when I have a better time |
cantecim
changed the title
Updated the instructions for writing findings in the story file to specify a new subsection for follow-ups.
Updated instructions to include 'Marking Review Follow-ups' in the modification areas.
cantecim
changed the title
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md`:
- Line 21: Update the permission description so it matches where the workflow
actually operates: in bmad-dev-story/workflow.md change the permission entry
that currently reads "Dev Agent Record (Debug Log, Completion Notes, Marking
Review Follow-ups)" to instead reference the Tasks/Subtasks subsection "Review
Follow-ups (AI)"; ensure the phrase "Marking Review Follow-ups" is replaced with
"Review Follow-ups (AI)" so the permission accurately reflects that the agent
may modify the Tasks/Subtasks → Review Follow-ups (AI) subsection used by the
checks/counts in the workflow (see the Task/Subtask headings created by the step
that appends "### Review Follow-ups (AI)").
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9eaa07d-2265-4b33-a4fd-95a147153854
📒 Files selected for processing (2)
src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.mdsrc/bmm-skills/4-implementation/bmad-dev-story/workflow.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bmm-skills/4-implementation/bmad-dev-story/workflow.md
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
This looks ready for a maintainer review @bmadcode |
2 participants
What
Agent allowed to write to Review Findings section in dev story skill
Why
In a strict mode, after a successfull dev story run, the agent can not mark corresponding findings completed.
How
In step 8, it expects "Review Follow-ups (AI)" list, but according to the review skill, the reviewer agent writes the findings under a general "Review Findings" section.
This patch could be quick fix for a strict run, but we better fix review skill to use expected style in the dev story workflow
Testing