---
name: github-pull-request-review
description: Review a GitHub pull request the way a strong senior reviewer does, judging intent, risk and missing tests before style, and sorting feedback into blocking, should-fix and nit. Use when the user asks to review, check, summarise or sign off a pull request, or wants to know whether a change is safe to merge.
---

# Review a pull request

A review answers one question: **is it safe and sensible to merge this?** Style comes last. A review that lists twenty nits and misses the broken edge case has failed.

## 1. Understand intent before reading code

Read the title, description, linked issue and the author's comments with `pull_request_read` (method `get`). Write down, in one sentence, what the change is supposed to do. If the description is empty and the diff is non-trivial, say so; that is itself feedback.

Check the facts that change how you review:
- **Draft?** Review for direction, not polish.
- **Base branch** is not `main`/the default? It may be a stacked PR; review only its own commits.
- **Size.** Over about 400 changed lines of real code, or mixing a refactor with a behaviour change: say that it should be split, then still review the risky part. Do not silently skim.

## 2. Review in risk order, not file order

Fetch the file list (`pull_request_read`, method `get_files`) and sort it yourself:

1. **Read first, hardest:** auth, permissions, payments, data deletion, migrations, concurrency, anything parsing untrusted input.
2. **Core logic** the description is about.
3. **Tests.**
4. **Glue, config, docs.**
5. **Skip line by line:** lockfiles, snapshots, generated code, vendored files. Check only that they are consistent with the source change (a dependency bump with no lockfile change is a finding).

Read the code around each hunk with `get_file_contents` at the head ref. A hunk alone hides the function it sits in.

## 3. What diffs hide: look for what is missing

The most valuable findings are not in the diff. Ask:
- **A changed function signature or return shape.** Find every caller with `search_code`. Any caller not updated in this PR is a likely break.
- **A new env var, flag or config key.** Is it added to the deploy config, `.env.example` and docs?
- **A schema migration.** Is it reversible, safe on a big table (no long lock, no backfill in the same transaction), and is the code deployable *before* and *after* the migration runs?
- **A new endpoint or query.** Authorisation check present? Input validated? Paginated? Index for the new filter?
- **An error path.** What happens on null, empty list, timeout, retry, partial failure?
- **A new dependency.** Maintained, licensed compatibly, worth its weight?
- **A deleted check, test or `skip`.** Why?

## 4. Judge the tests, not just their existence

A test is worth something only if it **fails without the fix**. Check that new tests assert behaviour (outputs, state) rather than that a mock was called. For a bug fix, there should be a test that reproduces the bug. Flag tests that can never fail, such as asserting on the value they just set.

## 5. Read CI properly

Use `pull_request_read` (method `get_status`) and `actions_list` for the head commit. A green run on an *older* commit means nothing. Red checks: say which and whether they relate to this change or look flaky or pre-existing (compare with the base branch's latest run). Never wave a failure away as "probably flaky" without evidence.

## 6. Sort every comment

Label each finding so the author knows what must happen:

| Label | Meaning | Blocks merge |
|---|---|---|
| **Blocking** | Bug, security hole, data loss, broken contract, missing test for risky behaviour | Yes |
| **Should fix** | Likely future bug or real maintainability cost | Discuss |
| **Question** | You cannot tell if it is a problem; ask, do not assert | No |
| **Nit** | Taste, naming, formatting | No |

Cap nits at three and say "plus similar nits elsewhere". If a linter or formatter could catch it, do not comment on it at all.

Be specific and kind: quote the line, say what goes wrong and for which input, and offer a fix. "This breaks when `items` is empty because line 48 indexes `items[0]`" beats "handle edge cases".

## Output

```
Verdict: <ready | ready with small fixes | blocked> — <one sentence why>

What it does: <one sentence>
Scope: <files/lines reviewed; anything skipped and why>

Blocking
1. path:lines — what breaks, for which input, suggested fix
Should fix
...
Questions
...
Nits (max 3)
...
Missing tests: <behaviour with no coverage>
CI: <state on the head commit, relevant failures>
Could not check: <honest gaps>
```

## Posting the review

Drafting is free. **Do not post, approve, request changes or merge** without the user's explicit yes in this conversation, and never approve a pull request the user authored. When the user does approve posting, put all comments in **one** review rather than many single comments, so the author gets one notification, and use `suggestion` blocks only for small, certain fixes.

## Done when

- The verdict is stated first and is justified by at least one concrete finding or by an explicit "I checked X, Y, Z and found nothing".
- Every blocking item has a file, a line range and a fix.
- The "could not check" line is honest.