pr-review

Files (1)

• SKILL.md

SKILL.md (first lines)

# PR Review

One command, all the lenses, one ranked list. This skill doesn't review by itself —
it *fans out* the repo's review skills in parallel over the same diff, then
reconciles what comes back into a single verdict the user can act on.

Fans out: `code-review` ∥ `ponytail:ponytail-review` ∥ `security-review` ∥
[pr-review-po](../pr-review-po/SKILL.md) ∥ [pr-review-sl](../pr-review-sl/SKILL.md) ∥
[pr-review-az](../pr-review-az/SKILL.md), plus
[sql-review](../sql-review/SKILL.md) when the diff touches SQL.

## 0. Resolve the target, and read it

Establish exactly what diff is under review before dispatching anything:

- **Branch** → `git diff <base>...HEAD`, base from the PR or the repo default.
  This is the path any non-GitHub forge takes; the caller resolves the PR and
  hands over a branch.
- **PR number / URL** → `gh pr view <n> --json …` for metadata, `gh pr diff <n>`
  for the patch. GitHub only.
- **Nothing given** → the working diff (`git diff HEAD` plus staged).

A lens should not re-raise what a human already raised. If the caller names a
file of existing review comments, read it. On GitHub with no file named, fetch
all three sources. Each one alone misses the others.

- Inline line comments: `gh api --paginate repos/<o>/<r>/pulls/<n>/comments`.
- Review summary bodies: `gh api --paginate repos/<o>/<r>/pulls/<n>/reviews`.
- Conversation comments: `gh api --paginate repos/<o>/<r>/issues/<n>/comments`.

Two checks that are cheap here and expensive later:

- **Is the local branch what the PR actually shows?** If a PR was named, compare
  its head SHA to local `HEAD`. A stale or diverged local branch means the lenses
  review code the reviewer never saw — say so before proceeding.
- **Is the changed code reachable?** Trace at least one entry point. Code behind a
  route that still points elsewhere, a flag that's off, or an unexported symbol
  changes every finding's severity from "bug" to "not yet live".

## 1. Fan out — the lenses, in parallel

Dispatch **parallel sub-agents** (superpowers `dispatching-parallel-agents`), one
per lens, each reading the same resolved diff. They don't mutate, so this is safe
to run wide.

If step 0 found existing comments, pass them in each lens's prompt. Tell the
lens to skip points they already raise.

| Lens | Catches |
|---|---|
| `code-review` | correctness, bugs, reuse, simplification |
| `ponytail:ponytail-review` | over-engineering — what to delete or replace with stdlib/native |
| `security-review` | vulnerabilities in the pending changes |
| `pr-review-po` | silent failure modes, tests that prove nothing, type lies, drift |
| `pr-review-sl` | wrong layer, wrong name, wrong schema, scope creep |
| `pr-review-az` | type-system lies, undefined smuggling, exceptions as control flow, comment/naming hygiene, schema back-compat |
| `sql-review` | SQL vs database standards — naming, types, migrations, sqlc config |

The last three are not stylistic garnish — they cover classes the mechanical
lenses systematically miss. `code-review` will pass a test suite that asserts a
throw no client can observe; `pr-review-po` is the lens that catches it. `code-review`
will pass a correct function living in the wrong package; `pr-review-sl` catches
that. `code-review` will pass an event rename that breaks every already-persisted
event; `pr-review-az` catches that.

`sql-review` is conditional. If the diff touches `.sql` files, migration
directories, or `sqlc.yaml`, dispatch it as an additional lens. Otherwise
skip it.

### Lens selection

Default is every lens. `--lenses=<a,b,c>` restricts the fanout to the keys named.

| Key | Lens |
| --- | --- |
| `code-review` | `code-review` |
| `ponytail` | `ponytail:ponytail-review` |
| `security` | `security-review` |
| `po` | `pr-review-po` |
| `sl` | `pr-review-sl` |
| `az` | `pr-review-az` |
| `sql` | `sql-review` |

Group aliases expand to several keys. They mix freely with single keys and with
each other; the union runs, deduped.

| Group | Expands to |
| --- | --- |
| `persona` | `az`, `sl`, `po` |
| `audit` | `sql`, `ponytail`, `security` |

So `--lenses=persona` runs the three persona lenses, and
`--lenses=persona,code-review` adds the mechanical one.

An unrecognised key or group is an error. Stop and name the one that did not
match. Never silently drop it.

`sql` stays conditional on the diff. Naming it does not force it when no SQL
changed.

Reconcile and report as below, however few lenses ran. A one-lens run still
verifies every finding. It still ranks by severity.

**Collect everything before acting on any of it.** A ponytail "delete this" and a
code-review "fix this" routinely target the same lines.

## 2. Reconcile — one list, not five

Merge the raw findings into a single ranked list:

- **Dedupe** — the same defect surfaces in different words from different lenses.
  Merge them and note it was found more than once; that is a confidence signal.
- **Resolve conflicts** — when two lenses disagree (add a guard vs. delete the
  branch), decide, and say which lens you sided with and why. Never emit both.
- **Verify before reporting.** Every finding names a file, a line, and a concrete
  failure scenario — inputs → wrong output. A finding you can't make concrete is a
  vibe; drop it. Check each claim against the actual code, not the diff alone: the
  surrounding lines a hunk doesn't show are where false positives come from.
- **Rank by severity**, not by lens. Blockers first, then material, then nits.

From Esequiel378/skills, MIT.

More from Esequiel378/skills