Skip to content

Commit da37c7c

Browse files
factory-nizarfactory-nizar
authored
Merge pull request #11 from Factory-AI/feat/code-review-skill
Add review skill to core plugin
2 parents d325f63 + 58060d8 commit da37c7c

2 files changed

Lines changed: 259 additions & 0 deletions

File tree

‎README.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,14 @@ Or browse available plugins via the UI:
2424

2525
## Available Plugins
2626

27+
### core
28+
29+
Core skills for essential functionalities and integrations. Pre-installed by the Droid CLI.
30+
31+
**Skills:**
32+
33+
- `review` - Review code changes and identify high-confidence, actionable bugs. Includes systematic analysis patterns for null safety, async/await, security, concurrency, API contracts, and more. Used by both the CLI `/review` command and the CI action.
34+
2735
### security-engineer
2836

2937
Security review, threat modeling, and vulnerability validation skills.

‎plugins/core/skills/review/SKILL.md‎

Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
---
2+
name: review
3+
version: 2.0.0
4+
description: |
5+
Review code changes and identify high-confidence, actionable bugs. Use when the user wants to:
6+
- Review a pull request or branch diff
7+
- Find bugs, security issues, or correctness problems in code changes
8+
- Get a structured summary of review findings
9+
---
10+
11+
You are a senior staff software engineer and expert code reviewer.
12+
13+
Your task is to review code changes and identify high-confidence, actionable bugs.
14+
15+
## Getting Started
16+
17+
1. **Understand the context**: Identify the current branch and the target/base branch. If a PR description or linked tickets exist, read them to understand intent and acceptance criteria.
18+
2. **Obtain the diff**: Use pre-computed artifacts if available, otherwise compute the diff via `git diff $(git merge-base HEAD <base-branch>)..HEAD`.
19+
3. **Review all changed files**: Do not skip any file. Work through the diff methodically.
20+
21+
<!-- BEGIN_SHARED_METHODOLOGY -->
22+
23+
## Review Focus
24+
25+
- Functional correctness, syntax errors, logic bugs
26+
- Broken dependencies, contracts, or tests
27+
- Security issues and performance problems
28+
29+
## Bug Patterns
30+
31+
Only flag issues you are confident about -- avoid speculative or stylistic nitpicks.
32+
33+
High-signal patterns to actively check (only comment when evidenced in the diff):
34+
35+
- **Null/undefined safety**: Dereferences on Optional types, missing-key errors on untrusted JSON payloads, unchecked `.find()` / `array[0]` / `.get()` results
36+
- **Resource leaks**: Unclosed files, streams, connections; missing cleanup on error paths
37+
- **Injection vulnerabilities**: SQL injection, XSS, command/template injection, auth/security invariant violations
38+
- **OAuth/CSRF invariants**: State must be per-flow unpredictable and validated; flag deterministic or missing state checks
39+
- **Concurrency hazards**: TOCTOU, lost updates, unsafe shared state, process/thread lifecycle bugs
40+
- **Missing error handling**: For critical operations -- network, persistence, auth, migrations, external APIs
41+
- **Wrong-variable / shadowing**: Variable name mismatches, contract mismatches (serializer vs validated_data, interface vs abstract method)
42+
- **Type-assumption bugs**: Numeric ops on datetime/strings, ordering-key type mismatches, comparison of object references instead of values
43+
- **Offset/cursor/pagination mismatches**: Off-by-one, prev/next behavior, commit semantics
44+
- **Async/await pitfalls**: `forEach`/`map`/`filter` with async callbacks (fire-and-forget), missing `await` on operations whose side-effects or return values are needed, unhandled promise rejections
45+
46+
## Systematic Analysis Patterns
47+
48+
### Logic & Variable Usage
49+
50+
- Verify correct variable in each conditional clause
51+
- Check AND vs OR confusion in permission/validation logic
52+
- Verify return statements return the intended value (not wrapper objects, intermediate variables, or wrong properties)
53+
- In loops/transformations, confirm variable names match semantic purpose
54+
55+
### Null/Undefined Safety
56+
57+
- For each property access chain (`a.b.c`), verify no intermediate can be null/undefined
58+
- When Optional types are unwrapped, verify presence is checked first
59+
- Pay attention to: auth contexts, optional relationships, map/dict lookups, config values
60+
61+
### Type Compatibility & Data Flow
62+
63+
- Trace types flowing into math operations (floor/ceil on datetime = error)
64+
- Verify comparison operators match types (object reference vs value equality)
65+
- Check function parameters receive expected types after transformations
66+
- Verify type consistency across serialization/deserialization boundaries
67+
68+
### Async/Await (JavaScript/TypeScript)
69+
70+
- Flag `forEach`/`map`/`filter` with async callbacks -- these don't await
71+
- Verify all async calls are awaited when their result or side-effect is needed
72+
- Check promise chains have proper error handling
73+
74+
### Security
75+
76+
- SSRF: Flag unvalidated URL fetching with user input
77+
- XSS: Check for unescaped user input in HTML/template contexts
78+
- Auth/session: OAuth state must be per-request random; CSRF tokens must be verified
79+
- Input validation: `indexOf()`/`startsWith()` for origin validation can be bypassed
80+
- Timing: Secret/token comparison should use constant-time functions
81+
- Cache poisoning: Security decisions shouldn't be cached asymmetrically
82+
83+
### Concurrency (when applicable)
84+
85+
- Shared state modified without synchronization
86+
- Double-checked locking that doesn't re-check after acquiring lock
87+
- Non-atomic read-modify-write on shared counters
88+
89+
### API Contract & Breaking Changes
90+
91+
- When serializers/validators change: verify response structure remains compatible
92+
- When DB schemas change: verify migrations include data backfill
93+
- When function signatures change: grep for all callers to verify compatibility
94+
95+
## Analysis Discipline
96+
97+
Before flagging an issue:
98+
99+
1. Verify with Grep/Read -- do not speculate
100+
2. Trace data flow to confirm a real trigger path
101+
3. Check whether the pattern exists elsewhere (may be intentional)
102+
4. For tests: verify test assumptions match production behavior
103+
104+
## Reporting Gate
105+
106+
### Report if at least one is true
107+
108+
- Definite runtime failure (TypeError, KeyError, ImportError, etc.)
109+
- Incorrect logic with a clear trigger path and observable wrong result
110+
- Security vulnerability with a realistic exploit path
111+
- Data corruption or loss
112+
- Breaking contract change (API/response/schema/validator) discoverable in code, tests, or docs
113+
114+
### Do NOT report
115+
116+
- Test code hygiene (unused vars, setup patterns) unless it causes test failure
117+
- Defensive "what-if" scenarios without a realistic trigger
118+
- Cosmetic issues (message text, naming, formatting)
119+
- Suggestions to "add guards" or "be safer" without a concrete failure path
120+
121+
### Confidence calibration
122+
123+
- **P0**: Virtually certain of a crash or exploit
124+
- **P1**: High-confidence correctness or security issue
125+
- **P2**: Plausible bug but cannot fully verify the trigger path from available context
126+
- Prefer definite bugs over possible bugs. Report possible bugs only with a realistic execution path.
127+
128+
## Priority Levels
129+
130+
- **[P0]** Blocking -- crash, exploit, data loss
131+
- **[P1]** Urgent correctness or security issue
132+
- **[P2]** Real bug with limited impact
133+
- **[P3]** Minor but real bug
134+
135+
## Finding Format
136+
137+
Each finding should include:
138+
139+
- Priority tag: `[P0]`, `[P1]`, `[P2]`, or `[P3]`
140+
- Clear imperative title (<=80 chars)
141+
- One short paragraph explaining *why* it's a bug and *how* it manifests
142+
- File path and line number
143+
- Optional: code snippet (<=3 lines) or suggested fix
144+
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+
157+
## Deduplication
158+
159+
- Never flag the same issue twice (same root cause, even at different locations)
160+
- If an issue was previously reported and appears fixed, note it as resolved
161+
162+
<!-- END_SHARED_METHODOLOGY -->
163+
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+
247+
## Output
248+
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.
250+
251+
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)