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.
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:
- 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. - 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.
- 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.
- The voice is calibrated, not improvised.
reference/voice.mdholds the rules and a rated corpus. Read it before writing any comment. Do not invent comment shapes that aren't in it. - 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 diffagainst a branch -> thereviewskill. - 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 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:
- Seed the todo list — one item per phase, one per lane (see Progress reporting).
- 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 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 eachcriticalreally 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
taskor cover its files yourself.
Then checkpoint (see Progress reporting).
Step 4 - Gate or tag, then group
No budget cut happens here.
First, run the verifier — this is not optional
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:
- Window vs file. Each finding's quoted source is diffed against the real file. A mismatch means the lane fabricated it — rejected, not downgraded.
- Addressability. Marks
anchorable: falsefor any line outside every diff hunk. - Cheap gate. Computes cheapness from
verify_steps; a procedure that names a second file or says trace / assuming is not cheaply verifiable. - 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-ledgerdisables it. - 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 inevidence. 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-statusdisables 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_criticalby definition. - 1–2 positive remarks when genuinely warranted, suppressed entirely if
anything
criticalwas 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,
suggestionblocks, "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 valueis 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 seemsis 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.
python3 "$SKILL_DIR/scripts/build_report.py" \
--candidates /tmp/pr-<N>-candidates.json --repo <owner/name> --pr <N> \
--out-md ~/Documents/agents/pr-reviews/pr-<N>-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:
python3 "$SKILL_DIR/scripts/rejections.py" --record \
--repo <owner/name> --pr <PR> \
--candidates /tmp/pr-<PR>-candidates.json \
--approved /tmp/pr-<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:
python3 "$SKILL_DIR/scripts/post_comments.py" \
--repo <owner/name> --pr <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:
python3 "$SKILL_DIR/scripts/post_comments.py" \
--repo <owner/name> --pr <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:
[
{"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.
- After the plan (Step 2) — the mode/scope question, then size, skips, budget, lane count, and the promise that nothing posts without approval.
- 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. - At the decision (Step 6) — the table, the un-anchorable list, the report path, the ETA.
- 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.