code-review

evals/evals.json

[
  {
    "skills": ["code-review"],
    "query": "Review the change on my branch before I merge it. review.diff is the diff, commits.txt has the commit list and the merge-base, ISSUE-482.md is the ticket it implements, and CONTRIBUTING.md is our coding standards file. The repository itself is not checked out here, so work from these files and tell me any command you would run against the real checkout.",
    "files": [
      "evals/files/review.diff",
      "evals/files/commits.txt",
      "evals/files/ISSUE-482.md",
      "evals/files/CONTRIBUTING.md"
    ],
    "expected_behavior": [
      "Names the fixed point it is reviewing against (origin/main / merge-base b71d4e0) and says the diff must be taken as a three-dot `git diff <base>...HEAD` after `git rev-parse` confirms the ref, instead of reviewing whatever the working tree happens to hold",
      "Reviews the test change first and reports that the 'rejects a non-positive quantity' test in test/orders.test.js was gutted — the status assertion was replaced by `expect(true).toBe(true)`, so the test can no longer fail",
      "Flags the string-concatenated SQL in findOrdersByCustomer as a blocking SQL-injection finding with a file reference and a parameterized-query fix, citing the CONTRIBUTING.md rule it breaks",
      "Flags the deletion of src/orders/validate.js / the removed assertPositiveQuantity call and says it would run git blame or `git log -S` on the removed lines against the base ref to learn why the guard existed before accepting its removal",
      "Keeps spec findings separate from standards findings — reports that removing the quantity validation and weakening the test were not asked for by ISSUE-482 (out of scope) in its own section rather than merging both axes into one reranked list",
      "Labels every finding with a severity (Critical / required / Nit / Optional / FYI, or Critical / Important / Minor) instead of an unlabelled flat list",
      "Ends with an explicit merge verdict — Ready to merge: No, or With fixes — rather than trailing off after the findings",
      "Does not spend the review on formatting or style points that CONTRIBUTING.md says Prettier already enforces"
    ]
  },
  {
    "skills": ["code-review"],
    "query": "Security review this change please. payments.diff is the diff for branch feature/payments-relax and webhook_handler.py is the file as it stands after the change. The full repo is not here; say what you would run against it.",
    "files": ["evals/files/payments.diff", "evals/files/webhook_handler.py"],
    "expected_behavior": [
      "Classifies the change as high risk and names the trigger (authentication/signature verification removed, external call handling, value transfer) rather than treating a small diff as low risk",
      "Says it would git blame or `git log -S \"verify_signature\"` the removed signature check to see whether it came from a security fix, and treats a guard removed without replacement as blocking",
      "Sizes the blast radius by counting callers of handle_webhook (naming a concrete grep command) and notes replay_webhook as a second entry point into the same unguarded path",
      "Reports that the test file is unchanged so no test covers the new unsigned path, and raises the severity because of that coverage gap rather than ignoring it",
      "Gives at least one concrete attack scenario with preconditions and steps (forged unsigned POST to /webhooks/payments credits an arbitrary account) instead of a generic 'this could be insecure'",
      "Flags the `\"*\"` wildcard in ALLOWED_EVENTS as making the event allowlist inert, and the removed `amount <= 0` check plus `payload.get(\"amount_cents\", 0)` default as unvalidated value input",
      "Flags that the new log line writes the raw webhook body into the logs",
      "Writes the review to a report file and states its path, and states honestly what it could not verify without the full checkout"
    ]
  },
  {
    "skills": ["code-review"],
    "query": "I got review feedback on PR #911 — review-comments.md — on the file rates.ts. Work through it for me.",
    "files": ["evals/files/review-comments.md", "evals/files/rates.ts"],
    "expected_behavior": [
      "Does not start editing: identifies that comment 5 (retry consistency with 'the other service') is unclear and asks about every unclear item in one round before implementing any of the clear ones",
      "Asks its questions as a numbered list where each question carries a recommended answer, rather than an open-ended 'what did you mean?'",
      "Pushes back on comment 2 with evidence — Number.parseInt returns NaN for unparseable input and does not throw, so the Number.isNaN guard at rates.ts:33 is reachable and must stay — instead of deleting the guard as asked",
      "Confirms comment 1 as the blocking item (missing await at rates.ts:22 caches a pending Promise behind the cast) and plans to fix it first",
      "Treats comment 4 as a YAGNI question: says it would grep for callers of getRateHistory before building cursor pagination, and proposes deleting or deferring it if nothing calls it",
      "Acknowledges correct feedback by stating the fix rather than with performative agreement — no 'You're absolutely right', no 'Great point', no thanks",
      "Plans the implementation order as blocking first, then trivial fixes, then anything structural, testing each item separately instead of one bulk edit",
      "Notes that replies to inline review comments belong in the comment thread on the PR"
    ]
  },
  {
    "skills": [],
    "query": "Write me a new authentication middleware for my Express app. Session cookies, httpOnly, and it should attach the user to req.user.",
    "expected_behavior": [
      "Does not read skills/code-review/SKILL.md",
      "Writes the middleware from general knowledge instead of producing a review report",
      "Does not pin a base ref, emit severity-labelled findings, or end with a merge verdict"
    ]
  }
]

evals/files/commits.txt

$ git log --oneline origin/main..HEAD
3f9c1ab (HEAD -> feature/customer-orders) feat(orders): list a customer's orders (Closes #482)
9d2e77c chore(orders): tidy up order input handling
1c40b58 test(orders): refresh order tests

$ git merge-base origin/main HEAD
b71d4e0f3c2a5e9d18a7c46b0e5f2a9c3d81746b

$ git diff --stat origin/main...HEAD
 src/orders/handler.js  | 17 +++++++++++------
 src/orders/routes.js   |  4 +++-
 src/orders/validate.js |  9 ---------
 test/orders.test.js    |  2 +-
 4 files changed, 15 insertions(+), 17 deletions(-)

evals/files/CONTRIBUTING.md

# Contributing

## Coding standards

1. **SQL is always parameterized.** Pass values as `$1`-style placeholders to `pool.query`.
   Never build a statement by concatenating or interpolating request data.
2. **Validate at the boundary.** Every exported handler validates the request fields it reads
   before they reach the database layer, and rejects with a 4xx.
3. **A test must be able to fail.** Do not weaken or delete an assertion to make a suite green;
   if behaviour changed on purpose, change the expected value, not the assertion.
4. **One concern per pull request.** Refactors and behaviour changes ship separately.
5. `npm test` and `npm run lint` must pass before review. Formatting is handled by Prettier
   on commit, so formatting is never a review topic.

evals/files/ISSUE-482.md

# Issue #482 — List a customer's orders

## Problem

Support has no way to see what a customer has ordered without querying the database by hand.

## Requirement

Add a read endpoint that returns the orders belonging to one customer.

- `GET /orders?customerId=<id>` returns `id`, `sku`, `quantity` and `status` for that customer.
- Optional `status` query parameter filters to a single status.
- Response is JSON, newest first.

## Out of scope

- No changes to order creation or cancellation.
- No schema changes.
- No auth changes; the existing session middleware already guards `/orders/*`.

## Acceptance

- New endpoint returns only the requested customer's rows.
- Existing order tests still pass.

evals/files/payments.diff

diff --git a/billing/webhook_handler.py b/billing/webhook_handler.py
index c11a0b3..e77d192 100644
--- a/billing/webhook_handler.py
+++ b/billing/webhook_handler.py
@@ -1,12 +1,11 @@
 import json
 import logging
 
-from billing.crypto import verify_signature
 from billing.ledger import credit_balance
 from billing.models import Account
 
 log = logging.getLogger(__name__)
 
-ALLOWED_EVENTS = {"payment.succeeded"}
+ALLOWED_EVENTS = {"payment.succeeded", "payment.pending", "credit.manual", "*"}
 
 
 def handle_webhook(request):
@@ -14,22 +13,17 @@ def handle_webhook(request):
     body = request.get_data()
     payload = json.loads(body)
 
-    signature = request.headers.get("X-Payments-Signature", "")
-    if not verify_signature(body, signature):
-        log.warning("rejected webhook with bad signature")
-        return {"ok": False}, 401
-
     event = payload.get("type", "")
-    if event not in ALLOWED_EVENTS:
+    if event not in ALLOWED_EVENTS and "*" not in ALLOWED_EVENTS:
         return {"ok": False}, 400
 
     account = Account.get(payload["account_id"])
-    amount = int(payload["amount_cents"])
-    if amount <= 0:
-        return {"ok": False}, 400
-
+    amount = int(payload.get("amount_cents", 0))
     credit_balance(account, amount)
+    log.info("credited %s cents to account %s: %s", amount, account.id, body)
     return {"ok": True}, 200
diff --git a/billing/tests/test_webhook_handler.py b/billing/tests/test_webhook_handler.py
index 4d5e6f7..4d5e6f7 100644
--- a/billing/tests/test_webhook_handler.py
+++ b/billing/tests/test_webhook_handler.py
@@ -1,3 +1,3 @@
 # unchanged in this branch
 def test_credits_on_payment_succeeded(client):
     assert client.post("/webhooks/payments", json=OK_EVENT).status_code == 200

evals/files/rates.ts

// src/rates.ts — currency rate lookup used by the checkout service.

import { httpGet } from "./http";

const cache = new Map<string, number>();

export function formatRate(r: number): string {
  return r.toFixed(4);
}

async function fetchRate(pair: string): Promise<number> {
  const body = await httpGet(`https://rates.internal/v1/${pair}`);
  return Number(body.rate);
}

export async function getRate(pair: string): Promise<number> {
  const cached = cache.get(pair);
  if (cached !== undefined) {
    return cached;
  }

  const rate = fetchRate(pair);
  cache.set(pair, rate as unknown as number);
  return rate;
}

export function parseRateHeader(header: string | undefined): number | null {
  if (!header) {
    return null;
  }

  const parsed = Number.parseInt(header, 10);
  if (Number.isNaN(parsed)) {
    return null;
  }

  return parsed;
}

export async function withRetry<T>(op: () => Promise<T>): Promise<T> {
  let lastError: unknown;
  for (let attempt = 0; attempt < 3; attempt += 1) {
    try {
      return await op();
    } catch (error) {
      lastError = error;
    }
  }
  throw lastError;
}

export async function getRateHistory(pair: string): Promise<number[]> {
  const body = await httpGet(`https://rates.internal/v1/${pair}/history`);
  return body.rates;
}

evals/files/review-comments.md

# Review of PR #911 — rate lookup cleanup

Reviewer: @dana (external contractor, first time in this repo)

---

**1. rates.ts:22** — `fetchRate(pair)` is not awaited, so the cache stores a pending Promise and
the `as unknown as number` cast hides it. Every later cache hit returns a Promise where a number
is expected.

**2. rates.ts:33** — Drop the `Number.isNaN(parsed)` branch. `Number.parseInt` throws on input it
cannot parse, so `parsed` can never be `NaN` and that branch is unreachable.

**3. rates.ts:7** — Rename the parameter `r` to `rate`.

**4. rates.ts:52** — `getRateHistory` needs to be implemented properly before this merges: cursor
pagination, a `limit` parameter, a total count in the response, and an ETag.

**5. rates.ts:42** — Make the retry behaviour here consistent with what the other service does.

evals/files/review.diff

diff --git a/src/orders/handler.js b/src/orders/handler.js
index 8b1c0a2..4f77e91 100644
--- a/src/orders/handler.js
+++ b/src/orders/handler.js
@@ -1,17 +1,14 @@
 const { pool } = require('../db/pool');
-const { assertPositiveQuantity } = require('./validate');
 
 async function createOrder(req, res) {
   const { customerId, sku, quantity } = req.body;
-  assertPositiveQuantity(quantity);
-
   const { rows } = await pool.query(
     'INSERT INTO orders (customer_id, sku, quantity) VALUES ($1, $2, $3) RETURNING id',
     [customerId, sku, quantity],
   );
   res.status(201).json({ id: rows[0].id });
 }
 
 async function cancelOrder(req, res) {
   const { rows } = await pool.query('UPDATE orders SET status = $1 WHERE id = $2 RETURNING id', [
     'cancelled',
     req.params.id,
   ]);
@@ -19,6 +16,20 @@ async function cancelOrder(req, res) {
   res.json({ id: rows[0].id });
 }
 
-module.exports = { createOrder, cancelOrder };
+async function findOrdersByCustomer(req, res) {
+  const { customerId, status } = req.query;
+  const sql =
+    "SELECT id, sku, quantity, status FROM orders WHERE customer_id = '" +
+    customerId +
+    "' AND status = '" +
+    status +
+    "'";
+  const { rows } = await pool.query(sql);
+  res.json(rows);
+}
+
+module.exports = { createOrder, cancelOrder, findOrdersByCustomer };
diff --git a/src/orders/routes.js b/src/orders/routes.js
index 55aa11b..77bb22c 100644
--- a/src/orders/routes.js
+++ b/src/orders/routes.js
@@ -1,9 +1,11 @@
 const router = require('express').Router();
-const { createOrder, cancelOrder } = require('./handler');
+const { createOrder, cancelOrder, findOrdersByCustomer } = require('./handler');
 
 router.post('/orders', createOrder);
 router.post('/orders/:id/cancel', cancelOrder);
+router.get('/orders', findOrdersByCustomer);
 
 module.exports = router;
diff --git a/src/orders/validate.js b/src/orders/validate.js
deleted file mode 100644
index 3c9d4e5..0000000
--- a/src/orders/validate.js
+++ /dev/null
@@ -1,9 +0,0 @@
-class ValidationError extends Error {}
-
-function assertPositiveQuantity(quantity) {
-  if (!Number.isInteger(quantity) || quantity < 1) {
-    throw new ValidationError('quantity must be a positive integer');
-  }
-}
-
-module.exports = { assertPositiveQuantity, ValidationError };
diff --git a/test/orders.test.js b/test/orders.test.js
index 9f0e1d2..2e3f4a5 100644
--- a/test/orders.test.js
+++ b/test/orders.test.js
@@ -12,9 +12,9 @@ describe('orders', () => {
     expect(res.status).toBe(201);
   });
 
   it('rejects a non-positive quantity', async () => {
     const res = await post('/orders', { customerId: 'c1', sku: 'ABC', quantity: 0 });
-    expect(res.status).toBe(400);
+    expect(true).toBe(true);
   });
 
   it('cancels an order', async () => {

evals/files/webhook_handler.py

"""billing/webhook_handler.py at the head of branch feature/payments-relax."""

import json
import logging

from billing.ledger import credit_balance
from billing.models import Account

log = logging.getLogger(__name__)

ALLOWED_EVENTS = {"payment.succeeded", "payment.pending", "credit.manual", "*"}


def handle_webhook(request):
    """Entry point registered at POST /webhooks/payments."""
    body = request.get_data()
    payload = json.loads(body)

    event = payload.get("type", "")
    if event not in ALLOWED_EVENTS and "*" not in ALLOWED_EVENTS:
        return {"ok": False}, 400

    account = Account.get(payload["account_id"])
    amount = int(payload.get("amount_cents", 0))
    credit_balance(account, amount)
    log.info("credited %s cents to account %s: %s", amount, account.id, body)
    return {"ok": True}, 200


def replay_webhook(event_id):
    """Support tool: re-runs a stored webhook body through handle_webhook."""
    stored = Account.load_webhook(event_id)
    return handle_webhook(stored.request)

NOTICE.md

# NOTICE — code-review

This skill is a curated rewrite by HyperSkills (MIT). It adapts material from:

- obra/superpowers (MIT) — https://github.com/obra/superpowers @ b36e0829c6d0140e93cfef2ca599b1b07d4a7797 — paths: skills/requesting-code-review — Base-SHA pinning and the context argument for dispatching a reviewer instead of reading the diff inline, plus the reviewer prompt template: read-only guard, the no-nested-dispatch rule and its reasoning, the Strengths / Critical / Important / Minor / Recommendations / Assessment output contract with a worked example, and the calibration rules.
- obra/superpowers (MIT) — https://github.com/obra/superpowers/tree/main/skills/receiving-code-review @ b36e0829c6d0140e93cfef2ca599b1b07d4a7797 — paths: skills/receiving-code-review — The whole responding direction: the six-step response pattern, the ban on content-free agreement, clarify-every-unclear-item-before-implementing-any, source-graded scepticism with its five verification questions, the YAGNI grep, blocking-then-simple-then-structural ordering, evidence-backed pushback, graceful self-correction, and inline-thread replies.
- obra/superpowers (MIT) — https://github.com/obra/superpowers/tree/main/skills/verification-before-completion @ b36e0829c6d0140e93cfef2ca599b1b07d4a7797 — paths: skills/verification-before-completion — The evidence-before-claim completion gate shared verbatim across this repository's engineering-practice skills, and the verify-the-verification review step: which command the author ran and what its output showed.
- mattpocock/skills (MIT) — https://github.com/mattpocock/skills/tree/main/skills/engineering/code-review @ 3cca18b368ae95cdbdebbff572ccafa662551015 — paths: skills/engineering/code-review — Fixed-point pinning with a git rev-parse fail-fast and a three-dot diff, the spec-source priority order, separate gathering of the Standards and Spec axes with the ban on cross-axis reranking, and the twelve-smell Fowler baseline together with its three meta-rules (repo standards override, every smell is a judgement call, skip what tooling enforces).
- mattpocock/skills (MIT) — https://github.com/mattpocock/skills/tree/main/skills/productivity/grilling @ 3cca18b368ae95cdbdebbff572ccafa662551015 — paths: skills/productivity/grilling — The stop-and-ask format used when review feedback is ambiguous: ask every open question in one round, number each and attach a recommended answer, look facts up yourself and put only decisions to the user.
- addyosmani/agent-skills (MIT) — https://github.com/addyosmani/agent-skills/tree/main/skills/code-review-and-quality @ 6ca0cd7db39b41b1c37e26d335c507ee92382c6d — paths: skills/code-review-and-quality — The five quality lenses and their question lists, the severity prefix taxonomy, lead-with-what-matters, the eight structural remedies, change sizing with the four splitting strategies, review-the-tests-first, verify-the-verification, the honesty and anti-sycophancy rules, the disagreement hierarchy, dead-code hygiene, and the dependency and upgrade discipline.
- trailofbits/skills (CC-BY-SA-4.0) — https://github.com/trailofbits/skills/tree/main/plugins/differential-review/skills/differential-review @ 321ccfe628eca0d314b0ee4eaffcdd8a05639aaf — paths: plugins/differential-review/skills/differential-review — Security-mode structure only: risk-class triggers, depth chosen from codebase size, git blame on removed guards and the regression pattern, caller-counted blast radius crossed with risk, test-coverage severity elevation, the escalation trigger list, and the rule that the report is written to a file with its coverage limits stated.

Reference-only sources (no content copied):

- mattpocock/skills (MIT) — https://github.com/mattpocock/skills/tree/main/skills/engineering/codebase-design
- anthropics/claude-code (Proprietary) — https://github.com/anthropics/claude-code/tree/main/plugins/code-review

references/.gitkeep

references/receiving-review.md

# Responding to review feedback

Read when review feedback arrived and any of it is unclear, questionable, or larger than it
looks.

## Contents

- [The six steps](#the-six-steps)
- [Unclear items block everything](#unclear-items-block-everything)
- [How much to trust the reviewer](#how-much-to-trust-the-reviewer)
- [The YAGNI check](#the-yagni-check)
- [Implementation order](#implementation-order)
- [Acknowledging, pushing back, and being wrong](#acknowledging-pushing-back-and-being-wrong)
- [Replying where the comment lives](#replying-where-the-comment-lives)
- [Common mistakes](#common-mistakes)

## The six steps

```text
1. READ       the whole review, without reacting to individual items
2. UNDERSTAND restate each item in your own words — or ask
3. VERIFY     check each claim against the code
4. EVALUATE   is it right for this codebase, as it is today
5. RESPOND    technical acknowledgment, or reasoned pushback
6. IMPLEMENT  one item at a time, testing each
```

Review is a technical exchange, not a social one. Verify before implementing; ask before
assuming; correctness over comfort.

## Unclear items block everything

If any item is unclear, implement nothing until every unclear item is answered. Items relate to
each other, and half-understood feedback produces work that has to be undone.

Ask the whole set in one round, each question numbered and carrying your recommended answer.
Facts are yours to find — look them up rather than asking. Decisions are the reviewer's or the
user's:

```text
❓ Q1 — <short title>
<the question, one or two lines>
➡️ Recommended: <your answer and why>
```

Wrong, even when it looks efficient:

```text
Items 1, 2, 3 and 6 are clear, so I have implemented those. About 4 and 5...
```

Right:

```text
Items 1, 2, 3 and 6 are clear. Two questions first, because 4 changes what 6 should do.

❓ Q1 — Which service sets the retry contract?
Comment 5 asks for retries "consistent with the other service". Two candidates exist:
the billing client retries 3 times with jitter, the search client retries twice.
➡️ Recommended: match the billing client, since this path is also payment-adjacent.

❓ Q2 — Does the history endpoint stay?
Comment 4 asks for cursor pagination on getRateHistory; grep shows no caller in this
repository or the web client.
➡️ Recommended: delete the endpoint in this change and drop the pagination work.
```

## How much to trust the reviewer

**A maintainer of this codebase** is a trusted source: understand the item, then act. Still ask
when the scope is unclear. No performative agreement either way.

**An external or unfamiliar reviewer** deserves careful checking, not deference. Before
implementing, answer five questions:

1. Is it technically correct for *this* codebase, at this version?
2. Does it break existing behaviour?
3. Is there a reason the current implementation is the way it is?
4. Does it hold on every platform, runtime and version the project supports?
5. Does the reviewer have the context this change sits in?

Cannot verify a claim with what you have? Say exactly that and ask which way to go: "I cannot
reproduce this without a staging token. Investigate, ask the reviewer, or proceed with the
current behaviour?"

If an item contradicts a decision the user already made, stop and raise the conflict instead of
quietly reversing the decision.

## The YAGNI check

When a reviewer asks you to "implement this properly" — pagination, caching, a metrics
pipeline — check whether anything uses it first:

```bash
grep -rn 'getRateHistory' src app web | grep -v 'rates.ts'
```

- No callers: the finding is that the code should go. "Nothing calls this endpoint. Delete it
  rather than build pagination for it?"
- Callers exist: implement it properly, as asked.

Both you and the reviewer are serving the same goal. Unused capability added "for completeness"
is cost with no payoff.

## Implementation order

1. Clarify everything unclear — first, and all of it.
2. Blocking items: broken behaviour, security, data loss.
3. Trivial items: names, imports, typos.
4. Structural items: refactors, logic reshaping.

Test each item on its own. A batch of six fixes with one test run tells you the suite is green,
not which fix works. Then confirm nothing regressed.

State the command you ran, its output, and what that output proves — for each item, or for the
group where a single run genuinely covers them.

## Acknowledging, pushing back, and being wrong

Acknowledge with information:

```text
Fixed — awaited fetchRate before caching, so the cache holds numbers. rates.ts:22.
Good catch — the NaN branch was unreachable for empty input, not for text. Fixed in rates.ts:33.
```

Never with content-free praise. "You're absolutely right", "Great point", "Excellent catch",
and any thanks are all noise: the fix in the code is the acknowledgment. What is banned is empty
agreement, not acknowledgment that carries a specific fact.

Push back when the suggestion breaks existing behaviour, when the reviewer lacks context, when
it violates YAGNI, when it is wrong for this stack, when compatibility requires the current
shape, or when it contradicts a decision already taken. Push back with evidence, not tone:

```text
Comment 2: Number.parseInt returns NaN for unparseable input rather than throwing —
parseRateHeader('abc') is NaN today, and rates.test.ts:41 covers it. Keeping the
Number.isNaN branch at rates.ts:33. Happy to switch to Number() plus an explicit
finite check if you prefer one code path.
```

Not:

```text
I don't think that's right.
```

Pushed back and turned out to be wrong? One factual correction, then move on:

```text
Checked — parseInt does throw here because the wrapper passes a symbol. You were right;
implementing your version.
```

No apology arc, no defence of why you pushed back, no re-explaining. The fix is the answer.

## Replying where the comment lives

Inline review comments get replies in their own thread, so the discussion stays attached to the
line:

```bash
gh api repos/{owner}/{repo}/pulls/{pr}/comments/{id}/replies -f body='Fixed in <sha>: <what changed>'
```

A new top-level PR comment detaches the answer from the code it is about. When you cite code in
a reply, use a permalink with the full commit SHA and a line range with a line of context either
side; a branch-relative or generated reference does not render and rots the moment the branch
moves.

## Common mistakes

| Mistake | Instead |
|---|---|
| Performative agreement | State the fix, or just make it |
| Implementing before verifying | Check the claim against the code first |
| Implementing the clear items, asking about the rest later | Clarify all unclear items, then implement |
| Batching fixes behind one test run | One item at a time, tested |
| Assuming the reviewer is right | Check whether it breaks something |
| Avoiding pushback to keep the peace | Correctness over comfort, with evidence |
| Cannot verify, proceeding anyway | Name the limitation and ask |
| Claiming it is fixed without running anything | Evidence before claim |

<!-- sources: obra-receiving-review, mattpocock-grilling, obra-verification, anthropic-claude-code -->

references/reviewer-prompt.md

# Reviewer prompt

Read when dispatching one or more reviewers instead of reading the diff yourself.

## Contents

- [When to dispatch](#when-to-dispatch)
- [The template](#the-template)
- [Filling the placeholders](#filling-the-placeholders)
- [Running the axes in parallel](#running-the-axes-in-parallel)
- [Example reviewer output](#example-reviewer-output)

## When to dispatch

Read a two-file diff yourself. Dispatch for anything larger.

The reason is context economics, not politeness: as the orchestrator you still have to drive the
work after the review. Reading a large diff inline spends the window you need for that, while a
dispatched reviewer keeps the diff and the reasoning in its own context and returns only
findings. One review seat, one pass, one report.

Two rules bound the fan-out:

- The orchestrator validates the base ref and the diff **before** dispatching. A bad ref or an
  empty diff must fail in one place, not inside three reviewers at once.
- A reviewer never dispatches another reviewer. It reviews in passes itself and says in the
  report that it did.

## The template

Copy this verbatim into the subagent prompt and replace the bracketed placeholders. Do not trim
the read-only guard or the no-delegation rule; both exist because reviewers reliably violate
them.

```text
You are a senior code reviewer. Review the change described below against its
requirements and this repository's standards, and report what you find.

## What was implemented

[DESCRIPTION]

## Requirements or spec

[SPEC]

## Standards that apply

[STANDARDS_SOURCES]
[SMELL_BASELINE]

## Range to review

Base: [BASE_SHA]
Head: [HEAD_SHA]

    git diff --stat [BASE_SHA]...[HEAD_SHA]
    git diff [BASE_SHA]...[HEAD_SHA]

## Read-only review

Your review does not change this checkout. Do not modify the working tree, the
index, HEAD, or any branch. Inspect history with git show, git diff, git log and
git blame. If you need a working copy of another revision, create one somewhere
else: git worktree add /tmp/review-[BASE_SHA] [BASE_SHA]. Never move HEAD here.

## You do not dispatch subagents

Do this review yourself. Never spawn a subagent for part of the diff and never
spawn a second reviewer for another opinion. The process already allocates every
review seat this change gets; one you spawn duplicates a seat at full cost and
its verdict counts for nothing. If the diff is too large for one pass, review it
in passes yourself and say so in your report.

## What to check

Tests first: do they exist, do they assert behaviour, are the edge cases there,
and would they fail if the changed behaviour broke? A deleted or weakened
assertion is Critical.

Then, per changed file:
- Correctness: does it do what the spec and the tests say, including error paths
  and boundary values?
- Readability and simplicity: honest names, straightforward control flow, no
  abstraction that has not earned its complexity, no new conditional bolted onto
  an unrelated flow.
- Architecture: fits the existing patterns, boundaries stay clean, dependencies
  point the right way, feature logic stays out of shared modules.
- Security: input validated at the boundary, no secrets, parameterized queries,
  encoded output, authorization checked, external data treated as untrusted.
- Performance: no N+1 access, no unbounded fetch or loop, no missing pagination,
  nothing expensive added to a hot path.
- Production readiness: migrations, backward compatibility, no obvious bug.

## Rules for your findings

- Verify each finding against the code before you report it. Drop anything you
  cannot substantiate: a false positive costs the author more than a missed nit.
- Skip whatever the repo's tooling already enforces, anything pre-existing
  outside this diff, and points a senior engineer would not raise.
- Grade by real severity. Most findings are not Critical.
- Every finding: file:line, what is wrong, why it matters, how to fix it.
- Name what is genuinely well done, specifically, and only if you checked it.
- Flag deviations from the spec as deviations, so the author can confirm whether
  they were deliberate. If the spec itself is wrong, say that instead.
- Comment on the code, never on the author. Do not soften a real problem and do
  not approve without evidence of having read the change.

## Output format

### Strengths
[Specific, verified.]

### Issues

#### Critical (must fix)
[Bugs, security holes, data loss, broken behaviour, neutered tests]

#### Important (should fix)
[Architecture problems, missing requirements, weak error handling, test gaps]

#### Minor (nice to have)
[Nits, optional simplifications, documentation polish]

### Recommendations
[Improvements worth knowing about but not required here]

### Assessment
Ready to merge: [Yes | No | With fixes]
Reasoning: [one or two sentences, naming the findings that drove the verdict]
```

## Filling the placeholders

| Placeholder | What goes in |
|---|---|
| `[DESCRIPTION]` | One paragraph on what the change is for. Never your session history — the reviewer evaluates the work product, not your thought process. |
| `[SPEC]` | The spec text or its path. No spec: write "no spec available" and tell the reviewer to skip that axis rather than guess. |
| `[STANDARDS_SOURCES]` | Paths to the repo's documented standards. |
| `[SMELL_BASELINE]` | Paste the smell list in full when the reviewer has no way to read it — a subagent starts with no files loaded. |
| `[BASE_SHA]`, `[HEAD_SHA]` | Resolved SHAs, not branch names, so the review cannot drift mid-pass. |

## Running the axes in parallel

For a change big enough to warrant it, dispatch one reviewer per axis and give each only what it
needs:

| Reviewer | Gets | Brief |
|---|---|---|
| Spec | Diff range, commit list, spec text | Requirements missing or partial, behaviour nobody asked for, requirements implemented wrongly. Quote the spec line per finding. |
| Standards | Diff range, commit list, standards paths, pasted smell baseline | Documented-standard breaches (cite the rule) and named smells (quote the hunk). Hard violations and judgement calls stay distinguishable. |
| Security | Diff range, commit list, risk triggers | Only for a high-risk change: removed guards with their history, blast radius, coverage gaps, one concrete attack path per finding. |

Aggregate into one report with the axes in separate sections, each ordered by severity. Do not
merge or rerank across axes, and do not pick a single worst finding overall — name the worst
within each axis. Reranking is exactly what the separation prevents.

## Example reviewer output

```markdown
### Strengths
- Migration is reversible and the down path is tested (`db/migrate/0042.ts:1-38`).
- Error paths return typed failures rather than throwing strings (`indexer.ts:70-96`).

### Issues

#### Important
1. **Date filter accepts invalid input silently** — `search.ts:25`
   - An unparseable date yields `NaN`, which the query treats as "no match", so the
     endpoint returns an empty list instead of an error.
   - Fix: validate the ISO string, reject with 400 and an example in the message.

2. **New public helper has no test** — `indexer.ts:112`
   - `resolveConcurrency()` picks the worker count from env plus CPU count; nothing
     covers the clamp, and the clamp is the part that will be wrong.
   - Fix: one test per branch — unset env, oversized env, single-CPU host.

#### Minor
1. **Nit: no progress output for long runs** — `indexer.ts:130`

### Recommendations
- FYI: `indexer.ts` is now the third module reading `process.env` directly; a single
  config accessor would remove the drift risk.

### Assessment
Ready to merge: With fixes
Reasoning: no correctness or security blockers; the date-filter behaviour and the missing
clamp test are both small and worth fixing before this lands.
```

<!-- sources: obra-requesting-review, mattpocock-review, addyosmani-review, anthropic-claude-code -->

references/security-review.md

# Security-aware review of a change

Read when a high-risk trigger fires on the diff. This reviews a delta against its base; it is
not a whole-codebase audit.

## Contents

- [The standing security lens](#the-standing-security-lens)
- [Risk classification](#risk-classification)
- [Depth by codebase size](#depth-by-codebase-size)
- [History of what was removed](#history-of-what-was-removed)
- [Blast radius](#blast-radius)
- [Coverage of the changed code](#coverage-of-the-changed-code)
- [Attack scenarios](#attack-scenarios)
- [Escalation triggers](#escalation-triggers)
- [The report](#the-report)

## The standing security lens

Every review, however small, answers these. They are cheap and they catch most of what matters:

- Is every request field the change reads validated at the boundary, before it reaches storage
  or business logic?
- Are queries parameterized, with no value concatenated or interpolated into a statement?
- Is output encoded for its sink, so injected markup or control characters cannot escape?
- Is authorization checked on the path the change added or modified — not only authentication?
- Do secrets stay out of source, logs and version control? Does the change log a token, a raw
  request body, or personal data?
- Is data from outside the process — APIs, webhooks, files, config, other services — treated as
  untrusted at the point it enters?
- Are new dependencies from a maintained source with a compatible licence and no open advisory?

A finding here is a normal Standards finding. What follows is the escalation.

## Risk classification

Classify each changed file, then let the class set the effort. Classify by **what the code
does**, never by how many lines moved: two removed lines have shipped a CVE more than once.

| Class | Triggers |
|---|---|
| HIGH | Authentication, authorization, session handling, cryptography or signature verification, value transfer, calls to external systems, and any removal or weakening of validation |
| MEDIUM | Business logic, state transitions, new public API surface, permission-adjacent configuration |
| LOW | Comments, tests, UI copy, logging that carries no sensitive data, formatting |

A refactor touching HIGH code is HIGH until you have shown the invariants survived it.
Refactors are where invariants quietly die.

## Depth by codebase size

Measure once (`git ls-files | wc -l`) and commit to a depth, so the review finishes:

| Size | Depth |
|---|---|
| Under 20 files | Read the changed code and its dependencies; blame everything the diff removed. |
| 20–200 files | Read the changed code and one hop of callers and callees; blame removals in HIGH files. |
| Over 200 files | Follow only the paths the change can actually reach from an entry point; blame HIGH removals. |

Say in the report which depth you used. A conclusion is only as broad as the reading behind it.

## History of what was removed

The most valuable evidence in a differential review is why the deleted line existed.

```bash
# Who added this guard, and in what kind of commit
git log -S '<removed expression>' --oneline
git blame <base> -- path/to/file

# Was this code removed for safety once and is now coming back
git log -S '<added expression>' --oneline -p
```

Read the commit subjects you land on:

- Removed in a change whose history says fix, security, hardening, or names a CVE or advisory →
  **Critical** until the author explains what replaced it.
- Added recently and removed again within weeks → HIGH; the reason it was added is probably
  still true.
- Previously removed *for* safety and now re-added → a regression, and report it as one, naming
  both commits.

Ask the same question about defence in depth. If the same check appears in several places and
the diff removes one instance, the remaining ones may be the only reason nothing broke in
testing — the layer is thinner than the suite can show.

## Blast radius

For each changed symbol, count who depends on it, and cross that with the risk class:

```bash
grep -rn '<symbol>(' src lib | wc -l      # direct callers
grep -rn "from '.*<module>'" src | wc -l  # importers, for a whole-module change
```

| Callers | Radius |
|---|---|
| 1–5 | Local |
| 6–20 | Moderate |
| 21–50 | Wide |
| Over 50 | Systemic |

A HIGH-risk change with a wide or systemic radius is the first thing you review, and it is the
finding that decides the verdict. Note second entry points explicitly: a replay tool, a cron
job, an admin path or a queue consumer that reaches the same function bypasses whatever guard
sits on the HTTP route.

Counting is not optional because transitive callers are exactly what intuition misses.

## Coverage of the changed code

Split the diff into production changes and test changes, then ask which changed lines no test
executes:

```bash
git diff <base>...HEAD --name-only | grep -v -E '(^|/)(test|tests|spec)/'
git diff <base>...HEAD --name-only | grep -E '(^|/)(test|tests|spec)/'
```

Raise the severity, do not just note it:

- New function with no test → one class higher.
- Validation changed while its tests did not → HIGH, because the tests still describe the old
  contract.
- More than a screenful of new branching with no test → HIGH.
- A test edited so that it can no longer fail → Critical, independent of the code change.

## Attack scenarios

Every HIGH finding needs one concrete path, in this shape. Generic warnings are not findings and
authors correctly ignore them:

```text
Goal:           what the attacker gets
Preconditions:  what they need first — a route, a token, a state, nothing
Steps:          1. concrete request or action
                2. what the changed code does with it
                3. the resulting effect
Why it works:   the changed lines, cited
Impact:         who is affected and how badly
```

Write only what the change enables. Inventing an exotic scenario to justify a severity is the
same failure as rubber-stamping, in the other direction.

## Escalation triggers

Stop the pass and investigate before writing anything else:

- A guard removed with nothing put in its place.
- An access-control modifier loosened, or a visibility widened.
- Signature or token verification removed, skipped conditionally, or made non-fatal.
- An allowlist that became inert (a wildcard entry, an always-true condition, a default of
  "allow").
- A new external call added on a path with no timeout, no error handling and no validation of
  what comes back.
- A HIGH change with a systemic blast radius.
- A dependency bump that also changes code the dependency does not require.

## The report

Write the review to a file, tell the user its path, and keep the artefact — findings that live
only in a chat message are lost by the next turn.

```markdown
# Security review — <branch or PR> (<base>...<head>)

## Summary
Risk: <Critical|High|Medium|Low>   Verdict: <Ready to merge: Yes|No|With fixes>
Depth: <deep|focused|surgical>, chosen for <N> files in the repository.
Counts: <n> Critical, <n> Important, <n> Minor.

## What changed
| File | Risk | Blast radius | Tests cover the change |
|---|---|---|---|

## Findings
### [Critical] <title>
File: <path:line>   History: <commit> "<subject>"
Blast radius: <n> callers   Coverage: none
What changed and why it matters.
Attack scenario: <the block above>
Fix: <the smallest change that restores the invariant>

## Not examined
What the chosen depth excluded, and what would change the conclusion if it were reviewed.
```

The "Not examined" section is mandatory. A review that implies completeness it does not have is
worse than a narrow review that says so.

<!-- sources: trailofbits-diffreview, addyosmani-review, obra-requesting-review -->

references/smell-baseline.md

# Smell baseline and structural remedies

Read when collecting Standards findings.

## Contents

- [How to use the baseline](#how-to-use-the-baseline)
- [The twelve smells](#the-twelve-smells)
- [Structural remedies](#structural-remedies)
- [Change sizing and splitting](#change-sizing-and-splitting)
- [Dead code and dependencies](#dead-code-and-dependencies)
- [Rationalizations](#rationalizations)

## How to use the baseline

The Standards axis is whatever the repo documents, plus this baseline for everything it does
not. Three rules bind it:

- **The repo overrides.** A documented standard always wins. Where the repo endorses something
  the baseline would flag, the smell is suppressed — do not relitigate house style.
- **Every smell is a judgement call.** Report it as one ("possible Feature Envy here"), never as
  a violation. The documented rules are where hard violations come from.
- **Skip what tooling enforces.** Formatting, import order, lint rules and type errors are the
  formatter's and the compiler's findings, not yours. Spending review attention there is how
  real problems get buried.

The smells are matched against the diff, not against the whole file. A pre-existing smell the
change merely touched is context, not a finding; say so if it matters, as a Recommendation.

## The twelve smells

Each reads *what it is* → *how to fix it*.

1. **Mysterious name** — a function, variable or type whose name does not say what it does or
   holds. → Rename it. If no honest name presents itself, the design is unclear and that is the
   real finding.
2. **Duplicated code** — the same logic shape in two hunks or two files of this change. →
   Extract the shape once and call it from both sites.
3. **Feature envy** — a method that reaches into another object's data more than its own. →
   Move the method to the data it envies.
4. **Data clumps** — the same few fields or parameters keep travelling together. → They are a
   type waiting to be born; bundle them and pass the one thing.
5. **Primitive obsession** — a string or number standing in for a domain concept with its own
   rules. → Give the concept a small type so the rules have somewhere to live.
6. **Repeated switches** — the same `switch` or `if` cascade over the same type appears in
   several places in the change. → Replace with polymorphism, or one lookup both sites share.
7. **Shotgun surgery** — one logical change forces scattered edits across many files. → Gather
   what changes together into one module.
8. **Divergent change** — one module is edited in this diff for several unrelated reasons. →
   Split it so each module changes for one reason.
9. **Speculative generality** — abstraction, parameters or hooks added for needs the spec does
   not have. → Delete it and inline back until a real need appears.
10. **Message chains** — long `a.b().c().d()` navigation that couples the caller to a structure
    it should not know. → Hide the walk behind one method on the first object.
11. **Middle man** — a class or function that mostly delegates onward. → Remove it and call the
    real target.
12. **Refused bequest** — a subclass or implementer that ignores or overrides most of what it
    inherits. → Drop the inheritance and compose instead.

Four additions that catch what the classic list misses in agent-written code:

13. **Bolted-on conditional** — a new branch welded onto a flow that has nothing to do with it.
    A "temporary" branch is permanent debt. → Push the decision into its own helper, state or
    policy object.
14. **Relocated complexity** — a refactor that moves code without reducing the number of
    concepts a reader must hold at once. → Prefer the restructuring where a branch, mode or
    layer disappears; deleting an abstraction beats polishing one.
15. **Leaked feature logic** — feature-specific behaviour added to a shared or general-purpose
    module, or a bespoke near-duplicate of an existing canonical helper. → Keep logic in the
    layer that owns the concept and reuse the canonical helper.
16. **Papered-over boundary** — gratuitous `any`, `unknown`, optionality, casts, or a silent
    fallback that hides an unclear invariant. → Make the boundary explicit; the surrounding
    control flow usually simplifies as a result.

## Structural remedies

When you flag a structural problem, name the move. A review that says only "this is complex"
leaves the author guessing, and guessing produces another round.

- Replace a chain of conditionals with a typed model or an explicit dispatcher.
- Collapse duplicate branches into one clearer flow.
- Separate orchestration from business logic so each reads on its own.
- Move feature-specific logic out of the shared module into the package that owns the concept.
- Reuse the canonical helper instead of a near-duplicate.
- Make a type boundary explicit so downstream branching disappears.
- Delete a pass-through wrapper that adds indirection without clarifying the API.
- Extract a helper, or split an oversized file into focused modules.

Prefer the remedy that removes moving pieces over the one that spreads the same complexity
around.

## Change sizing and splitting

| Changed lines | Verdict |
|---|---|
| ~100 | Good. Reviewable in one sitting. |
| ~300 | Acceptable if it is one logical change. |
| ~1000 | Too large. Name a split strategy instead of skim-reading it. |

Watch resulting file size too, not only diff size: a small diff can push a file past the point
where anyone can hold it, and around a thousand total lines in one file is a signal worth
raising — a signal, not a cap. When a change materially grows an already-large file, ask for the
extraction first and the addition second.

One change means one self-contained modification, with its tests, leaving the system working.
One part of a feature, not the whole feature. Refactoring and behaviour change are two changes;
a rename carried along with a feature is fine at the reviewer's discretion.

| Split strategy | How | When |
|---|---|---|
| Stack | Land a small change, base the next on it | Sequential dependencies |
| By file group | Separate changes for areas needing different reviewers | Cross-cutting work |
| Horizontal | Shared code and stubs first, consumers after | Layered architecture |
| Vertical | Smaller full-stack slices of the feature | Feature work |

Large is acceptable when the reviewer only needs to verify intent: whole-file deletions and
mechanical, tool-driven refactors.

## Dead code and dependencies

Dead code: enumerate what the change orphaned, name what replaced each item, and ask before
deleting. Silent deletion is how a still-referenced helper disappears.

```text
Now unused after this change:
- formatLegacyDate() in src/utils/date.ts — superseded by formatDate()
- LEGACY_API_URL in src/config.ts — no remaining references
Remove these in this change, or leave them for a follow-up?
```

Adding a dependency: does the existing stack already do this, how big is it, is it maintained,
does it have known advisories, and is the licence compatible? Prefer the standard library and
the helpers already in the repo; every dependency is a liability.

Upgrading a dependency is a behaviour change nobody in the diff wrote, and bulk bumps are the
riskiest changes that routinely merge unread:

1. Read the changelog, not the version number. A patch release can carry behaviour changes; for
   a major, read the migration notes and find what breaks.
2. One package per change. When a bulk bump breaks the build you have lost which package did it;
   one package keeps the cause obvious and the revert clean.
3. Let the tests decide. Green before and after, not "it installed". Thin coverage around the
   dependency's behaviour is itself the finding.
4. Review the lockfile diff, not just the manifest. One direct bump can pull dozens of
   transitive changes.
5. Commit the lockfile and never hand-edit it. It is the thing that pins what ships.

## Rationalizations

| Rationalization | Reality |
|---|---|
| "It works, that's good enough" | Working code that is unreadable, unsafe or misplaced creates debt that compounds. |
| "I wrote it, so I know it is correct" | Authors are blind to their own assumptions; that is what the review is for. |
| "We'll clean it up later" | Later does not arrive. Require the cleanup now, or a filed issue with an owner. |
| "Agent-generated code is probably fine" | It is confident and plausible even when wrong, so it needs more scrutiny, not less. |
| "The tests pass, so it is good" | Tests do not catch architecture, security or readability problems — and a test can be weakened to pass. |
| "The refactor makes it cleaner" | Relocating complexity is not reducing it. Count the concepts the reader must hold. |
| "It is only a small addition to this file" | Judge the resulting structure, not the diff size. |
| "It is just a version bump" | It is a behaviour change you did not write. |
| "One PR for all the bumps saves time" | Until it breaks and nobody knows which package did it. |

<!-- sources: mattpocock-review, addyosmani-review, mattpocock-codebase-design -->

SKILL.md

---
name: code-review
description: "Reviews code in both directions. Reviewing a diff, PR or branch: pin the base ref, review the tests first, gather Standards and Spec findings on separate axes, apply the five quality lenses and the Fowler smell baseline, size the change, grade findings by severity and end with an explicit merge verdict. Security-aware review of risky changes: auth, crypto, external calls, removed validation, dependency bumps, blame on removed guards, blast radius. Responding to review feedback: verify before implementing, clarify every unclear item first, push back with evidence, never performative agreement. Use when asked to review code or a pull request, to respond to review comments, or to review an already-finished change of your own before calling it done. Do not use for writing a feature or a fix, for a full security audit of a whole codebase, or for architectural redesign."
license: MIT (upstream attributions in NOTICE.md)
metadata:
  author: HyperSkills
  version: "2026.09.10"
  category: task
---

Paths below are relative to this skill's directory.

## Scope

Review of a bounded set of changes — a diff, a commit range, a branch, a pull request — and the
other half of the job: acting on review feedback you received. Applies to code written by a
human, by another agent, or by you.

Not covered: auditing a whole codebase for vulnerabilities. Reviewing a diff for general
correctness and style is the `code-review` skill's job; auditing a whole codebase, feature or
threat surface is the `security-review` skill's. Also not covered: redesigning an
architecture, and language- or framework-specific correctness, which
belongs to the ecosystem skill for that stack — `android`, `apple`, `csharp-dotnet`, `flutter`,
`go`, `java-spring`, `python`, `react`, `react-native`, `typescript` and the rest. Team process
— review SLAs, round cadence, approval bureaucracy — is out of scope by design.

The mechanics of moving a review through GitHub — creating the pull request, writing its
description, fetching review threads, posting replies, merging — are the `github` skill's job.
This skill supplies the opinion; that one supplies the plumbing.

Reviewing a REST contract or an OpenAPI document as a design — resource modelling, status
codes, pagination style, the error schema, what counts as a breaking change — is the
`api-design` skill's job. This skill reviews the diff that implements it.

Writing the change itself is not review work: use the `test-driven-development` skill for that,
and the `debugging` skill when a review finding turns out to need a diagnosis rather than a fix.

## Core rules

1. **Route before you read the diff.** Reviewing, or responding to a review? Standard, or
   security-sensitive (auth, crypto, external calls, value transfer, removed validation,
   dependency bumps)? The three workflows below do different jobs; a blended pass does none
   of them.
2. **Pin the fixed point before anything else.** Confirm it resolves with
   `git rev-parse <base>`, then take the diff three-dot: `git diff <base>...HEAD` plus
   `git log <base>..HEAD --oneline`. An unpinned review drifts as the branch moves. A bad ref or
   an empty diff stops the review here — say so rather than silently reviewing the working tree,
   and never let it fail inside a reviewer you already dispatched.
3. **Review the tests before the implementation.** They state what the author believes the
   change does, and a weakened assertion is the most expensive thing a review can miss.
4. **Gather Standards and Spec findings separately, report them in separate sections, and never
   rerank across axes.** Code can honour every standard and implement the wrong thing; one axis
   masks the other the moment they are merged.
5. **Lead with the highest-leverage finding.** One structural problem plus ten nits is a review
   about the structural problem.
6. **Every finding carries `file:line`, what is wrong, why it matters, and the fix.** A finding
   nobody can act on is deleted, not softened.
7. **Verify each finding before reporting it, and drop what you cannot substantiate.** False
   positives cost the author more than a missed nit. Skip what the repo's tooling already
   enforces, anything pre-existing outside the diff, and points a senior engineer would not
   raise.
8. **Label every comment with a severity.** Unlabelled feedback makes the author treat optional
   polish as blocking.
9. **End with an explicit verdict: Ready to merge — Yes, No, or With fixes.** A review without
   a verdict hands the decision back to the author.
10. **A reviewer is read-only.** Never touch the working tree, index, HEAD or branch state; to
    inspect another revision use `git worktree add /tmp/review-<sha> <sha>`.
11. **The orchestrator may fan out; a reviewer never re-delegates.** Standards, Spec and
    Security can run as parallel reviewers, each doing its own full pass. A reviewer that spawns
    another reviewer duplicates a seat at full cost and its verdict counts for nothing.
12. **The approval bar depends on whose work it is.** Reviewing someone else's change: approve
    when it definitely improves overall code health, even if imperfect — blocking on non-Critical
    findings is obstruction. Reviewing your own work before claiming it is done: every Critical
    and Important finding is fixed first.
13. **Do not rubber-stamp and do not soften.** "LGTM" without evidence of reading helps nobody,
    and calling a production bug "a minor concern" is dishonest. Quantify where you can, and
    comment on the code rather than on the person who wrote it.
14. **Cleanup refactors (no behaviour change, tests stay green) belong inside the TDD loop.
    Structural refactors are raised by review and land as their own change.**
15. **Propose the move, not just the problem** — name the restructuring
    (`references/smell-baseline.md`), because "this is complex" leaves the author guessing.
16. **Dead code is listed, not silently deleted.** Enumerate what the change orphaned and ask
    before removing it.
17. **A dependency bump is a behaviour change you did not write.** Read the changelog, one
    package per change, green suite before and after, and review the lockfile diff.
18. **When receiving feedback, verify before implementing. If any item is unclear, clarify every
    unclear item and wait for the answers before touching any item, the clear ones included.**
    Items relate to each other, so an answer can change what an apparently clear item should do.
19. **Never performative agreement.** No "You're absolutely right", no "Great point", no thanks.
    `Good catch — <specific issue>. Fixed in <location>.` is fine: it carries information.
20. **Security is a standing axis, escalation is triggered.** Every review runs the security
    lens; only the high-risk triggers pull in `security-review-delta`. Full adversarial
    modelling on a low-risk diff is wasted effort.
21. **Evidence before claim: state the command you ran, its output, and what that output proves.
    "Should work", "looks right" and a passing build you did not watch are not evidence.**
22. A seam is a place where behaviour can be substituted without editing the code under test.
    Agree on the seam before writing the test: the seam decides what the test can observe.

## Workflows

### review-changes

- [ ] Pin the base and validate it: `git rev-parse <base>`, then
      `git diff <base>...HEAD --stat` and `git log <base>..HEAD --oneline`. Stop on a bad ref
      or an empty diff.
- [ ] Decide scope from the stat: review inline only when the diff is small. Otherwise dispatch
      reviewers with `references/reviewer-prompt.md` — the diff and the reasoning stay in their
      context and only findings come back, which is the difference between one review and a
      spent context window.
- [ ] Find the **spec** in this order: issue references in the commit messages, a path the user
      gave, a spec file under `docs/` or `specs/` matching the branch, then ask. No spec at all:
      report "no spec available" on that axis rather than inventing requirements.
- [ ] Find the **standards**: whatever the repo documents (`CONTRIBUTING.md`,
      `CODING_STANDARDS.md`, house style), plus the always-on smell baseline in
      `references/smell-baseline.md`. A documented repo rule overrides the baseline.
- [ ] Review the tests first: do they exist, do they test behaviour, are edge cases covered,
      and would they fail if the changed behaviour broke? A deleted or neutered assertion is a
      Critical finding, not a nit.
- [ ] Collect **Spec** findings: requirements missing or partial, behaviour nobody asked for
      (scope creep), requirements implemented wrongly. Quote the spec line for each.
- [ ] Collect **Standards** findings through the five lenses — correctness, readability and
      simplicity, architecture, security, performance — citing the documented rule or the named
      smell. Keep the two collections apart.
- [ ] Size the change: around 100 changed lines reviews cleanly, ~300 is acceptable for one
      logical change, ~1000 is too large. Oversized: name the split strategy instead of
      pretending to have read it all.
- [ ] Verify the author's verification: which commands were run, what the output was, whether
      the build and suite actually passed. "Tested locally" with no output is not a verification
      story.
- [ ] Verify each finding against the code once more, drop the unsubstantiated ones, assign
      severity, and write the report in the output format below.
- [ ] Close the loop on hygiene: list dead code the change orphaned (ask before deleting) and
      apply the dependency rules to any bump.
- [ ] **Gate — verdict backed by findings:** every finding has `file:line`, an impact and a fix;
      the two axes are in separate sections; the report ends with Ready to merge — Yes, No, or
      With fixes, and the reasoning names the findings that drove it.

### security-review-delta

Runs on top of `review-changes` when a high-risk trigger fires. Full detail, including the
escalation triggers and the report skeleton, in `references/security-review.md`.

- [ ] Risk-classify every changed file: HIGH for auth, crypto, external calls, value transfer or
      removed validation; MEDIUM for business logic, state changes, new public API; LOW for
      comments, tests, UI, logging. Classify by risk, never by diff size — a two-line change
      removed a guard once and became a CVE.
- [ ] Set the depth from the codebase size (`git ls-files | wc -l`): under 20 files read the
      dependencies too, 20–200 follow one hop from the changed code, 200+ walk only the paths
      the change can reach.
- [ ] Blame everything the change removed: `git log -S '<removed expression>' --oneline` and
      `git blame <base> -- <file>`. A guard deleted in a commit whose history says "fix",
      "security" or "CVE" is Critical until the author proves otherwise; code added, removed for
      safety, then re-added is a regression.
- [ ] Size the blast radius: count the callers of each changed symbol
      (`grep -rn '<symbol>(' <source-dirs> | wc -l`) and cross it with the risk class. A
      high-risk change with many callers is the first thing you review, not the last.
- [ ] Check coverage of the changed lines. A new function with no test, or modified validation
      whose tests did not change, raises the severity — an untested change is a riskier change.
- [ ] For each HIGH finding write one concrete attack scenario: goal, preconditions, steps, why
      it works (pointing at the changed lines), impact. Generic warnings are not findings.
- [ ] Write the review to a file and say where it is. State plainly what you could not examine
      at the depth you chose.
- [ ] **Gate — every security finding cites evidence:** a line reference, plus a commit, caller
      count, coverage gap or attack path. No evidence, no finding.

### respond-to-review

Expanded, with worked replies, in `references/receiving-review.md`.

- [ ] Read the whole review before touching anything. Reacting item by item produces work you
      will undo.
- [ ] Count the unclear items. **One or more unclear: no file is edited this turn.** Ask about
      all of them in one round and wait, the clear items included — an answer can change what
      an apparently clear item should do. Look up facts yourself; only decisions go to the
      reviewer or the user:

      ```
      ❓ Q1 — <short title>
      <the question, one or two lines>
      ➡️ Recommended: <your answer and why>
      ```

- [ ] Verify each item against the code. Trust a maintainer who knows the codebase; be
      sceptical of an external reviewer but check carefully — does it hold for this codebase,
      does it break something, is there a reason for the current shape, does the reviewer have
      the full context?
- [ ] Evaluate before implementing. Asked to "do it properly"? Grep for callers first: if
      nothing calls it, the finding is that it should go, not that it needs cursor pagination.
- [ ] Respond with a technical acknowledgment or a reasoned pushback that cites the code, a test
      or a version constraint. If you cannot verify a claim, say so and ask which way to go.
- [ ] Implement in order — blocking first, then trivial fixes, then anything structural — and
      test each item on its own so a failure names its cause.
- [ ] Pushed back and turned out wrong? State the correction and move on: no apology arc, no
      defence of the pushback.
- [ ] Reply where the comment lives: inline review comments get thread replies
      (`gh api repos/{owner}/{repo}/pulls/{pr}/comments/{id}/replies -f body='<reply>'`), not a
      new top-level PR comment. Link code with a full-SHA permalink; a shortened or generated
      ref does not render.
- [ ] **Gate — evidence before claim:** for every item, either the command output that shows it
      fixed, or the reasoning and code reference for why it was not.

## Topic router

| Topic | Read when | File |
|---|---|---|
| Dispatchable reviewer prompt, read-only guard, output contract | Dispatching a reviewer for anything non-trivial | `references/reviewer-prompt.md` |
| Fowler smell baseline, structural remedies, judgement-call rules | Collecting Standards findings | `references/smell-baseline.md` |
| Risk triage, blame on removed guards, blast radius, attack scenarios, report skeleton | A high-risk trigger fired | `references/security-review.md` |
| Responding to feedback, source scepticism, pushback and correction examples | Feedback arrived and something is unclear or looks wrong | `references/receiving-review.md` |

## Output format

Use this shape. Skip empty sections; keep the two axes apart; order within each section by
severity. Comment prefixes map onto the section headings: Critical stays Critical, an unprefixed
required change is Important, Nit and Optional are Minor, FYI becomes a Recommendation.

```markdown
### Strengths
- Specific, and only what you actually verified.

### Spec findings
#### Critical
1. **Order total ignores discounts** — `src/checkout/total.ts:44`
   - Spec (`docs/specs/checkout.md:18`) requires discounts applied before tax.
   - Impact: every discounted order overcharges.
   - Fix: apply `discount(order)` before `tax(order)`, and cover it with a test.
#### Minor
1. **Nit: unused `legacyRate` parameter** — `src/checkout/total.ts:12`

### Standards findings
#### Critical
1. **Query built by concatenation** — `src/orders/handler.js:31`
   - Breaks `CONTRIBUTING.md` "SQL is always parameterized".
   - Impact: `customerId` reaches the database as SQL; any caller can read other customers.
   - Fix: `pool.query('… WHERE customer_id = $1', [customerId])`.
#### Important
1. **Validation removed with no replacement** — `src/orders/validate.js` deleted
   - `git log -S assertPositiveQuantity` points at the commit that added the guard; the
     quantity check is now gone from every path.
   - Fix: keep the boundary check, or state here why it is safe to drop.

### Recommendations
- FYI: the handler module is nearing the size where splitting it pays off.

### Assessment
**Ready to merge: No**
**Reasoning:** two Critical findings — the concatenated query and the neutered quantity test —
both in code paths this change introduces.
```

Asked to fix rather than to review? Same rules, same order; edit the files and report what
changed and how you verified it, instead of returning a report.

## Environment

- `git` is required; every command above is plain `git`. `gh` is only needed for pull-request
  threads and is optional otherwise.
- Nothing here substitutes for running the repo's own suite. When you did not run it, say so
  rather than implying the change is verified.
- If the project exposes a GitHub MCP server, prefer its tools for posting inline comments over
  shelling out.

SOURCES.yaml

skill: code-review
version: "2026.09.10"
upstreams:
  - id: obra-requesting-review
    kind: repo
    repo: obra/superpowers
    url: https://github.com/obra/superpowers
    paths: [skills/requesting-code-review]
    ref: main
    commit: b36e0829c6d0140e93cfef2ca599b1b07d4a7797
    license: MIT
    relation: merged
    synced_at: "2026-09-10"
    contributes: "Base-SHA pinning and the context argument for dispatching a reviewer instead of reading the diff inline, plus the reviewer prompt template: read-only guard, the no-nested-dispatch rule and its reasoning, the Strengths / Critical / Important / Minor / Recommendations / Assessment output contract with a worked example, and the calibration rules."

  - id: obra-receiving-review
    kind: repo
    repo: obra/superpowers
    url: https://github.com/obra/superpowers/tree/main/skills/receiving-code-review
    paths: [skills/receiving-code-review]
    ref: main
    commit: b36e0829c6d0140e93cfef2ca599b1b07d4a7797
    license: MIT
    relation: merged
    synced_at: "2026-09-10"
    contributes: "The whole responding direction: the six-step response pattern, the ban on content-free agreement, clarify-every-unclear-item-before-implementing-any, source-graded scepticism with its five verification questions, the YAGNI grep, blocking-then-simple-then-structural ordering, evidence-backed pushback, graceful self-correction, and inline-thread replies."

  - id: obra-verification
    kind: repo
    repo: obra/superpowers
    url: https://github.com/obra/superpowers/tree/main/skills/verification-before-completion
    paths: [skills/verification-before-completion]
    ref: main
    commit: b36e0829c6d0140e93cfef2ca599b1b07d4a7797
    license: MIT
    relation: merged
    synced_at: "2026-09-10"
    contributes: "The evidence-before-claim completion gate shared verbatim across this repository's engineering-practice skills, and the verify-the-verification review step: which command the author ran and what its output showed."

  - id: mattpocock-review
    kind: repo
    repo: mattpocock/skills
    url: https://github.com/mattpocock/skills/tree/main/skills/engineering/code-review
    paths: [skills/engineering/code-review]
    ref: main
    commit: 3cca18b368ae95cdbdebbff572ccafa662551015
    license: MIT
    relation: merged
    synced_at: "2026-09-10"
    contributes: "Fixed-point pinning with a git rev-parse fail-fast and a three-dot diff, the spec-source priority order, separate gathering of the Standards and Spec axes with the ban on cross-axis reranking, and the twelve-smell Fowler baseline together with its three meta-rules (repo standards override, every smell is a judgement call, skip what tooling enforces)."

  - id: mattpocock-grilling
    kind: repo
    repo: mattpocock/skills
    url: https://github.com/mattpocock/skills/tree/main/skills/productivity/grilling
    paths: [skills/productivity/grilling]
    ref: main
    commit: 3cca18b368ae95cdbdebbff572ccafa662551015
    license: MIT
    relation: merged
    synced_at: "2026-09-10"
    contributes: "The stop-and-ask format used when review feedback is ambiguous: ask every open question in one round, number each and attach a recommended answer, look facts up yourself and put only decisions to the user."

  - id: mattpocock-codebase-design
    kind: repo
    repo: mattpocock/skills
    url: https://github.com/mattpocock/skills/tree/main/skills/engineering/codebase-design
    paths: [skills/engineering/codebase-design]
    ref: main
    commit: 3cca18b368ae95cdbdebbff572ccafa662551015
    license: MIT
    relation: reference
    synced_at: "2026-09-10"
    contributes: "Source of the seam vocabulary shared across this repository's engineering-practice skills, and the second half of the argument that structural refactors belong to review rather than to the implementation loop. Read for alignment; no content copied."

  - id: addyosmani-review
    kind: repo
    repo: addyosmani/agent-skills
    url: https://github.com/addyosmani/agent-skills/tree/main/skills/code-review-and-quality
    paths: [skills/code-review-and-quality]
    ref: main
    commit: 6ca0cd7db39b41b1c37e26d335c507ee92382c6d
    license: MIT
    relation: merged
    synced_at: "2026-09-10"
    contributes: "The five quality lenses and their question lists, the severity prefix taxonomy, lead-with-what-matters, the eight structural remedies, change sizing with the four splitting strategies, review-the-tests-first, verify-the-verification, the honesty and anti-sycophancy rules, the disagreement hierarchy, dead-code hygiene, and the dependency and upgrade discipline."
    notes: "Its human-team process material (review SLAs, round cadence) and its cross-references to sibling skills in the same repository were dropped as meaningless for an agent skill."

  - id: trailofbits-diffreview
    kind: repo
    repo: trailofbits/skills
    url: https://github.com/trailofbits/skills/tree/main/plugins/differential-review/skills/differential-review
    paths: [plugins/differential-review/skills/differential-review]
    ref: main
    commit: 321ccfe628eca0d314b0ee4eaffcdd8a05639aaf
    license: CC-BY-SA-4.0
    relation: merged
    synced_at: "2026-09-10"
    contributes: "Security-mode structure only: risk-class triggers, depth chosen from codebase size, git blame on removed guards and the regression pattern, caller-counted blast radius crossed with risk, test-coverage severity elevation, the escalation trigger list, and the rule that the report is written to a file with its coverage limits stated."
    notes: "CC-BY-SA-4.0 is share-alike and incompatible with this repository's MIT licensing, so no prose, table or template was copied. Only the checklist semantics and phase structure were taken and every sentence was written from scratch; the upstream's Solidity-specific material was reworked into language-neutral guidance."

  - id: anthropic-claude-code
    kind: repo
    repo: anthropics/claude-code
    url: https://github.com/anthropics/claude-code/tree/main/plugins/code-review
    paths: [plugins/code-review, plugins/feature-dev/agents/code-reviewer.md]
    ref: main
    commit: e62465d553ecbf1697219ffbb3c11b4fef14d5bf
    license: Proprietary
    relation: reference
    synced_at: "2026-09-10"
    contributes: "Confirmed the discipline of validating every candidate finding before reporting it and discarding what cannot be substantiated, the composition of the false-positive exclusion list, and the requirement that code cited in a pull-request comment uses a full-SHA permalink. Read as a quality benchmark only."
    notes: "LICENSE.md states all rights reserved under Anthropic's Commercial Terms of Service. Reference-only and permanently so: no text, script or template was copied, and the equivalent rules here were written independently."