# Review

> Review code, run QA analysis, or audit visual design, with subcommands code (default), qa, and design. Three-pass code review with 71-category checklist, 30-rule QA analysis with PICT and coverage delta, and frontend design/accessibility/performance/SEO audit. Use when user says "review this PR", "review my code", "check this diff", "QA analysis", "test coverage gaps", "design audit", "check accessibility", "check performance", "check SEO", or wants feedback on a specific change. For full architecture assessment, use /assessment; for security scanning, use /audit; for shipping code, use /ship.

- Skill: `gufranco/review` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx get-skill add gufranco/review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/gufranco/review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Marketing & Growth
- Author: gufranco (https://skillmd.com/u/gufranco)
- Updated: 2026-11-14
- Page: https://skillmd.com/skills/gufranco/review

---


## Overview

Unified review skill covering code quality, QA analysis, and visual design audit.

## When to Use

- The user says "review this PR", "review my code", "check this diff", "QA analysis", "test coverage gaps", "design audit", "check accessibility", "check performance", or "check SEO".
- A pull request, merge request, or local branch needs findings before merge.
- A feature needs its behavior paths mapped against its tests.
- A page or component needs a contrast, responsive, accessibility, or performance audit.

For a full architecture assessment, use `/assessment`; for project-wide security scanning, use `/audit`; for comments on your own PR, use `/respond`; for shipping, use `/ship`.

## Process

| Invocation | Action |
|-----------|--------|
| `/review` or `/review code` | Code review (PR or local) |
| `/review qa` | QA analysis: test coverage gaps and scenarios |
| `/review design` | Visual design, UX, and accessibility audit |

If no subcommand is given, default to `code`.

---

## code

Review a pull request, merge request, or local branch changes with rigorous, detail-oriented analysis. Every line of the diff is scrutinized for correctness, security, performance, maintainability, and adherence to best practices.

Use two references:
1. [`../../checklists/checklist.md`](../../checklists/checklist.md) for all 71 quality categories.
2. [`reviewer-prompt.md`](reviewer-prompt.md) in this directory for comment format and examples.

### Arguments

- No arguments: review the PR for the current branch. If no PR, fall back to local mode.
- PR number(s) or URL(s): review those PRs sequentially, then correlate them before presenting.
- `--local`: review local branch diff against base.
- `--post`: post review as inline comments without asking, someone else's PR only.
- `--backend`: review only backend/infra files.
- `--frontend`: review only frontend files.

### Scope Filtering

When `--backend` or `--frontend` is passed, classify each file:

**Frontend:** paths containing `frontend/`, `web/`, `client/`, `src/app/`, `src/pages/`, `src/components/`, `src/hooks/`, `src/styles/`, `public/`. Extensions: `.tsx`, `.jsx`, `.vue`, `.svelte`, `.css`, `.scss`.

**Backend:** paths containing `backend/`, `server/`, `api/`, `services/`, `workers/`, `jobs/`. Extensions: `.go`, `.py`, `.rb`, `.rs`, `.java`, `.kt`. Infra included with backend.

**Shared:** `packages/`, `libs/`, `shared/`, root config, `prisma/`, `migrations/`. Included in both scopes.

The filter bounds which files get a line-by-line pass. It never bounds which files a finding may name. Whenever a changed file declares a type, a constant, a validation rule, a format, or a helper that its counterpart on the other side of the wire also declares, read that counterpart and report the divergence, even when the filter excluded it.

The duplication itself is the finding, and it is invisible from one side. A response type written out in both the service and the API client, a fingerprint helper copied across the boundary with a comment asking the two to stay identical, a date parsed in one language on the server and another on the client: none of these live in a shared folder, because the absence of a shared folder is the defect. A scoped review that reports only its own half hands back a diagnosis that reads as complete and is not.

Say in the terminal report which side got the line-by-line pass and which side was only traced for contracts.

### Steps

1. **Gather context**, parallel: remote URL, branch, CLI tool, account resolution. Parse flags.
2. **Determine mode**: PR mode if PR exists, local mode otherwise. Check PR state, must be OPEN.
3. **Get diff and context**: PR mode gets metadata and diff via `GH_TOKEN=$(gh auth token --user <account>) gh pr diff`/`glab mr diff`. Local mode detects base, fetches, diffs. Warn about uncommitted changes in local mode. Fail fast here, before any subagent is spawned: confirm the base ref resolves with `git rev-parse --verify <base>` and the diff is non-empty. A bad ref or an empty diff ends the review with one line naming the ref or the empty range.
   - **Independent reviewer.** When this session authored the diff, meaning it made the commits under review or edited those files earlier in the conversation, run steps 5 through 10 and the [Spec Axis](#spec-axis) in a fresh subagent. Brief it with the diff command or PR number, the commit list, and the checklist path, never with this session's reasoning about the change. The authoring context holds every assumption that shaped the code, so reviewing from inside it confirms those assumptions instead of testing them.
4. **Apply scope filter** if `--backend` or `--frontend` passed.
5. **Read context**: PR description, commit messages, every changed file in full, imported modules, verify PR description matches diff. For existing feedback, sweep every channel in [`../../standards/pr-comment-channels.md`](../../standards/pr-comment-channels.md): inline threads, review bodies, PR-level conversation, and commit comments. Reading only inline threads means re-raising a point another reviewer already made in the conversation box, or contradicting a decision the author already explained there. Start the [Spec Axis](#spec-axis) subagent now, in parallel with the remaining steps.
6. **Read the checklist, then discover applicable standards and rules.** Open [`../../checklists/checklist.md`](../../checklists/checklist.md) and read it in this session. Not "recall the 71 category names", not "apply the summary in the system prompt": read the file. The category headings are an index; the findings live in the individual bullets under them, and a bullet you have not read is a bullet you will not check. A review that never opened the file is a review that silently downgraded itself to whatever the model happened to remember.

   Then read `~/.claude/rules/index.yml`. Scan the project for technology signals: file extensions, framework markers, `package.json`, `go.mod`, `Cargo.toml`, `Gemfile`, `requirements.txt`, `pyproject.toml`, import statements in changed files, directory names, and config files. Match signals against trigger keywords in the `on_demand` section. Load **every** matched standard file plus all `always_loaded` rules.

   This makes the review aware of domain-specific best practices. A PR that adds a database migration is reviewed against [`standards/database.md`](../../standards/database.md). A PR that adds a GraphQL resolver is reviewed against [`standards/graphql-api-design.md`](../../standards/graphql-api-design.md). A PR that adds a queue consumer is reviewed against [`standards/message-queues.md`](../../standards/message-queues.md).

   Also load these rules for review context:
   - [`rules/verification.md`](../../rules/verification.md): when reviewing claims about test coverage or build success
   - [`rules/pre-flight.md`](../../rules/pre-flight.md): when reviewing whether the author checked for existing solutions
   - [`rules/security.md`](../../rules/security.md): security criteria, OAuth 2.1, passkeys, NIST 800-63B, secrets management, supply chain
   - [`rules/writing-precision.md`](../../rules/writing-precision.md): quality gate for review comments themselves
   - [`rules/code-style.md`](../../rules/code-style.md): completeness, immutability, error classification, type conventions, LLM trust boundary, TypeScript 5.x
   - [`rules/testing.md`](../../rules/testing.md): mock policy, test structure with the three bare AAA markers as the only comments, faker, deterministic tests, contract testing, performance regression
   - [`standards/performance.md`](../../standards/performance.md): Core Web Vitals budgets, API latency targets, bundle size limits
   - [`rules/privacy-defaults.md`](../../rules/privacy-defaults.md): data minimization, retention, erasure when the diff touches personal data
   - [`rules/ai-guardrails.md`](../../rules/ai-guardrails.md): when the diff processes, stores, or acts on LLM-generated output

   Before suggesting fixes that call library APIs, check [`standards/llm-docs.md`](../../standards/llm-docs.md) for the library's documentation URL. Verify the API exists.

   Record which standards were loaded for inclusion in the review verdict.

7. **Scope detection.** Categorize the diff into scope signals to determine which specialist checks to prioritize:

   | Signal | Detection | Specialist focus |
   |--------|-----------|-----------------|
   | SCOPE_FRONTEND | `.tsx`, `.jsx`, `.vue`, `.css` files, `components/`, `pages/` | Performance budget (cat 54), design quality (cat 52), accessibility |
   | SCOPE_BACKEND | `.go`, `.py`, `.rs`, service files, handlers | API design, error handling, concurrency |
   | SCOPE_API | Route definitions, controllers, OpenAPI changes | API versioning, rate limiting, backward compatibility |
   | SCOPE_AUTH | Auth middleware, token handling, login/signup | OAuth 2.1, passkeys, rate limits, NIST 800-63B |
   | SCOPE_MIGRATIONS | Migration files, schema changes | Schema-migration sync (cat 69), expand-contract (cat 55), backward compatibility |
   | SCOPE_EVENTS | Event handlers, queue consumers, publishers | Event-driven patterns (cat 57), idempotency, DLQ |
   | SCOPE_DEPS | package.json, go.mod, requirements.txt changes | Supply chain (cat 56), SBOM, typosquatting |
   | SCOPE_INFRA | Dockerfile, terraform, k8s manifests, CI config | Container security, zero-downtime (cat 55) |
   | SCOPE_LLM | LLM client calls, prompt templates, AI output processing | LLM trust boundary (cat 53), output validation |
   | SCOPE_WEBHOOK | Webhook handlers, signature verification, external system callbacks | Idempotency key stability, replay protection, concurrent delivery, input canonicalization, startup secret validation |
   | SCOPE_TESTS | Test files | Mock policy, coverage, three marked AAA blocks with no other comment, faker |
   | SCOPE_DOCS | README, docs/, CHANGELOG | Documentation accuracy, stale references |
   | SCOPE_CONFIG | .env, config files, settings | Secret exposure, env var completeness |

   Use scope signals to prioritize review depth: apply the most thorough analysis to categories matching the detected scope. Categories outside the scope still receive baseline checks.

8. **Blast radius analysis**: the diff is not the review boundary, the project is. For every changed file, trace outward to find code that depends on the change. Read every impacted file, not just the diff.

   **7a. Identify what changed at the interface level.** Extract every modified export, function signature, type, interface, enum, route, database column, env var, config key, event name, and public API contract from the diff.

   **7b. Find all consumers.** For each changed interface, grep the entire project for:

   | What changed | Search for |
   |-------------|-----------|
   | Exported function or class | All `import { name }` and call sites |
   | Type or interface | All files that reference the type name |
   | Enum or constant | All files that use the enum or constant |
   | API route or endpoint | All `fetch`, `axios`, `trpc`, `href`, `action` references to that path |
   | Database model or column | All services, repositories, seed files, and migrations referencing it |
   | Env var | All `process.env` reads and `.env.example` |
   | Event name or message type | All publishers and subscribers |
   | Config key | All consumers of the config module |
   | CSS class or design token | All `className` references and Tailwind config |

   **7c. Read every impacted file.** Read the full content of every consumer found in 7b, not just the import line. Verify the consumer still works correctly with the new interface. A function that changes its return type from `string` to `string | undefined` might have 40 callers that do not handle `undefined`.

   **7d. Flag impact findings.** For each consumer that would break or behave differently after the change, record: the consumer file and line, what it expects, and how the change violates that expectation. These findings have the same severity as bugs found in the diff itself.

   **7e. Cross-PR correlation, when more than one PR is under review.** An interface change in one PR and a consumer of it in another are invisible from either side. Each branch typechecks against the base, both report green, and the break appears only when the second one merges. Reviewing the PRs one after another and presenting two independent verdicts hands back a diagnosis that reads as complete and is not.

   For every symbol one PR adds, narrows, or makes required, grep the other PRs' branches for constructors and callers that would no longer satisfy it: a new required field on a shared interface, a narrowed return type, a changed function signature, a bumped wire-format version, a renamed env var, a new enum member that widens an exhaustive switch. Check the base branch too, since a symbol absent there is what lets both PRs pass in isolation.

   Report the collision on both PRs, not one. Name which PR is the natural home for the fix, and say plainly that merge order decides whether the base branch breaks. When one PR's infrastructure already provisions what the other's code expects, say so: that turns a design argument into a missing line.

9. **Behavioral Flow Analysis, MANDATORY, do not skip.** Per-file analysis catches syntax-level bugs. This step catches design-level bugs: broken idempotency, race conditions, data corruption under concurrency, and attack vectors. Skipping this step is the failure mode that sends a pull request back for a second review pass.

   **9a. Trace every request lifecycle end-to-end.** For each new handler, endpoint, consumer, or webhook in the diff, trace the complete request from HTTP entry through every service call to every database write and external side effect. Do not review files in isolation. Follow the data.

   | What to trace | Verify |
   |--------------|--------|
   | Input parsing and validation | Every field validated. Canonicalization applied (lowercase emails, trimmed strings). No implicit trust of external input shape |
   | Authentication and authorization | Secrets validated at startup, not just at request time. Signature verification uses constant-time comparison. Timestamp windows reject future values, not just old ones |
   | Idempotency key derivation | What input produces the key? Does that input change on retry by the external system? If derived from request metadata (timestamp, nonce), verify stability across application-level retries vs network-level replays |
   | Idempotency key lifecycle | If the key is consumed (stored) before the protected operation completes, what happens when the downstream step fails? Can the operation be retried, or is it permanently blocked? |
   | Multi-step mutations | Are all steps in a single transaction? If not, what state does the system reach if it fails between steps? Can a retry recover, or is the data permanently corrupted? |
   | Return values to external systems | Do response codes prevent unnecessary retries (200 for known non-errors)? Do they cause silent failures when misconfigured (200 for promotion-not-found hides config errors)? |

   **9b. Model concurrent actors.** For every write path, identify every actor that can trigger it concurrently: retries from the same external system, parallel requests from different systems, user actions that touch the same data, cron jobs that overlap. Draw a timeline with at least two concurrent actors and verify no interleaving produces incorrect state.

   Run the `concurrency-auditor` agent unconditionally when the diff touches a write path, meaning any service, handler, route, consumer, job, or repository that persists, mutates shared state, charges, sends, or enqueues. It returns one actor timeline per write path plus file:line findings, and its output feeds the Behavioral Flow Analysis summary in step 14. Reference standards: [`../../standards/concurrency.md`](../../standards/concurrency.md) for the race taxonomy and the correctness ladder, [`../../standards/idempotency.md`](../../standards/idempotency.md) for key stability and replay.

   | Concurrency scenario | What to check |
   |---------------------|--------------|
   | Two webhook deliveries for the same logical event | Do they produce the same idempotency key? If not, do they race through shared mutations? |
   | Webhook + user action | Can a user action (place bet, spend token, change email) interleave with a webhook mutation (void bonus, activate promotion)? Is the shared state protected by a lock? |
   | Retry after partial success | If step 1 of 3 succeeded but step 2 failed, does a retry re-execute step 1? Is step 1 idempotent or protected? |
   | Concurrent requests with different idempotency keys | If the dedup key is derived from mutable request metadata (timestamp), two requests for the same logical operation get different keys and both proceed |

   **9c. Read the database schema.** For every application-level uniqueness check, `findFirst` then `create`, `SELECT` then `INSERT`, read the actual schema file and verify a corresponding `UNIQUE` constraint, `@@unique`, or `UNIQUE INDEX` exists. An application check without a DB constraint is a TOCTOU race that produces duplicates under concurrency. Never assume a constraint exists because the code checks for it.

   **9c-bis. Schema-migration parity. MANDATORY when SCOPE_MIGRATIONS detected.** This gate exists because a pull request in a production repository shipped a migration with 7 raw-SQL `CREATE INDEX` statements and zero matching `@@index` declarations in `schema.prisma`, and the drift went undetected for 17 days. Reference: the `## Prisma` section of [`standards/orm-migrations.md`](../../standards/orm-migrations.md). Reference checklist: category 69.

   For every changed migration file, perform these checks in order. Skipping any of them is a review failure.

   1. **Enumerate every DDL statement in the migration.** List `CREATE INDEX`, `CREATE UNIQUE INDEX`, `DROP INDEX`, `ALTER TABLE ADD COLUMN`, `ALTER TABLE DROP COLUMN`, `CREATE TABLE`, `DROP TABLE`. Extract the exact identifier name from each statement.
   2. **For each enumerated DDL, grep `schema.prisma` for the matching declaration.** Report the result per item using this exact format:

      ```
      [PARITY OK]    CREATE INDEX "Game_homeTeam_sportsbook_search_idx" -> @@index([homeTeam], map: "Game_homeTeam_sportsbook_search_idx")
      [PARITY MISS]  CREATE INDEX "Line_name_sportsbook_search_idx" -> NOT FOUND in schema.prisma (P0 BLOCKING)
      ```

   3. **Run `prisma migrate diff` and paste the full output.** Command:

      ```bash
      pnpm exec prisma migrate diff \
        --from-schema-datamodel <schema.prisma path> \
        --to-migrations <migrations dir> \
        --exit-code
      ```

      Non-zero exit code is a P0 blocking finding. The exact diff output must appear in the review.
   4. **If a fresh DB is available, run end-to-end verification.** `prisma migrate deploy` followed by `prisma migrate dev`. If the second command generates a follow-up migration, drift exists. Paste the generated migration in the review and flag as P0.
   5. **Verify every index name uses an explicit `map:`.** Auto-generated Prisma names collide with raw-SQL names and cause the exact drift described above. Names must follow `<Model>_<col>(_<col>)*_<purpose>_idx`.
   6. **Verify any DDL without a schema counterpart is documented.** Extensions, materialized views, GIN/GiST trigram indexes, custom triggers, and RLS policies cannot be modeled in Prisma. The PR description or an ADR must name the unmanaged object class and why no schema entry exists; a comment in the migration file does not count, because comments are banned in every language. Missing documentation is a P1 finding.

   None of these steps can be skipped on the grounds that "the hook would have caught it." The hook is layer 2; the review skill is layer 4. Defense-in-depth means each layer runs every check independently.

   **9d. Model attacker capabilities, when SCOPE_AUTH, SCOPE_WEBHOOK, SCOPE_API detected.** Assume the attacker has: the signing secret, compromised dependency, leaked env var, the ability to send arbitrary requests at high volume, knowledge of the endpoint contract. For each assumption, trace what damage is possible.

   | Attacker capability | What to verify |
   |--------------------|---------------|
   | Compromised signing secret | Can they pre-sign requests with future timestamps? Can they generate requests that bypass replay protection? |
   | High-volume valid requests | Is there rate limiting? Can they exhaust Redis memory (long TTL keys), DB connection pool, or external API quotas? |
   | Payload manipulation | Is email canonicalized? Can mixed-case or Unicode variants bypass dedup? Can oversized arrays or deeply nested objects cause resource exhaustion? |
   | Replay with modification | If the attacker replays a valid request with a different timestamp (new signature), does idempotency catch it? |

   **9e. Verify startup and configuration safety.** For every secret, API key, or credential read from environment variables at request time, verify there is a startup check that fails loudly if the value is missing or empty. A missing webhook signing secret that silently rejects all deliveries is a silent outage.

10. **Three explicit passes**, applied to the diff AND to impacted files from step 8, informed by behavioral flow analysis from step 9:
   - **Pass 1: Per-file analysis.** Every applicable category from `checklist.md`, 1 through 71. This includes the extended categories: 53, LLM Trust Boundary when code processes AI output, 54, Performance Budget for frontend changes, 55, Zero-Downtime Deployment for migration and deploy changes, 56, Supply Chain for dependency changes, 57, Event-Driven for queue and event handler changes, and 58, Licensing for new or modified source files. Additionally, for each standard loaded in step 6, verify that changed code follows the patterns in that standard. When a finding originates from a loaded standard, note the standard internally for your own tracking, but never reference it in externally-posted comments. The posted comment must state the engineering reason directly. Apply to changed files first, then to impacted consumer files where the change alters behavior. Use scope signals from step 7 to prioritize depth. Findings from step 9, behavioral flow analysis feed directly into this pass: race conditions become category 4 findings, idempotency gaps become category 18 findings, missing constraints become category 5 findings.

     **Security pattern analysis, when SCOPE_AUTH, SCOPE_API, SCOPE_WEBHOOK, or SCOPE_BACKEND is detected.** Read `~/.claude/skills/security-patterns.md` and apply it as an additional security lens. For each changed file that handles user input, perform source-to-sink tracing: map entry points to dangerous sinks, SQL, command, template, file, SSRF, redirect, XSS, deserialization, and verify sanitization at each transition. Check for vulnerability patterns matching the diff: race conditions on financial operations, IDOR, mass assignment, JWT weakness, CORS misconfiguration, missing idempotency. When a security finding is identified, run `git blame -L <start>,<end> <file>` on the vulnerable lines to determine when it was introduced and how long it has been exposed.
   - **Pass 2: Cross-file and project-wide consistency.** Category 15. Contradictions, import chain side effects, config completeness, contract alignment, error path consistency. Verify that every consumer identified in step 8 still compiles, passes type checks, and behaves correctly. Check for: stale type assertions, missing null checks on new optional returns, tests that assert old behavior, documentation that describes old behavior, and mocks that replicate old signatures.

     **Placement.** Correct code in the wrong module is still a defect, and it is the one the other passes structurally cannot see, because every file reads fine on its own. For each new symbol ask: which layer owns this fact, and does the project already have a home for it?

     | Signal | What it usually means |
     |--------|----------------------|
     | A provider's wire format assembled outside the only module that talks to that provider | The adapter is making its caller speak the provider's dialect. Push the serialisation inward |
     | A hand-rolled guard where the project already has a middleware, base class, or helper for that job | Nobody looked. The inconsistent error shape it produces is a second, downstream finding |
     | The same type, constant, or helper declared on both sides of a boundary | It belongs in the shared package the project already has |
     | A workaround for an upstream quirk applied in every client | Normalize once at the layer that owns the response |
     | A docstring that tells the caller what it must do first | Read it as a confession. "The controller handles that conversion" names the leak out loud |

     Grep before asserting a home is missing. The strongest version of this finding names a facility the project already has and the diff did not use: an existing validation middleware the new route bypassed, an existing shared package that already holds the sibling type. That is checkable and cheap to act on. A placement finding that instead proposes a new abstraction is speculation, and it should be dropped.

     **Smell baseline.** Match the diff against these twelve code smells from Fowler's *Refactoring*, chapter 3. Report each as a judgement call, "possible Data Clumps", at confidence 5 to 6 per Confidence Scoring, never as a blocking violation. A documented project convention that endorses the shape suppresses the smell, unless it contradicts a rule in [`../../CLAUDE.md`](../../CLAUDE.md). Skip any smell a linter in the project already enforces.

     | Smell | What it is | Fix |
     |-------|-----------|-----|
     | Mysterious Name | A name that does not reveal what the symbol does or holds | Rename; when no honest name comes, the design is unclear |
     | Duplicated Code | The same logic shape in more than one hunk or file | Extract the shared shape and call it from both |
     | Feature Envy | A function that reads another object's data more than its own | Move it onto the data it envies |
     | Data Clumps | The same few fields or parameters travel together | Bundle them into one type and pass that |
     | Primitive Obsession | A primitive standing in for a domain concept | Give the concept its own branded or value type |
     | Repeated Switches | The same branch on the same type recurs across the change | One shared map or polymorphism for every site |
     | Shotgun Surgery | One logical change forces scattered edits across many files | Gather what changes together into one module |
     | Divergent Change | One module edited for several unrelated reasons | Split so each module changes for one reason |
     | Speculative Generality | Parameters, hooks, or abstractions no requirement asks for | Delete and inline until a real need appears |
     | Message Chains | Navigation such as `a.b().c().d()` the caller depends on | Hide the walk behind one method on the first object |
     | Middle Man | A unit that mostly delegates onward | Remove it and call the real target |
     | Refused Bequest | A subclass or implementer that overrides most of what it inherits | Replace inheritance with composition |
   - **Pass 3: Cascading fix analysis.** Category 16. For every issue: if the author fixes it exactly as suggested, what new problems could that introduce?
11. **Run local verification**: test, with coverage, lint, build. After tests pass, verify that coverage on changed files and their direct dependents is 100%, with no exclusion directive added. Apply [`../../checklists/checklist.md`](../../checklists/checklist.md) category 8. If coverage is below threshold, flag it as a blocking finding.
12. **Check external sources.** If the PR description, commit messages, or code comments reference external projects, articles, or third-party codebases as inspiration, apply [`../../checklists/checklist.md`](../../checklists/checklist.md) category 50, Clean Room. If no references are found, ask the author: "Were any external projects or codebases used as reference during implementation?" If yes, run the clean room checks against the diff. If no, skip category 50.
13. **Check branch freshness, CI, test evidence, PR size**, parallel. PR > 400 lines = warning, > 1000 = blocking.

    A branch behind its base is blocking only when the drift touches files the PR changes. Compute the intersection of the two changed-file lists rather than reading the commit count: `comm -12` over `git diff --name-only <head>...<base>` and `git diff --name-only <base>...<head>`, both sorted. An empty intersection is drift, and it belongs in the terminal report as an observation, never in the verdict. A non-empty one is a real rebase requirement, and naming the overlapping files is what makes it actionable.
14. **Present review in the terminal** with verdict: APPROVE, REQUEST_CHANGES, or COMMENT. Include operational risk assessment for non-trivial changes. Include a blast radius summary listing every file outside the diff that is affected by the change. Include a Behavioral Flow Analysis summary covering the lifecycle traces, concurrent actor timelines, and attacker models from step 9, plus a Standards Applied line. Present the Spec Axis report under its own `## Spec` heading, after the standards findings and apart from them. All of it is for the person running the skill. None of it is published: the pull request receives inline findings and a review event, nothing else.

    **External output is a separate channel.** The in-terminal presentation includes internal scaffolding such as Standards Applied, the Behavioral Flow Analysis section heading, and severity tiers like P0, P1, and P2. What goes to GitHub does not. Before building the JSON payload, do a forward pass that strips:

    - Conventional Comments label prefixes, `issue, blocking:`, `issue, non-blocking:`, `nitpick:`, `suggestion:`, `question:`, `thought:`, `praise:`, `chore:`, `todo:`. Use no label.
    - Internal severity tiers, `P0`, `P1`, `P2`, and section headings, `Standards Applied`, `Behavioral Flow Analysis`, `Blast Radius`, `Spec`.
    - The skill's own invocation flags, `--backend`, `--frontend`, `--local`, `--post`.
    - Internal file references such as `~/.claude/`, `rules/<name>.md`, `standards/<name>.md`, `checklist.md`, or [`CLAUDE.md`](../../CLAUDE.md).
    - "Category N" or "checklist item N" phrasings; reach for the engineering reason directly.

    This is a forward filter, not a post-hoc rewrite. If you find yourself rewriting after generation to scrub these, the generation prompt was wrong. Cleanliness from the first draft is the bar. See [`standards/code-review.md`](../../standards/code-review.md) "No Internal Config Leakage" and the `internal-config-leakage.py` hook, which will block the post if you miss something.
15. **Next steps**:
    - **Own PR / local**: offer to fix issues. Convergence loop, max 5 iterations: fix, re-verify, re-audit. If 5 iterations are exhausted with findings still open, stop, list the remaining issues, and inform the author. Five iterations is enough for any reasonable convergence; remaining issues likely need a design change, not another fix pass.
    - **Someone else's PR**: offer to post inline comments. Show the exact payload first: each comment with file, line, body text, and suggestion blocks. Ask for confirmation before posting. `--post` skips the confirmation prompt but still shows the payload summary. Inline findings are the only thing that gets posted; the review carries no summary body, per [`../../rules/pr-comment-discipline.md`](../../rules/pr-comment-discipline.md).

### Spec Axis

The checklist passes ask whether the change is built right. The spec axis asks whether it is the right thing. A change can pass one and fail the other: code that meets every standard while implementing the wrong behavior, or code that does exactly what was asked while breaking conventions. Reported in one list, the passing axis hides the failing one.

1. **Find the spec**, stopping at the first hit:
   1. The `## Requirements` section of the active plan folder under `<root>/plans/`, where the workspace root is `<repo>/.work/` or `<repo>/docs/` resolved by [`hooks/_lib/project_workspace.py`](../../hooks/_lib/project_workspace.py), per [`../../rules/project-workspace.md`](../../rules/project-workspace.md). The active folder is the newest one whose slug matches the branch or the PR title.
   2. The `ADDED`, `MODIFIED`, and `REMOVED` delta sections that plan folder carries against `specs/current/`, per [`../../rules/living-specs.md`](../../rules/living-specs.md).
   3. A ticket referenced from the commit messages, the branch name, or the PR body, such as `Closes #45` or a Linear key, fetched with the account resolved in step 1.
   4. Nothing found: record `Spec: no spec found` and skip the axis. A missing spec never blocks the review, and the axis never infers requirements from the code.
2. **Run it in an isolated subagent.** The brief carries the diff command or PR number, the commit list, and the spec path or fetched text, and nothing from the standards passes. It ends with: "Perform this review directly. Do not invoke /review or spawn agents. Under 400 words." The subagent returns three lists, and every finding quotes the spec line it rests on:
   - Requirements missing or only partially implemented.
   - Behavior in the diff that no requirement asked for.
   - Requirements that look implemented but are implemented wrong.
3. **Report it apart.** The terminal report carries it under `## Spec`. Never merge, rerank, or deduplicate it with the standards findings. The closing line names the worst finding within each axis, never one winner across both. A spec finding anchored to a diff line follows the same posting rules as any other finding.

### Posting Comments via Pending Review

Posting applies to a pull request someone else authored. On your own pull request a finding is a commit, so nothing is posted.

[`../../rules/pr-comment-discipline.md`](../../rules/pr-comment-discipline.md) bounds what a review may carry: inline findings anchored to a line, and nothing else. No summary body, no verdict prose, no praise line, no count of what was checked. The verdict reaches the author through the review event, `APPROVE`, `REQUEST_CHANGES`, or `COMMENT`, which is one word and cannot be padded. Everything the summary body used to hold goes to the terminal, for the person running the skill.

When posting, always use the pending review API to batch all comments into a single notification. Use a JSON file with `--input` to avoid shell escaping issues with markdown, tables, and code blocks in comment bodies.

**Step 1: Get the latest commit SHA.**

```bash
GH_TOKEN=$(gh auth token --user <account>) gh pr view <PR_NUMBER> --json commits --jq '.commits[-1].oid'
```

**Step 2: Write the review payload to a JSON file.**

```json
{
  "commit_id": "<COMMIT_SHA>",
  "event": "REQUEST_CHANGES",
  "body": "",
  "comments": [
    {
      "path": "src/auth.ts",
      "line": 20,
      "body": "Plain-prose comment body, no label prefix. A ```suggestion\nblock\n``` can be embedded when proposing a code change."
    },
    {
      "path": "src/auth.ts",
      "line": 35,
      "body": "Second comment"
    }
  ]
}
```

Write the file with `cat <<'EOF' > /tmp/review-payload.json`, single-quoted delimiter to prevent shell expansion. Clean up after posting.

**Step 3: Submit the review in a single API call.**

```bash
GH_TOKEN=$(gh auth token --user <account>) gh api repos/:owner/:repo/pulls/<PR_NUMBER>/reviews \
  -X POST \
  --input /tmp/review-payload.json \
  --jq '{id: .id, state: .state}'
```

This creates and submits the review in one step. No separate "create PENDING then submit" flow needed.

**JSON payload rules:**
- `line` is the line number in the file, new version. Do not use `side` or `position`, they are not valid on this endpoint
- `event` in the top-level object sets the review type directly, and it is the whole verdict
- **The top-level `body` is always the empty string.** It is the review summary slot, and a summary is never published. `body` inside each comment is the inline comment text
- A review with an empty body cannot have its body edited later, which the GitHub API enforces. That is the intended outcome: there is nothing to edit
- For multi-line comments, add `start_line` alongside `line`
- Always clean up the temp file after posting: `rm /tmp/review-payload.json`

**Establish which lines are anchorable before writing any comment.** Fetch the per-file patch once with `GH_TOKEN=$(gh auth token --user <account>) gh api repos/:owner/:repo/pulls/<n>/files` and read the hunk headers from that. `gh pr diff --patch`, and a `--paginate` call piped through a `jq select`, both concatenate patches across commits or pages, so the same file comes back with several hunk sets whose line numbers do not describe head. A comment anchored from those lands on the wrong line or is rejected outright.

Files with status `added` are the safe case: every line is in the diff, so any line number is valid. For a `modified` file, only lines inside a hunk range are valid. A finding that falls outside every hunk cannot be anchored and therefore cannot be posted; report it in the terminal so the person running the skill can raise it directly.

**Event type mapping:**

| Verdict | Event |
|---------|-------|
| Minor, non-blocking suggestions | `APPROVE` |
| Blocking issues that must be fixed | `REQUEST_CHANGES` |
| Neutral feedback, questions | `COMMENT` |

Never post comments individually. Even a single comment goes through the JSON file flow. This prevents notification spam and avoids shell escaping failures with complex markdown.

### Comment Voice (MANDATORY)

Every posted comment must read like a human colleague wrote it. The [`standards/code-review.md`](../../standards/code-review.md) "Review Comments" section is the source of truth: no prefix labels, no structured templates, no checklist appearance. State the engineering reason directly.

Internal triage, severity, category, source rule, stays internal. It informs which comments you choose to write and how blocking they are in the verdict, but it never appears in the posted text. The reader has no access to the configuration and must never learn it exists.

**Forbidden in posted bodies:**

- Conventional Comments label prefixes like `issue, blocking:`, `issue, non-blocking:`, `nitpick:`, `suggestion:`, `question:`, `thought:`, `praise:`, `chore:`, `todo:`. These signal "I am working from a template" to any reader. Write the same content without the label.
- Internal severity tiers: `P0`, `P1`, `P2`, or any heading like `## P0 Blocking`. The verdict you compute internally maps to GitHub's `APPROVE` / `REQUEST_CHANGES` / `COMMENT` event, full stop.
- The skill's own invocation flags. If you ran the review with `--backend`, the posted comment is not "Backend-only review (`--backend`)". The first-person voice is "I went through the backend changes" or just plain English.
- References to internal config: `~/.claude/`, `rules/<name>.md`, `standards/<name>.md`, `checklist.md`, [`CLAUDE.md`](../../CLAUDE.md), "category 17", "checklists item 3".
- Structural section headings carried over from internal taxonomy: "Behavioral Flow Analysis", "Blast Radius Summary", "Standards Applied". Internal-only sections; their content reaches the reader in plain prose, not under those labels.

**Comment shape:**

Open differently across comments. Sometimes a description, sometimes a question, sometimes a one-line observation. Variet

…(truncated)