| name | pr-review |
| description | Review a GitHub pull request with empirical verification and clearly-labeled review comments, optionally posting them inline on GitHub or staging them as a pending draft review the user finishes in the GitHub UI. Use when asked to review a PR, post review findings as PR comments, or stage them for approval. Trigger phrases include "review PR", "review this pull request", "post review comments", "stage review comments". |
| argument-hint | <pr-number-or-url> [--post|--stage] |
| version | 0.9.5 |
PR review — verified findings, unambiguous comments
Arguments: $ARGUMENTS — a PR number or URL (if omitted, run gh pr list and ask which one), plus optionally one of --post (publish the review to GitHub after composing it) or --stage (create it on GitHub as a pending draft review — nothing reaches the PR author unless the user submits it from the GitHub UI; Step 4 owns the semantics and mechanics). If both flags are passed, stage — it is the reversible reading of an ambiguous ask.
Step 1 — Gather
gh pr view <n> --json title,body,author,baseRefName,headRefName,state,additions,deletions,changedFiles,mergeable,mergeStateStatus,statusCheckRollup,url,createdAt
gh pr diff <n> for the diff; fetch the branch locally: git fetch origin <base> 'pull/<n>/head:pr-<n>'
- Check whether the PR is behind its base:
git diff origin/<base>...pr-<n> --stat vs git diff origin/<base> pr-<n> --stat (if they differ, note the drift).
- Read the existing review conversation before reviewing —
gh api repos/<owner>/<repo>/pulls/<n>/comments --paginate (inline threads, with in_reply_to_id and author replies) and gh pr view <n> --json reviews,comments. A PR is often mid-conversation: a prior round may have raised your finding already, and the author may have replied/fixed it on a newer commit. For each candidate finding, check it against those threads — if it's already open, reply in that thread (build on it, or note the sub-point still unaddressed) instead of posting a fresh top-level comment. Duplicating a live thread is exactly the noise that makes review a bottleneck.
- Check for the user's own pending review on every round —
gh api repos/<owner>/<repo>/pulls/<n>/reviews --paginate --jq '.[] | select(.state == "PENDING")'. A prior --stage round the user never submitted (or a review they hand-started in the UI) holds draft comments invisible to /pulls/<n>/comments — read them via .../reviews/<id>/comments --paginate — and it blocks any new review create on the PR (staged or posted, the create 422s). If one exists, surface it and get the user's decision before composing, because the drafts' fate forks the round: the call is theirs — they submit it in the UI, or ask you to discard it (the DELETE in Step 4). Submitted → the drafts are now published threads; treat them like any prior round's comments (reply in-thread or stay silent, per the bullets above and below). Discarded → the drafts are gone; re-raise in your own round whatever still holds.
- Verify claimed fixes against the head — "fixed" is a claim, not evidence. When a prior thread is marked resolved or the author replied "done/fixed", read the current code at that spot rather than trusting the reply. Authors routinely fix the headline of a comment while leaving a sub-point untouched (e.g. "memoized the value" addresses re-querying but not the broad
catch the same comment flagged). If the fix is real, leave the thread closed and post nothing — no reply, no "thanks, that's fixed", no resolved-thread acknowledgment. Silence is the confirmation: GitHub already shows the author that their commit addressed the comment, so re-commenting to say so is pure noise, and it is the single most common re-review slop. Only when a flagged sub-point survives do you reply in-thread — and that reply carries only the surviving sub-point and its fix, never a preamble crediting the parts that were resolved. For a re-review of an updated PR, diff against the commit you last reviewed (git diff <last-reviewed-sha>..pr-<n>) to focus on what changed, but still confirm each prior finding is genuinely resolved in the head — and confirm it silently, by not commenting.
Step 2 — Verify, don't just read
Findings must be grounded in evidence, scaled to what the change touches:
- Always read the full files being changed (not just the hunks), and
git grep the whole tree for symbols/properties the PR renames or removes — silent breakage hides outside the diff (e.g. filtered resources referencing a renamed Maven property).
- Build-config changes (pom.xml, Gradle, CI): build both the base branch and the PR branch in throwaway worktrees (
git worktree add --detach /tmp/<name> <ref>), then compare: test counts per module, produced and installed artifacts (watch for attached artifacts like -tests.jar that downstream consumers depend on), mvn dependency:tree diffs, and packaged-archive entry lists plus filtered resources (unzip -Z1, unzip -p ... | diff). Remove the worktrees afterwards (git worktree remove --force).
- Parent-POM or dependency upgrades: download the new parent/dependency POM and confirm that everything deleted locally is actually provided upstream (dependencies vs dependencyManagement, plugin executions, properties, distributionManagement). Check published artifacts on the repository (e.g.
curl -sL .../maven-metadata.xml) when removal of an attached artifact would break consumers.
- Claims about runtime behavior: run the relevant tests instead of asserting from code.
- Absence claims ("nothing registers X", "the framework never does Y") must be positively verified against the dependency's sources — a local checkout of the dependency (check sibling directories of the repo) or its upstream repo at the ref the project actually builds against — never inferred from jar scans or config-file greps alone: annotation-registered/component-scanned beans are invisible to XML inspection, and installed SNAPSHOT artifacts can lag or lead the sources.
- A zero-hit search is evidence only after a positive control: before trusting "no matches", prove the same pipeline finds a string you know is present (e.g.
java/lang/Object in a class file). Shell grep may be shadowed by an alias/function (ugrep shims silently skip binary-ish input, e.g. strings output of class files) — for evidence-grade searches use command grep, git grep, or the dedicated search tools, and never add 2>/dev/null to an evidence-gathering command.
- When the one experiment that could falsify a finding can't run (build stopped, environment unavailable), the finding ships as a
question:, not a fact — and a "confirmation" that structurally cannot fail (a standalone repro that omits the very component in question) is not verification.
Dimensions to sweep
Cover each of these, scaled to what the PR touches — an unnamed dimension is one you'll silently skip: correctness (verified, not eyeballed), test coverage (section below), security (input validation, authorization checks, secrets in diffs or CI config, injection, unsafe deserialization), performance (N+1 queries, work added to hot paths or loops, blocking calls, unbounded growth), and project conventions (the change should read like the surrounding code — but don't flag what CI formatters already enforce).
Trace outward — the bugs live outside the diff
The diff is a piece of a bigger machine; review the machine, not the patch. Follow each thread at least one level out from the changed lines:
- Unchanged neighbors. Re-read the unchanged code, comments, Javadoc, and docs adjacent to or depending on the change — anything stating an assumption ("the only caller", "X is the only path that…"). Ask of each: does this PR make that statement false? Diff-scoped review structurally cannot catch these, because the now-stale line never appears in the diff.
- Consumers and published artifacts. For everything the changed code produces — events fired, attached/published artifacts (e.g.
-tests.jar classifiers), API responses, filtered resources — identify who consumes it, in-repo and downstream, and verify the contract still holds.
- Optional dependencies and lifecycle. For
provided-scope deps, soft module references (aware_of_module), and registration-order-sensitive code (Spring beans, listeners, schedulers), walk through what happens when the dependency is absent or the ordering shifts at runtime.
- The merged result, not the commit stack. Review the final combined state of the PR — and re-check it against the current base tip if the branch is behind — rather than commit-by-commit. Individually sensible commits can compose into a contradiction.
Test coverage is a review dimension
For each behavior change the PR introduces, name the test that exercises it — one that would fail on the base branch, passes on the PR, and asserts the runtime effect, not a proxy ("the generated string looks right" or "mirrors an existing pattern" verifies the shape or the sibling, not the behavior). A behavior change with no such test is a finding, even when the code looks obviously correct. If part of the path genuinely can't be tested, name the specific blocked sub-path — and confirm the adjacent runnable path is still exercised.
Convergence
A pass that surfaced a substantive finding cannot be your last pass: one real find is evidence of unexplored ground, so sweep again until a pass turns up nothing substantive. The converse also holds — don't invent findings to fill a quota; a clean PR earns a short review.
Verify thoroughly — but the evidence belongs inside the finding it supports, not in a standalone recap, and a cleared concern usually needs no words at all (see Step 3).
Step 3 — Compose the review
The review's job is to surface what needs action. Reviewer and author attention is the scarce resource — PR review is a bottleneck, and every line that doesn't change what the author does is pure cost. Lead with action.
The body is earned, not mandatory, and the default is none. It exists only to carry what no single inline line can: a cross-cutting concern with no line to sit on, or a direct answer to a question the author asked on the PR. Summarizing or triaging the inline comments is not such a thing. Each inline comment already states its own blocking-ness in its first line (the inline rules below require it), so a body that partitions them ("three block, three are optional") only re-derives what the comments already say. That is not a neutral convenience: it adds reading with no new information, which for an author already fatigued by comments is a net cost, and the "verdict + theme + what blocks" shape reads as bot output. A blocking/non-blocking tally therefore never earns a body. The only cross-finding note that does is genuinely synthetic: a dependency the per-comment text can't state ("land #2 before #1 or the fix won't apply"), or that several comments share one root cause so only that root need be fixed. The aggregate verdict is not by itself one of those things. A single blocking inline comment already conveys that the PR is not ready through its own wording, so a body that merely restates it or points back to it ("this isn't ready to merge; see the comment on X") is pure redundancy, and the author reads that announce-the-obvious sentence as bot output. A comment that says the change doesn't do what the ticket asks, or merges code nothing reaches, needs no cover note to declare that it blocks. So when one inline comment already carries the verdict, post the inline comments with an empty body (Step 4 shows how). Reach for a body only when the disposition genuinely is not derivable from the inline comments on their own. A multi-paragraph cover note that re-tells each finding is the loud failure mode; a one-line body re-announcing a verdict the inline comment already implies is the quiet one. Resist both, and keep the "if it would just be LGTM, post nothing" rule from Step 4.
When a body is warranted, its shape is:
- Verdict first — disposition + one headline reason + scope, nothing else. Disposition is "merge", "merge after X", or "needs work"; the headline is the single biggest reason (point at the finding numbers); scope is the bar you're judging against (e.g. "as a POC none block it; as production code they do"). "Verdict first" means open with the call in your own words — not print a
Disposition: / Verdict: / Summary: label in front of it. A literal label header is the same bot fingerprint as a stamped taxonomy tag (see the inline rules below): write "I think this is good to merge — nothing here blocks" or "This needs work first — …", never "Disposition: merge — …". Don't tour the architecture or grade it ("the design is coherent", "the happy path is plausible") — that's narration-plus-praise the author can't act on. A maintainer scanning the PR should get the call and the one reason in a sentence or two.
- Findings — but each lives in exactly one place. When you post inline (
--post or --stage), the inline comment is the finding and carries its own evidence (its failure-mode sentence, or the verification that grounds it); the body must not restate it. Point to it with a bare reference — "the inline question on the JacksonConfig change" — never a one-line recap of what it says ("…that the shared mapper also reaches the scheduler"): the moment the clause explains the finding, it's a restatement in disguise. A body that re-narrates its inline comments is the same findings said twice — pure duplication, and the most common way the summary bloats. Only a finding with no inline home — a cross-cutting or PR-level concern — gets stated in the body itself.
- End by making explicit what blocks merge — a short list when it's more than one thing, a single sentence when several findings share one blocker, and nothing when nothing blocks. (If the only body-worthy content would be a statement of what blocks — whether one blocker or several — there is no body at all: each inline comment already states its own block, per the default above.) Don't reach for a fixed boilerplate header: a verbatim string like "Needs action before merge:" stamped on every review is itself a bot fingerprint. Say what's left to do the way you'd say it to a colleague.
A "ready to merge" verdict with nothing left to act on is not something you post — see Step 4. It goes to the person who asked for the review, in conversation, not as a comment on the PR.
Evidence lives inside the finding it supports, not in a standalone "what I verified" section — a real finding is self-justifying once the reader checks it against the code, so a separate verification recap is redundant overhead. Checked it and it's fine → say nothing. Do not note cleared concerns "so they aren't re-raised": if the thing is trivially checkable, a later reviewer checks it as fast as you did and your note saves no round; if it isn't, a one-line reply when/if someone actually raises it costs less than pre-empting it on every review.
There is no exception — and on this the body is absolute: verified-clean material never appears in it, not as a section, a bullet, a terse line, or folded into prose. Even when cross-cutting verification was the whole point of the review (e.g. a pom.xml PR's "are the artifacts byte-identical?"), the result is reflected only in the verdict's disposition and the confidence behind it — never written out as a verified-clean note. If such a result genuinely must be on the record, it goes in a single inline note (no action needed): anchored to the relevant line, never in the body. On a re-review this bans the resolved-finding recap specifically: do not open with "all N points from last round are addressed" or otherwise tally what got fixed. When every prior finding is resolved and nothing new surfaced, there is nothing to post at all: tell the user it's ready to merge in the conversation (Step 4), rather than posting a review body that just announces "merge" — a bot-style "this is ready to merge / LGTM" comment is exactly what makes the author feel they're dealing with a machine instead of a person.
Rules for every inline comment, no exceptions (this is the part PR authors complained about when missing):
- Every inline comment must either request an action or ask a question, and its first line must make clear what kind of comment it is and what (if anything) the author has to do. Carry that the way a human reviewer would — in plain language ("This needs fixing before merge — …", "Optional: …", "Is this intended?") — rather than stamping every comment with a taxonomy tag. A column of
issue (blocking): / suggestion (non-blocking): / nit: prefixes down the page is itself a bot fingerprint — and so are the em-dash and colon variants (Question — …, Non-blocking suggestion: …, Minor: …): a label is a label whatever punctuation trails it, and "the first word names the category" is the tell. Open with the thing itself instead — the actual question ("Was this side effect intended?") or the actual ask ("Worth logging this rather than dropping it silently — …"). Casual vocabulary doesn't launder a label: Naming trivia, take or leave: … and Tiny style thing: … are the same stamp in softer clothes. The test for any opening phrase + colon is whether the phrase diagnoses the code or classifies the comment — Leftover from the removed handler: X and Y are no longer used survives because deleting the phrase loses the why; Naming trivia: deletes clean, so delete it and state the thing itself, letting any take-or-leave disposition trail the substance rather than front it. A pet opener recurring across comments or review rounds ("take or leave" in two consecutive rounds) turns even mild phrasing into a template. Reach for an explicit label only when the prose doesn't already make the blocking-ness obvious, or when the maintainers have asked for those tags. Whichever form you use, these are the distinctions that must come through:
- blocking — must be fixed before merge
- suggestion (non-blocking) — recommended change the author may decline
- question — a genuine question whose answer affects the verdict. Also use this for uncertainty about an observed side effect ("this changes X — intended?"); a question invites confirmation from people with more context far better than an FYI does.
- nit — trivial, take or leave
- note (no action needed) — exceptional. Allowed only when it does one of these jobs:
- records that a behavior change was seen and deliberately accepted, so the audit trail shows it wasn't missed if something breaks later; or
- warns a specific future reader at the line they will be looking at (e.g. whoever cuts the next release).
If it does neither and isn't really a question in disguise, it's narration — don't post it anywhere.
- A blocking comment must contain a concrete failure-mode sentence: "if merged as-is, X breaks because Y." If you cannot write that sentence, it is not blocking — downgrade it or drop it. "Low risk" without naming the risk, "matches the existing pattern", "probably fine" are dodges, not verdicts: name what breaks, who notices, and how.
- Silent-failure upgrade: when the failure mode produces wrong behavior without throwing (a swapped repo id, a dropped field, a silently replaced MockMaker), treat it one level more seriously than a crash — CI cannot catch what doesn't throw, so the cost of discovery lands on users.
- When presenting alternatives, always state the recommended default. Never end on "either option is defensible" without picking one.
- Do not narrate what the diff does — the author wrote it.
- Keep each inline comment to its finding and the fix — don't repeat the same cross-cutting point across comments. Verified-clean cross-cutting verification does NOT get relocated to the body (see Step 3 — the body carries none); at most it is a single inline
note (no action needed):.
- Use GitHub ```suggestion blocks when the fix is a concrete replacement of added lines (RIGHT side only — suggestions cannot attach to deleted lines).
- Scale to the requested effort: by default, fewer high-confidence findings. At high effort, widen the sweep and surface uncertain findings too — explicitly as
question:, never as unverified issues.
- Fewer, sharper comments beat exhaustive ones.
Step 4 — Post or stage (only with consent)
Posting publishes under the user's own GitHub account. Post only when the user passed --post or explicitly asked; stage only when they passed --stage or asked for a draft or to stage; with neither, present the full review in the conversation and offer to post (or stage). --stage sits between conversation-only and --post: the review is created on GitHub in PENDING state — invisible to the author, no notification fires — and the user finishes it in the UI: edits wording, deletes comments, then submits or discards. Staging is a checkpoint, not a lower bar — the user may submit the staged comments unedited, so a staged review must already pass every rule in this file, including the gate below.
A clean "ready to merge" verdict is reported to the user here, never posted to the PR. When the review turns up nothing to act on — fresh PR or re-review where every prior finding is resolved — there is no review to publish or stage: tell the user it's ready to merge in the conversation and stop. Do not open a review (even with --post or --stage) whose body just announces the PR is good to go. Posting "this is ready to merge / LGTM / all looks good now" is the clearest tell that a bot, not a person, is on the other end, and it's the thing PR authors most dislike. Posting still happens normally when there ARE action items — you post those findings; you just never add a standalone merge-readiness flourish.
Pre-posting gate — before presenting, staging, or posting, write these four as four explicit, standalone lines in your report to the user — verbatim, not paraphrased, not woven into prose narration. If any cannot be said truthfully, go back and fix the review first:
"Every finding was verified by [building / running / tracing / upstream check], not just read off the diff."
"Neither the summary verdict nor any inline comment's first line leads with a stamped label — not issue: / suggestion: / nit:, not the em-dash/colon variants (Disposition: merge, Question — …, Non-blocking suggestion: …), and not their casually-worded cousins (Naming trivia, take or leave: …, Tiny style thing: …); each says in plain language what it is and what the author must do, and either requests an action or asks a question. Every blocking comment contains its failure-mode sentence; and no comment — fresh or threaded reply — exists merely to acknowledge a fix or credit resolved work."
"The summary body carries only what no inline line can, and when a single inline comment already conveys the verdict it is empty. I did not add a body to announce a disposition the inline comments already imply: a lone blocking comment whose wording shows the PR is not ready needs no cover note. A body appears only for an unanchored cross-cutting concern, the author's question, or a genuinely synthetic cross-finding note (a fix-ordering dependency, or a shared root cause); it never merely summarizes or tallies the blocking-ness that each inline comment already states, and even when present it never restates a finding (a pointer is a bare reference, not a one-line recap of what the comment says; a body paragraph reproducing an inline comment is the finding said twice). Zero verified-clean or cleared-concern content anywhere in it: no 'what I checked' line, bullet, section, or aside, and none folded into prose; and on a re-review, no 'prior points addressed' recap, since a resolved finding earns silence, not a tally. The verification I did shows up only as the confidence behind the verdict, never written out; if a result must be on the record it is a single inline note, not body text."
"Needs action before merge: [list, or 'none' — and if 'none', this verdict is reported to the user here, not posted to the PR nor staged]."
Mechanics that are easy to get wrong:
- Submit one review:
gh api repos/<owner>/<repo>/pulls/<n>/reviews --input payload.json with "event": "COMMENT" (posted rounds only — a staged round omits event, two bullets down; never APPROVE or REQUEST_CHANGES unless the user explicitly chose that), a body, and a comments array.
- Before any review create, posted or staged: re-run the Step 1 pending-review check. A user gets one pending review per PR — either kind of create 422s "User can only have one pending review per pull request" against it, and REST cannot append to an existing one. On a hit (the pre-check, or that 422 — one can appear between Step 1 and now), stop and ask rather than touching it.
- Staging: same payload but omit
event entirely — GitHub creates the review in PENDING state ("you will need to submit the pull request review when you are ready").
- Staged thread replies: the
/replies endpoint below publishes immediately and cannot stage. Attach the reply to the pending review via GraphQL instead: addPullRequestReviewThreadReply with pullRequestReviewId (the pending review's node_id, returned by the create call), pullRequestReviewThreadId, and body. Find the thread id in the PR's reviewThreads connection by matching the target comment's REST id against fullDatabaseId (github.com's schema no longer exposes databaseId on review comments — older GHES schemas are the reverse, so if fullDatabaseId errors as an unknown field, match databaseId instead; fullDatabaseId comes back as a string, so a numeric jq comparison silently matches nothing). For a replies-only round, create the pending review first with no comments so there is something to attach to — an empty {} payload suffices.
- Verifying a staged review: pending comments do not appear in
/pulls/<n>/comments — list them with gh api repos/<owner>/<repo>/pulls/<n>/reviews/<review_id>/comments --paginate instead. The expected set is everything the composed review contains — the payload's comments plus every thread reply it called for (attached via GraphQL); match replies by body and in_reply_to_id — they are not duplicates to clean up. Compare by {path, body}, not {path, line}: until the review is submitted, pending comments report line and side as null (only the diff-relative position is populated), so a line-based diff false-mismatches on every staged comment. If something looks missing, re-query after a few seconds before repairing — reads lag writes here too. If the staged set is genuinely wrong, repair per the fixing bullet below (a missing reply attaches in place; anything else is delete-and-restage) — never per-comment DELETE, which would silently eat your own drafts.
- Hand-off for a staged review: end the report with the review URL and say plainly that it is visible only to the user until they submit it (Files changed → "Finish your review"), and that a forgotten pending review is a review that never happened. Discard from the CLI is
gh api -X DELETE repos/<owner>/<repo>/pulls/<n>/reviews/<review_id> (pending reviews only). Which event to submit with — comment, approve, request changes — is the user's call to make in the UI, not something to instruct.
- Build
payload.json with python3 + json.dump — raw tabs/newlines in hand-written JSON break the API call.
- Anchoring:
"side": "LEFT" with line numbers from the merge-base version of the file for deleted lines; "side": "RIGHT" with line numbers from the PR-head version for added/context lines. Get exact numbers via git show <ref>:<path> | cat -n. Confirm the merge-base first (git merge-base origin/<base> pr-<n>); if it equals the base tip, base-branch line numbers are safe for LEFT.
- Multi-line comments use
start_line/start_side plus line/side, and the whole span must fall inside a single diff hunk.
- Comments appear under the user's name — write them in first person, collegial tone.
- Threaded replies (Step 1 may send you here to build on an existing thread instead of posting fresh):
gh api repos/<owner>/<repo>/pulls/<n>/comments/<comment_id>/replies -f body=.... These post as standalone review comments — they won't appear in any review payload's comments array and carry their own ids, so don't expect them in the main-review verification below. With --stage this endpoint is wrong — it publishes instantly; use the staged-thread-replies path above.
- Verify after posting — but trust content, not counts:
gh api repos/<owner>/<repo>/pulls/<n>/comments --paginate --jq '.[] | select(.pull_request_review_id == <id>) | {path, line, side, body}'. The --paginate is mandatory: gh api returns 30 comments per page, and a busy PR (a prior round + replies) easily exceeds that, so without it the query gives false negatives. Reads can also lag a few seconds behind the write. Diff what comes back against what you submitted by {path, line}; if something looks missing, re-query (allowing for lag) and inspect — never re-post on a mismatch, that is exactly how duplicate comments get created. If duplicates already exist, delete the extras with gh api -X DELETE repos/<owner>/<repo>/pulls/comments/<comment_id>. Then report the review URL.
- Fixing a review after posting: edit in place, never post a second review. Update the body with
gh api -X PUT repos/<owner>/<repo>/pulls/<n>/reviews/<id> --input body.json (a {"body": "..."} file); {"body": ""} is accepted and blanks the body outright, which is the right move when you realize the body only re-announced what an inline comment already said. Edit inline comment wording with gh api -X PATCH repos/<owner>/<repo>/pulls/comments/<comment_id> -f body=.... Editing leaves no duplicate-review or duplicate-comment churn. A staged review needs none of this — the user edits pending comments directly in the UI; if the staged set itself is broken, the repair depends on the break: a missing or newly requested thread reply attaches in place via the staged-thread-replies path (the pending review's node_id is in the reviews listing), no restage needed; anything else (bad anchors, a missing top-level comment) is delete-and-restage, and the rebuild source depends on when. Before hand-off no user edit can exist: rebuild from your composition with the defect fixed. After hand-off, list the current drafts first (.../reviews/<review_id>/comments --paginate) — on wording and deletions the drafts win (restaging your original would silently revert the user's UI edits, and your reverted text then publishes when they submit trusting changes no longer there), while the piece the repair exists to restore (the missing comment or reply, the corrected anchor) comes from your composition, the only place it exists. Deletion evaporates the GraphQL-attached replies along with everything else, so restage the replies too, then re-verify against the corrected set you just rebuilt — never against a rebuild that inherited the loss, which would certify the very gap you came to repair.
Anti-patterns
- Don't invent concerns to look thorough. Review depth should track risk; a clean PR gets a short, confident review. Diminishing returns are a real signal, not a failure.
- Don't post unverified suspicion as an
issue. If you couldn't verify it, it's a question: — or more homework for you, not the author.
- Don't critique an inflated version of the PR. When disagreeing with an approach, respond to the author's actual scope; larger redesigns are follow-up
suggestions, not blockers on this PR.
- Don't let severity labels substitute for analysis. A finding is blocking because of its written failure mode, not because it pattern-matches "correctness". Small-looking code patterns (an unclosed resource, a swapped identifier) are routinely under-labeled.
- Don't restore confidence the author already has. A PR author shipped because they believe the change is right; a recap of what they got right is cost without benefit. The review's value is the delta — what must change, plus the rare verified-clean item a later reviewer would re-raise. Passing CI is not a reason to soften the action items: both the expensive bugs and the bottleneck live in what the tests already wave through.
- Don't acknowledge fixes (especially on re-review). A human re-reviewer comments only on what's still wrong — they don't reply "thanks, that's resolved" to every thread the author closed, and they don't open the body tallying what got addressed. Fixed and no remaining problem → no comment; the disposition flip to "merge" is the acknowledgment. Writing the confirmation out feels polite, but it reads as slop and buries the one comment that does still need action. This is the gap authors notice most: the re-review that should have been two lines turns into a wall of "looks good now."
- Don't let the review read as machine output. The content rules above are about substance; the form matters too, because authors disengage the moment a review smells automated. Three habits give it away even when every finding is sound: a rigid recurring template (a
Disposition: / Verdict: header → numbered findings → a verbatim "Needs action before merge:" footer on every PR); a taxonomy tag stamped on the front of every comment (including its em-dash/colon variants like Question — … or Non-blocking suggestion: …, and their casual cousins like Naming trivia: …); and a comment that opens by announcing its own speech act (A correction to my earlier rationale: …, One observation: …) instead of leading with the substance — when retracting something you claimed in an earlier round, open with the corrected fact and let the retraction trail it ("It turns out X, so part of my reasoning above was wrong"), never with the announcement. Write the way a careful human colleague writes — vary the structure to fit the PR, let prose carry blocking-ness, and never post a standalone "ready to merge / LGTM" comment; that verdict goes to the person who asked, in conversation. A subtler tell lives in the prose itself: a heavy run of em-dashes (the — character now reads as a strong "written by a model" signal) and uniformly polished, balanced sentences. In the posted review, prefer commas, periods, and parentheses over em-dashes, and let the phrasing be a touch plainer than a machine would buff it. The goal is a review the author reads as coming from a person who actually looked, not a form a tool filled in.
- Don't fake confidence on a PR too large to review. When a PR is too big or sprawling to review with real confidence as one unit, say so plainly and suggest splitting it (by concern, or behavior-change vs. mechanical churn) rather than producing a sweep that reads as thorough but can't be. A reviewer's honest "this exceeds what I can verify in one pass — here's how I'd split it" is worth more than five findings that miss the sixth. Name what you did verify and what you couldn't reach.