refactor(code-review): harmonize step-01 intent cascade#2206
Conversation
📝 WalkthroughWalkthroughThis is a documentation-only change to the code review context-gathering step. It replaces phrase-based intent detection with a five-tier cascading logic: explicit arguments, recent conversation history, sprint tracking, git state confirmation, and user prompts, restructuring control flow to skip remaining tiers once a target is identified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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: 2
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-code-review/steps/step-01-gather-context.md (1)
57-62:⚠️ Potential issue | 🟠 MajorInstruction 3 missing handlers for staged-only and uncommitted diff modes.
Tier 1 keywords (lines 26-27) detect "staged" and "uncommitted" modes, and instruction 2 (lines 51-52) offers these as explicit options. However, instruction 3's diff construction logic only handles:
- Branch diff (line 58)
- Commit range (line 59)
- Provided diff (line 60)
- File list (line 61)
When Tier 1 detects "staged changes" or "uncommitted changes" and proceeds directly to instruction 3 (per line 48), there's no corresponding construction handler. This creates a logic gap where identified targets have no execution path.
Proposed fix to add staged and uncommitted handlers
3. Construct `{diff_output}` from the chosen source. + - For **staged changes only**: run `git diff --cached` to capture staged changes. + - For **uncommitted changes** (staged + unstaged): run `git diff HEAD` to capture all uncommitted changes. - For **branch diff**: verify the base branch exists before running `git diff`. If it does not exist, HALT and ask the user for a valid branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md` around lines 57 - 62, Instruction 3 lacks handlers for "staged" and "uncommitted" modes so add explicit branches that construct {diff_output} for those cases: for "staged" generate the diff from the staged index (use the cached/staged git diff form) and for "uncommitted" generate the diff of the working tree vs HEAD/index (include unstaged changes and untracked files using --no-index where needed for new files); in both handlers validate the result is parseable and non-empty and HALT with a clear prompt if empty or unparseable, matching the same error/ask behavior described for branch/commit/provided/file-list paths in Instruction 3.
🧹 Nitpick comments (1)
src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md (1)
42-43: Consider rewording to reduce repetition.Three successive sentences begin with "If", which creates a slightly monotonous flow. While grammatically correct, varying the sentence structure improves readability.
Suggested rewrite
**Tier 4 — Current git state.** - If version control is unavailable, skip to Tier 5. Otherwise, check the current branch and HEAD. If the branch is not `main` (or the default branch), confirm: "I see HEAD is `<short-sha>` on `<branch>` — do you want to review this branch's changes?" If confirmed, treat as a branch diff against `main`. If declined, fall through. + Skip to Tier 5 if version control is unavailable. Otherwise, check the current branch and HEAD. When the branch is not `main` (or the default branch), confirm: "I see HEAD is `<short-sha>` on `<branch>` — do you want to review this branch's changes?" Treat as a branch diff against `main` if confirmed, otherwise fall through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md` around lines 42 - 43, Edit the "Tier 4 — Current git state." paragraph to remove repetitive sentence starts by combining checks and varying sentence openings: first state to check the current branch and HEAD, then describe the branch-not-main flow with a confirmation prompt phrased differently (e.g., "When the branch is not `main` (or the default), prompt: 'I see HEAD is `<short-sha>` on `<branch>` — do you want to review this branch's changes?'; if yes, treat as a branch diff against `main`, otherwise fall through"), ensuring the same logic is preserved but sentence openings are varied to improve readability.
🤖 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-01-gather-context.md`:
- Line 34: Replace the phrase "wants reviewed" in the sentence on
step-01-gather-context.md with a standard form such as "wants to be reviewed"
(or "wants reviewing") so the sentence reads e.g. "Do the last few messages
reveal what the user wants to be reviewed?" — locate the exact string "wants
reviewed" and update it accordingly.
- Line 24: The Tier 1 branch should always assign the provided spec path to the
spec_file variable regardless of whether baseline_commit is present: update the
logic where you inspect spec frontmatter/baseline_commit so that when a spec is
provided (Tier 1) you set spec_file = <provided spec> in both the
baseline_commit-found and baseline_commit-not-found branches; then modify
Instruction 4 (the question block currently phrased "Is there a spec?") to check
the spec_file variable instead of re-asking, and remove the ambiguous "note the
spec for instruction 4" behavior so downstream steps rely on spec_file.
---
Outside diff comments:
In
`@src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md`:
- Around line 57-62: Instruction 3 lacks handlers for "staged" and "uncommitted"
modes so add explicit branches that construct {diff_output} for those cases: for
"staged" generate the diff from the staged index (use the cached/staged git diff
form) and for "uncommitted" generate the diff of the working tree vs HEAD/index
(include unstaged changes and untracked files using --no-index where needed for
new files); in both handlers validate the result is parseable and non-empty and
HALT with a clear prompt if empty or unparseable, matching the same error/ask
behavior described for branch/commit/provided/file-list paths in Instruction 3.
---
Nitpick comments:
In
`@src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md`:
- Around line 42-43: Edit the "Tier 4 — Current git state." paragraph to remove
repetitive sentence starts by combining checks and varying sentence openings:
first state to check the current branch and HEAD, then describe the
branch-not-main flow with a confirmation prompt phrased differently (e.g., "When
the branch is not `main` (or the default), prompt: 'I see HEAD is `<short-sha>`
on `<branch>` — do you want to review this branch's changes?'; if yes, treat as
a branch diff against `main`, otherwise fall through"), ensuring the same logic
is preserved but sentence openings are varied to improve readability.
🪄 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: 48709b51-f55b-4dfa-acbe-1d15242cd32e
📒 Files selected for processing (1)
src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md
src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md
Outdated
Show resolved
Hide resolved
src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md
Outdated
Show resolved
Hide resolved
🤖 Augment PR SummarySummary: Refactors the Step 1 “Gather Context” logic to determine what code should be reviewed using a structured intent cascade. Changes:
Technical Notes: Tier 1 can route via PR/commit/branch or spec 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md
Outdated
Show resolved
Hide resolved
src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md
Outdated
Show resolved
Hide resolved
src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md
Outdated
Show resolved
Hide resolved
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
Human Review — Claude Opus 4.6
Overall Assessment: Approve with minor suggestions
The 5-tier priority cascade is a clear improvement over the previous flat keyword-matching approach. The structure mirrors checkpoint-preview and quick-dev patterns, which is good for consistency across the skill suite.
The cascade design (Tiers 1-5)
Solid. The priority ordering is intuitive and matches how users actually invoke reviews:
- Explicit argument — highest signal, correct to check first
- Recent conversation — natural for multi-turn workflows where the user just finished implementing something
- Sprint tracking — good ambient intelligence fallback
- Git state — sensible heuristic when nothing else is available
- Ask — correct last resort
The "stop as soon as the review target is identified" rule and "never ask extra questions beyond what the cascade prescribes" constraint are good guardrails that prevent over-prompting.
Backward compatibility for diff-mode keywords
Preserved correctly. The keyword list (staged, uncommitted, branch diff, commit range, provided diff) is identical to the original. Moving them into Tiers 1-2 as sub-checks rather than top-level entry points is the right call — they narrow scope within an already-identified target rather than being the target themselves.
Issues worth addressing
1. spec_file variable gap (agree with CodeRabbit + Augment)
Both bots flagged the same real issue: when a spec is provided in Tier 1 without baseline_commit, the instruction says "note the spec for instruction 4" but never sets {spec_file}. This is vague and conflicts with the "never ask extra questions" constraint — instruction 4 would re-ask for a spec the user already provided. Fix: set {spec_file} immediately in Tier 1 when a spec is provided, regardless of baseline_commit. Then make instruction 4 conditional on {spec_file} not already being set.
2. Instruction 3 missing staged/uncommitted handlers (agree with CodeRabbit)
Tier 1 can detect "staged" or "uncommitted" and proceed to instruction 3, but instruction 3 only has handlers for branch diff, commit range, provided diff, and file list. The staged/uncommitted handlers existed implicitly before but need explicit sub-cases now. This is outside the diff but the refactor creates the gap — worth fixing in this PR.
3. {branch} / {sha} placeholder ambiguity (disagree with Augment — low risk)
Augment flagged {branch} and {sha}..{sha} as potentially conflicting with variable resolution. I'd rate this low: these appear inside quoted keyword examples (e.g., "compared to {branch}"), not as standalone variable references. An LLM reading this will understand them as pattern descriptions. If the project has strict variable validation that would flag these, then escaping makes sense, but otherwise this is cosmetic.
4. Minor grammar: "wants reviewed" (line 34)
Dialectal phrasing — "wants to be reviewed" is standard. Trivial fix.
Summary
Good refactor. The two substantive items (spec_file variable coordination and staged/uncommitted handlers in instruction 3) should be addressed before merge. The rest is polish.
…v and checkpoint-preview Replace keyword-matching entry point with 5-tier priority cascade: explicit argument → recent conversation → sprint tracking → git state → ask. Diff-mode keyword detection preserved as sub-check within tier 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alexeyv
force-pushed
the
refactor/code-review-cascade
branch
from
87fa2b4 to
cd3d697
Compare
- Set {spec_file} immediately in Tier 1 when spec provided
- Add staged/uncommitted handlers to instruction 3 dispatch table
- Replace undefined {branch}/{sha} placeholders with angle brackets
- Fix {story_key} vs {{story-id}} placeholder mismatch
- Correct "wants reviewed" grammar to "wants to be reviewed"
Triage Summary6 findings triaged — FIX: 5, DISMISS: 1, DEFER: 0
All fixes in commit cc209dd. |
2 participants
Summary
Review patches
Test plan