# Github Pr Review > Review a GitHub pull request and leave inline comments in a natural human reviewing voice, indistinguishable from comments typed by hand on the GitHub web UI. Deliberately bounded to defects a reader can settle by looking at the anchored line, rather than a deep audit, so every comment is cheap for the author to check. Scales to 2-20 comments by PR size, parallelises analysis across sub-agents, drafts for approval, then posts individually with human pacing. Use when the user says "review this PR", "review PR 123", pastes a github.com/.../pull/N link, or asks for comments on a pull request. Don't use for reviewing local uncommitted changes or a diff against a branch (use the `review` skill for that). - Skill: `google/github-pr-review` (Agent Skill, multi-file: 22 files) - Install (CLI): `npx skillmds@latest add google/github-pr-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/google/github-pr-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: Google (https://skillmd.com/u/google) - Updated: 2026-09-22 - Page: https://skillmd.com/skills/google/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.
Living inside the google/adk-samples checkout 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 `skills/`, 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.
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 --repo --json number,title,body,headRefOid,additions,deletions,changedFiles,state python3 "$SKILL_DIR/scripts/existing_comments.py" \ --repo --pr --out /tmp/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 /tmp/pr- -- --depth 1 --no-tags && \ cd /tmp/pr- && git fetch --depth 1 origin pull//head && git checkout FETCH_HEAD # or, against a clone you already have: git -C fetch origin pull//head && git -C 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 --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 --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 `skills/`, `.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 --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 ``, ``, `` (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--raw.json --repo-root \ --repo --pr --head-sha \ --existing /tmp/pr--existing.json \ --out /tmp/pr--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 silently drop them, and never anchor them to an unrelated nearby line to force them through: a comment about `requires-python` pinned to a dependency line twenty rows away is worse than no comment. **The gate drops every `not_cheap` finding.** Report a bare count at the checkpoint (`23 findings dropped as not cheaply verifiable`) — not a list. Handing over the list would pull the user into evaluating exactly the material the gate exists to spare them. **Then apply the fact-anchoring filter yourself**, on the survivors. `verify_steps` catches findings that are expensive to check; it does not catch findings that are cheap to check but ask the reader to *derive* the defect. Delete the question from the comment: if what remains is not a statement of something visible at the line, drop it. This is the filter that separated all 16 accepted comments on #2373 from all 4 rejected ones — see the rated outcomes in `reference/voice.md`. **Cheapness is a property of the final comment text**, not the raw finding. `` `ty` is on 3.10 while `requires-python` says 3.11 `` is cheap because it names both sides; *"this contradicts the setting above"* is the same defect and expensive. Re-check at the end of Step 5 once the wording exists. Survivors are ordered `critical` first. There is no other label — `confidence` was removed because after the verifier runs, everything surviving is high-confidence by construction, so the column never discriminated. Severity orders the output and nothing else. It never changes how a comment is worded (`reference/voice.md`). Rules that still apply at this stage: - **If there are not enough real issues, produce fewer.** Padding produces exactly the vacuous filler that reads as machine-generated. If a PR is genuinely clean, say so and post nothing. - **≤ 2** cosmetic comments (stale years, a constant that disagrees, a naming inconsistency) — all `no_critical` by definition. - **1–2** positive remarks when genuinely warranted, **suppressed entirely if anything `critical` was found**. Nobody compliments the styling on a PR with a security hole. ## Step 5 - Voice phase > **Never delegate this step.** Not to a sub-agent, not split across two passes. > Everything below is a property of the comment set *as a whole* — the register mix, > the clustering, the sentence-shape variety, the cosmetic quota. An agent holding > one shard cannot honour any of them, and the failure is invisible per-comment and > glaring in aggregate. Read `reference/voice.md` now. Summary of the binding rules: - Every full-sentence comment **ends in a question mark**. Declarative multi-clause statements are banned. - Or use a lowercase fragment of 2–6 words with no terminal period. - **Tone never escalates with severity.** Command injection is raised as politely as a missing try/catch. - One thought per comment. No causal chains, no prescriptions stacked on diagnoses, no line numbers in prose. - Banned: bold labels, severity prefixes, emoji, headers, bullets, `suggestion` blocks, "Consider …", "It would be better to …". - Backticks on identifiers and paths. - **No crafted payloads, and no claim resting on code you didn't read** — the proof tell and the depth tell. Write the comment you would have written *before* doing the work, not after. - **Name the referent, and carry the verification path.** A comment must say what it points at, and if it depends on anything off the anchored line it must name that and say where. `old version still holds the value` is unusable; `Does the previous version get destroyed anywhere?` is not. Aim for ~60% Register A / 40% Register B. Findings arrive from workers as blunt technical prose (`user-controlled filename is interpolated into os.system()`). That is raw material, not a draft. Rewrite every one from scratch against the corpus. **Severity must not leak into the wording.** A `critical` comment is not sharpened and a `no_critical` one is not softened. The user reads the ordering; the PR author reads only the comment, and it has to look the same either way. ### Distribution matters as much as wording - **Cluster on the substantive files.** Uniform coverage — one comment per file, evenly spread — is a machine signature. Real reviewers dwell where the risk is and skim the rest. Fanning out by file makes this *worse* by default, since every lane returns findings: when you select, deliberately let some lanes contribute nothing. - Do not let two comments share a sentence shape. Three comments opening with `It seems` is a tell. - Vary length. A review where every comment is one line, or every comment is two sentences, looks generated. - Cross-reference in Register B when the same issue recurs (`same issue as above`) rather than restating it. **Before leaving this step**, re-check the `cheap` label against the wording you actually wrote (Step 4), and run the cold-read test from `reference/voice.md` on every comment: anchored line ±10 plus the comment text, nothing else — can you tell what it refers to and how you'd check it? If not, rewrite or drop it. ## Step 6 - Present, take a decision Generate the report, print the summary, then **stop and wait.** The report goes to `~/Documents/agents/pr-reviews/`. Don't ask — that folder is the destination unless the user names another path in the same breath. ```bash python3 "$SKILL_DIR/scripts/build_report.py" \ --candidates /tmp/pr--candidates.json --repo --pr \ --out-md ~/Documents/agents/pr-reviews/pr--review.md ``` That writes one markdown report — **summary table first, detail sections below** — carrying the code window beside every comment, which is the whole point: the user must be able to run the check without opening a file. `--out-csv` still exists for spreadsheet triage but is off by default; the CSV held the same rows as the markdown tables, so writing both left two copies of one report on disk. ### Present the set One table, ordered `critical` first, then by path. Every row carries the source line it is anchored to, so the user can settle it without opening a file. ``` 16 comments ready (2 critical), ~14 min to post. 3 more findings are un-anchorable — see below. 47 findings dropped as not cheaply verifiable. Report: ~/Documents/agents/pr-reviews/pr-2373-review.md ``` **Recommend, don't decide.** - Don't propose "criticals only" by reflex. A review that is entirely critical security findings is itself implausible; real ones mix a sandbox escape with a stale Python version. - Point out anything you are unsure of rather than burying it. The user cutting a comment costs nothing; a bad comment posted costs their credibility. **Then apply the size budget from Step 2.** If the set exceeds the cap, trim and say exactly what was dropped. Never trim silently. #### Must-post findings **H48** — an `ownership.team` naming a company, a filler word, or the author's own GitHub handle — is exempt from the budget and from the "is it worth a slot?" cut. It survives trimming even on a PR already at its cap. It is the only field in the repo where a wrong value passes every deterministic check: the schema asks for a non-empty string and stops there. Nothing downstream notices, and the recipe ships with no reachable owner. So the comment goes out every time the checker fires, and when `manifest.yaml` is outside the diff hunks it goes in the un-anchorable top-level comment rather than being dropped. Ask who maintains the recipe. Never propose a team name — the same reason as H17. Never post without explicit approval, and let the user cut or reword anything. A wrong comment posted in their name costs their credibility with the PR author, so this step is where they take ownership of the review as their own. **Record what they cut**, right after they choose and before posting: ```bash python3 "$SKILL_DIR/scripts/rejections.py" --record \ --repo --pr \ --candidates /tmp/pr--candidates.json \ --approved /tmp/pr--comments.json ``` `--candidates` is everything drafted, `--approved` is what they said yes to; the difference is the rejection. **Pass the matching pair from this run** — mismatched files record the wrong set and poison the ledger. Step 1's suppression only sees comments that were *posted*. One the user read and cut left no trace on GitHub, so without this it returns on the next review and has to be rejected again — which happened three times on the #2373 re-review. The verifier loads the ledger automatically; nobody has to remember to pass it. ### Un-anchorable findings If the `unanchorable` bucket is non-empty, present it as its own short table and offer **one** top-level issue comment covering all of them: ``` Un-anchorable (3) — real, but the lines aren't in any diff hunk: pyproject.toml:69 [tool.ruff] tables CI-FAIL pyproject.toml:38 requires-python >=3.10 CI-FAIL pyproject.toml:2 name != folder basename CI-FAIL Offer as one top-level comment? ``` This is the one permitted exception to "no summary comment" (Step 7). Keep it to a bulleted list of the findings, with no preamble about the review as a whole and no restatement of the inline comments. Order them **CI-failures first** — an author will act on "this blocks the build" and may not act on a convention nit. Mark advisory items as such, and never claim CI will fail when `.github/review-rules.md` says the check is a `::notice`. ## Step 7 - Post Write the approved set to JSON and dry-run it first — blocking, it's fast, and it is the gate: ```bash python3 "$SKILL_DIR/scripts/post_comments.py" \ --repo --pr --file /tmp/pr-comments.json --dry-run ``` It validates every line against the diff hunks and fails loudly if a line isn't addressable. Then run the real thing **as a background job** and hand control back: ```bash python3 "$SKILL_DIR/scripts/post_comments.py" \ --repo --pr --file /tmp/pr-comments.json ``` ``` Posting 9 comments, ~8 min. Running in the background - you'll be notified when it's done, so carry on with whatever you like. ``` Do not sleep-and-poll it. You get a completion notification; `tail` the job only if the user asks how far along it is. The script posts each comment individually, in file order, sleeping a random 10–20s between them. **Do not shorten the gaps** — the pacing is the point (`reference/rationale.md`). If it fails partway, the `.posted` state file records what landed; re-run the identical command to resume. **A pending review blocks everything** — GitHub allows one per user and this endpoint implicitly opens one, so every comment 422s. The script preflights and stops with instructions. Do **not** submit or discard their pending review for them: submitting posts publicly under their name and may carry a verdict; discarding destroys drafts they wrote. Surface it and let them choose. **No verdict and no summary comment.** Do not approve, do not request changes, do not leave a top-level recap. The user handles the verdict themselves. The single exception is the un-anchorable list from Step 6, when the user approves it — a bulleted list of findings whose lines are in no diff hunk. Not a recap: it says nothing about the review as a whole and repeats no inline comment. comments.json format: ```json [ {"path": "src/api.py", "line": 42, "body": "missing await here"}, {"path": "src/api.py", "line": 88, "body": "Can we use `subprocess.run()` with a list here?"} ] ``` `line` must be a line present in the diff. Add `"side": "LEFT"` to comment on a deleted line. ## Progress reporting The user cannot see analysis happening. On a large PR the gap between "review this" and the draft is minutes, and the post run is minutes more. Fill it. **Todo list**, seeded in Step 2 and kept current — this is the live progress bar: ``` - Gather PR + existing comments - Plan lanes and budget - Lane 1 analysis (4 files, 320 lines) - Lane 2 analysis (6 files, 295 lines) - ... - Consistency lane - Merge, dedupe and label findings - Draft comments in voice - Present table and await approval - Post (background) ``` Mark each done as it lands, not in batches. On a small PR collapse the lane items into a single "Analyse changed files". **Checkpoints.** Four short messages, no more. Prose, not tables. 1. *After the plan* (Step 2) — the mode/scope question, then size, skips, budget, lane count, and the promise that nothing posts without approval. 2. *After analysis* (Step 3/4) — the funnel, so the shape of the cut is visible: ``` Analysis done. 74 findings across 12 lanes. 3 rejected by the verifier, 25 un-anchorable, 26 dropped as not cheaply verifiable or style. 20 candidates, drafting now. ``` 3. *At the decision* (Step 6) — the table, the un-anchorable list, the report path, the ETA. 4. *At posting* (Step 7) — backgrounded, ETA, notification promise. `batch_task` does not stream partial results, so nothing arrives between checkpoints 1 and 2. That makes checkpoint 1 load-bearing: it must state that analysis takes a few minutes, or the silence reads as a hang. If anything degrades — a lane fails, the diff is unfetchable, the PR is closed, every file is skipped — say so immediately rather than quietly working around it.