Reviewing Code

Reviewing Code

Produce a PR review optimized for posting as a single GitHub comment. The review applies a security lens, a comprehension audit, and a structural check, then labels findings by severity with a copy-paste-safe action checklist.

Read first (always)

List learnings/ and read every file relevant to the current PR (the touched modules, frameworks, or risk areas). Project-specific review conventions, severity adjustments, banned patterns, or scope rules live there and override the defaults in this SKILL.md. If a learning conflicts with this file, the learning wins — mention it to the user.

Tradeoff — when to apply, when to lighten up

Apply the full discipline (security lens + comprehension audit + checklist + grep) for production-bound PRs touching shared services, payments, auth, user data, AI runtime, or anything externally observable.

Lighten formality for: docs-only changes, lockfile bumps with no API change, single-line typo fixes, internal-tooling-only changes behind a feature flag not yet enabled. Still apply the security lens — even doc PRs can leak secrets in examples.

Hard rules — output is a chat message, not a side effect

The review is a chat message the human copies and posts. It is never a side effect:

• Never run gh pr review, gh pr comment, or any GitHub-mutating command. The user posts the review themselves.
• Never run git commit, git add, git push. Only the human commits.
• Never write the review to a file. Output goes in the chat.
• Never use #N to label findings — GitHub auto-links it to PR/issue N. Use B1, I2, R3, etc.
• Never @mention users unless the user explicitly asks.
• Never include Fixes #N, Closes #N, Resolves #N — they auto-close issues on merge.
• Never paste full file contents — link with path/to/file.ts:42 and quote 3-10 relevant lines max.
• Never use absolute local paths (/Users/..., /home/...). Use repo-relative paths only.
• All review documents are written entirely in English, regardless of the conversation language.

Workflow

1. Read the PR end-to-end before commenting. Files, diffs, test changes, and the PR description. If the description claims something not in the diff, that's a finding by itself. → verify: you can summarize the PR in 1–3 sentences and name the dominant risk area (auth, payments, AI, infra, UI, refactor, etc.).

2. Apply the security lens to every significant block (see references/security-checks.md). Five questions: what is being trusted unverified? what's the authoritative source for this behavior? what happens on the failure path? who controls each value and could they lie? what is the blast radius if this is wrong? → verify: every "unverified assumption" has been mapped to either a finding or a defensible justification in the diff.

3. Apply the comprehension audit. For every non-trivial block: can the author summarize it in 1–3 sentences? Is the why captured durably (test name, Why: comment, PR ## Design Decisions, ADR)? Or is the block "AI wrote it, I trust it"? → verify: every non-obvious choice has a durable anchor for its rationale. Red flags trigger a finding (see references/security-checks.md § Comprehension Audit).

4. Apply the structural checks (see references/structural-checks.md). File-structure hard limits (500 LOC / 5 different-prefix functions / 10 same-prefix / 1 main class / cyclomatic ≤ 10 / nesting ≤ 4). SOLID. Pragmatic principles (DRY, KISS, YAGNI, Demeter). Architecture / layer separation. → verify: every violation is labeled by severity; "would a senior call this overengineered/duplicated?" is your gut check.

5. Apply the platform-agnostic checks. Paths, encoding, shell, temp/config dirs, line endings, time, CI matrix. Full grep recipes in references/grep-recipes.md; the reviewer checklist is in references/structural-checks.md. → verify: no Windows-only bug ships through this PR.

6. Apply the correctness checks (see references/correctness-checks.md). The third class of bug: code that is right on the happy path and silently wrong on an edge/error path. New enum member → audit every enumeration site. Control-flow/signal exceptions (pause/cancel/retry) propagate unwrapped through every layer. Degrade-don't-crash contracts never hard-raise. → verify: every new state and every failure branch has a test that would fail if its line were deleted.

7. Apply the testing checks. Coverage gate (≥75%, target 80%) shown by the author. Tests challenge the code, not confirm it. Both happy path AND adversarial. Anti-patterns absent (Mirror, Liar, Giant, Mockery, Inspector, Chain Gang, Flaky, Snowball). Full testing checks in references/structural-checks.md. → verify: if you removed a line of business logic, at least one test would fail.

8. Compile the findings by severity (B = Blocker, I = Important, R = Recommended, N = Nice-to-have). Output format in references/output-format.md. → verify: every finding has a label, a file:line reference, a problem statement, an impact statement, and a suggested fix.

9. Render the review in the structure below (one ## Code Review Summary section, severity sections in order, then the Action Checklist). → verify: the rendered output passes the copy-paste safety check (no #N, no @mentions, no absolute paths, length under cap).

10. Capture a learning (final step). Ask: did I encounter a review pattern, codebase quirk, recurring violation, or severity adjustment not in this SKILL.md or references/? If yes, append a learnings/YYYY-MM-DD-slug.md. If no, skip.

Severity scoring

Use these labels consistently throughout the review. Never #N — GitHub auto-links it.

Required output structure

## Code Review Summary

<2-4 sentence verdict: ship / changes-requested / blocked, and the headline reason>

**Verdict:** <Approve | Approve with comments | Request changes | Block>
**Findings:** <N blockers, M important, K recommended>

---

## ⛔ Blockers (resolve before merge)

### B1 — <Short title>
**File:** `path/to/file.ts:42-58`
**Issue:** <1-3 sentence problem statement>
**Why it matters:** <impact / blast radius>
**Suggested fix:** <concrete action>

<details>
<summary>Code reference</summary>

```ts
// quote only the 3-10 most relevant lines

B2 —

...

⚠️ Important (preferably this PR)

I1 —

...

💡 Recommended (can ship as a follow-up)

R1 —

...

✅ Action checklist for the author

Blockers (resolve before merge):

• B1 —
• B2 —

Important (preferably this PR):

• I1 —

Recommended (can ship as a follow-up PR):

• R1 —

Target length: 300–800 lines of markdown. Hard cap: 1500 lines (GitHub truncates long comments). One finding = one section.

## The five questions to ask on every PR

For every significant block of code:

1. **What is this code trusting without verifying?** — inputs, tokens, signatures, IDs, headers, flags from outside the system.
2. **What is the authoritative source for this behavior?** — is the implementation based on official docs/SDK, or on assumption/tutorial?
3. **What happens in every failure path?** — exceptions swallowed? failed checks that silently pass? errors leaking internals?
4. **Who controls each value, and could they lie?** — client-sent fields, URL params, headers are all forgeable unless verified server-side.
5. **What is the blast radius if this is wrong?** — data exposure? privilege escalation? forged events? resource exhaustion?

Unverified assumptions are where vulnerabilities live. They look like reasonable code.

## Blocker categories (zero-tolerance review)

These get a `B` label automatically — full detail in `references/security-checks.md` and `references/structural-checks.md`:

- **PII in logs.** Any email, name, phone, address in a `logger`/`print`/`console` call. Approved identifiers: `auth_id`, `user_id`, `stripe_id`, internal `id`.
- **Secrets in code.** Hardcoded API keys, tokens, passwords. Webhook secrets not in env vars.
- **DRY violation.** Duplicate types, classes, or logic (5+ lines × 2+ occurrences).
- **File structure violation.** > 500 LOC / > 5 different-prefix functions / > 10 same-prefix / > 1 main class / cyclomatic > 10 / nesting > 4.
- **Mixed responsibility prefixes in one file.** `validate*` AND `format*` in the same file = violation.
- **Comprehension debt.** Author can't defend a block. "AI wrote it, I trust it." `Why:` missing on a non-obvious choice.
- **AI security violation** (when applicable). Direct DB access from AI layer. Secrets in system prompt. Output rendered raw. No human-in-the-loop on irreversible actions.
- **AI runtime missing resilience** (when applicable). No timeout, no circuit breaker, no fallback, no kill switch, no cost ceiling.
- **Third-party integration done wrong.** Custom signature verification instead of provider spec. `==` instead of timing-safe compare. Reading payload fields before verification.
- **AI-generated code in high-risk paths unverified.** Auth, payments, signatures, crypto — every assumption needs to be checked against the authoritative source.

## Output format

The review comment, ready to paste. Print it in the chat. Don't write a file. Don't run `gh`.

Detailed rules — labeling schemes, length, length, link format, copy-paste safety check — in `references/output-format.md`.

## See also

- `references/output-format.md` — GitHub-comment formatting rules, label schemes, structure skeleton, copy-paste safety check.
- `references/security-checks.md` — PII zero-tolerance, AI security, third-party integration, AI-untrusted-code lens, AI runtime resilience.
- `references/structural-checks.md` — file structure hard limits, SOLID verification, pragmatic principles, layer separation, error handling, testing, platform agnosticism.
- `references/correctness-checks.md` — enum/state-machine completeness, control-flow exceptions propagate unwrapped, degrade-don't-crash contracts + the failure-path test spec.
- `references/grep-recipes.md` — copy-paste grep commands for catching common violations across languages.
- `references/checklist.md` — the full checklist + perfect-score criteria + framework-specific layouts.
- `learnings/` — project-specific review conventions, recurring violation patterns, severity adjustments.

This config is review-only. It carries no skills for writing code; when a finding needs the
rationale behind a rule, state the rule and its impact in the finding itself.