google/adk-recipes

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:

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. 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.

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.

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:

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

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:

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).
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, 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:

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.

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 f