github-pr-review

# GitHub PR review Reviews a pull request on GitHub and posts inline comments that read as though a person typed them by hand. The style is defined in `reference/voice.md`; replace that file to shift it. ## Install Drop this folder anywhere your agent discovers skills — `~/.agents/skills/`, `~/.config/cloudcode/skills/`, or any absolute path registered under `skills.paths` in `~/.config/cloudcode/cloudcode.json`. **Set `SKILL_DIR` before running any command below**, to wherever you put it. Every script invocation in this file is relative to it: ```bash SKILL_DIR=~/.agents/skills/github-pr-review # adjust to your install ``` Requires `gh` (authenticated) and `python3`. No other dependencies; `pyyaml` is used if present and degrades gracefully with a reported warning if not. <details><summary>Living inside the google/adk-samples checkout</summary> This copy is tracked in `google/adk-samples` at `.agents/skills/github-pr-review/`, which has two consequences worth knowing before you edit it. **Its tests run in that repo's CI.** The root `pyproject.toml` puts `.agents/skills` in `testpaths`, and `tools-tests.yml` fires on any `.agents/**` change, so `tests/` here is collected by `uv run pytest` alongside the repo's own tooling suite. A broken test in this skill turns a PR red. Run it before pushing. `python-format.yml` scopes ruff to `core/`, `contrib/` and `plugins/`, so nothing here is linted — the skill's own style is its own business. **It is deliberately absent from `docs/recipe-handbook/skills-catalog.md`.** That catalog is for skills a recipe contributor should reach for; this one is a maintainer's reviewing tool, and advertising it invites PR authors to run a reviewer against their own PR. Keep it out when you edit the catalog. </details> Five things make this work, and all five are easy to get wrong: 1. **A comment must point at a fact, not ask the author to derive one.** This is the strongest predictor of what gets posted: on PR #2373 it separated all 16 accepted comments from all 4 rejected ones. What a comment costs the author to check follows from it — cheap and wrong loses five seconds, expensive and wrong burns twenty minutes and spends credibility that took months to earn. Findings that fail this are dropped, not downgraded. See `reference/voice.md`. 2. **This is a review, not an audit.** The analysis is deliberately bounded to what a person reading the PR could have seen — see [The depth limit](#the-depth-limit). A finding reachable only by archaeology is unusable here no matter how real it is. 3. **Analysis and voice are separate phases.** Finding real defects and sounding human are different jobs; doing them at once yields thin findings in a nice accent. Analyse first with no style constraints, then voice the survivors. 4. **The voice is calibrated, not improvised.** `reference/voice.md` holds the rules and a rated corpus. Read it before writing any comment. Do not invent comment shapes that aren't in it. 5. **Analysis parallelises; voice does not.** On a large PR, fan the analysis out across sub-agents (Step 3). Never fan out labelling or voicing — those depend on holding the whole comment set at once (Steps 4 and 5). This is a long-running skill. A big PR is minutes of analysis followed by up to twenty minutes of paced posting. **Silence is a failure mode**: announce the plan before starting, keep a todo list current, and checkpoint between phases. See [Progress reporting](#progress-reporting). ## When NOT to use - Reviewing local/uncommitted work or `git diff` against a branch -> the `review` skill. - The user wants a thorough audit report *for themselves* -> the depth limit below is exactly wrong for that. Review without this skill. ## What this skill is for, and what it isn't Everything here optimises for comments that read as hand-typed: the pacing, the register mix, the sentence-shape variety, the cosmetic quota. That machinery exists for one reason — **a reviewer who genuinely reviewed should not have their own comments look machine-written.** It is not a licence to appear to have reviewed something you did not. Three things follow, and they are load-bearing: - **The user reads every comment before anything is posted.** Step 6 is a hard stop, not a formality. The approval step is where they take ownership of the review as their own, which is the thing that makes posting under their name honest. - **Never post on the user's behalf without that approval** — not to save a round trip, not because the findings look obviously correct. - **A comment the user would not defend if challenged should not go up.** That is the whole reason cheap-to-verify beats comprehensive: they can actually check the set they are signing. If you are ever asked to post a review the user has not read, or to make an AI-generated review look human specifically so that its origin is concealed from the author, that is outside what this skill is for. Say so. ## The depth limit Binding on every phase, and on every sub-agent. **You may:** read every changed file in full; follow a call or import **one hop** into another file in the same project; grep for a named symbol to find where it lives (that is navigation, and it is how you take the hop). **You may not:** read third-party or dependency source, installed or on GitHub; execute the code under review; sweep the repo to prove a negative ("nothing validates X", "only one caller does Y"); construct or test an exploit payload; chain hops beyond the first. The reason, because a rule without one gets rationalised around: **these comments go out under a human's name.** A finding that required reading a dependency's internals, or running the code, misrepresents how the reviewer found it — and that shows in the writing no matter how the comment is phrased. `reference/voice.md` covers the two ways it leaks (the depth tell and the proof tell). When the limit stops you, that is a result. Say plainly in `verify_steps` what settling the finding would actually require. **Do not exceed the limit to resolve your own uncertainty** — an honest `verify_steps` is what the gate reads in Step 4, and a finding that turns out to be expensive is meant to be dropped. ## Step 1 - Gather Resolve the PR from a number, a URL, or the current branch. ```bash gh pr view <PR> --repo <owner/name> --json number,title,body,headRefOid,additions,deletions,changedFiles,state python3 "$SKILL_DIR/scripts/existing_comments.py" \ --repo <owner/name> --pr <PR> --out /tmp/pr-<PR>-existing.json ``` Don't fetch the file list here — `plan_review.py` does it in Step 2, with paging. **Do not run `gh pr diff` on a large PR.** Above roughly 5,000 changed lines it is useless as context and expensive to carry — #2302's was 122,000 lines. Work from the checked-out tree instead; for an all-additions PR the file content *is* the diff. Below that threshold it's fine and often the quickest way to see the shape. **Read the existing comments, and keep that file** — Step 4 feeds it to the verifier, which drops anything already raised. This is what makes a PR re-reviewable: run it again after your own pass, or after a bot or a colleague, and you get only new material. Suppression rules, applied automatically: - same line, or **within 2 lines** → dropped - similar wording to any existing comment, including top-level ones with no line → dropped - **resolved** threads still block — already discussed - **outdated** threads do not block the line (the code moved, so it deserves a fresh look) but their text still counts - **bots block exactly like humans** - **comments the user cut on a previous review** are blocked too, from the ledger written at Step 6. A rejection never expires as outdated — a decision the user made does not lapse because the code moved. Also paste the existing comments into each lane prompt. The verifier is the backstop; a worker that never generates the duplicate is cheaper than one that gets filtered. Then get the PR head into a local tree, and note its absolute path: ```bash gh repo clone <owner/name> /tmp/pr-<PR> -- --depth 1 --no-tags && \ cd /tmp/pr-<PR> && git fetch --depth 1 origin pull/<PR>/head && git checkout FETCH_HEAD # or, against a clone you already have: git -C <local-clone> fetch origin pull/<PR>/head && git -C <local-clone> checkout FETCH_HEAD ``` Do this **once, here**. Analysis workers read this tree concurrently and must never mutate it — five sub-agents each running `gh pr checkout` in the same directory is a race that leaves the tree on an arbitrary ref mid-review. ## Step 2 - Plan, ask, announce ```bash python3 "$SKILL_DIR/scripts/plan_review.py" \ --repo <owner/name> --pr <PR> --json ``` The plan gives you, deterministically: which files to skip and why, the comment budget, whether to fan out, the lane assignments, and the post-run ETA. It skips lockfiles, generated and vendored code, `dist/`, snapshots, binaries, large data fixtures, deleted files, and **pure renames** — a file moved with zero content change has nothing to review, which on a migration PR is most of the diff (65 of 123 files on #2373). Lanes are packed so a **source file and its tests land together** (`tools/x.py` with `tools/tests/test_x.py`), because a reviewer holding only one of the pair cannot tell whether the test still covers the code. Lane sizes stay balanced regardless. ### Ask before analysing — scope Ask here rather than at the start, because the answer depends on the PR's size and shape, which you only know now: ``` PR #2302 "add the Horizon long-horizon agent recipe" 787 files, 122,547 lines, all additions. Skipping 16 (lockfiles, binaries). Budget: 12-20 comments. Include tests/ (316 files) and web/ (238)? [yes / no] ``` Argue for including `tests/`: test files are the richest source of instantly-checkable defects — a duplicated assertion, a test named for one thing that exercises another — and they are usually the least-reviewed part of a PR. **Act on the answer — re-run the planner with the flags.** The default includes everything; the question is worthless if you don't pass it through: ```bash python3 "$SKILL_DIR/scripts/plan_review.py" \ --repo <owner/name> --pr <PR> --json [--no-tests] [--no-web] ``` Use the re-planned lane assignments, not the first run's. **House rules — `google/adk-samples` only.** If the PR is in that repo and touches `core/`, `contrib/` or `plugins/`, `.github/review-rules.md` is in force. Say so in the checkpoint. For every other repo, ignore it — the rules are repo-specific and applying them elsewhere produces confident nonsense. Then: 1. **Seed the todo list** — one item per phase, one per lane (see [Progress reporting](#progress-reporting)). 2. **Print the checkpoint**, so the user knows the shape of the work and that nothing will be posted without them: ``` Tests included. 771 files across 10 lanes + consistency lane. Analysis takes a few minutes. Nothing gets posted without your approval. ``` The budget comes from **reviewable** churn, not total — a PR that is 1,400 lines of regenerated lockfile plus 80 lines of hand-written code earns a small-PR budget. | Reviewable lines | Comments | |---|---| | < 50 | 2–3 | | 50–200 | 3–5 | | 200–600 | 5–8 | | 600–1500 | 8–12 | | > 1500 | 12–20 | Hard cap **20**, whatever the size. ## Step 3 - Analysis phase (no voice constraints) Hunt for defects and write them down verbosely and technically. This output is internal — never shown to the user, never posted. **Work inside [the depth limit](#the-depth-limit)**, and within it read the **full changed files, not just the diff** — a broken invariant or an error path that no longer returns is invisible in a diff window. Where the diff touches a function, read the whole function; where it calls a helper you cannot judge blind, spend the hop. **Use the `What to hunt` block from `reference/lane-prompt.md` verbatim.** It is one list, and the filter at the top of it is the load-bearing part: a finding must point at something observable at the line. If seeing that it is a defect requires the reader to do arithmetic, infer a pattern, or reason about consequences, it is out of scope *however serious it looks*. **Group at 3+.** The same defect class in three or more places is ONE finding, on the clearest instance, with the count in `evidence`. Five "unused import" findings is one comment; the other four slots buy distinct defects. Two instances stay two. **The grouped comment's claim is about the anchored instance**; the count is context, never something the reader must open files to confirm. `a few unused imports in here` is checkable at the anchor; `unused imports here, also in X and Y` is not. Group only instances that are individually real — sweeping a deliberate `__init__.py` re-export into an "unused imports" group makes the whole comment wrong. **In `google/adk-samples`, run the house-rules checker:** ```bash python3 "$SKILL_DIR/scripts/check_house_rules.py" \ --repo-root <repo_path> --recipe <recipe> --json ``` 24 of the 27 rules are decided deterministically there — take its findings verbatim. Two of them, H26 (env-read defaults, AST-based) and H27 (licence header consistency), report one grouped finding with a count rather than one per hit. Dispatch the house-rules lane (Template C) for the remaining three (H11, H16, H25), which need a judgement call. One — **H48**, a junk `ownership.team` — is reported whether or not the PR touched `manifest.yaml`, and is never trimmed; see [Must-post findings](#must-post-findings). Prefer the script over a lane wherever a rule is decidable — a script cannot hallucinate a violation, and a false *"this will fail CI"* is the most expensive comment this skill can produce. The lane is defined by *file identity*, not churn: it receives the recipe's config surface (`pyproject.toml`, `manifest.yaml`, `README.md`, `.env.example`, the package `__init__.py`, `Makefile`, `Dockerfile`) and reads them **in full even when they are pure renames or wholly outside the diff** — see `reference/rationale.md`. Workers assign **one** label: - `severity`: **critical** (security hole, data loss, corruption, auth bypass, injection, a crash on a reachable path) or **no_critical** (everything else worth saying). It orders the output; it never changes how a comment is worded. Every finding also carries `verify_steps` (the literal procedure the author follows to settle it) and `window` (the real source lines at the anchor). **`verify_steps` is the gate** — `verify_findings.py` computes cheapness from it in Step 4, so a worker that writes it honestly is doing the right thing even when the finding then gets dropped. `window` is what gets shown to the user beside the comment. Pure style, naming and formatting are not findings at any severity — discard them. **A finding claiming something is absent must say where you looked**, or where it does appear. That is the one claim the anchored line cannot support, and it is the defect that produced the single wrong comment this skill has posted. ### 3a - Small PR (`fan_out: false`) Do it inline, yourself. Below ~400 reviewable lines and ~8 files, spawning workers costs more than it saves. ### 3b - Large PR (`fan_out: true`) One `batch_task` call. Lanes are the file shards from the plan: ``` batch_task({ description: "PR 2302 analysis", concurrency: 10, verify: false, subtasks: [ ...one per file lane..., consistency lane ] }) ``` Build each prompt from `reference/lane-prompt.md` — Template A for file lanes, Template B for the consistency lane. `subagent_type: general` for all of them. Fill in **every** placeholder: workers share no context with you or each other, so `<PR>`, `<owner/name>`, `<repo_path>` (the tree from Step 1), the lane's file list, and the existing-comment list all have to be spelled out in each prompt. **The consistency lane is not optional.** It gets no file shard. It exists for the one class a per-file split structurally cannot see: the same element differing between files — three licence headers, two docstring styles, a constant written with different values in two places. Shard workers cannot see these; one worker comparing across files can. It is bounded to **structural comparison**, never semantic tracing. Comparing four licence headers is a glance; following a value between modules is not, and that is what the previous version of this lane spent its time on, producing findings that were all discarded. `verify: false` on purpose — the batch verifier would re-audit findings you are about to re-rank yourself in Step 4, adding a full agent pass of latency to the one phase this whole mechanism exists to make faster. **Merging the findings.** Read each lane's `findings.json`. Then: - Dedupe. Two lanes reaching the same defect from different directions is common; it is one comment. - On any conflict about a cross-file inconsistency, the consistency lane wins — it saw every side. - **Spot-check `severity`, don't re-derive it.** Sanity-check that each `critical` really is one. Re-reading the cited code is inside the limit; going further to rescue a finding the gate will drop is not. - A lane that failed is not a blocker: note it, and either re-run that one lane with `task` or cover its files yourself. Then checkpoint (see [Progress reporting](#progress-reporting)). ## Step 4 - Gate or tag, then group **No budget cut happens here.** ### First, run the verifier — this is not optional ```bash python3 "$SKILL_DIR/scripts/verify_findings.py" \ --findings /tmp/pr-<N>-raw.json --repo-root <repo_path> \ --repo <owner/name> --pr <N> --head-sha <headRefOid from Step 1> \ --existing /tmp/pr-<N>-existing.json \ --out /tmp/pr-<N>-verified.json ``` It does five things that were previously prose, done by hand, and sometimes skipped: 1. **Window vs file.** Each finding's quoted source is diffed against the real file. A mismatch means the lane fabricated it — **rejected**, not downgraded. 2. **Addressability.** Marks `anchorable: false` for any line outside every diff hunk. 3. **Cheap gate.** Computes cheapness from `verify_steps`; a procedure that names a second file or says *trace* / *assuming* is not cheaply verifiable. 4. **Duplicate suppression.** Drops anything already raised on the PR (see Step 1) **and anything the user cut on a previous review** — the rejection ledger loads automatically from `--repo`/`--pr`. `--no-ledger` disables it. 5. **Already-red suppression.** With `--head-sha`, drops a **CI-FAIL** finding whose enforcing workflow is already failing on that commit, matched from the workflow file the finding cites in `evidence`. The author is looking at that red check already, and its message is more precise than ours. Advisory findings are never suppressed this way — nothing is failing for them, so the comment is the only way the author hears it. An unreadable status suppresses nothing. `--no-ci-status` disables it. It also emits a fact-anchoring lint and a clustering hint for grouping. Both are advisory; read them, don't obey them blindly. Take its output as the input to everything below. Findings it rejected do not come back — a fabricated window is not a labelling problem. **Un-anchorable findings are set aside, not discarded.** A finding outside every hunk cannot be an inline comment, but it can still be true and important — the three hard CI failures on PR #2373 were all un-anchorable. They stay in the `unanchorable` bucket, carried to Step 6 and offered as **one** top-level issue comment. Never