Self-review before a PR

Not a general code-review checklist — the diff has already been read and the tests already pass. This is two questions that neither of those answers.

They are not invented. Over 2026-08-25/27, across leaders-app, backend-services and organization-dashboard, a reviewer raised fourteen Major findings on PRs that had green CI and a self-review behind them. Every single one fell into one of two families. Tests never caught any of them, because the tests were written from the same incomplete reading as the code.

Run both passes before pushing, and put the answers in the PR description. Writing them down is the mechanism: the failure mode here is confidence, not ignorance, and confidence survives being asked "did you check?" but not being asked "list them".

Pass 1 — Path enumeration

Wrong question: "does my path work?" Right question: "which other paths reach the state I just changed?"

Answer with grep, not from memory. For every piece of state the change touches — a field, a table, a flag, a decision — list every caller and every writer, and say what happens in each. A path you did not name is a path you did not handle.

grep -rn "functionName" lib/ src/ apps/ | grep -v test     # every caller
grep -rn "fieldName\|table_name" --include="*.dart" --include="*.ts"   # every writer

How this family actually presented:

the miss	path handled	path forgotten
a hidden event kept being scanned	prepareEvent	the background flush, the more common one
per-code refusal had no memory of server admissions	the local decision	the server hand-off path
a visibility guard 404'd valid resources	the community branch	the group branch — both scopes can be non-empty
a list figure went stale	first load	the reload on returning to the page
an internal field leaked to API clients	the write	every read that returns the document
two broken auth routes were found, a third shipped	the two seen by eye	the third, found only on a real review

Three sharpenings worth keeping:

• if / else if on scopes, roles or sets — ask whether both can be true. Usually they can.
• A field added to a document — find every response that returns that document, not just the code that writes it.
• A behaviour added to one entry point — find the other entry points. There is almost always a background/timer path next to the interactive one, and it is usually the busier of the two.

What this family looked like in practice:

miss	the path handled	the path forgotten
hidden event kept being scanned	prepareEvent	flush → runCycle, the more common one
per-code refusal had no memory	decide() (local)	the server-admission path
visibility guard 404'd valid events	the community branch	the group branch — both scopes can be non-empty
ticket usage went stale	first load	the reload on returning to the page
an internal field leaked to clients	the write	every read that returns the document
three broken auth routes	the two found by eye	the third, found only on a real review

Pass 2 — Async lifecycle

For every await added or moved:

1. Who awaits me? Trace up to the nearest unawaited(...) / fire-and-forget / event handler. A throw reaching that point is an unhandled async error, and everything after the throw is silently skipped.
2. What did I read before the await and write after it? If two invocations can overlap, the slower one lands last and clobbers the newer value. Re-check the precondition before writing.
3. Do two consecutive operations need to be atomic? A check followed by a mutation is a race unless both sit in one transaction. Ask what a concurrent write between them would do.

And a rule rather than a question: unawaited on a write that a later read depends on is a bug. If the point of the write is that something else can see it, it must be awaited.

Pass 3 — Verifying "there are no open review comments"

Mechanical, and the same error as Pass 1 applied to the review itself: measuring one place and generalising from it.

• A green check is not a review. gh pr checks passing says CI ran, nothing more.
• reviewThreads over GraphQL misses findings posted outside the diff range — those live in the review .body, not as threads. Read both:

gh api graphql -f query='{ repository(owner:"O", name:"R") { pullRequest(number:N) {
  reviewThreads(first:50) { nodes { isResolved path } } } } }'
gh api repos/O/R/pulls/N/reviews --jq '.[] | select(.body | test("Outside diff range";"i")) | .body'

• A CHANGES_REQUESTED verdict can predate the fix commit and never be re-submitted. Compare the review's submittedAt against the commit before treating it as outstanding.
• An empty result is not a pass. gh pr checks printing "no checks reported", or a status rollup of 0 entries, is no data — say so rather than reading it as clean. This is especially live right after a force-push, when no check has registered yet.
• Before claiming an absence from a shell query, confirm the query resolved. In zsh "$var:path" is parsed as the ${var:a} modifier and the command fails silently — quote it "${var}:path".

What to write in the PR

State the enumeration, not the conclusion. "Checked all callers" is worth nothing; a list is worth something:

burnOrderProductLine is the single burn for scans, reached from applyOfflineScan (offline) and scanOrderProductCode (online), so this covers both. validateOrderTickets deliberately passes no code hash — it burns all remaining units and has no code to attribute them to.

Name what you deliberately did not do, and why. A reviewer who can see the boundary of the change stops guessing at it.

When to skip

Pure text, copy, docs, or a config value with a single consumer. Anything that changes behaviour, touches shared state, adds an await, or edits a guard gets both passes.

Related

• address-review — the other end of the same loop: this skill tries to prevent findings, that one handles the ones that arrive. It already reads both inline comments and get_reviews, so it does not share the Pass-3 blind spot.
• Human Review in an AI Workflow