Skip to content

Commit 58060d8

Browse files
Add full two-pass pipeline to review skill
Include candidate generation (file grouping, parallel subagents, aggregation) and validation (confidence-based filtering, strict deduplication) in SKILL.md. This makes the skill the single source of truth for the review process, used by both the CLI /review command and the CI action. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1 parent 0bf642b commit 58060d8

1 file changed

Lines changed: 97 additions & 2 deletions

File tree

plugins/core/skills/review/SKILL.md

Lines changed: 97 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
---
22
name: review
3-
version: 1.0.0
3+
version: 2.0.0
44
description: |
55
Review code changes and identify high-confidence, actionable bugs. Use when the user wants to:
66
- Review a pull request or branch diff
@@ -142,15 +142,110 @@ Each finding should include:
142142
- File path and line number
143143
- Optional: code snippet (<=3 lines) or suggested fix
144144

145+
If you have **high confidence** a fix will address the issue and won't break CI, include a suggestion block:
146+
147+
```suggestion
148+
<replacement code>
149+
```
150+
151+
Suggestion rules:
152+
- Keep suggestion blocks <= 100 lines
153+
- Preserve exact leading whitespace of replaced lines
154+
- Use RIGHT-side anchors only; do not include removed/LEFT-side lines
155+
- For insert-only suggestions, repeat the anchor line unchanged, then append new lines
156+
145157
## Deduplication
146158

147159
- Never flag the same issue twice (same root cause, even at different locations)
148160
- If an issue was previously reported and appears fixed, note it as resolved
149161

150162
<!-- END_SHARED_METHODOLOGY -->
151163

164+
## Two-Pass Review Pipeline
165+
166+
The review process uses two passes: candidate generation and validation.
167+
168+
### Pass 1: Candidate Generation
169+
170+
#### Step 0: Understand the PR intent
171+
172+
1. Read the PR description to understand the purpose and scope of the changes.
173+
2. If the PR description contains a ticket URL (e.g., Jira, Linear, GitHub issue link) or a ticket ID, **always fetch it** to understand the full requirements and acceptance criteria.
174+
175+
#### Step 1: Triage and group modified files
176+
177+
Before reviewing, triage the PR to enable parallel review:
178+
179+
1. Read the diff to identify ALL modified files
180+
2. Group files into logical clusters based on:
181+
- **Related functionality**: Files in the same module or feature area
182+
- **File relationships**: A component and its tests, a class and its interface
183+
- **Risk profile**: Security-sensitive files together, database/migration files together
184+
- **Dependencies**: Files that import each other or share types
185+
186+
3. Document your grouping briefly, for example:
187+
- Group 1 (Auth): src/auth/login.ts, src/auth/session.ts, tests/auth.test.ts
188+
- Group 2 (API handlers): src/api/users.ts, src/api/orders.ts
189+
- Group 3 (Database): src/db/migrations/001.ts, src/db/schema.ts
190+
191+
Guidelines for grouping:
192+
- Aim for 3-6 groups to balance parallelism with context coherence
193+
- Keep related files together so reviewers have full context
194+
- Each group should be reviewable independently
195+
196+
#### Step 2: Spawn parallel subagents to review each group
197+
198+
Use the Task tool to spawn parallel `file-group-reviewer` subagents. Each subagent reviews one group of files independently.
199+
200+
**IMPORTANT**: Spawn ALL subagents in a single response to enable parallel execution.
201+
202+
For each group, invoke the Task tool with:
203+
- `subagent_type`: "file-group-reviewer"
204+
- `description`: Brief label (e.g., "Review auth module")
205+
- `prompt`: Must include the PR context, the list of assigned files, the relevant diff sections, and instructions to return a JSON array of findings
206+
207+
#### Step 3: Aggregate subagent results
208+
209+
After all subagents complete, collect and merge their findings:
210+
211+
1. **Collect results**: Each subagent returns a JSON array of comment objects
212+
2. **Merge arrays**: Combine all arrays into a single comments array
213+
3. **Deduplicate**: If multiple subagents flagged the same location (same path + line), keep only one comment (prefer higher priority: P0 > P1 > P2)
214+
4. **Filter existing**: Remove any comments that duplicate issues already reported
215+
5. **Write reviewSummary**: Synthesize a 1-3 sentence overall assessment based on all findings
216+
217+
### Pass 2: Validation
218+
219+
The validator independently re-examines each candidate against the diff and codebase.
220+
221+
#### Validation rules
222+
223+
Apply the same Reporting Gate as above, plus reject if ANY of these are true:
224+
225+
- It's speculative / "might" without a concrete trigger
226+
- It's stylistic / naming / formatting
227+
- It's not anchored to a valid changed line
228+
- It's already reported (dedupe against existing comments)
229+
- The anchor (path/side/line/startLine) would need to change to make the suggestion work
230+
- It flags missing error handling / try-catch for a code path that won't crash in practice
231+
- It describes a hypothetical race condition without identifying the specific concurrent access pattern
232+
- It's about code that appears in the diff but is not part of the PR's primary change
233+
234+
#### Confidence-based filtering
235+
236+
- **P0 findings**: Approve if the trigger path checks out. These should be definite crashes/exploits.
237+
- **P1 findings**: Approve if you can verify the logic error or security issue is real.
238+
- **P2 findings**: Reject by default. Only approve if ALL of these are true: (1) you can independently verify the bug exists, (2) the bug has a concrete trigger a user or caller could realistically hit, and (3) the finding is NOT about edge cases, defensive coding, or style. When in doubt about a P2, reject it.
239+
240+
#### Strict deduplication
241+
242+
Before approving a candidate:
243+
1. **Among candidates**: If two or more candidates describe the same underlying bug (same root cause, even if anchored to different lines), approve only the ONE with the best anchor and clearest explanation. Reject the rest with reason "duplicate of candidate N".
244+
2. **Against existing comments**: If a candidate repeats an issue already covered by an existing PR comment, reject it.
245+
3. Same file + overlapping line range + same issue = duplicate, even if the body text differs.
246+
152247
## Output
153248

154-
Analyze the changes and provide a structured summary of findings. List each finding with its priority, file, line, and description.
249+
When invoked locally (TUI/CLI), analyze the changes and provide a structured summary of findings. List each finding with its priority, file, line, and description.
155250

156251
Do **not** post inline comments to the PR or submit a GitHub review unless the user explicitly asks for it.

0 commit comments

Comments
 (0)