|
| 1 | +--- |
| 2 | +name: pr-review |
| 3 | +description: Review a branch against its merge base with main as a maintainer of this repo would, looking for correctness bugs, duplicated mechanisms, poor fit with existing extension points, behavior changes on empty input, stored state that goes stale, missed precedent, user-facing naming and text, imprecise doc comments, and weak or misplaced tests. Use when asked to review a branch, a commit, or a PR in this repository. |
| 4 | +--- |
| 5 | + |
| 6 | +# PR review |
| 7 | + |
| 8 | +Review the change as a maintainer who knows this codebase would. The question |
| 9 | +is both "is this correct" and "does this fit what is already here". Produce a |
| 10 | +small number of short, specific comments that make the author look at |
| 11 | +something they missed. |
| 12 | + |
| 13 | +Judge the change against what the codebase already does, not against the |
| 14 | +author's own design. The PR description tells you what the author intended; |
| 15 | +your job includes asking whether that intent is right. "This is how the new |
| 16 | +code is designed" never justifies a behavior on its own. |
| 17 | + |
| 18 | +## 1. Collect the change |
| 19 | + |
| 20 | +The review target is `HEAD` unless you were given a commit. Set `TARGET` to it |
| 21 | +and `BASE` to its merge base with main: |
| 22 | + |
| 23 | +```bash |
| 24 | +TARGET=HEAD |
| 25 | +BASE=$(git merge-base "$TARGET" origin/main) |
| 26 | +git diff --stat "$BASE" "$TARGET" |
| 27 | +git diff "$BASE" "$TARGET" |
| 28 | +``` |
| 29 | + |
| 30 | +If the change has a PR, read its title and description to learn the intent. |
| 31 | +Do not read existing review comments before forming your own view. |
| 32 | + |
| 33 | +Read every changed non-generated file in full at `TARGET`, not just the hunks |
| 34 | +(`git show "$TARGET":path`). Skim generated files (schemas, codegen output, |
| 35 | +acceptance test output) only to confirm they follow from the source change. |
| 36 | +Read the repo's agent rules that apply to the changed files, under |
| 37 | +`.agents/rules/` (testing, comments, language style guides). |
| 38 | + |
| 39 | +## 2. Build situational awareness |
| 40 | + |
| 41 | +Before judging the change, find what it touches in the rest of the codebase. |
| 42 | +For every new type, function, flag, env var, config field, or code path: |
| 43 | + |
| 44 | +- Search the codebase at `TARGET` (`git grep`) for code that already does the |
| 45 | + same job: the same noun combined with the same verb ("resolve", "match", |
| 46 | + "load", "parse", "retry"). Look in the same package first, then callers. |
| 47 | +- Read the callers of the functions the change modifies, and the code that |
| 48 | + runs before and after the new code. |
| 49 | +- Look for the helper the codebase normally uses for the small things the |
| 50 | + change does by hand: reading env vars, parsing booleans, normalizing paths |
| 51 | + or hosts, formatting errors, retrying. In this repo, `libs/env` has `Get`, |
| 52 | + `Lookup` and `GetBool`. |
| 53 | + |
| 54 | +Note, for yourself, how the new code differs from each existing piece you |
| 55 | +find. |
| 56 | + |
| 57 | +## 3. Required passes |
| 58 | + |
| 59 | +Do every pass. Passes 1, 4, 5, 6, 7 and 8 each produce a table for the |
| 60 | +worksheet, even when they find nothing; filling in the table is how you find |
| 61 | +the comments. Each finding must be backed by code you read; drop anything you |
| 62 | +can't point to, and re-check it against the code before keeping it. Comment |
| 63 | +on code the change adds or modifies, not on pre-existing issues next to it. |
| 64 | + |
| 65 | +1. **Correctness.** For each changed function, trace the main path and the |
| 66 | + edge cases: nil or empty values, errors that are dropped or wrapped |
| 67 | + inconsistently, loops and early returns, concurrency and shared state, |
| 68 | + resources that aren't closed, off-by-one and ordering assumptions, and |
| 69 | + behavior that differs between platforms. |
| 70 | + Table: `line | input or state | what happens | is that right?` |
| 71 | +2. **One mechanism per job.** Does the change add a second way to do |
| 72 | + something the codebase already does, or copy logic that now has to be kept |
| 73 | + in sync ("same logic as X")? Point at the existing one and ask how they |
| 74 | + differ and whether they can be unified. If they legitimately differ, check |
| 75 | + that the new one's doc comment says how. |
| 76 | +3. **Use the existing extension point.** If the codebase composes behavior |
| 77 | + through a chain or registry (loaders, mutators, middleware, hooks, resource |
| 78 | + adapters), new behavior should plug into it instead of being special-cased |
| 79 | + in a caller. Say where it would go and what that buys. In particular, a |
| 80 | + step that reads remote or stored state to decide what to do (recreate, |
| 81 | + skip, migrate) usually belongs where the framework already reads state, |
| 82 | + such as a resource's read or diff method, not in a later step. |
| 83 | +4. **Every new error path.** List each error return the change adds. For |
| 84 | + each, write down the input that reaches it and ask whether that input |
| 85 | + should fail at all. First write `Feature acts on: ...`: the input the |
| 86 | + change was introduced to handle, not every input it reads. When that input |
| 87 | + is absent, the code should normally leave the existing behavior in place. |
| 88 | + Fill "without the change" by tracing the same input through the code at |
| 89 | + `BASE`, not by restating the new code. "The new code requires it" is not a |
| 90 | + reason to answer "yes". |
| 91 | + Table: `line | input that reaches it | what happens without the change | |
| 92 | + should it fail?` |
| 93 | +5. **Sibling routes check the same things.** Find the routes in the change |
| 94 | + that accept, select or process the same kind of object differently (for |
| 95 | + example create vs update, matched by value vs named explicitly, deploy vs |
| 96 | + destroy). Write down what the change promises, from the description and |
| 97 | + doc comments, and list per route which promises it keeps. If one route |
| 98 | + checks or handles something its sibling doesn't, ask about it as a short |
| 99 | + question. |
| 100 | + Table: `route (line) | promised property | handled here? (line or "no")` |
| 101 | +6. **Every new env var, flag, config field and output field.** Check the name |
| 102 | + first: it is user-facing API and should use words users already see in |
| 103 | + config, docs and output, not an internal term from the implementation. |
| 104 | + Then check how the value is used: a presence-only check on an on/off |
| 105 | + switch means `false` and `0` turn it on. Check that new config fields |
| 106 | + follow how similar existing fields are declared, validated and documented. |
| 107 | + Table: `line | name | user-facing word or internal term? | how the value is |
| 108 | + used | existing precedent` |
| 109 | +7. **Stored values over time.** For every value the change writes to state, |
| 110 | + files or caches, write down when it is first written, when it is updated, |
| 111 | + and who reads it later. Ask whether it goes stale after the first write, |
| 112 | + whether comments and output describe that lifecycle correctly, and what |
| 113 | + should happen when the stored value disagrees with the source of truth |
| 114 | + (often: an error, not a silent fallback). |
| 115 | + Table: `value (line) | written when | updated when | read by | can it |
| 116 | + disagree with the source of truth, and what happens then?` |
| 117 | +8. **Every new doc comment.** List them: |
| 118 | + |
| 119 | + ```bash |
| 120 | + git diff "$BASE" "$TARGET" -- '*.go' ':!*_test.go' | grep -E '^\+(//|func |type |var |const )' |
| 121 | + ``` |
| 122 | + |
| 123 | + Each comment above a new top-level declaration goes in the table. It |
| 124 | + should say what is decided, from which input, and where the result comes |
| 125 | + from. Flag comments that are wrong, that blur similar nouns, or that |
| 126 | + describe the mechanism but not the intent. Offer the sharper sentence. |
| 127 | + Also look for code that works around surprising behavior of an API or |
| 128 | + tool (adds a suffix, strips a prefix, retries a specific error) without |
| 129 | + a comment saying why; ask why it happens. |
| 130 | + Table: `line | what the comment claims | what the code does | precise |
| 131 | + enough?` |
| 132 | +9. **Tests.** Review test code as carefully as the code under test. |
| 133 | + - Level: follow `.agents/rules/testing.md`. User-visible CLI behavior and |
| 134 | + the bundle mutator pipeline are tested with acceptance tests first; pure |
| 135 | + functions and helpers with unit tests next to the existing ones. Flag a |
| 136 | + unit test that should be an acceptance test, and logic with many input |
| 137 | + combinations that has no unit tests (name a case they would catch). |
| 138 | + - Does each new or changed test show the effect it claims? The output |
| 139 | + should include the state after the action (list or print it again after |
| 140 | + a sync, a deploy or a migration) and the exact value the change is |
| 141 | + about, not just a successful exit. |
| 142 | + - Are fixtures readable? Prefer generating archives and other binary |
| 143 | + inputs in the test script over committing opaque files. |
| 144 | + - Does the test reuse the repo's existing test helpers and scripts, and |
| 145 | + keep helpers that a planned follow-up will need again? |
| 146 | + - One long script that walks many failure modes is hard to review: |
| 147 | + suggest splitting it by category. |
| 148 | +10. **User-facing text.** Read every new or changed changelog fragment |
| 149 | + (`.nextchanges/`), error message, warning and help text. It should be |
| 150 | + short, accurate and written for users, follow `.nextchanges/README.md` |
| 151 | + for changelog entries, and reuse an existing constant instead of |
| 152 | + repeating the same text in several places. |
| 153 | + |
| 154 | +Skip formatting, naming of internal identifiers, anything a linter catches, |
| 155 | +and questions about defensive checks. |
| 156 | + |
| 157 | +## 4. Write the comments |
| 158 | + |
| 159 | +**Coverage.** Every problem you confirmed gets a comment. There is no target |
| 160 | +count: a small change may need one comment and a large one fifteen or more. |
| 161 | +Never drop a real finding to keep the review short, and never pad a review |
| 162 | +with weak ones to look thorough. A problem row in the worksheet is dropped |
| 163 | +only if re-checking the code shows it is wrong, already handled, or |
| 164 | +documented as intended (in a comment, a test, or the description); say which |
| 165 | +in the Evidence section. |
| 166 | + |
| 167 | +**Each comment must:** |
| 168 | +- Point at the single line where a reader would trip (the error return, the |
| 169 | + vague comment, the presence check, the copied block). |
| 170 | +- Say what is wrong in terms of behavior, with a concrete input and outcome: |
| 171 | + "With a plan saved while the snapshot was clean, `deploy --plan --force` |
| 172 | + stays on the modified snapshot, while `bundle deploy --force` moves to a |
| 173 | + fresh one". Name the other code involved by file and symbol when the |
| 174 | + comment depends on it. |
| 175 | +- End with what you want: the change you'd make, or the question the author |
| 176 | + needs to answer. "Can we reuse `X` from `path`?", "Should this fail |
| 177 | + instead?". A comment that only describes is not finished. |
| 178 | + |
| 179 | +**Voice:** |
| 180 | +- Short and plain: usually one to three sentences, more only when the |
| 181 | + consequence needs a second example. No headings, bullets or severity labels |
| 182 | + inside a comment. Don't restate the code or list test cases. |
| 183 | +- Prefer a question when the author may know something you don't: "We don't |
| 184 | + check X here?", "Can you look at how this differs from Y and whether they |
| 185 | + can be unified?" |
| 186 | +- State what you expected when the code contradicts it: "I thought this |
| 187 | + would fall back to Y." |
| 188 | +- Give an out on genuine judgment calls ("or is this not worth it?"), not on |
| 189 | + bugs. |
| 190 | +- No praise, no verdict, no summary of the change. |
| 191 | + |
| 192 | +**Order.** List inline comments by severity: wrong behavior users can hit |
| 193 | +first, then design and duplication, then tests, then docs and wording. Put |
| 194 | +nits (wording, small naming) together at the end, or in the review body as a |
| 195 | +short list if there are several. |
| 196 | + |
| 197 | +**Before printing:** |
| 198 | +1. Check coverage: every problem row in the worksheet appears either as a |
| 199 | + comment or under "Not raised" in the Evidence section with the reason. |
| 200 | +2. Re-check each comment against the code once more; fix or drop any that |
| 201 | + misread it. |
| 202 | +3. Edit each comment: cut what doesn't help the author act, remove praise |
| 203 | + and verdicts, and drop any comment that answers its own question. |
| 204 | +4. Raise each issue once. When the same problem appears in several places, |
| 205 | + comment on the first and list the other locations in that comment. The |
| 206 | + review body doesn't repeat inline comments; use it for cross-cutting asks |
| 207 | + (missing tests, a design question that spans files). |
| 208 | + |
| 209 | +## 5. Output |
| 210 | + |
| 211 | +Print, do not post: |
| 212 | + |
| 213 | +``` |
| 214 | +## Inline comments |
| 215 | +
|
| 216 | +### path/to/file.go:LINE |
| 217 | +<comment> |
| 218 | +
|
| 219 | +## Review body |
| 220 | +<cross-cutting asks and grouped nits, or nothing> |
| 221 | +
|
| 222 | +## Evidence (not for posting) |
| 223 | +- path/to/file.go:LINE: <what in the code backs each comment> |
| 224 | +
|
| 225 | +Not raised: |
| 226 | +- <worksheet row>: <why: wrong on re-check, already handled, or documented as intended> |
| 227 | +
|
| 228 | +## Worksheet (not for posting) |
| 229 | +<the tables from the passes> |
| 230 | +``` |
| 231 | + |
| 232 | +Use line numbers from `TARGET`. Only post to GitHub if asked. |
0 commit comments