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 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:

# 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

# 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