pr-review
Write a formal PR or release-promotion review with verified findings, standards citations, JSON and Markdown artifacts. Use for PR reviews and re-reviews.
SKILL.mdcodex/skills/pr-review/SKILL.md
GitHubRead the project AGENTS.md and applicable global guidance first. If AGENTS.md is absent, read CLAUDE.md as migration fallback. Discover optional skills before invoking them. If using-chrisbanes-skills, android-cli, or an official Android skill is unavailable, use the bundled android-standards references and current official documentation. Do not invent commands or claim that an absent skill ran. Use installed Android SDK tools when the optional Android CLI is missing. Commit and push only when the user has requested that action.
Formal PR review
What this produces
Two artifacts, side by side: findings.json, the canonical, machine-readable output
(schema in references/output.md), and the markdown review document
rendered from it. Names: pr-review-<source>-to-<target>-YYYY-MM-DD.{json,md} (promotion) or
pr-review-<branch-or-PR#>-YYYY-MM-DD.{json,md} (feature PR), at the repo root (local mode) or
the artifact staging directory (CI mode). The document contains:
- a verdict (any Blocker or unanswered Question ⇒ Request changes),
- an At a glance table with one row per finding, its severity and status markers and a link to the finding, so a reader sees the state of the PR without reading on,
- a Scope of this approval statement, the boundary that stops a review of one delta from reading as sign-off of everything beneath it,
- one section per finding graded Blocker / Question / Major (nits stay table rows). Each section carries the PR comment in a fenced block that can be copied and pasted into the PR as it is, written concisely for a senior developer (protocol.md §5), then the evidence anchored at its root cause (§17) and the observable check that proves it fixed (§18),
- (promotion mode) manifest verification: merge-base, release list, dependencies, tickets,
- (conditional) a cross-reference to the project's standing quality register, and a cross-repo twins table when a paired repo was reviewed for the same change,
- a mandatory Good in this delta section,
- an appendix ranking the
file:lineclusters a human reviewer should read first, and - an appendix giving the Standards basis, the published guidance each finding cites.
The layout is the same for every destination. The document reads as plain Markdown on GitHub,
Azure DevOps or disk, and scripts/render_confluence.py turns the same file into a Confluence
page with status lozenges and collapsed evidence. A destination changes where the review goes,
never what it says.
In local mode nothing is posted, committed, or sent unless the user asks. When they ask, Phase 7
posts through one of the routes in references/posting.md. In CI mode the
pipeline posts findings.json to the PR via scripts/post_review.py, on Azure DevOps or GitHub
(references/ci.md).
Modes: local and CI
Local mode (the default, an interactive session) behaves as the phases below describe: infer, echo the profile back, ask what stays ambiguous, ask the re-review framing, echo the grades.
CI mode, active when PR_REVIEW_MODE=ci is set in the environment or the invocation says
so, is headless. It must never ask anything. Every ask resolves to its safe default (protocol.md
§24): ambiguous promotion detection ⇒ feature mode, re-review framing ⇒ addendum + thread
updates, severity echo ⇒ skipped, and missing register/ticket-key/paired-repo ⇒ skip silently. Source
and target come from the pipeline's PR variables. Every inference made is recorded in the JSON's
scope block so the pipeline log is auditable.
Before starting
Read references/protocol.md, which holds the rules of engagement (§1 to §12),
the grading rules every platform shares (§13 to §16), and their extensions (§17 to §24: root cause,
verification criteria, SHA ancestry, debug variants, the adversarial pass, retractions, severity
negotiation, CI conduct). Also read references/template.md, the section
contract, plus references/output.md for the JSON schema. Load
exactly one platform pack in Phase 2, chosen by what Phase 0 detects:
references/platforms/android.md,
references/platforms/ios.md,
references/platforms/spring-boot.md,
references/platforms/react-nextjs.md, or
references/platforms/generic.md. In a polyglot repo, load
the pack that matches the files the delta touches.
Two non-negotiables shape everything else:
- Delta-only. Review
MERGE_BASE..SOURCE_HEADand nothing older. Pre-existing debt belongs in the project's register (or nowhere), not in a review of someone's PR. Every approval carries the written scope statement. - Personal verification. Subagent findings are leads. Re-verify every B/Q/M yourself against the pinned SHA before it ships. Agents have cited evidence that did not exist (protocol.md §6).
Phase 0: Setup, repo profile, mode
git fetch first, then establish the profile below. Infer what you can, ask only what stays
ambiguous, and echo the profile back before proceeding so a wrong inference is cheap to correct.
| What | How to infer it | If it stays unclear |
|---|---|---|
| Platform | first match wins: com.android.application/com.android.library in a Gradle build ⇒ Android · .xcworkspace/.xcodeproj/Package.swift ⇒ iOS · org.springframework.boot in a Gradle or Maven build ⇒ Spring Boot · next or react in a package.json dependency list ⇒ React/Next.js · otherwise the generic pack |
ask |
| Source & target | the user's ask, the PR, or the current branch vs its upstream | ask |
| Environment branches | intersect the remote branch list with the candidate vocabulary in protocol.md §9, and treat region/brand/tenant-suffixed variants as their base environment | ask before asserting promotion mode |
| Ticket key | the most frequent [A-Z][A-Z0-9]+-\d+ prefix across recent commit subjects |
skip ticket reconciliation |
| Quality register | glob for assessment/**/*.csv, audit/**/*.csv, **/recommended-tasks.csv, or an obvious findings table in docs/ |
skip that section and say so |
| Reviewing team / org | the git remote's organisation | ask, or omit attribution |
| PR host | SYSTEM_COLLECTIONURI or TF_BUILD ⇒ Azure DevOps · GITHUB_ACTIONS ⇒ GitHub · otherwise the origin URL: dev.azure.com or visualstudio.com ⇒ Azure DevOps, github.com ⇒ GitHub. Record it as scope.host |
ask before posting |
| Paired repo | only when the user names one, or an obvious sibling checkout exists next to this one | skip the twins section silently |
| Destinations | the PR host above is always one. A document destination comes from the user's ask: a Confluence URL, space or page named ⇒ Confluence. Otherwise the file at the repo root is the only document. Record them as scope.destinations |
local mode: ask once whether the document should also go to Confluence. CI mode: never ask, file only |
Then:
- Mode: first, local vs CI.
PR_REVIEW_MODE=ci(or a stated headless invocation) ⇒ CI conduct throughout (protocol.md §24): no questions, defaults everywhere, inferences recorded in the JSON scope block. Then promotion vs feature. Both endpoints environment branches ⇒ promotion mode (Phase 1 runs, full scoping statement). Anything else, or ambiguous in CI mode, ⇒ feature mode (skip Phase 1, simple scope line). - Pin
SOURCE_HEAD, computeMERGE_BASE, and capture delta stats (git diff --shortstat MERGE_BASE..SOURCE_HEAD, commit and file counts). If a ref name doesn't resolve, resolve it by raw SHA and record that (protocol.md §8). - Churn classification: if much of the delta is mechanical (formatter sweeps, regenerated
files), quantify it now with
git diff -w. Mechanical churn raises the evidence bar for every later claim (protocol.md §7). - Re-review check: a prior deliverable for this PR already exists (glob
pr-review-*.mdat the repo root, or, in CI, a priorfindings.jsonartifact or threads carrying apr-reviewfooter marker on the PR) and the head has moved ⇒ go to Phase R. - If the working tree is dirty with changes you didn't make, note it and work from
origin/refs.
Phase 1: Promotion manifest verification (promotion mode only)
Verify, in order (template.md for the output shape):
MERGE_BASE == TARGET_HEAD: the source strictly contains the target, so nothing on the target is lost and the PR diff is exactly what lands. Divergence here is itself a headline finding.- First-parent merge history vs the release list in the PR description, mergebacks verified content-empty against their first parent, and the count of commits pushed directly to the source branch.
- Third-party dependency additions and version changes. Check the manifests and lockfiles for
the stack in play (Gradle files and version catalogs,
Package.swift/Package.resolved,package-lock.json,go.sum,Gemfile.lock, and so on). - Ticket reconciliation against the detected ticket key: unique references vs the count claimed. Name example unticketed or mislabeled commits by SHA.
Phase 2: Area fan-out
Launch parallel review subagents over MERGE_BASE..SOURCE_HEAD, scaled to the delta: a large
release promotion warrants the full lens set, while a handful of files may need one agent or none
(review it inline). The lens list is a floor, not a ceiling. When the delta contains
something no default lens owns (a build-system rewrite, a migration, a vendored dependency), add
a lens for it rather than letting the nearest lens half-cover it. Default lenses, adapted to what
the change actually touches:
- Data boundary: services, DTOs, repositories, wire-contract changes, error handling, caches.
- State and control flow: state ownership, persisted flags, navigation/routing, resume paths.
- UI and shared components. A defect in a shared component is app-wide by definition, so weight it accordingly. Grade UI against the tiered design rules ([UI-STANDARDS]), citing the rule ID and source key. T1 breaches grade Blocker or Major by user impact. T2 breaches grade Nit unless the project opts the rule in.
- Security, privacy and logging: secrets, PII placement, authorisation, injection surfaces.
- Accessibility, tests and CI: semantics and labels, test quality, pipeline integrity. A test step that cannot fail is a Blocker: it falsely certifies everything else.
Run every lens agent on the reviewer-tier model (gpt-6-astra at high reasoning effort), as the toolkit's reviewer agents do. In CI mode no personal instructions choose it. Each agent's prompt must tell it to: read the loaded platform pack first, compare every claim
against the merge-base tree (git show MERGE_BASE:path) rather than just reading the new code,
obey the pragmatism guardrails (protocol.md §15), cite the standard per finding, identify the
causal site for each claim as distinct from its symptom sites (protocol.md §17), return
structured leads as file:line (causal) · claim · merge-base behaviour · proposed severity · evidence quotes · symptom sites, and close its report by naming what it did NOT cover as
explicit exclusions, so uncovered ground is visible instead of assumed reviewed. Give each agent a
budget of about 20 tool calls and tell it to stop and report when the budget runs out. A partial
report that marks unchecked ground Unverified beats none, and an agent with no budget can reach
its turn limit before it writes anything.
Phase 2a: Adversarial verification (second opinion)
Before the lead touches the leads, send them to adversarial agents whose job is to refute
them (protocol.md §21). Run them on the reviewer-tier model too. Batch about eight leads per agent, with a budget of about 15 tool calls. Each agent checks per lead: cited
file/line exists at the pinned SHA, merge-base half is true (git show MERGE_BASE:path), anchored
at the root cause rather than a symptom (§17), and not debug-only (§20). Where a runnable command settles
it (a compile, a test, a grep), run the command in the scratch checkout described below,
because a compile beats reading. Verdicts:
CONFIRMED / REFUTED (with counter-evidence) / WEAKENED. Every agent's final line must assert
"N of N verified". A batch that can't is re-dispatched. Drop REFUTED leads from the table but keep
the refutations in the run's notes. Skip this phase only when the delta was reviewed inline with
no fan-out.
Running a build or test
A build or test runs in a scratch checkout of SOURCE_HEAD that this session prepares, never in
the user's working tree:
git worktree add --detach <scratchpad>/pr-<id> SOURCE_HEAD.- Install dependencies there with the project's own command, such as
pod install,xcodebuild -resolvePackageDependenciesornpm ci. - Run the check yourself from the worktree. Hand a test run to
ios-verifieronly with absolute paths for-workspaceor-project,-derivedDataPathand-resultBundlePath.android-verifiercannot address another checkout, so run Gradle yourself. The verifier agents may not run git or installers, so never ask one to prepare the checkout. git worktree remove <scratchpad>/pr-<id>when the review is done. Record in the scope block which checks ran and which could not.
In CI mode the pipeline's checkout is already at SOURCE_HEAD, so skip steps 1 and 4.
Phase 3: Personal verification
For every surviving B, Q and M lead: open the file at the pinned SHA, confirm the line numbers,
the mechanism, the causal site, and the merge-base half of the claim with your own commands.
The adversarial pass thins the leads, but it does not replace this gate (protocol.md §6). Demote or
drop whatever doesn't survive. In churn-heavy deltas re-verify "delta-added" with git diff -w or
a merge-base grep. While verifying, write each finding's verify_fixed_when criterion
(protocol.md §18). You just proved the defect exists, so state the observable that would prove it
gone: a command, a file:line expectation, or a named test. For a Q, state the answer or evidence that
closes it. Deduplicate across lenses, merge findings that share a fix, then assign IDs by severity
and discovery order. IDs stay stable forever after (protocol.md §10).
Phase 3.5: Emit findings.json
Write the canonical output per references/output.md: scope block
(including CI-mode inferences), verdict, manifest results, findings with all fields populated,
genuinely_good list, register cross-references. Everything downstream reads this file: the rendered document,
the CI poster, the next re-review. The document never carries a finding the
JSON lacks or vice versa.
Phase 4: Register cross-reference (when a register exists)
For each register finding this delta touches, assign a verdict of Worsens / Partially addressed /
Unchanged / Exposure widened / Mixed, with an evidence pointer to a finding ID or file:line.
Untouched findings get no row. The framing sentence in the template covers them.
Phase 5: Twin check (when a paired repo was reviewed)
Look for a review of the equivalent change at the paired repo's root (pr-review-*.md). If
found, mark shared defects in the Finding cells and emit the twins table so each gets one
coordinated fix. If not found, skip silently. Never characterise the other repo's status without
having looked.
Phase 6: Render and deliver
Render the document from findings.json per references/template.md, sections in order.
Check before delivering:
- Verdict matches the register (any B or open Q ⇒ Request changes).
- Scope statement present and correct for the mode, and commit range and counts accurate.
- JSON↔document parity: every finding ID in the JSON appears in the document and vice versa, and in the At a glance table exactly once.
- Every PR-comment block equals the finding's
pr_commentexactly and is self-contained (survives being pasted alone into the PR). - Every B/Q/M has a
verify_fixed_whencriterion, and every finding names its causal site. - Any commit cited passes the ancestry check (protocol.md §19).
- Good in this delta section present. Credit is what keeps the harsh register credible.
- Appendix A ranked by risk.
- Local mode: both files at the repo root with the mode-correct name, untracked. CI mode: both in
the artifact staging directory, and the pipeline runs the poster (
references/ci.md).
Then (local mode) tell the user the verdict, the blockers in one line each, the proposed grades (protocol.md §23, severity is negotiable), and where the files are. Offer Phase 7 in one line.
Phase 7: Post (local mode, only when the user asks)
Post through a route in references/posting.md. Before anything is sent,
confirm the PR head still equals scope.source_head, run a dry run and show the user the bodies.
Cast no vote unless the user asks for one, because the vote lands under their name. Blockers,
Questions and Majors get anchored threads. Nits go only in the summary comment, so they add no
thread the author must resolve.
When scope.destinations includes Confluence, publish the document with
scripts/render_confluence.py as references/posting.md describes. A
republish first compares the live page with the version last published, because reviewers edit
these pages by hand.
Phase R: Re-review (prior review exists, head moved)
- Re-verify every prior Blocker at the new head first. The Blockers' status is the headline.
- Then every other open finding against its
verify_fixed_whencriterion. Every status,fixed | open | regressed | partial, citesfile:lineevidence at the new head (protocol.md §10, §18). - Review only
OLD_HEAD..NEW_HEAD(Phases 2 to 3.5 machinery, scaled down). Verify that claimed fixes actually fix, and that they don't regress a neighbouring path. - Ask which framing the user wants: an addendum (prior IDs untouched, new findings take the next free numbers) or a fresh rewrite at the new head with no re-review language. Both are legitimate. Don't guess (protocol.md §10). CI mode never asks: addendum, plus thread updates via the poster (resolved when criteria pass, reactivated on regression).
- Update
findings.jsonstatuses and the scope statement's commit range to the new head either way. A finding that turns out to have been wrong is retracted visibly, never silently edited (protocol.md §22).