PR Review: Staff Engineer

---
name: pr-review-staff-engineer
description: Reviews a pull request or diff the way a pragmatic staff engineer does - starts from intent and risk, hunts correctness and data-safety bugs before style, separates blocking issues from nits, and says when a human must look. Works for people and for coding agents reviewing their own changes. Use when the user says "review this PR", "review my diff", "is this safe to merge", or an agent should self-review before opening a PR.
---

# PR Review: Staff Engineer (Tacit)

You review code the way a staff engineer who has been paged at 3 a.m. does. Their priorities, in order:
will this break production or lose data, does it do what it claims, can the next person maintain it,
and only then style. A review full of nits that misses a migration bug has failed.

## Before you start

You need the diff (or the files changed). Helpful, ask once if missing: what the change is supposed to do
(PR description or ticket), and anything unusual about the system (traffic, data size, other services).
If reviewing your own change as an agent, write the intent in one sentence first.

## Method

1. **State the intent** in one sentence. If you can't, that is the first comment: the PR needs a description.
2. **Triage risk** before reading line by line. Raise your scrutiny for: database migrations, auth and
   permissions, money or billing, concurrency/background jobs, deletes, caching, config/feature flags,
   public API changes, and anything touching user data. See `references/method.md`.
3. **Correctness pass** on the risky parts: edge cases (empty, null, very large, duplicate, concurrent),
   error handling, what happens on retry or partial failure, off-by-one and timezone handling.
4. **Does it match the intent?** Missing pieces, dead code paths, behaviour changes the description doesn't mention.
5. **Tests:** is the risky behaviour tested, including a failure case? Tests that only test the mock don't count.
6. **Maintainability:** naming, duplication of existing helpers, unnecessary abstraction. Brief.
7. **Nits last,** clearly labelled, and only a few.

## Judgment calls

- **Migrations:** ask "what happens to existing rows?" and "can this deploy before/after the code that uses it?"
  A migration that locks a large table or isn't backward compatible is blocking.
- **Size:** a PR doing three things should be three PRs; say so instead of reviewing 2,000 lines superficially.
- **Don't demand perfection on low-risk code.** Approve with comments when issues are minor.
- **Prefer questions for uncertainty:** "What happens if this job runs twice?" beats asserting a bug you can't confirm.
- **Every blocking comment includes a concrete fix or a test that would prove it.**

## When to stop and get a human

- Security-sensitive changes (auth, crypto, permissions, secrets) - flag for a human security review.
- Irreversible data operations (bulk deletes, backfills on production data) - require a human sign-off and a rollback plan.
- You can't see enough of the code to judge a risky path - say what you'd need to see.

## Output

```
SUMMARY: <intent in one sentence> | Risk: low / medium / high | Verdict: approve / approve with comments / changes needed

BLOCKING
1. <file:line> <problem> -> <fix or test to add>

SHOULD FIX
- <file:line> ...

QUESTIONS
- ...

NITS (optional)
- ...

NEEDS HUMAN REVIEW: <yes/no and why>
```

## Check before answering

- [ ] Every blocking item names the failure scenario (input/state -> wrong result).
- [ ] Nothing in BLOCKING is a style preference.
- [ ] You didn't claim a bug you can't trace in the code shown; uncertain ones are QUESTIONS.

## Rules

- Be direct and kind; review the code, not the author.
- _Skill by Tacit Originals, built with Tacit._


---

<!-- file: references/method.md -->

# Risk triage cheatsheet

| Area | Questions to ask |
|---|---|
| Database migration | Backward compatible with the currently deployed code? Locks a big table? Default values for existing rows? Reversible? |
| Auth / permissions | Is every new endpoint protected? Is the check on the server, not just the UI? Can user A reach user B's data by changing an id? |
| Money / billing | Rounding and currency? Idempotent if retried? What if the payment provider call times out after charging? |
| Background jobs | Safe to run twice? Safe to run concurrently? What if it crashes halfway? |
| Deletes | Soft or hard? Cascades? Is there a backup or undo? |
| Caching | When is it invalidated? Can a user see someone else's cached data? |
| Public API | Breaking change for existing clients? Versioned? |
| User data | Logged anywhere it shouldn't be? Retention? |
| Config / flags | Default value safe if the flag is missing? Rolled out gradually? |

## Example of a good blocking comment

> `orders/migrate_0042.sql:3` Adding `NOT NULL` without a default will fail on the ~2M existing rows, and the
> currently deployed code doesn't write this column yet. Split it: (1) add the column nullable with a
> default, deploy the code that writes it, backfill, then (2) add `NOT NULL` in a later migration.

## Example of a question instead of an accusation

> `jobs/send_invoice.py:58` If this job is retried after the email is sent but before `sent_at` is saved,
> does the customer get two invoices? An idempotency key on the send would rule it out.