review

name	review
description	Reviews a pull request for code quality and correctness. Use when asked to review a PR or when running as an automated PR reviewer.
argument-hint	[PR number]
metadata	{"internal":true}

PR Review

Review a pull request.

PR to review: $ARGUMENTS

Workflow

Follow these steps in order.

0. Load environment skills

Load /tend-ci-runner:running-in-ci first — it contains CI security rules, polling conventions, and comment formatting guidance. It will also prompt you to load any repo-specific skills (e.g., running-tend).

1. Pre-flight checks

Before reading the diff, run the initial snapshot. It pins the open head, resolves the bot's review state, and prepares the accurate incremental when one applies:

/usr/bin/python3 -E -s \
  "${CLAUDE_PLUGIN_ROOT}/scripts/review_preflight.py" start <number>

On skip, finish. The JSON fields replace the shell variables named below; incremental_path is either null or a file containing the commits and per-file line counts authored since the last review, excluding base-branch churn.

When force_full_review is true, bypass both the already-reviewed and trivial- increment shortcuts: becoming ready asks for a full non-draft review.

If force_pushed_since is true, the commit the bot reviewed was rewritten away: ignore last_review_sha entirely and review head_sha in full. The incremental can't run either — last_review_sha now names the current head rather than anything the bot read, so the range is empty and every trivial-skip heuristic keyed on it under-reports. A prior APPROVED is re-anchored onto the rewritten head too, so it reads as an approval of code nothing reviewed — step 6's dismissal rule clears it, along with the ordinary-push case below.

Otherwise, if last_review_sha == head_sha and force_full_review is false, this commit has already been reviewed — finish without posting. An unanswered conversation question directed at the bot (check below) is the exception: proceed so the review can answer it.

If the bot reviewed a previous commit (last_review_sha exists but differs from head_sha), judge what was pushed since. Read two signals, both leak-free against base-merges:

• The PR's three-dot diff — gh pr diff <number> (merge-base→head, the same diff step 3 uses) — for the current state. Base-merge commits never enter it.
• The accurate incremental in incremental_path — read the whole file for the trivial-skip decision below. The script excludes everything reachable from the base tip, so a base merge's own commits are not counted as new PR work.

The incremental scopes the review, not anything this run writes about the PR as a whole: if you also edit the PR description, scope its claims to the merge base per Keeping PR Titles and Descriptions Current in /tend-ci-runner:running-in-ci.

If force_full_review is false and the incremental changes are trivial, skip the full review — go directly to step 8 to resolve any bot threads addressed by the new changes. After resolving threads: if the most recent bot review was a COMMENT that flagged issues, and those issues are now addressed, submit an APPROVE with an empty body so the PR isn't left in limbo — and the author-readiness gate under step 6 applies here too, since these are the bot's own findings closing out rather than the author's. Use the recipe under step 6, which pins the commit read here. Otherwise do not submit a new review — the existing one stands. Do NOT proceed to steps 2–7; finish. Rough heuristic: changes under ~20 added+deleted lines that don't introduce new functions, types, or control flow are typically trivial.

Commit and PR authorship do not affect review behavior. Apply the same trivial-vs-substantive heuristic regardless of who pushed the new commits. When tend-notifications or tend-ci-fix pushes a fix to a human-authored PR, reviewing (and re-approving) the updated state is expected — the reviewer role is independent of commit authorship.

Then read all previous bot feedback and conversation:

uv run --script "${CLAUDE_PLUGIN_ROOT}/scripts/bot_review_state.py" \
  feedback <number>

Apply the sibling-workflow dedup rule from running-in-ci to both the review body and inline comments. If a prior bot comment in the conversation already covers a point — a previous review on this or an earlier commit, a tend-mention reply, a tend-triage post, anything from a tend workflow — omit it from this review and stick to diff-grounded findings. If that leaves no new diff-grounded finding on the incremental changes and the only outstanding concern is a still-unresolved thread from an earlier bot review, do not post a new review: that thread already blocks the PR, and restating "the prior thread still applies" on every push is noise. Resolve any bot threads the new commits addressed (step 8), then finish without posting. A fresh review is warranted only when the incremental diff introduces a new finding, or resolves the last open one (then approve with an empty body — the author-readiness gate under step 6 applies here too, since these are the bot's own findings closing out rather than the author's). When concurrent runs race (a new push while the first run is still responding), both see the same unanswered question — check whether a bot reply exists after the question's timestamp before answering. Address remaining unanswered questions in the review body (not via gh pr comment).

Draft mode

If is_draft is true, run a lighter review:

• Skip step 2 (overlap with other PRs) — landing-readiness concern, premature for WIP.
• Skip the duplication scan in step 4 — the author is still shaping the design.
• Submit as COMMENT only, never APPROVE. GitHub blocks approving drafts, and the author hasn't asked for a verdict yet.
• Make the review's context clear: this is feedback on work in progress, not a merge verdict, and the author can mark it ready to request the full review.
• Include the exact hidden marker <!-- tend:draft-review --> anywhere in the review body. Posting mechanics uses it to replace this COMMENT with a full verdict when the PR becomes ready; it is not part of the reader-facing prose. Carry it through any body you recompose — re-targeting after a mid-review push and the 422 body-only retry both rewrite the body, and dropping the marker there forfeits the replacement silently.
• Skip step 7 (CI monitoring) — drafts churn; CI failures are the author's to chase.
• Skip step 9 (push fixes) — never push to a WIP branch.

Steps 1, 3, 4 (without duplication scan), 5, 6 (COMMENT path), and 8 still apply. Stay silent if there's nothing actionable; don't post a "looks fine" comment.

2. Check for overlapping PRs

Before reading the diff, scan other open PRs for file overlap. If another PR touches the same files with a similar fix, flag it in the review so one can be closed as a duplicate.

3. Read and understand the change

1. Read the PR diff with gh pr diff <number>.
2. Before going deeper, look at the PR as a reader would — not just the code, but the shape: what files are being added/changed, and does anything look off?
3. Read the changed files in full (not just the diff) to understand context.

4. Review

Scale depth to the change. A docs-only PR or a mechanical rename needs a skim for correctness, not the full checklist. A new algorithm or state-management change needs trace analysis. Don't over-analyze trivial changes.

Check the project's instruction files for language-specific review criteria and conventions. Load any project-specific review skill if available.

Code quality:

• Is the code clear and well-structured?
• Are there simpler ways to express the same logic?
• Does it avoid unnecessary complexity, feature flags, or compatibility layers?
• In the bot's own automation (its skills, and the scheduled workflows that invoke it) and the repo's CI config: is the change worth its weight? Challenge new mechanism whose only payoff is saved compute — retries, skip-gates, caches, scheduling arithmetic — per Weighing a Fix in running-in-ci. Judge the whole change: a PR well-argued line by line can still cost more machinery than the compute it saves, and the review should say so plainly. Recommend the simple knob, or closing the PR, over refinement.

Correctness:

• Are there edge cases that aren't handled?
• Could the changes break existing functionality?
• Are error messages helpful and consistent with the project style?
• Trace failure paths, don't just note error handling exists. For code that modifies state through multiple fallible steps, walk through what happens when each error fires. What has already been mutated? Is the system left in a recoverable state?

Testing:

• Are the changes adequately tested?
• Where a doc comment and the code disagree, the finding is which one is wrong — don't suggest a test that pins the current output, which freezes behavior nobody has called intended.

Same pattern elsewhere:

When a PR fixes a bug or changes a pattern, search for the same pattern in other files. If found in the diff, add inline suggestions; if found outside the diff, offer to push a fix commit.

Citing code outside the diff:

The checkout uses refs/pull/N/merge, so you read the merged tree (PR head + current base branch). When a finding involves code outside gh pr diff — code the PR didn't touch but now interacts with — identify the interacting code by semantic anchor (selector, function name, declaration text) and quote enough of the relevant property/value that the reader can grep for it. Don't lead with the line number: the author's local branch may be behind main, so line numbers from the merged tree won't match their checkout, and a line-number-first citation reads as a hallucination when they check the line on their stale branch. Same principle as the blob/main/...#L42 link rule in running-in-ci — line numbers are fragile across diverging refs.

Duplication check (for new functions/types):

For every new public or module-level function added in the diff, search the codebase for existing functions that do the same thing. LLM-generated code frequently reinvents internal APIs — this is the highest-value check for externally contributed PRs.

Two search strategies, both required:

1. Similar names and signatures. Search for functions with similar names, return types, or parameter types.
2. Overlapping subgoals. Identify the intermediate steps the new code performs and search for existing code that does the same sub-tasks.

Flag duplicates — reuse is almost always better than a parallel implementation.

5. Second pass

Run a /tend-ci-runner:code-review pass over the PR's merged tree. Every review that reaches this step runs one — trivial diffs included; step 4's depth-scaling sets how deep the pass goes, never whether it happens. It's a structured second pass — correctness and cleanup angles, then a verify pass — that returns findings rather than posting anything, and it supplements step 4's manual checks rather than replacing them.

Scale its depth to how core the change is:

• Peripheral or mechanical (config, dependency bumps, test-only, docs that don't assert how the code behaves): tell it the change is peripheral, so it runs the short angle set in one pass.
• The project's core logic, or prose asserting how it behaves: tell it the change is core, so it fans the angles out and sweeps for gaps. Prose is checked by reading the code it describes, so a one-line Markdown diff can still be core.

What counts as core is repo-specific; let the project's own instruction files, a repo review skill, or your judgment decide. Both passes feed one verdict: fold its findings into the review you submit in step 6. It only reports back — it never posts a review, comment, or commit of its own, so the dedup and single-review path is preserved. Its findings are not the review: when it returns, continue to step 6.

6. Submit

For a review that reached step 5, before submitting, say what that pass returned — its confirmed findings, or "no findings". That statement is a compliance check: say it in the session, not the review body, so an empty-body APPROVE stays empty. The findings themselves still get folded into the review, per step 5. If you can't say, the pass didn't run — go back to step 5 and run it. A full review that reaches this point without it is not submittable. Step 1's trivial-increment and dedup close-out paths deliberately skip steps 2–7 and are exempt.

If there are no issues, approve with an empty body — silence means correct.

Unless the author withheld merge readiness. When the PR body — or a later comment from the author or a maintainer — says the change should not merge yet — "should not merge until…", "not ready", design questions the author calls unresolved — the verdict is withheld the same way the draft flag withholds it, and plenty of contributors state it in prose rather than toggling draft. Submit COMMENT instead, naming the stated blocker that holds the verdict; name it once, and on a later pass that finds nothing new stay silent rather than restating it — the surrounding dedup rules are keyed on threads, so they don't reach a body-only COMMENT. Your own findings being closed out does not clear it: "everything the reviewer raised is fixed" and "the author says this must not merge" are independent conditions, and only whoever stated the blocker retracts it. The asymmetry is why this is worth a condition: withholding a warranted approval costs a re-review on the next push, while an APPROVE standing on a PR its author gated is a wrong outward signal that persists until someone notices.

REPO=$(gh repo view --json nameWithOwner --jq '.nameWithOwner')
# Read the sha first and bail if it isn't there: inlined as `$(cat ...)` a
# missing file substitutes the empty string and the POST still runs, which is
# the unpinned review this pins against.
REVIEWED=$(cat "$TMPDIR/reviewed-head") || exit 0
/usr/bin/python3 -E -s "${CLAUDE_PLUGIN_ROOT}/scripts/review_preflight.py" post <number> -- \
  gh api "repos/$REPO/pulls/<number>/reviews" --method POST \
    -f event=APPROVE -f commit_id="$REVIEWED" -f body=""

$TMPDIR/reviewed-head holds the commit this session reviewed — written in step 1, rewritten by Posting mechanics if HEAD moved. Every path that posts a review reads it back; see Pin every review to the commit you read.

If there are actionable findings, submit them as a review with inline suggestions for concrete fixes. The review is a decision surface for the author, not a record of the reviewer's work: publish only distinct points that require a change or decision, with enough mechanism and evidence to make each credible and actionable. Correct paths, unaffected behavior, verification inventory, and search history stay in the session. Follow Reader-facing prose in running-in-ci for any supporting detail.

Don't explain what the code does — the author wrote it. Don't nitpick formatting — that's what linters are for. Explain why the consequence warrants a change.

Bad:

The new delegation path looks correct. I also verified that the threshold logic is unchanged.

Good:

The failure message still names the removed local path, so it directs users to an option that no longer exists. Update it to name the delegated command.

A findings review never supersedes a standing approval — dismiss it. GitHub moves reviewDecision only on an APPROVED or a CHANGES_REQUESTED, so a COMMENT posted over the bot's own earlier approval leaves the PR reading as bot-approved and mergeable over the findings you just posted. How the head moved makes no difference: an ordinary push leaves the approval standing exactly as a rewrite does. So whenever this round posts a COMMENT that withholds the verdict, dismiss the approval that still decides the PR — after the review POST lands, so a failed post doesn't leave the PR with neither a verdict nor findings. Findings are the common case, but the withheld-merge-readiness COMMENT above is reachable with an approval already standing, and it leaves the same wrong signal: the PR reads bot-approved while the review names a blocker. A COMMENT that withholds nothing does not qualify — step 1's unanswered-question exception posts one at a head the approval already covers, and dismissing there withdraws a verdict the code still earns.

# Re-read rather than reusing step 1's blob — a whole review has passed since.
# The script no-ops once a dismissal or later CHANGES_REQUESTED has cleared it.
uv run --script "${CLAUDE_PLUGIN_ROOT}/scripts/bot_review_state.py" \
  dismiss <number> "Superseded by the review on a later commit."

Form your own opinion independently. Do not factor in other reviewers' comments or approvals when deciding whether to approve — the value of this review is as an uncorrelated signal.

When confidence is low, go beyond checking the implementation — question the approach: "Does this bypass or duplicate an existing API?" "What does this change not handle?" If the design involves a judgment call, flag it for human review as a COMMENT.

Attribute a withheld approval to whatever actually decided it. Cite repo guidance as the reason only when you can name the file and heading that guidance lives in. When the call is your own judgment, identify the risky consequence and the human decision it needs; judgment is sufficient authority without inventing a repository policy.

Self-authored PRs (author == bot_login in step 1's JSON — compare the literal bot login string, not "authored by someone senior" or "by the repo owner"): Complete steps 2–5 — self-review catches real issues (lint failures, edge cases) and is intentionally valuable. Do NOT attempt an APPROVE — GitHub rejects self-approvals. That covers step 1's close-out approvals too: on a self-authored PR the threads are the only thing to close out. Submit as COMMENT when there are concerns, or stay silent and skip to step 7. The self-review exists to find concerns, not to publish a clean-path verdict or proof that earlier findings were resolved. Always post a current CI failure as a COMMENT because it is itself a concern.

Not confident enough to approve (unfamiliar module, subtle logic): Add a +1 reaction instead — no review needed unless there are specific observations.

REPO=$(gh repo view --json nameWithOwner --jq '.nameWithOwner')
gh api "repos/$REPO/issues/<number>/reactions" -f content="+1"

Posting mechanics

Before composing the final payload, run the preflight without a command. It checks the PR is open, re-targets onto a newer descendant head, and stops duplicate reviews:

/usr/bin/python3 -E -s \
  "${CLAUDE_PLUGIN_ROOT}/scripts/review_preflight.py" post <number>

On skip, post nothing and finish. A re-targeted result also prints delta: <path> and updates $TMPDIR/reviewed-head. Read that entire file in chunks, update the review without dropping the draft marker when one is present, then run the preflight again. Do not post from the re-targeting pass.

A non-zero exit from this commandless check means nothing was decided. Fix the error and re-run it. In command mode below, post: means the outward command was attempted; handle that command's failure directly. For a review POST that returns 422, use the orphan-aware recovery procedure below instead of blindly rerunning the preflight.

Every review POST below passes its gh api command to the preflight after --. In that mode the preflight does not re-target: it runs the command only when two live snapshots and bot_review_state.py agree that the pinned head is open and unreviewed. This keeps the final check beside the outward action.

A push mid-review re-targets the review. Everything read so far still holds for the code it was read against, and the delta is the only new information — however many pushes it spans. Read it, then post against the new head:

• Review the delta to the standard step 4 sets — it is new code, and the review you post covers it. A skim is not enough.
• Run step 5 again over the updated merged tree. The second pass must see the delta before step 6 can post against the new head.
• Findings the delta left alone stand. Post them.
• Findings the delta fixed drop out. If that empties the review and the delta itself reads clean, approve the new head: an empty-body approval is a verdict here, not the absence of one.
• Finish without posting only when you can't judge the delta — it rewrites what you just reviewed, or it is a review's worth of new code in its own right. The queued run then reviews the new head in full.
• Inline comments resolve against the commit the review pins, so re-verify each one against the current gh pr diff, which now returns the new head's. On a file the delta didn't touch, the line is unchanged and the comment stands. On one it did, move the comment to the line the code sits on now; where the line no longer falls inside a hunk, put the finding in the review body as a fenced quote with its path, as under Recovering from inline comment 422 errors.
• Read both halves of the delta file as a pair. It contains two logs in sequence: the scoped one is the author's new code; the base merge: lines are base merges, and are the only place they appear. The two together distinguish an empty delta from an "Update branch" click. When a base merge: line appears, re-verify every inline comment against the new gh pr diff even if the scoped log printed nothing — the merge re-scopes hunks in files the scoped delta cannot show, so the "file the delta didn't touch" shortcut above does not hold.
• Also read git show --cc <merge sha> on a base merge, for what the merge itself changed. Where it conflicted, the author's resolution is committed inside the merge, and both logs miss it: the scoped log excludes merge commits, and the merges line says a merge happened, not what it changed. A finding the resolution already fixed must drop out. --cc prints only hunks differing from every parent, so a resolution that took the base side prints nothing at all — it tells you what a merge changed, never that a merge changed nothing, which is why re-verification above is unconditional.
• Re-compose every suggestion block after re-targeting, reading the new content with git show "$(cat "$TMPDIR/reviewed-head")":<path> — the workspace still holds the tree you reviewed, so disk gives you the old lines. A suggestion carried over unchanged can revert the author's newest edit or base-merged content.

Pin every review to the commit you read — commit_id in every posting recipe, read back from $TMPDIR/reviewed-head. Two things depend on the pin. GitHub otherwise anchors the review at whatever is live when the POST lands, so the review claims code this session never saw. And the anchor is what step 1 reports as last_review_sha: pinned to the head you re-targeted onto, the queued run finds that head already reviewed and finishes without posting a second review of the same code.

Before APPROVE specifically, run the snapshot below and require its head_sha to equal $TMPDIR/reviewed-head. The rollup is pinned to $TMPDIR/reviewed-head; a head mismatch means it does not cover the live head, so post findings if you have them, otherwise finish and leave the approval to the queued run. Then inspect that rollup: if any check has reached terminal FAILURE, do not emit an empty-body APPROVE — the close-out reads as the bot rubber-stamping over the visibly red signal. Re-check the author-readiness gate on the same pass — a comment withholding merge readiness can land after the review began, and the conversation you read in step 1 is by now stale.

An approval you post at a re-targeted head is yours to stand behind: the queued run reads that head as reviewed and finishes, so no successor session dismisses the approval if a check goes red. Step 7's poll is the whole net — run it to terminal before ending the session.

Reduce the rollup to the latest entry per check name and workflow before reading it. When a concurrency-cancelled run is replaced, GitHub keeps both check runs on the commit, so an un-deduped scan reports the superseded FAILURE alongside the replacement's SUCCESS — and keeps reporting it forever. Key on workflowName as well as the name: two workflows can register the same check name, and collapsing those into one entry would hide a genuine red behind an unrelated green.

uv run --script "${CLAUDE_PLUGIN_ROOT}/scripts/poll_pr_checks.py" \
  snapshot <number> "$(cat "$TMPDIR/reviewed-head")"

The JSON reports head_sha, pending, and failed for the pinned commit after dropping this run, this workflow, and superseded check runs. When those filters leave no external contexts, both lists are empty; that is the clean failed empty branch for this pre-approval snapshot, not evidence that the repository registered a gating check. Re-run the snapshot after any poll; no shell state survives between calls.

Don't treat a mid-flight rollup as settled. A FAILURE co-existing with a non-empty pending list is often a stale cancellation-cascade artifact, not a real failure: when several events fire near-simultaneously (e.g. Dependabot opening a PR), the tests concurrency group cancels all but the latest, and a cancelled contributor makes an if: always() merge-gate omnibus (like PRQL's check-ok-to-merge) resolve to conclusion FAILURE — not cancelled, so it slips past the post-approve cancellation awareness below and reads as red. A fresh replacement run is already in flight and will re-register the omnibus. So decide on the settled rollup:

• failed non-empty and pending non-empty — the rollup hasn't settled. Foreground-poll until non-own checks are terminal (the Step 7 / running-in-ci CI-monitoring loop), then re-run the snapshot. Judge the settled state, not the mid-flight snapshot — a stale cancellation-cascade FAILURE drops out once the replacement omnibus registers, but only via the reduction above; the superseded check run itself never leaves the commit.

• failed non-empty and the poll cap expired with pending non-empty — settlement is out of reach this session; a release or nightly matrix routinely outlasts the cap. Re-run the snapshot first. Then decide on provenance, not on settlement: resolve each remaining failure URL to its run and read that run's own conclusion.

  For each snapshot URL containing /actions/runs/<run-id>/, inspect that run:

  gh run view <run-id> --json conclusion --jq '.conclusion'
  

  A URL without an Actions run id is unresolved third-party status, not a cancellation you can prove.

  Every one cancelled — the red is superseded, so APPROVE and name the still-unverified checks in the body. cancelled is the only conclusion that earns an approval here: a real failure, an empty conclusion (the run is still going, so the job failed on its own merits), or an unresolvable URL (a third-party status context like codecov/patch, never an Actions run) all take the terminal-red branch below. Don't leave this to improvisation: the same stale red must not draw an APPROVE on one PR and a withheld approval on the next.

• failed non-empty and pending empty — genuine terminal red. Skip the close-out and finish. But if no prior substantive bot review stands on this PR, don't exit fully silent or leave only a +1 reaction — a clean external-dependency bump then carries zero review signal. Post a brief COMMENT stating the diff assessment and the failing check that withholds approval. Any earlier substantive review already stands as the active verdict — leave it. On a bot PR where you intend to push the fix yourself (step 9), post that COMMENT before pushing, while the rollup it describes is still the current one.

• failed empty — proceed with APPROVE.

Step 7's "approve, foreground-poll CI, dismiss if a check fails" pattern only recovers while the session is still alive — the job timeout or a poll cap can leave a post-approve failure undismissed and the PR carrying a misleading APPROVED state. A synchronous pre-APPROVE peek catches the case where the failure is already in the rollup — including non-required checks like codecov/patch that an overlay treats as a merge gate. Reducing to the latest entry per name and workflow — and, when the cap expires first, checking each FAILURE's run conclusion — is what keeps a superseded red from being mistaken for a real one.