Parent directory

SKILL.md

12879 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?
  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

  • 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.

A blocker requires a concrete bad outcome you can name: wrong data, a crash, a broken contract, a security hole. The following are NOT blockers, however much analysis went into finding them:

  • The PR description doesn't match what the code does. That is a suggestion to fix the description.
  • Existing behavior is preserved where the author expected change, or vice versa, with no harmful result. That is a question asking which they want.
  • A deliberate mechanism in untouched code behaves in a way the author may not have anticipated. That is a question. Machinery someone built on purpose is not a defect because this PR interacts with it.
  • It took you significant work to uncover. Effort spent is not severity.

If your own analysis concludes the data and runtime outcome is fine, you do not have a blocker. Notice when you are about to contradict yourself.

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.

Proportionality: the verdict describes the change, not the review. Before requesting changes, state the concrete harm of merging as-is in one sentence. If you can't, the verdict is "approved, with suggestions". Blocking a net-positive change over a description error or a stylistic disagreement costs the author real time and burns the trust you need for the next review.

Review Dimensions

Scale the fan-out to the diff. More agents on a small diff does not find more bugs -- it manufactures findings, because an agent handed a dimension will fill it rather than return an empty array.

| Diff | Agents | | ------------------------------------------------------ | ---------------------------------------------------------- | | Config-only, pure deletion, or under ~50 changed lines | 1 (correctness; fold in anything else you notice yourself) | | Single-concern change, one or two files | 2 (correctness, design) | | Multi-file feature work, new subsystems, migrations | 4 (all dimensions below) |

Take the first row that matches, and where two could apply, take the smaller count. A 2,000-line deletion of a retired subsystem is still a deletion; a two-file migration that only adds a column is still single-concern.

Dimension assignment is a lens, not a quota. Tell every agent explicitly that returning [] is the correct answer when the code is fine.

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 for happy path, 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 the number of code-reviewer agents the fan-out table calls for, 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

Rules for writing those prompts:

  • Never pass your own hypotheses to agents as established fact. Don't write "another reviewer verified X" or "note that X happens" for a conclusion you reached yourself. Agents build on it and echo it back, and you will read your own idea returning three times as independent corroboration. To get a hypothesis checked, send it to exactly one agent as an open question with no answer attached.
  • Scope: findings must land on lines in the diff. Issues in files the PR doesn't touch get one sentence each under an "adjacent, out of scope" heading, and can never be a blocker or affect the verdict. Reject "pre-existing but newly load-bearing" framing from agents -- if the PR didn't change it, it isn't this PR's finding.
  • Don't hand agents a curated list of files to read and things to look for. That's a scavenger hunt, and it produces hunt-shaped results.

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. Pay special attention to blockers that rest on EXTERNAL or RUNTIME facts the diff cannot prove -- a metric/series existing, a prod config value, an exporter's emitted names, third-party API behavior. Confirm these against the authoritative source (the live system, a query) if you have the tool; if you cannot, do not post them as blockers -- demote to a question that asks the author to confirm. This is exactly the confidently-wrong claim (e.g. "this metric doesn't exist") that wastes the author's time.
  4. Cut. Drop what the author loses nothing by not reading: several phrasings of one root cause, trivia (a count that's off by one, a typo in the description), and anything outside the diff beyond its single sentence. Three real findings beat eleven padded ones -- reviews that cry wolf get skimmed.
  5. Determine verdict from aggregate severities, then apply the proportionality check from Verdicts.

4. Report

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

Budget the output to the diff. A four-file config change does not warrant a 15KB review. If the honest answer is "two things, both minor", write exactly that and stop. Length is not thoroughness, and padding a thin review to look rigorous is the most common failure mode here.

Skip the scaffolding: no emoji severity badges, no table recounting the verification you performed (state a fact inline where it supports a finding), no bolded sentence fragments, no closing praise paragraph. Praise only something genuinely non-obvious and worth repeating, in one sentence, and never in the same breath as requesting changes.

If you could not verify the fact the review hinges on, lead with that and make it the single ask. Do not backfill the gap with findings about whatever you could inspect instead.

End with a Questions section, if any.

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