git:20260902.f6f9654 to git:20260902.51473c6

---

name: deadeye-pr

description: PR review across four lenses -- over-engineering, correctness, performance, security -- printed locally, opt-in to post.

license: MIT

argument-hint: "[<PR number or URL>] [--post]"

---

<!-- deadeye-pr: canonical rubric; edit internal/prreview/ruleset.md, the skill and every host rendering are generated from it -->

# Deadeye PR Review

One shot over a whole pull request: four lenses, one pass, tagged findings.

This is broader than `/deadeye-review` (lean lens only) and `/deadeye-guard`

(security only) on purpose — a PR wants correctness and performance judged

too. It reuses those two skills' exact tag rubrics and adds two more, and

keeps the same terse one-line-per-finding discipline. Correctness- and

performance-heavy PRs can still be escalated to the host's own deep reviewer;

this is the fast, on-demand, deadeye-flavored pass.

## Scope

Resolve the target PR, then review only its diff:

- No argument → the PR for the current branch.

- Fetch the diff and metadata with the GitHub CLI:

- `gh pr diff <N>` (or `gh pr diff` for the current branch) for the unified diff.

- `gh pr view <N> --json title,body,additions,deletions,files,baseRefName,headRefName` for the header.

- Read the changed hunks **plus enough surrounding context to judge a trust

boundary or a caller contract** — "is this input validated" and "does this

break a caller" both need the code around the hunk, not just the `+` lines.

Preconditions and graceful degradation:

- `gh` not installed or not authenticated → say so plainly and stop, or, if

the user has a local branch, offer to review `git diff <base>...HEAD`

instead. Do not invent PR contents.

- Not a GitHub repo / no PR for the branch → say so; don't substitute a

different scope.

- Huge PR (more than ~40 changed files or a few thousand lines) → review it ALL

by fanning out one subagent per ~2,500-line package-grouped cluster, in

parallel, each returning findings in the standard format. Spawn each cluster

subagent at the cheapest tier that fits it, but the review floor is tier 1

(sonnet) for any cluster with real logic — drop to tier 0 only for a purely

mechanical cluster (generated code, lockfiles, vendored deps, pure renames),

and reserve the top tier for a cluster on a risky surface (auth, crypto,

concurrency, raw SQL or shell, money). Verify every returned finding yourself

before reporting it. Never truncate, and never report partial coverage as

## Verify before reporting

Before claiming a check is MISSING — a sanitizer, an authz guard, a

nil-check — grep OUTSIDE the diff AND follow the value into the callee: a

base class, a caller that guards, or the deeper function

it's handed to — the real guard often lives one call down. An `authz`/bypass

claim needs a concrete input that reaches the sink, or drop it; one wrong

finding erodes trust in all of them.

**Every finding carries its proof.** Append a `proof:` clause naming the

concrete thing in THIS repo that makes the finding true — the caller you

traced, the grep that came back empty, the auditor line, the test that

fails. A finding you cannot prove from the code in front of you is a guess;

drop it. Precision is the product: one finding that's true beats ten maybes,

and every hosted reviewer drowns in the maybes — that's the gap you win on.

**Run the repo's own checks and fuse them in.** Before you finalize, run what

the project already ships when it's present — `go vet`, `tsc --noEmit`, the

linter, the tests the diff touches — and let their output confirm or kill

findings. Mark a finding `(confirmed)` when a tool or a failing test agrees,

otherwise it stands as `likely`. You can run the code; a diff-only bot can't

— that is the edge, so use it.

A `deadeye: <shortcut>. ceiling: <limit>. upgrade: <trigger>.` comment over a

hunk is a recorded DECISION, not a finding — someone already chose to ship

that corner with eyes open. Count those separately as accepted, don't flag

them. Never flag the one runnable check coder mode leaves behind for

deletion — lean code without its check is unfinished.

## Rigor — where reviews miss

Precision is the floor. Four habits separate a real review from a plausible one:

- **Sweep every instance.** One leak, missing registration, or hollow test → check every sibling, in AND out of the diff. A fix with an unfixed twin is a half-fix — name the twin.

- **Disprove your own mitigation.** "X covers it" isn't a pass until X provably runs on the failing path — an early `return`/guard that fires first makes X moot. For a branch gated on a non-null/present field, read the migration: is old data backfilled?

- **The bugs a scan slides past:** two arms handling one value (success/error) should mirror — flag the one missing a capture/close/guard; a rewritten condition must keep every predicate it AND-ed (a dropped `ok &&` re-admits what it rejected); a value can pass `isinstance`/`!= undefined` yet be wrong (`str` subclass, `null` vs `undefined`); an error branch returning a nil used later; in-place mutation of a list aliased from a default arg, shared config, or module cache; every `await` — can it never resolve, and does pre-await state still hold after (abort, concurrent completion)?

- **Sweep the cheap layer:** dead scaffolding, unused imports, placeholder secrets, unpinned deps, a `default:` giving a CPU host a GPU image; a test that mocks its own unit proves nothing.

## The four lenses

Review the diff through each lens. One line per finding, ranked most-severe

first within each lens:

Each finding is one comment — write it like a sharp human reviewer, not a

linter firing rules:

`<glyph> path:line — <tag>: <what actually happens, concretely>. Fix: <fix>. proof: <evidence>.`

- **Lead with the consequence, in plain words** — what breaks or what an

attacker reaches, not just the tag. "The raw user URL reaches `http.Get`, so

`target=http://169.254.169.254/` walks to your cloud metadata" lands;

"unvalidated input" does not.

- The path is required — a PR spans files. If a sibling path shares the bug,

name it in the same breath.

- `proof:` is required (see "Verify before reporting"). For `inject`/`authz`/

`logic`/`race`, the proof IS a reproduction: the concrete input and the sink

it reaches.

- Append `(confirmed)` when a tool or test backs the finding; otherwise it

reads as `likely`. Direct, not rude — you're helping a peer ship.

### Over-engineering (lean lens — from `/deadeye-review`)

- `delete:` — code that shouldn't exist at all (speculative, dead, duplicated)

- `stdlib:` — reinvents what the standard library, or a dependency already in the project, ships

- `native:` — reinvents a platform feature (HTML input types, CSS, DB constraints)

- `yagni:` — flexibility nothing uses (interface with one impl, config for a constant)

- `shrink:` — works, but a shorter form does the same job

Log spam is over-instrumentation, cut it: a line per loop iteration, a metric

nobody reads, a span on a trivial call → `delete:`/`shrink:`. But the one

breadcrumb at a real failure boundary is signal, not bloat — leave it.

Before `yagni:`/`delete:`, grep for implementers/callers outside the diff — a

second impl in a test file makes it a false positive. Footer:

`net: -<N> lines possible.` or, if already minimal, `Lean already.`

### Correctness

- `nil:` — an unchecked nil / null / undefined, a swallowed/ignored error, or a failure path that leaves no diagnostic behind

- `bound:` — off-by-one, slice/array overrun, integer overflow

- `contract:` — violates a caller assumption or the function's own documented contract

- `break:` — a removed/renamed export, or a changed public signature/behavior, that breaks existing consumers — even when the diff compiles.

Rank by likelihood of actually firing. Footer: `<N> correctness risks.` or

`Reads correct.`

### Performance

- `alloc:` — a needless allocation or copy on a hot path

- `nplus1:` — a query or expensive call repeated in a loop that could be batched

- `complexity:` — O(n²) or worse where n grows with real input

- `blocking:` — synchronous I/O or a lock held on a latency-sensitive path

- `copy:` — a large value passed or returned by value where a reference would do

Only flag what a realistic input size makes matter — a triple loop over three

config keys is not a finding. Footer: `<N> perf risks.` or `No hot-path cost.`

### Security (from `/deadeye-guard`)

- `secret:` — a credential literal, or a secret handled where it can leak (logs, errors, client output)

- `authz:` — a decision or resource access with no confirmed permission check

- `crypto:` — hand-rolled or weak crypto (MD5/SHA1 for passwords, non-CSPRNG token, TLS verification off)

- `expose:` — sensitive data returned or logged beyond what the caller needs

- `dep:` — a vulnerable or superseded dependency

- `dos:` — untrusted input sizes an allocation, an unbounded loop, or unbounded recursion → memory or CPU exhaustion. Cap it, or bound the input first.

**A guard is only as good as its weakest path.** When the diff adds or hardens

a check on a sink, grep the file and package for *every other path to the same

sink* — a second `http.Client`, a raw fetch, a probe that runs *before* the

guarded call, a duplicate "is-this-safe" predicate that can drift. A guard on

one path with an unguarded sibling is a fix-shaped diff, not a fix: flag the

sibling with the same tag and cite both lines in `proof:`. The SSRF that ships

is almost always the door nobody guarded.

## Don't repeat what's already on the PR

Before you report, read what's already there — re-posting a finding another

reviewer already made is how a review loses trust. Fetch the existing comments

(bots like CodeRabbit / CodeAnts post here too):

- `gh api repos/{owner}/{repo}/pulls/<N>/comments` — inline review threads

- `gh api repos/{owner}/{repo}/issues/<N>/comments` — the PR conversation

Drop anything already raised — match on the sink or the fix, not exact wording

(you and a bot word the same bug differently). Report only net-new, and print

one honest line so coverage stays clear —

`N findings already raised by existing reviewers — skipped` — whether you're

posting or just printing.

## Output

Lead with a one-line header, then the four lens sections, then a verdict:

```

PR #<N> "<title>" +<adds>/-<dels>, <files> files

```

Findings are a LIST. Do not apply or push any code change unless asked.

## Posting back to the PR (opt-in only)

Default is print-only — nothing is sent anywhere. Post the review to GitHub

ONLY when the user passes `--post` or explicitly asks:

- Show the exact comment body first and get an explicit yes — posting is

outward-facing and public on the PR.

- **Redact any secret value** a `secret:`/`expose:` finding surfaced before it

goes into a public comment — name the location, never the credential.

- Post ONE review that anchors each finding to its line — not a wall of text

in a single comment. `gh pr review` only posts a summary body, so use the

API for inline anchors: build a JSON payload and

`gh api repos/{owner}/{repo}/pulls/<N>/reviews --input -` with

- `event: "COMMENT"`,

- `body`: the tally + verdict (the summary),

Get `{owner}/{repo}` from `gh repo view --json nameWithOwner`. Anchor `line`

to a line the diff actually touches, or GitHub rejects the comment.

- `event: "COMMENT"` only — never approve, request-changes, merge, or close.