admin/.cursor/skills/code-review/SKILL.md

---
name: code-review
description: |
  Multi-phase code review against task specs with structured findings output.
  6-phase workflow: context loading, spec compliance, code quality, security quick-scan, performance scan, cross-task consistency.
  Produces a structured report with severity-ranked findings and a PASS/FAIL/PASS_WITH_WARNINGS verdict.
  Invoked by /implement skill after each batch, or manually.
  Trigger phrases:
  - "code review", "review code", "review implementation"
  - "check code quality", "review against specs"
category: review
tags: [code-review, quality, security-scan, performance, SOLID]
disable-model-invocation: true
---

# Code Review

Multi-phase code review that verifies implementation against task specs, checks code quality, and produces structured findings.

## Core Principles

- **Understand intent first**: read the task specs before reviewing code — know what it should do before judging how
- **Structured output**: every finding has severity, category, location, description, and suggestion
- **Deduplicate**: same issue at the same location is reported once using `{file}:{line}:{title}` as key
- **Severity-ranked**: findings sorted Critical > High > Medium > Low
- **Verdict-driven**: clear PASS/FAIL/PASS_WITH_WARNINGS drives automation decisions

## Input

- List of task spec files that were just implemented (paths to `[TRACKER-ID]_[short_name].md`)
- Changed files (detected via `git diff` or provided by the `/implement` skill)
- Project context: `_docs/00_problem/restrictions.md`, `_docs/01_solution/solution.md`

## Phase 1: Context Loading

Before reviewing code, build understanding of intent:

1. Read each task spec — acceptance criteria, scope, constraints, dependencies
2. Read project restrictions and solution overview
3. Map which changed files correspond to which task specs
4. Understand what the code is supposed to do before judging how it does it

## Phase 2: Spec Compliance Review

For each task, verify implementation satisfies every acceptance criterion:

- Walk through each AC (Given/When/Then) and trace it in the code
- Check that unit tests cover each AC
- Check that blackbox tests exist where specified in the task spec
- Flag any AC that is not demonstrably satisfied as a **Spec-Gap** finding (severity: High)
- Flag any scope creep (implementation beyond what the spec asked for) as a **Scope** finding (severity: Low)

## Phase 3: Code Quality Review

Check implemented code against quality standards:

- **SOLID principles** — single responsibility, open/closed, Liskov, interface segregation, dependency inversion
- **Error handling** — consistent strategy, no bare catch/except, meaningful error messages
- **Naming** — clear intent, follows project conventions
- **Complexity** — functions longer than 50 lines or cyclomatic complexity > 10
- **DRY** — duplicated logic across files
- **Test quality** — tests assert meaningful behavior, not just "no error thrown"
- **Dead code** — unused imports, unreachable branches

## Phase 4: Security Quick-Scan

Lightweight security checks (defer deep analysis to the `/security` skill):

- SQL injection via string interpolation
- Command injection (subprocess with shell=True, exec, eval)
- Hardcoded secrets, API keys, passwords
- Missing input validation on external inputs
- Sensitive data in logs or error messages
- Insecure deserialization

## Phase 5: Performance Scan

Check for common performance anti-patterns:

- O(n^2) or worse algorithms where O(n) is possible
- N+1 query patterns
- Unbounded data fetching (missing pagination/limits)
- Blocking I/O in async contexts
- Unnecessary memory copies or allocations in hot paths

## Phase 6: Cross-Task Consistency

When multiple tasks were implemented in the same batch:

- Interfaces between tasks are compatible (method signatures, DTOs match)
- No conflicting patterns (e.g., one task uses repository pattern, another does raw SQL)
- Shared code is not duplicated across task implementations
- Dependencies declared in task specs are properly wired

## Output Format

Produce a structured report with findings deduplicated and sorted by severity:

```markdown
# Code Review Report

**Batch**: [task list]
**Date**: [YYYY-MM-DD]
**Verdict**: PASS | PASS_WITH_WARNINGS | FAIL

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Critical | Security | src/api/auth.py:42 | SQL injection via f-string |
| 2 | High | Spec-Gap | src/service/orders.py | AC-3 not satisfied |

### Finding Details

**F1: SQL injection via f-string** (Critical / Security)
- Location: `src/api/auth.py:42`
- Description: User input interpolated directly into SQL query
- Suggestion: Use parameterized query via bind parameters
- Task: 04_auth_service

**F2: AC-3 not satisfied** (High / Spec-Gap)
- Location: `src/service/orders.py`
- Description: AC-3 requires order total recalculation on item removal, but no such logic exists
- Suggestion: Add recalculation in remove_item() method
- Task: 07_order_processing
```

## Severity Definitions

| Severity | Meaning | Blocks? |
|----------|---------|---------|
| Critical | Security vulnerability, data loss, crash | Yes — verdict FAIL |
| High | Spec gap, logic bug, broken test | Yes — verdict FAIL |
| Medium | Performance issue, maintainability concern, missing validation | No — verdict PASS_WITH_WARNINGS |
| Low | Style, minor improvement, scope creep | No — verdict PASS_WITH_WARNINGS |

## Category Values

Bug, Spec-Gap, Security, Performance, Maintainability, Style, Scope

## Verdict Logic

- **FAIL**: any Critical or High finding exists
- **PASS_WITH_WARNINGS**: only Medium or Low findings
- **PASS**: no findings

## Integration with /implement

The `/implement` skill invokes this skill after each batch completes:

1. Collects changed files from all implementer agents in the batch
2. Passes task spec paths + changed files to this skill
3. If verdict is FAIL — presents findings to user (BLOCKING), user fixes or confirms
4. If verdict is PASS or PASS_WITH_WARNINGS — proceeds automatically (findings shown as info)

## Integration Contract

### Inputs (provided by the implement skill)

| Input | Type | Source | Required |
|-------|------|--------|----------|
| `task_specs` | list of file paths | Task `.md` files from `_docs/02_tasks/todo/` for the current batch | Yes |
| `changed_files` | list of file paths | Files modified by implementer agents (from `git diff` or agent reports) | Yes |
| `batch_number` | integer | Current batch number (for report naming) | Yes |
| `project_restrictions` | file path | `_docs/00_problem/restrictions.md` | If exists |
| `solution_overview` | file path | `_docs/01_solution/solution.md` | If exists |

### Invocation Pattern

The implement skill invokes code-review by:

1. Reading `.cursor/skills/code-review/SKILL.md`
2. Providing the inputs above as context (read the files, pass content to the review phases)
3. Executing all 6 phases sequentially
4. Consuming the verdict from the output

### Outputs (returned to the implement skill)

| Output | Type | Description |
|--------|------|-------------|
| `verdict` | `PASS` / `PASS_WITH_WARNINGS` / `FAIL` | Drives the implement skill's auto-fix gate |
| `findings` | structured list | Each finding has: severity, category, file:line, title, description, suggestion, task reference |
| `critical_count` | integer | Number of Critical findings |
| `high_count` | integer | Number of High findings |
| `report_path` | file path | `_docs/03_implementation/reviews/batch_[NN]_review.md` |

### Report Persistence

Save the review report to `_docs/03_implementation/reviews/batch_[NN]_review.md` (create the `reviews/` directory if it does not exist). The report uses the Output Format defined above.

The implement skill uses `verdict` to decide:
- `PASS` / `PASS_WITH_WARNINGS` → proceed to commit
- `FAIL` → enter auto-fix loop (up to 2 attempts), then escalate to user