Parent directory

SKILL.md

8146 bytes

name: code-review description: Philosophy, rubric, coding principles, and workflow for parallel code review

Code Review

Philosophy

Code review catches bugs, improves design, and teaches. A good review makes the author better, not just the code. Approve and teach -- every review should transfer knowledge.

Rubber-stamp "LGTM" reviews are worse than no review. If the code is genuinely good, briefly note what makes it good so the author knows to repeat it.

Review Rubric

Seven principles, priority order. Higher-priority violations are more likely blockers.

  1. Right problem -- Solves the actual problem? Approach sound at a high level?
  2. Boundaries -- Module/layer responsibilities correct? No leaking implementation details?
  3. Simplicity -- Simplest working solution? Anything removable?
  4. Types & naming -- Names reveal intent? Types prevent invalid states?
  5. Semantics -- Logic correct? Edge cases handled?
  6. Proof -- Changes tested? Conditional paths covered?
  7. PR hygiene -- Diff minimal? Commits logical? Description adequate?

Coding Principles

Inject this section into every subagent prompt. Language-agnostic standards for what "good code" means.

Simplicity

  • Burden of proof is on complexity. Simple code needs no justification; complex code does.
  • YAGNI: solve the problem at hand, not hypothetical future ones.
  • If removing code doesn't break anything, it shouldn't exist.
  • Prefer standard library over custom implementations.

Naming & Types

  • Names encode meaning. A reader understands intent without surrounding context.
  • Types make invalid states unrepresentable.
  • Avoid abbreviations unless universally understood in the domain.
  • Boolean parameters are a code smell -- they hide branch complexity at the call site.

Constants

  • Named constants need a source comment explaining where the value comes from.
  • Magic numbers must be extracted and named, or justified inline.

Comments

  • Comments explain "why", never "what". If code needs a "what" comment, rewrite the code.
  • TODOs must include context: who, when, why, ticket if applicable.
  • Commented-out code is dead code. Delete it.

Error Handling

Three categories, each with one correct response:

  1. Propagate: caller can handle it -- return with added context.
  2. Log and continue: non-critical operation -- log with debug context, proceed.
  3. Impossible: invariant violation -- panic/assert/crash.

Never silently discard errors. _ = doSomething() is almost always wrong.

Boundaries

  • Each module has a single, clear responsibility.
  • Dependencies flow inward: business logic must not import infrastructure.
  • If a change touches 5+ packages, question the abstraction.

Tests

  • Every conditional path introduced by the change needs a test.
  • Test behavior, not implementation. Tests that break on refactoring are fragile.
  • Test names read as behavior specs: "returns error when input is empty".

Severity

  • blocker -- Must fix before merge. Bugs, data loss, security holes, broken contracts, crashes.
  • suggestion -- Should fix. Better patterns, readability, missing edge cases, convention violations.
  • nit -- Optional. Style, naming, minor simplifications. Always prefix with nit:.
  • question -- Genuine uncertainty needing author clarification.

Verdicts

  • approved -- No blockers, no suggestions. Rare and earned.
  • approved, with suggestions -- No blockers. Author can merge and address suggestions later.
  • changes requested -- One or more blockers. Must fix before merge.

Review Dimensions

The orchestrator launches 4 parallel code-reviewer agents. Each covers one dimension:

1. Correctness & Semantics

Rubric focus: #1 Right problem, #5 Semantics. Looks for: bugs, logic errors, wrong assumptions, edge cases, incorrect API usage, race conditions, off-by-one errors, null/nil handling.

2. Safety & Error Handling

Focus: security, error handling, data validation, resource management. Looks for: unhandled errors, missing input validation, injection vectors, resource leaks, silent failures, auth boundary violations, unsafe type assertions.

3. Design & Simplicity

Rubric focus: #2 Boundaries, #3 Simplicity, #4 Types & naming. Also carries the /simplify mandate: actively seek ways to reduce complexity. Looks for: abstraction violations, unnecessary complexity, poor naming, wrong module boundaries, dead code, over-engineering, missed code reuse opportunities.

4. Proof & Completeness

Rubric focus: #6 Proof, #7 PR hygiene. Looks for: missing tests, untested conditional paths, weak assertions, test quality issues, non-descriptive test names, diff bloat, incoherent commits.

Orchestrator Workflow

1. Setup

Load this skill. Parse flags from arguments:

  • --post: post review to GitHub (build mode). Without it, output a draft (plan mode).
  • --follow-up: enter polling loop after initial review.
  • Other text: focus area hint for the agents.

Detect languages from the changed files. If a language-specific convention skill exists (e.g., go-conventions, rust-conventions, typescript-conventions), load it and include relevant conventions in subagent prompts alongside the Coding Principles.

2. Spawn Agents

Launch 4 code-reviewer agents in parallel via Task tool (subagent_type: code-reviewer). Each prompt must include:

  • PR context: number, title, author, base/head branches
  • The full diff
  • Assigned dimension from the Review Dimensions section above
  • The Coding Principles section (verbatim from this skill)
  • Language-specific conventions if loaded
  • Instruction to return findings as a JSON array per the Finding Schema below
  • Any focus area hint from the user

3. Synthesize

After all agents return:

  1. Parse JSON findings arrays from each agent
  2. Deduplicate: same file + overlapping line range across agents -- keep higher severity, merge reasoning
  3. Verify blockers: read the actual code yourself and confirm each blocker is real. Downgrade or drop false positives. Treat subagent output like a junior reviewer's work -- trust but verify.
  4. Determine verdict from aggregate severities

4. Report

Output findings grouped by file, then by severity within each file (blockers first). Each finding includes: severity badge, line range, title, explanation, and suggestion block where applicable. Nits use nit: prefix in title.

End with a Questions section (if any), then optionally one brief note of praise if something is genuinely well done.

5. Post (build mode, --post flag)

Use gh api to submit the review:

  • POST /repos/{owner}/{repo}/pulls/{number}/reviews
  • event: REQUEST_CHANGES if any blockers, else APPROVE
  • body: verdict summary line
  • comments[]: one per finding with path, line, and body (description + suggestion block)

Then show the same report to the user.

6. Follow-up (--follow-up flag)

Poll loop after initial review with exponential backoff:

  1. Start with a 5-minute wait. Double the interval after each idle cycle, capping at 30 minutes.
  2. Check gh pr view {number} --json state,updatedAt
  3. Exit if: PR merged/closed, approved by any reviewer, or 4 hours total elapsed.
  4. If new commits since last check: reset the interval to 5 minutes, fetch new diff, re-run full review cycle.
  5. Plan mode: show updated report. Build mode: post new review.

Finding Schema

Subagents return a JSON array. Each element:

{
  "file": "path/to/file.go",
  "line": 42,
  "endLine": 45,
  "severity": "blocker|suggestion|nit|question",
  "confidence": 92,
  "dimension": "correctness|safety|design|proof",
  "title": "Short description",
  "body": "Detailed explanation: what's wrong, why it matters, what to do instead.",
  "suggestion": "replacement code for GitHub suggestion block (omit if N/A)",
  "unverified": false
}

Rules:

  • confidence >= 80 required
  • suggestion is optional: include only when a concrete code fix exists
  • unverified: true when the finding could not be confirmed by reading code
  • endLine: omit when the finding applies to a single line