fix(skills): address code-review findings from PR #2007#2013
Conversation
- Remove name/description from step frontmatter (STEP-06) - Add explicit HALT before user menu in step-01 (STEP-04) - Escape story-id as template placeholder (REF-01) - Handle untracked files in file-list diff mode (P-6) - Formalize failed_layers variable handoff between steps (P-5) - Add Acceptance Auditor skip note for no-spec mode (P-4) - Guard false-clean when review layer failed (P-7) - Reword patch recommendation to avoid auto-fix solicitation (P-3) - Update SKILL.md description to reflect new capabilities (P-2)
🤖 Augment PR SummarySummary: This PR updates the Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| 4. **Drop** all `reject` findings. Record the reject count for the summary. | ||
|
|
||
| 5. If zero findings remain after dropping rejects, note clean review. | ||
| 5. If `{failed_layers}` is non-empty, report which layers failed before announcing results. If zero findings remain after dropping rejects AND `{failed_layers}` is non-empty, warn the user that the review may be incomplete rather than announcing a clean review. |
There was a problem hiding this comment.
{failed_layers} is referenced here, but this step has empty frontmatter (and workflow.md doesn’t define it), which likely violates tools/skill-validator.md REF-01 and can leave the placeholder unresolved at runtime.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| - `review-edge-case-hunter.md` (always) | ||
| - `review-acceptance-auditor.md` (only if `{review_mode}` = `"full"`) | ||
|
|
||
| HALT. Tell the user to run each prompt in a separate session and paste back findings. When findings are pasted, resume from this point and proceed to step 3. |
There was a problem hiding this comment.
In the fallback flow, it says to “resume … and proceed to step 3”, but step 3 is now earlier in the file; this can send execution backwards/loop instead of continuing forward through the numbered instructions.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| - For **commit range**: verify the range resolves. If it does not, HALT and ask the user for a valid range. | ||
| - For **provided diff**: validate the content is non-empty and parseable as a unified diff. If it is not parseable, HALT and ask the user to provide a valid diff. | ||
| - For **file list**: validate each path exists in the working tree. Construct `{diff_output}` by running `git diff HEAD -- <path1> <path2> ...`. If the diff is empty (files have no uncommitted changes), ask the user whether to review the full file contents or to specify a different baseline. | ||
| - For **file list**: validate each path exists in the working tree. Construct `{diff_output}` by running `git diff HEAD -- <path1> <path2> ...`. If any paths are untracked (new files not yet staged), use `git diff --no-index /dev/null <path>` to include them. If the diff is empty (files have no uncommitted changes and are not untracked), ask the user whether to review the full file contents or to specify a different baseline. |
There was a problem hiding this comment.
For “file list”, the instructions call for git diff HEAD -- ... plus git diff --no-index ... for untracked paths, but it’s not explicit how to merge those outputs into a single {diff_output}; there’s a risk of silently dropping either tracked or untracked file diffs.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughUpdates the bmad-code-review workflow documentation to introduce explicit user confirmation checkpoints (HALT prompts), track failed review layers during execution, expand diff source options including untracked files, and refine triage logic to account for incomplete reviews when layers fail. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan
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 |
2 participants
Summary
Fixes findings from the code review and skill validation of PR #2007:
name/descriptionfrom step file frontmatter (STEP-06 violation){story-id}as{{story-id}}template placeholder (REF-01)git diff --no-index(P-6){failed_layers}variable handoff between step-02 and step-03 (P-5)Test plan
npm run quality— all checks passbmad-code-review— no CRITICAL or HIGH findings