review-pr

name

review-pr

description

Review a GitHub pull request or base-to-head change set for actionable defects, data integrity issues, silent failures, incomplete migrations, and tests that can pass for the wrong reason. Use when asked to review, audit, red-team, or verify a PR or branch before merge. This is a change-set review, not a full scientific review of decision-grade experiment designs, results, or campaign memory; use review-experiment separately when those are in scope.

Review PR

Scope

Review the pinned change set: completeness, interfaces, migrations, tests, documentation, regressions, and PR claims. Inspect external evidence only to verify a load-bearing PR claim or boundary contract. If the PR publishes a decision-grade experiment design, result, or campaign-memory update, also run review-experiment independently against the process context and source artifacts, recording both the PR head and artifact-producing source SHA. Combine only after both passes: deduplicate root causes, preserve both sets of evidence and unresolved disagreements, and use the base/head comparison to separate introduced defects from pre-existing limitations.

Step 1 — Fetch PR context:

Check whether the user provided a PR number or URL. If yes, use the available GitHub integration or the gh CLI to fetch PR metadata first, including the immutable endpoints:

• gh pr view <number> --json number,title,body,url,baseRefName,headRefName,baseRefOid,headRefOid

Then fetch the named files and patch for that base/head pair. In a local clone, use git diff --name-only <base-sha>...<head-sha> and git diff <base-sha>...<head-sha>. Without a local clone, use a GitHub comparison addressed by those SHAs or clone the repository. Do not use a moving gh pr diff as though it were pinned.

If the user supplied base and head refs, a branch comparison, or two SHAs without a PR, resolve them to immutable SHAs and review that pair directly. Record that PR description, comments, and contributor engagement are unavailable rather than asking the user to choose an unrelated open PR. Only list open PRs when no review target was supplied at all.

Pin both ends of the diff. Record the base SHA and head SHA from PR metadata or the resolved comparison refs. Read from git show <head-sha> and compare git diff <base-sha>...<head-sha> rather than using the working tree or a later live diff. State both SHAs in the output. Anything executed must run against the pinned head in a throwaway checkout. Before finalising, fetch the PR metadata again. If either endpoint moved, restart on the new pair or clearly report that the review covers the old pair. A result from an unpinned or moving tree is uninterpretable.

Step 2 — Enumerate what the PR claims:

Before hunting for smells, write down what the diff asserts without saying so. A PR is a set of implicit claims: this flag reaches the trainer, this test fails if the guard is removed, this scorer is deterministic, this migration touched every site, this column exists upstream. Most of them are true. The review is the attempt to find the one that isn't.

Build the list from the diff alone, before you have a theory about what's wrong. A list written after the hypothesis only contains claims the hypothesis already covers. Keep it to the load-bearing ones (roughly 5-15 on a normal PR) and rank them by what breaks if the claim is false.

By the end of the review every claim carries one of three dispositions: falsified (that's a finding), verified (say how, and prefer a check you ran over a passage you read), or unverified (name the check that would settle it). An unchecked load-bearing claim is itself a finding - a review that leaves the riskiest claim untested hasn't covered the PR, however many findings it returns.

Step 3 — Work the four lenses:

Read the diff and the surrounding code, applying the techniques in references/review_techniques.md, then put the claim list through four lenses. Each lens must report. A lens with nothing to say is usually a lens that wasn't run.

• Lens 0 — repository and delivery contract. Does the change obey the repo's own rules: repository instruction files such as AGENTS.md or CLAUDE.md, the conventions the surrounding code already follows, one-feature commits, docs kept in step with the interfaces they document, the issues it claims to close actually closed?
• Lens A — the saboteur. What makes this green for the wrong reason? Take each passing test, each clean run, each plausible number, and try to produce a mundane explanation that isn't the mechanism the author intends: a fixture standing in for the behaviour, a filter that silently drops the failing rows, a default that masks the missing value.
• Lens B — the statistician. For any number the PR produces, moves, or relies on: does the implementation or PR description say more than the visible estimate, interval, sample size, or seed spread supports? Is the comparator the claimed one? Keep this at change-set level; route full design and decision-grade statistical validity to review-experiment.
• Lens C — the implementation auditor. Does the code do what the PR description, the commit messages, and the docstrings say it does? Where they disagree, the code is the ground truth and the mismatch is the finding.

Three rules make the roster adversarial rather than decorative:

• No empty lens. If a lens finds no defect, state the single strongest assumption it is leaning on and why that assumption is plausible. Don't manufacture cosmetics to fill a quota - an honest assumption is more useful to the human than a nitpick.
• Independent detection raises confidence, not severity. When two lenses reach the same finding by different routes, record the agreement and raise confidence if warranted. Severity still follows consequence and likelihood; lens overlap alone never promotes it.
• Break the self-review trap. If a lens comes up empty, read the relevant code bottom-up, state each function's contract before reading its body, and assume every external input could be malformed and every artifact load could silently return the wrong thing.

Do not read PR comments or linked issue comments during Steps 2 and 3 — reading them first anchors you to others' framing and weakens independent analysis.

Step 4 — Fetch comments and refine:

If the target is a branch or SHA comparison without a PR, skip comment fetching and continue to self-refutation. Keep PR communication and contributor engagement marked unavailable.

Run in parallel:

• gh pr view <number> --comments
• For any linked issues (e.g. fixes #N): gh issue view <N> --comments

Refine your findings using these to:

• Retract or downgrade findings already resolved in prior review rounds
• Treat a claimed resolution as in scope only when its fix exists in the pinned head. A comment about a later commit does not resolve a finding against the reviewed pair.
• Distinguish pre-PR reports from post-PR reports in linked issues. Pre-PR error reports are the motivation for the fix, not evidence against it. Only post-PR reports are evidence of regressions introduced by this PR.
• Revisit root-cause diagnoses: if the thread shows a proposed fix was already tried and failed, revise the diagnosis accordingly.

Step 5 — Refute your own findings:

An adversarial investigation raises the false-positive rate. That is the price of the posture, and the correction is to turn the same posture on your own output before it ships. Take each surviving finding and argue the other side: what would have to be true for this code to be correct as written? Then go and check whether it is.

The refutations that land, roughly in order of how often they do:

• the guard exists somewhere you didn't read - a caller, a decorator, config validation, a CI step
• the input can't reach the state your failure scenario needs
• the behaviour is deliberate and documented somewhere other than where you looked
• you read a version of the file outside the pinned base/head pair
• the test you called circular does reach the real producer, one layer further down

Refute by reading the code you skipped the first time, not by re-reading your own reasoning. Reasoning that produced a finding will keep producing it.

Then each finding takes one of three exits. Survived its refutation: ships, with the attempt recorded beside it ("checked the caller in x.py:40-58, no guard there") - that line is what makes the finding cheap for the human to trust. Refuted: does not ship, but the claim it came from goes into the ledger as verified, since a refuted finding is coverage, not waste. Neither confirmed nor refuted: ships downgraded and labelled with the check that would settle it, and never as a Blocker.

This step matters most on a collaborator's PR. Ten findings with three wrong ones in them reads as gatekeeping, costs the author more time than it saves, and teaches them to discount the seven that were right.

Then write the review output using the format below. Include the contributor engagement assessment. Do not draft a contributor-facing comment yet — wait for the reviewer to discuss findings and ask for one. See the "Contributor comment" section for guidelines when that time comes.

Review:

Help a human reviewer understand what changed, find real defects, and decide where to focus their attention. You are not the approver.

We are a data science team. Our PRs touch ML pipelines, data transformations, model serving, evaluation frameworks, LLM-driven agents, and the glue between them. Reviews should reflect that context — data integrity, numerical correctness, eval validity, and model/pipeline behaviour matter more than generic software engineering checklists.

Principles

• Evidence over assertion. Every finding needs a file path and line range (or function name + searchable token). No vague "somewhere in the diff."
• Uncertainty is fine. Label speculative findings as such. Include a confidence tag (High/Med/Low). Don't assert bugs you can't prove.
• High-signal only. Skip cosmetics. Focus on correctness, data integrity, silent failures, and whether tests/evals actually catch what they claim.
• Current-run-correct is not enough — flag latent-unsafe paths. When code is safe only because of how it happens to be invoked (a scorer that pins the validation baseline by convention with no guard against a test baseline; a fill value that's harmless only because eval never indexes it), and a plausible wrong invocation would silently leak, corrupt, or mis-score, that is a finding — sized by its blast radius, not by whether this run tripped it. Verifying the current run reproduces is necessary, not sufficient: say the run is clean and flag the unguarded path. This calibration is what most often separates a paranoid reviewer from a lenient one on a decision-grade gate; when in doubt on a gate, escalate the unguarded path rather than down-rate it to a provenance nitpick.
• Don't hallucinate repo context. Only assume what's visible in the diff, PR description, or files you've read. If a concern depends on something you haven't seen, ask — don't assert. This includes explanations for observed behaviour: don't assert why existing code behaves a certain way without reading it. Verify before putting it in a contributor comment.
• Verify external references. If a contributor references another plugin, library, or codebase to justify a design choice, fetch and read it before accepting the comparison. Don't characterise it based on inference.
• Read beyond the diff. Use the host's available file inspection and fast text-search capabilities, such as rg, to examine surrounding code when tracing data flows, verifying contracts, or checking for stale references. The diff alone is rarely sufficient.
• Carry the outside reviewer's disposition — you are usually the only reviewer. A genuinely different model family rarely reviews our code, so the value has to come from how this review is run, not from who runs it. Assume code is safe only by how it happens to be invoked until proven otherwise, and escalate an unguarded path (above) rather than down-rate it because this run didn't trip it. Run isolated (see below).

Large PRs: For PRs over ~500 lines, prioritise logic and pipeline files over generated outputs, data files, and lock files. State what you deprioritised and why.

Review isolation

Run the review in a fresh session or sub-agent that never saw the author's working transcript. Isolate by default when any of these hold: you helped write or plan the change; the PR is merge-gating or decision-grade; the change reverses prior project memory; the review turns on subtle provenance or artifact consistency.

Pass the isolated reviewer raw inputs only - PR number, base SHA, head SHA, repo path, and any explicitly relevant file paths. Do not pass suspected bugs, expected findings, prior conclusions, or your own private reasoning. A reviewer seeded with your hypothesis returns confirmation, not a second opinion, and it will read as agreement.

Reviewing in the main session is fine for a quick triage pass. When you do, say so in the output and name the inherited context as a residual risk rather than leaving the reader to assume the review was independent.

Evidence modes

Use the least expensive mode that can settle each load-bearing claim, and state which modes the review used:

1. Source inspection: read the pinned diff, surrounding code, tests, configuration, and repository process rules. Every review uses this mode.
2. External evidence inspection: inspect persisted data or run evidence when a PR claim depends on something outside the repository. This is distinct from running PR code.
3. PR-code execution: run tests, reproducers, or defect injection in a throwaway checkout when reading and persisted evidence cannot settle a consequential claim.

External research evidence

When a load-bearing claim depends on data, models, or run evidence outside the repository, read references/external_evidence.md and follow its read-only protocol. Do not load it for repository-only reviews. Verify only the PR claim or boundary contract; independent scientific recomputation belongs to review-experiment.

Throwaway execution checkout

The adversarial stance above is free and applies to every PR. Executing costs time and a checkout, so spend it where being wrong is expensive: merge-gating PRs, changes to scoring, eval or data paths, anything whose output feeds a published number, and any PR where a Step 2 claim cannot be settled by reading. A small refactor with green CI doesn't need it. Say which way you went and why - a review that only read should not sound like a review that ran.

When you execute PR code, use a throwaway worktree, never the reviewed repository's active working tree:

Put it under your scratchpad directory if the session has one, otherwise a temp path:

git fetch origin pull/<number>/head
git worktree add --detach <temporary-path>/review-pr-<number> <head-sha>
# run, break, and read in <temporary-path>/review-pr-<number>
git worktree remove <temporary-path>/review-pr-<number> --force

A worktree makes source mutations disposable; it is not a process-security boundary. It shares the host, credentials, network, and Git object store. Treat code, tests, build hooks, and dependency installers from the PR as untrusted. Run them only inside the host's real execution boundary, such as its sandbox, container, or disposable VM, with credentials removed and network disabled unless the check requires it. If that boundary is unavailable or execution needs new authority, request approval or leave the claim unverified. Never imply that a worktree alone made arbitrary PR code safe to execute.

gh pr diff works on any repo, but the worktree needs a local clone. If the PR is on a repo you don't have, either gh repo clone it first or say plainly that the review was read-only because the code wasn't available to run - don't quietly drop to reading and report as though you had run.

Three rules:

• Never mutate the reviewed working tree. The author may still be committing into it, and a review that leaves a dirty tree behind costs more than it found.
• Report reproducible commands and their results. Redact credentials, signed URLs, and sensitive arguments. A concrete pass or failure is worth more than reasoning about what a test probably does. If you only read it, say you read it.
• Don't commit, push, or post from the throwaway checkout. It is for reading and breaking. Fixes and comments go to the author, and posting anywhere needs an explicit instruction first.

Defect injection is the strongest verification available: introduce the bug a test claims to catch, and confirm the test fails. The throwaway checkout makes the source edit disposable; execution safety still comes from the boundary above. Edit the real source, run, then discard the whole worktree rather than restoring by hand. Prefer editing source over monkeypatching: from X import Y binds into the importing module, so patching the source module's attribute leaves the test's binding untouched and makes a working guard look broken. A test that still passes with the defect in place is a finding. Size its severity by the consequence of the behaviour the test claims to protect; it is a Blocker only when the unprotected defect is itself merge-gating.

---

Review techniques

Seven techniques, ranked by value from practice, live in references/review_techniques.md, each with the example that makes it recognisable. Read that file before forming findings.

1. Question the premise - does this need to be built, or does it already exist internally or externally?
2. Trace the data flow end-to-end - follow one value from entry to final use; the silent drop is in the middle.
3. Additional checks - weakened tests, stale references, outputs not derivable from inputs, orphaned artifacts, environment assumptions.
4. Is the change COMPLETE, not just correct - grep the concept's old form repo-wide; hunt the green test pinning the old contract; treat docs as callers.
5. Can this test actually fail - circular assertions, silent skips, break it on purpose.
6. Complexity compensating for wrong architecture - trace defensive clusters back to the constraint that forces them.
7. Implicit contracts at system boundaries - inspect the artifact the code consumes; don't trust its variable names.

---

Output format

Adapt the format to the PR. Don't force rigid sections when they add no value. The core deliverables are:

Orientation

What this PR does, why, and what it touches. Keep it short — the reviewer should understand scope in 30 seconds. State the pinned base and head SHAs, and whether the review ran isolated or in the main session.

Claim ledger

The load-bearing claims from Step 2, each marked falsified / verified / unverified. One line each. This is the coverage statement for the review: it tells the human what was actually tested, which a findings list on its own never does.

Change map

A table grouping changes by area, with risk level (Low / Medium / High) and a one-line "why risky" for anything Medium or High.

Findings (prioritised)

Each finding needs:

• Severity: Blocker / Important / Suggestion
• Title: one line
• Evidence: path:line-range or function name + searchable token
• Why it matters: what breaks, what's silent, what's unverified
• Refutation attempted: what you checked to try to kill this finding, and what you found
• Confidence: High / Med / Low
• Recommendation: what to do about it

Severity guidance:

• Blocker: merge should wait because a likely defect changes core behaviour, corrupts data, invalidates decision-grade or published output, bypasses a required safety boundary, or leaves a load-bearing defect demonstrably undetectable
• Important: meaningful correctness edge-case, weakened tests/evals, missing coverage for core behaviour, parameter silently ignored, dead code that misleads, or a latent-unsafe path unguarded against a plausible wrong invocation (safe-as-invoked ≠ safe)
• Suggestion: cleanup, stale references, minor inconsistency, nice-to-have tests

When two lenses reached a finding independently, say which two and reflect that in confidence without mechanically changing severity.

Assumptions surfaced

For each lens that returned no defect, the strongest assumption it was leaning on. These are the review's blind spots stated out loud, and they are often more useful to the human than a third Suggestion.

Follow-ups

Unrelated bugs surfaced during review go here as one-paragraph drafts, not folded into the in-flight PR's findings. Keeps the PR one-feature.

Where the human should look

Explicitly list the 3-5 specific files/locations the human reviewer should read themselves, with a one-line reason for each. The reviewer's time is scarce — direct it to the highest-leverage spots.

Questions for the author

Only questions that materially reduce risk. Each must say why you need the answer and where in the code it matters.

Contributor engagement assessment

Evaluate the PR for signs of thoughtful work vs low-engagement / unreviewed agent output. This is not about whether agents were used — it's about whether the contributor understands what they're submitting. Assess code and PR communication independently (a contributor may use agents for code but engage thoughtfully in discussion, or vice versa).

Signals to look for:

• Code understanding: Does new code reuse existing patterns and call into existing functions, or does it duplicate logic from scratch? Are unrelated changes bundled without explanation?
• PR communication: Does the description explain implementation decisions, or just echo the issue text? Do responses to review comments engage with the questions asked, or summarise what was done?
• Slop markers: Inconsistent formatting that doesn't match surrounding code. Mechanical edge-case handling (silent fallbacks, bare catches) without considering UX implications. Generic commit messages. Versioned storage keys and // no-op catch blocks as boilerplate. Complete rewrites between rounds submitted without any explanation of what changed or why — this is stronger signal than any single code quality issue.
• Cross-PR patterns: If the contributor has prior PRs on the repo, check those interactions. Do they engage with questions or just post summaries of what they changed? This is stronger signal than any single PR.

State your read briefly. This helps the reviewer calibrate how much architectural guidance to give vs comprehension questions to ask.

---

Contributor comment

After the reviewer has discussed and finalised the review findings, they may ask for a contributor-facing PR comment draft. When drafting:

Tone and format:

• Human, collegial. No corporate speak, no jargon walls, no em dashes. Write like a maintainer, not a review bot.
• Plain paragraphs, not bold headers or bullet walls. Numbered lists are fine for questions. A PR comment that looks like a structured report reads as agent-generated.
• Be direct about problems without being condescending. Nudge the contributor to discover issues themselves ("try switching notes during a chat session and see what happens") rather than stating the bug.
• Acknowledge what works briefly and specifically, but be careful what you reinforce.

Structure:

• Lead with findings that would change the entire approach. Save smaller items for later.
• Ask before telling. "What does X do in the existing flow?" reveals more than "you're missing X."
• Keep it short. One paragraph of substance, a few pointed questions.
• End with something specific, not a generic closer.
• Match guidance depth to evidence of effort. Genuine codebase engagement gets specific technical pointers. Low engagement gets comprehension questions first. Don't prescribe architectural solutions before verifying the contributor understands the current architecture.

Low-engagement multi-round PRs:

• If comprehension hasn't been demonstrated after one or more rounds, lead with: "Before we go further, can you walk me through the main architectural decisions in this PR and why you made them?"
• Don't give further findings until you have an answer.
• Questions with no Googleable answer (e.g. "why did you put X here rather than Y?") reveal understanding better than questions with obvious answers. If the contributor doesn't engage, that is itself the signal.

Probing questions:

• Test whether the contributor understands the existing code, not just whether they can fix a specific line.
• Don't ask questions you've already answered in the comment.
• If the PR description already explains a choice, don't re-ask about it.

---

What to skip

• Cosmetics. Don't comment on naming, formatting, or style unless it causes confusion.
• Style and visual comments when architecture is unsettled. If structural findings are still open (wrong component boundary, duplicated pipeline, missing abstraction), defer CSS and layout feedback to a later round. Flag them internally but don't raise them — the structure may change and the style discussion becomes moot.
• Generic checklists. Don't mechanically run through security/auth/deploy/rollback checklists unless the PR actually touches those areas. Irrelevant checklist items are noise.
• Merge risk summaries. The findings speak for themselves. Don't add a "safe to merge" / "needs changes" label — that's the human's call.
• Boilerplate sections. If a section would be empty or trivially "N/A", omit it entirely.