Submatter/.agents/skills/code-review-two-axis/SKILL.md

---
name: code-review-two-axis
description: Review code changes along two axes: Standards (does code follow repo conventions?) and Spec (does it match the originating issue/requirement?). Uses parallel sub-agents to keep concerns separate, then aggregates findings. Use when role-reviewer runs, a human requests PR review, or you're asked to review "changes since X".
---

# Code Review: Two-Axis Method

A rigorous but practical code review method that separates **Standards** from **Spec**, runs them as parallel sub-agents, and aggregates findings. Adapted from the approach in [mattpocock/skills](https://github.com/mattpocock/skills/blob/main/skills/engineering/code-review/SKILL.md) for Echodo's convoy pipeline.

## When to Activate

Use this skill when:
- **Role-reviewer** is triggered during convoy `pr_review` gate
- A human requests a PR review in Mission Control
- Someone asks "review the changes since commit X" or "review branch Y"
- You're asked to evaluate if code meets the acceptance criteria from the originating brief

## Core Method

### Two Axes

**1. Standards (Does the code follow repo conventions?)**
- Follows `AGENTS.md` and codebase style
- Uses existing patterns (file locations, naming, imports)
- Passes linter/formatter checks
- No obvious code smells (from Fowler's refactoring book)

**2. Spec (Does the code match what was asked for?)**
- Implements acceptance criteria from the originating brief
- Matches the description in `.convoys/<slug>/brief-*.md`
- Addresses the issue/story/task it claims to solve
- No missing functionality or edge cases

### Why Two Separate?

- **Separation of concerns:** A change might be well-written (Standards) but not solve the problem (Spec), or vice versa
- **Clearer feedback:** "This is good code but doesn't match the spec" vs "This matches the spec but violates our style"
- **Parallel sub-agents:** Run both in parallel to avoid context pollution between concerns

## The Process

### Step 1: Pin the Fixed Point

Whatever the user specifies as the comparison base:
- Commit SHA: `abc1234`
- Branch: `feature-x`
- Tag: `v1.2.0`
- Relative: `HEAD~5`, `main`

**Capture:**
- Diff command: `git diff <fixed-point>...HEAD` (three-dot for merge-base comparison)
- Commit log: `git log <fixed-point>..HEAD --oneline`

**Validate:**
- `git rev-parse <fixed-point>` must resolve
- Diff must be non-empty

### Step 2: Identify the Spec Source

Find what the change is supposed to implement, in this order:

1. **Issue references in commits:** `#123`, `Closes #45`, etc.
2. **Brief path:** User passed `.convoys/<slug>/brief-N-*.md`
3. **Matching spec file:** Look in `docs/`, `specs/`, `.scratch/` for files matching branch name
4. **Ask the user:** If none found, ask where the spec is

### Step 3: Identify the Standards Sources

- `AGENTS.md` at repo root
- `.cursor/rules/*.mdc` files (if not migrated to `.agents/skills/`)
- `docs/` for architecture decisions
- Existing code patterns in the same module
- **Fowler code smells baseline** (below)

### Step 4: Run Both Sub-Agents in Parallel

Run **Standards agent** and **Spec agent** as separate conversations with their own context. Each receives:
- The diff
- Its specific criteria
- Output format instructions

**Standards sub-agent receives:**
- Diff
- Standards sources (AGENTS.md, rules, patterns)
- Code smell baseline
- Instruction: "Review ONLY for standards violations"

**Spec sub-agent receives:**
- Diff
- Brief/spec source
- Acceptance criteria from originating task
- Instruction: "Review ONLY for spec conformance"

### Step 5: Aggregate Findings

After both sub-agents complete, merge their reports:

```markdown
# Code Review: <convoy/branch>

## Summary
<Combined 2-3 sentence summary>

## Standards Findings
<Issues from Standards axis, grouped by severity>

## Spec Findings
<Issues from Spec axis, grouped by severity>

## Recommendations
<Combined recommendations, prioritized>

## Verdict
[ ] Approve - meets standards and spec
[ ] Approve with comments - minor issues, not blocking
[ ] Request changes - significant issues must be addressed
[ ] Block - blocker-level issues (security, correctness)
```

## Code Smell Baseline (Standards)

Even if repo docs say nothing, apply this baseline. **Repo rules always override.**

### The 11 Smells

| Smell | What It Is | How to Fix |
|-------|------------|------------|
| **Mysterious Name** | Function/variable/type name doesn't reveal purpose | Rename to something descriptive |
| **Duplicated Code** | Same logic in multiple files/hunks | Extract to shared function |
| **Feature Envy** | Method reaches into another object's data more than its own | Move method to the data it envys |
| **Data Clumps** | Same fields/params travel together | Bundle into a type |
| **Primitive Obsession** | Primitives standing in for domain concepts | Create small types |
| **Repeated Switches** | Same switch/if-cascade repeats | Use polymorphism or map |
| **Shotgun Surgery** | One logical change forces edits in many files | Gather related changes together |
| **Divergent Change** | One file edited for unrelated reasons | Split by concern |
| **Speculative Generality** | Abstraction/params added for imagined future needs | Delete until real need appears |
| **Message Chains** | Long `a.b().c().d()` navigation | Hide behind method on first object |
| **Middle Man** | Class/function that mostly just delegates | Call target directly |
| **Refused Bequest** | Subclass ignores most of what it inherits | Use composition, not inheritance |

## Severity Levels

| Level | Meaning | Action |
|-------|---------|--------|
| **Blocker** | Security vulnerability, correctness bug, data loss risk | Must fix before merge |
| **Major** | Significant issue affecting functionality or maintainability | Should fix before merge |
| **Minor** | Nits, style issues, minor improvements | Fix before merge or follow-up |
| **Nit** | Style preference, whitespace, naming convention | Optional, can defer |

## Categories

| Category | What It Covers |
|----------|----------------|
| **Correctness** | Logic errors, bugs, edge cases not handled |
| **Security** | Vulnerabilities, input validation, auth issues |
| **Performance** | Inefficiency, unnecessary work, memory issues |
| **Readability** | Clarity, naming, documentation |
| **Style** | Formatting, lint violations (let linter handle these) |
| **Test-Coverage** | Missing tests for new functionality |

## Example Review Output

### Input

- **Convoy:** `feature-add-auth-endpoint`
- **Fixed point:** `main`
- **Brief:** `.convoys/feature-add-auth-endpoint/brief-1-implement.md`
- **Acceptance criteria from brief:**
  - [ ] Route at `apps/web/app/api/auth/register/route.ts`
  - [ ] zod schema validates email/password
  - [ ] Database insert creates user row
  - [ ] Returns 201 on success, 400 on validation error

### Output

```markdown
# Code Review: feature-add-auth-endpoint

## Summary
Implements POST /api/auth/register endpoint with zod validation and database insert. Route file correctly placed. Meets most acceptance criteria. One validation edge case and missing test coverage identified.

## Standards Findings

### Minor
- **[style]** `apps/web/app/api/auth/register/route.ts:15` — Consider extracting validation schema to `lib/validations/auth.ts` for reusability
- **[readability]** `apps/web/app/api/auth/register/route.ts:8` — `createUser` function name could be more descriptive: `createUserFromCredentials`

### Nit
- **[style]** Missing blank line between imports

## Spec Findings

### Major
- **[correctness]** `apps/web/app/api/auth/register/route.ts:28` — Returns 500 on database error instead of appropriate 4xx. Should return 409 if email already exists.

### Minor
- **[test-coverage]** No test file created for this route. Brief AC didn't explicitly require tests but Echodo standard is to add tests.

## Recommendations

1. **High priority:** Handle duplicate email (409) and database errors (500 → 500, but logged)
2. **Medium:** Consider extracting validation schema
3. **Low:** Add minimal test coverage

## Verdict

[ ] Approve
[X] Approve with comments - one major issue should be addressed
[ ] Request changes
[ ] Block

## Metadata
- **Reviewed by:** role-reviewer (qwen-coder-mlx)
- **Model:** qwen-coder-mlx
- **Files reviewed:** 1
- **Lines changed:** ~45
```

## Running as Role-Reviewer

When triggered as `role-reviewer` in a convoy:

1. **Find the brief:** Look at convoy's current brief in `.convoys/<slug>/brief-*.md`
2. **Extract AC:** Get acceptance criteria from brief body
3. **Pin base:** Use `main` as default fixed point, or ask if different
4. **Run parallel agents:** Standards + Spec in parallel
5. **Aggregate:** Merge into final review per format above
6. **Log:** Call `log_convoy_event` with event type `role_complete`, actor `role-reviewer`
7. **Transition:** If blocker/major issues, transition convoy back to `in_progress`. If minor/nits, transition to `pr_review`.

## Interaction with Other Skills

- **backlog-sync:** When review involves changes to markdown import logic
- **convoy-lifecycle:** When logging role_complete and transitioning status
- **model-routing:** When determining which model runs the review
- **tdd-red-green:** When suggesting tests that should exist

## References

- **Original skill:** https://github.com/mattpocock/skills/blob/main/skills/engineering/code-review/SKILL.md
- **Fowler Refactoring:** Chapter 3 - "Bad Smells in Code"
- **AGENTS.md:** Root of repo for Echodo-specific standards
- **Echodo patterns:** Use existing code in `apps/web/`, `packages/` as standards reference