Principal Engineer Code Review

十个审阅视角的合并评审：先读 diff 之外的相邻代码，按严重度排序，且不接受说不出具体触发输入的「发现」。

目标是揪出真 bug、对其余保持安静。它先要求理解改动本身并打开其调用方与数据读取方，再逐个视角审阅——正确性、边界情况、失败路径、并发与时序、安全、性能、资源释放、测试、一致性、可读性——配合 Blocker/Major/Minor/Nit 分级与最终结论。附加章节涵盖「压根没写的部分缺了什么」提问、在全新上下文中审阅自己生成的代码，以及标记 PR 描述与代码不符之处。

# Code Review

**Use when:** reviewing a diff, a pull request, or code you just generated.
**Pairs with:** [../core/self_critique.md](../core/self_critique.md), [security_review.md](security_review.md)

Generic review prompts produce style nitpicks and miss the bug. This one is built to find the bug and stay quiet about the rest.

---

## The prompt

```text
Review this change as a principal engineer. You did not write it. You are
deciding whether it can merge.

FIRST, UNDERSTAND IT
Do not review line by line yet.
- What is this change trying to do? One sentence.
- What is the mechanism? How does it actually work?
- What does it touch that is not in the diff? Find the callers. Find what
  reads the data this writes. Open those files.
A review of a diff without reading its neighbours is not a review.

THEN, REVIEW BY LENS
One lens at a time. Do not mix them.

1. Correctness
   Does it do what it claims on the normal path? Walk through it with a
   real input and say what happens at each step.

2. Edge cases
   Empty, null, zero, one, maximum, duplicate, out of order, unicode,
   very large, negative, and the boundary at every comparison. Name the
   specific input, not the category.

3. Failure paths
   Every call that can fail: what happens when it does? What does the
   user see? What state is left behind? Is anything half written?

4. Concurrency and ordering
   Two of these at once. This one interrupted halfway. This one racing
   with the thing next to it. Shared mutable state.

5. Security
   Where does untrusted input enter and what is done with it? Injection,
   path traversal, unchecked authorisation, leaked secrets, logged
   sensitive data, unsafe deserialisation.

6. Performance
   At production data size, not the test fixture. Any loop that makes a
   call. Any repeated work that could be done once. Any query without a
   bound.

7. Resource handling
   Opened and not closed. Allocated and not freed. Subscribed and not
   cancelled. Timers, observers, listeners, file handles, connections.

8. Tests
   Do the tests fail if the code is wrong? Change the logic in your head
   and ask which test catches it. A test that passes either way is not a
   test.

9. Consistency
   Does this match how the rest of the codebase does the same thing? A
   better pattern used once is worse than a worse pattern used everywhere.

10. Readability
    Would someone new understand this in six months without asking? Name
    the specific line that would confuse them.

FINDINGS FORMAT
  [Severity] file:line, what breaks, concretely
  Fix: the specific change

  Blocker: wrong behaviour, data loss, crash, security issue, or it does
           not do what it claims.
  Major:   works now, will hurt later, or hurts the user today.
  Minor:   real, small, would be a comment not a rejection.
  Nit:     taste. Mark it as a nit and keep it to a handful at most.

RULES
- Rank by severity. Blockers first. Never bury a Blocker under nits.
- Every finding names an input, a sequence, or a condition that triggers
  it. "This could fail" is not a finding.
- Do not comment on formatting that a formatter would fix.
- Do not restate what the code does back to me.
- Do not invent findings. If a lens is clean, say the lens is clean and
  why in one line.
- If you did not open a file you needed, say so instead of guessing.

END WITH
- Verdict: merge, merge after blockers, or rework.
- The one thing most likely to break in production, if any.
- What is missing that is not in the diff at all. Missing tests, missing
  migration, missing error handling, missing docs for a public API.
```

---

## The absence question

The most valuable single line in a review prompt:

```text
What is missing from this change that should be here? Not what is wrong
with what was written, but what was not written at all.
```

Reviewers, human and model, find flaws in what is present. Almost nobody finds absence. This question does.

---

## Reviewing your own generated code

Run it in a fresh context. Paste the diff with only this:

```text
You did not write this. Review it for merge. Assume there is at least one
real problem in it, because there usually is.
```

The "assume there is one" line changes the search from "is this okay" to "where is it". The model finds more, and it fabricates less than a quota like "find five issues" would cause.

---

## Severity discipline

Most model reviews collapse into a flat list where a naming preference sits next to a null dereference. Enforce the split:

```text
Give me the Blockers alone first, with nothing else. Then stop. I will ask
for the rest if I want it.
```

This one line makes model reviews usable on large diffs.

---

## For pull requests with history

```text
Also read the PR description and the commit messages. Flag any place
where the description claims something the code does not do, or the code
does something the description does not mention. Undisclosed changes are
a finding.
```