PR Review

A two-phase review loop. Phase 1 produces a verified, severity-ranked report in the terminal. Phase 2 — only after the user says go — pushes a GitHub review with inline comments on the blockers.

The two phases are separate on purpose. Pushing a review is outward-facing and hard to retract: it notifies the author, it shows up in their queue, and a wrong finding published inline costs them real time. Never merge the phases, and never push in the same turn as the report unless the user's request explicitly asked for both up front.

Phase 1 — Review

1. Orient

Pick the target from the argument:

• PR number / URL → the normal path:

  gh pr view <n> --json title,body,author,state,baseRefName,headRefName,additions,deletions,changedFiles,url
  gh pr diff <n> --name-only

• Branch name → git diff <base>...<branch> (base is usually main/master — check the repo). If the branch has an open PR (gh pr list --head <branch>), switch to the PR path.
• No argument → if the user is clearly asking about a PR, ask which one. If they're asking about their own changes, review the working diff: git diff HEAD plus untracked files from git status.

Branch and working-diff reviews end after the Phase 1 report — there's no PR to push to.

Read the PR body properly. A well-written description tells you what the author already knows is risky, what they tested, and what they deliberately deferred — all of which shapes where to look. A description claiming a defect class was already found and fixed is a hint to check whether the fix is complete, not a reason to skip that area.

Make sure you're in the right repo — the working dir may be a parent folder; cd into the app if needed.

2. Decide whether the frontend lens applies

Turn it on when the diff touches .tsx/.jsx/.ts/.css under a frontend app, or the repo (or a monorepo package the diff touches) has a components.json or vite.config.*. A PR that touches both a backend and a frontend gets both treatments — run the frontend lens on the frontend files only.

When it's on, confirm the project's actual conventions before reviewing — don't assume the org defaults apply:

• package.json — scripts, React/Router/Tailwind versions, Biome vs ESLint.
• components.json — shadcn config; is the @nebari registry registered? If so, Nebari rules apply.
• vite.config.ts / tailwind.config.* — Tailwind v4 (CSS-first) vs v3.
• src/index.css / globals.css — the defined semantic tokens. This is your allowlist for "is this color a real token or a hardcode?".
• biome.json — so a "lint" comment is a real finding, not a style opinion the tooling already owns.

The canonical "what good looks like" lives in two skills — read them when a checklist item needs the full rule: frontend-conventions (structure, naming, TanStack Query vs Jotai, the quality gate) and nebari-ui (the @nebari registry, theme tokens, cn(), render-prop composition, motion tokens, managed ui/*). For deeper design/UX critique of a specific view, impeccable is the specialist.

3. Run the deep review

Invoke the built-in code-review skill with the PR number (or branch). It reads the full diff, builds the branch in a scratch worktree, and writes throwaway reproduction tests to confirm suspicious paths — which is what separates a real finding from a plausible-sounding one. It runs in the background; don't duplicate its work in the meantime.

If the frontend lens is on, use that wait to work references/frontend-checklist.md against the frontend files — read it now; it's the substance of the frontend review. Read the full current version of each changed file, not just the hunk: a hunk hides whether a console.log is inside a debug guard, whether an import is now unused, whether the token exists in the theme. Its seven dimensions:

1. General code — error/loading/empty states, no any, nullability, duplication, frontend security (dangerouslySetInnerHTML, target="_blank").
2. Frontend craft — folder/naming/barrels, TanStack Query vs Jotai, no useEffect fetching, hook rules, co-located tests, no hand-edits to src/components/ui/.
3. Accessibility — semantic HTML, accessible names, keyboard/focus, forms, ARIA, contrast, motion-safe: gating.
4. Nebari design system — reuse existing components/variants, render prop over rewrapping, cn(), managed ui/*, motion tokens.
5. No hard-coded colors or styling — raw hex/rgb()/hsl(), named Tailwind colors, inline color styles, hand-added dark: patches. The test: would it render correctly in dark mode without change?
6. No stray console.* / debugger / alert().
7. Commented-out code & TODOs — dead blocks, untracked TODO/FIXME, placeholder content.

--quick: skip code-review and do a single direct pass (plus the frontend checklist if it applies). Say in the report that it was a quick pass and blockers weren't reproduced.

If code-review is unavailable, do the review directly, but keep its standard: every finding you report must be substantiated against the actual code, and blocking findings should be reproduced.

4. Verify the findings yourself — do not relay unchecked

Subagent findings are input, not output. Both times this process has run, the returned findings contained at least one claim that needed correcting before it was safe to relay. Check each finding cheaply before it goes in the report:

• Does the file exist, and is it actually in the diff? A finding may cite a real defect located in unchanged code. That is still a legitimate finding when the PR causes it (e.g. this PR canonicalizes emails at the store boundary, and an untouched findMember still compares exactly) — but say so explicitly, because it changes where the fix goes and it can't be anchored inline. Otherwise, pre-existing issues are out of scope unless the change makes them materially worse; tag any you mention [pre-existing].
• Do the line numbers resolve? git show <head-sha>:<path> | sed -n '<n>p' — cite lines from the PR head, not from your checked-out branch.
• Are "X does not exist" claims true? grep for it. Watch zsh globbing: grep -rn "Foo" . not grep -rn "Foo" --include=*.go . (zsh expands the glob and the command fails with no matches found).
• Does the reproduction actually reproduce? If the subagent describes a repro, the described mechanism must match what the code does. A repro that depends on a code path that isn't there is a dead finding.

Frontend findings come from pattern-matching, so they need their own check:

• A "hardcoded color" — is it truly not a token? Tokens are often defined as hsl/oklch values in the theme file; that's the definition, not a violation.
• A "console.log" — is it behind an import.meta.env.DEV guard or a logger util? Then it may be fine.
• A "missing label" — is there an aria-label, aria-labelledby, or wrapping <label> elsewhere in the file?

Drop what doesn't hold. Correct what's half-right. A finding you can't substantiate is worse than no finding, because it costs the author a round trip to disprove.

While verifying, also check two things the review usually misses:

• Migration/version-number collisions with other open branches (ls the migrations dir on both branches — goose and most migration tools error on duplicate versions, so whichever merges second must renumber).
• Structural merge conflicts with the branch you're on or other open PRs — a PR built on a type or field another open PR deletes is a rewrite, not a textual merge. Worth a line even though it isn't a defect.

5. Report

Open with a verdict, then group by severity, most severe first. Anchor every finding to file:line. State the failure concretely — inputs and state → wrong outcome — and give the fix.

## PR Review — <PR title / branch>

**Verdict:** Approve · Request changes
<one-sentence rationale>

## 🔴 Blocking

**1. <one-line claim>** — `path/to/file.go:59`

<mechanism, in the code's own terms. Quote the few lines that matter.>

<the evidence: what you reproduced, and what came out. Verbatim error text
if there is one.>

<why the test suite doesn't catch it, if it doesn't — this is often the
most useful sentence in the whole review.>

<the fix, specifically.>

## 🟡 Should fix

## Worth knowing
<migration-number collisions, structural conflicts with other open PRs —
not defects, but the author needs them before merging.>

Convention findings from the frontend lens are usually short — one line each is fine: `src/components/UserCard/UserCard.tsx:42` — Hardcoded `bg-white`; use `bg-background` (defined in index.css). Breaks dark mode. Save the full mechanism/evidence/fix treatment for behavioral defects.

Rules that keep the report worth reading:

• Lead with the mechanism, not the severity label. "This wraps mcpHandler, not handler, so the cap is discarded" beats "HIGH: security issue in handler wiring".
• Name why CI is green whenever it is. TestFoo using DevMode: true and thereby testing the one branch where the wrapper survives is the detail that makes the finding land.
• Distinguish reachable from latent. A defect gated behind a feature that hasn't shipped is real but not blocking — say which it is.
• No nits. Don't report naming preferences, formatting, wording, "could be slightly cleaner" refactors, or anything the linter/formatter already owns. If a finding wouldn't change behavior, correctness, security, accessibility, or a future reader's understanding of an invariant, leave it out entirely — not in a lower tier, not in "Worth knowing".
• Don't pad with praise, but do say when the PR's own hard work was right (a genuine catch in the author's review history is worth one sentence).
• If the PR is clean, say so plainly and skip to offering an approve.

For a PR, close the report by offering the push:

Want me to push these as inline comments and request changes?

Then stop. Do not push yet.

Phase 2 — Push (only after approval)

Default scope: inline comments for the blocking findings only, a short body, and the should-fix findings left in the terminal. Push more only if the user asks.

The body comment stays light

One to three sentences. It frames the review; it does not restate it. Good shape:

One blocker inline — the body cap is wired so it only takes effect in dev mode, the inverse of the intent. One-word fix. The rest is should-fix and can follow up.

What to keep out of it: a re-listing of the inline findings, a summary of the PR, praise padding, and hedging about your own confidence. If a finding needed a caveat, the caveat belongs in that finding's inline comment.

Mechanics

Read references/github-review-api.md before building the payload. It has the gh api invocation, the payload shape, and the four gotchas that will otherwise cost a failed request each — most importantly that an inline comment can only anchor to a line that appears inside a diff hunk, which is not the same as a line in a changed file.

The short version:

1. Get the head SHA: gh pr view <n> --json headRefOid -q .headRefOid.
2. For each blocker, find an anchorable line in the relevant hunk (see the reference).
3. Write the payload to a JSON file in the scratchpad — never inline in the shell, the quoting will bite you.
4. gh api repos/<owner>/<repo>/pulls/<n>/reviews --method POST --input <file>.

After pushing

Report back with the review URL, one line per inline comment placed, and — importantly — any place you had to deviate: a line you couldn't anchor and where you anchored instead, a suggestion block you had to drop, a finding you left out. The user needs to know the pushed review isn't identical to the report they approved.

Severity guide

• 🔴 Blocking — wrong behavior in a reachable path; a security or access-control control that doesn't apply where it claims to; data loss; a migration or startup path that can brick a deployment. Reproduce these. Frontend: breaks dark mode or a11y (an unlabeled icon-only control, a keyboard trap), edits managed ui/*, leaks a secret into the bundle.
• 🟡 Should fix — real defect with a narrow trigger, a guard that silently no-ops, a documented invariant the code doesn't hold, a regression in an adjacent surface. Frontend: convention violations with real consequences — server data in a Jotai atom, any, a missing loading/error state, hardcoded styling that happens to look right in light mode. Also 🟡: a defect latent behind an unshipped feature, and a stale comment that is the stated rationale for a workaround (the next reader will reason from a false invariant). Frontend: stray console.log/debugger, dead commented-out code, a TODO marking something incomplete in the code being merged.

There is no minor/nit tier. Anything below 🟡 — naming, style, cosmetic doc drift, "use this slightly nicer variant", ergonomics-only limits — is dropped, not reported.