Patrick Plate
159 lines
3.8 KiB
Markdown
159 lines
3.8 KiB
Markdown
---
|
||
name: code-review
|
||
description: Structured code review against implementation plan.
|
||
---
|
||
|
||
# Skill: code-review
|
||
|
||
Structured code review against implementation plan.
|
||
|
||
## Invoked by
|
||
|
||
🔍 Reviewer mode
|
||
|
||
## Required Inputs
|
||
|
||
| Input | Source | Example |
|
||
|-------|--------|---------|
|
||
| `TICKET_KEY` | Jira issue key | `PROJECT-123` |
|
||
| `MODULE` | Module/component name | `auth`, `api`, `core` |
|
||
|
||
## Output
|
||
|
||
Markdown file: `docs/<MODULE>/<TICKET_KEY>/<TICKET_KEY>-review.md`
|
||
|
||
## Steps
|
||
|
||
### 1. Read the plan document
|
||
|
||
```bash
|
||
cat docs/<MODULE>/<TICKET_KEY>/<TICKET_KEY>-plan.md
|
||
```
|
||
|
||
Extract: planned changes, affected files, expected patterns, acceptance criteria.
|
||
|
||
### 2. Read the test plan (if exists)
|
||
|
||
```bash
|
||
cat docs/<MODULE>/<TICKET_KEY>/<TICKET_KEY>-testplan.md
|
||
```
|
||
|
||
Cross-reference: are all planned test cases implemented?
|
||
|
||
### 3. Get the diff
|
||
|
||
```bash
|
||
cd <your-repo-path>-<TICKET_KEY>
|
||
git diff origin/main --name-only
|
||
git diff origin/main --stat
|
||
git diff origin/main
|
||
```
|
||
|
||
### 4. Read changed files
|
||
|
||
For each changed file, read the full file to understand context — not just the diff hunks.
|
||
|
||
### 5. Run the review checklist
|
||
|
||
For each changed file, verify:
|
||
|
||
| # | Check | What to look for |
|
||
|---|-------|-----------------|
|
||
| 1 | Plan compliance | All plan items implemented? Nothing missing, nothing extra? |
|
||
| 2 | Pattern correctness | Correct project patterns used? |
|
||
| 3 | No generated source changes | Generated sources must never be modified manually |
|
||
| 4 | Logging | Parameterized messages (`log.debug("x: {}", v)`) — no string concatenation |
|
||
| 5 | Error handling | Proper error responses checked before parsing? Null-safe patterns? |
|
||
| 6 | Test coverage | Every new/modified public method has a test? Edge cases covered? |
|
||
| 7 | Database migrations | Correct naming convention? Dual DB dialect support? |
|
||
| 8 | No hardcoded values | No hardcoded IDs, instance names, or secrets? |
|
||
| 9 | Field visibility | Appropriate access modifiers? |
|
||
| 10 | Annotations | Correct use of framework annotations? |
|
||
|
||
### 6. Check test quality
|
||
|
||
For each test file:
|
||
- Meaningful assertions (not just `assertNotNull`)?
|
||
- Edge cases covered?
|
||
- Mocking done correctly?
|
||
- Test naming convention: `test<What>_<Scenario>_<Expected>()`?
|
||
|
||
### 7. Run tests
|
||
|
||
```bash
|
||
cd <your-repo-path>-<TICKET_KEY>
|
||
mvn test -pl <module-path> -f pom.xml
|
||
```
|
||
|
||
### 8. Generate review document
|
||
|
||
```markdown
|
||
# Code Review: <TICKET_KEY> — <Summary>
|
||
|
||
**Date:** <today>
|
||
**Module:** <MODULE>
|
||
**Reviewer:** Lumen (Reviewer)
|
||
**Branch:** <branch name>
|
||
**Status:** ✅ Approved / ⚠️ Approved with comments / ❌ Changes requested
|
||
|
||
---
|
||
|
||
## Summary
|
||
|
||
<1-2 sentence summary of the review outcome>
|
||
|
||
## Reviewed Files
|
||
|
||
| File | Change | Assessment |
|
||
|------|--------|-----------|
|
||
| `<path>` | New/Modified | ✅ / ⚠️ / ❌ |
|
||
|
||
## Checklist
|
||
|
||
| # | Check | Result | Note |
|
||
|---|-------|--------|------|
|
||
| 1 | Plan compliance | ✅ | All planned changes implemented |
|
||
| ... | ... | ... | ... |
|
||
|
||
## Findings
|
||
|
||
### ⚠️ Warnings (non-blocking)
|
||
|
||
1. **<file>:<line>** — <description>
|
||
- Recommendation: <suggested fix>
|
||
|
||
### ❌ Blockers (must fix)
|
||
|
||
1. **<file>:<line>** — <description>
|
||
- Reason: <why this must be fixed>
|
||
|
||
## Tests
|
||
|
||
- **Executed:** <N> tests
|
||
- **Passed:** <N> ✅
|
||
- **Failed:** <N> ❌
|
||
- **Build:** ✅ Green / ❌ Red
|
||
|
||
## Recommendation
|
||
|
||
<Final recommendation: merge / fix and re-review / reject>
|
||
```
|
||
|
||
### 9. Store in BigMind
|
||
|
||
```python
|
||
memory_store_fact(
|
||
category="codebase",
|
||
fact=f"{TICKET_KEY}: Code review completed — {status}. {findings_count} findings ({blockers} blockers)."
|
||
)
|
||
```
|
||
|
||
## Severity Levels
|
||
|
||
| Level | Symbol | Meaning | Action |
|
||
|-------|--------|---------|--------|
|
||
| Blocker | ❌ | Must fix before merge | Changes requested |
|
||
| Warning | ⚠️ | Should fix, not blocking | Approved with comments |
|
||
| Info | ℹ️ | Suggestion for improvement | Approved |
|
||
| OK | ✅ | No issues | — |
|