refactor: allow dev-story skill to modify Review Findings todo items#2189
refactor: allow dev-story skill to modify Review Findings todo items#2189cantecim wants to merge 1 commit intobmad-code-org:mainfrom
Conversation
📝 WalkthroughWalkthroughA development workflow documentation file was updated to expand the permitted modification scope, adding "Review Findings" to the list of story-file sections that developers are authorized to edit. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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
| - Communicate all responses in {communication_language} and language MUST be tailored to {user_skill_level} | ||
| - Generate all documents in {document_output_language} | ||
| - Only modify the story file in these areas: Tasks/Subtasks checkboxes, Dev Agent Record (Debug Log, Completion Notes), File List, Change Log, and Status | ||
| - Only modify the story file in these areas: Tasks/Subtasks checkboxes, Dev Agent Record (Debug Log, Completion Notes), File List, Change Log, Review Findings, and Status |
There was a problem hiding this comment.
Critical: Write conflict with code-review workflow.
Adding "Review Findings" to the permitted modification areas creates a write conflict. The bmad-code-review workflow (specifically src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md:19-36) also creates and writes to the "### Review Findings" subsection in the story file. Both workflows now claim write access to the same section with no defined precedence or coordination mechanism, which can lead to:
- Race conditions if both workflows run concurrently
- Lost updates when one workflow overwrites the other's changes
- Undefined behavior regarding which agent's modifications take precedence
Recommendation: Clarify the ownership model for "Review Findings":
- If dev-story should only read Review Findings (to execute follow-ups), remove it from the write-permitted list
- If dev-story should mark findings complete, add a subsection like "Review Findings Status" that only dev-story writes to, while code-review owns the findings themselves
- If both need write access, document the coordination protocol (e.g., code-review creates, dev-story only checks boxes)
🤖 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` at 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.
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.
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