|
| 1 | +# Review Multiple Pull Requests in ABCA |
| 2 | + |
| 3 | +## Persona |
| 4 | + |
| 5 | +Hold the same bar as [`review_pr`](./review_pr.md): a **Principal AWS Solutions Architect** who |
| 6 | +is direct, specific, justifies every concern with a concrete risk and a `file:line`, distinguishes |
| 7 | +blocking issues from nits, and never rubber-stamps. This command orchestrates that single-PR |
| 8 | +review across a *set* of PRs — it does **not** restate the review process. Each PR is reviewed by |
| 9 | +the full `review_pr` workflow, so the review bar stays single-sourced. |
| 10 | + |
| 11 | +This is **ABCA (Autonomous Background Coding Agents on AWS)** — a self-hosted platform for |
| 12 | +background coding agents. Bounded blast radius applies to the *reviewer* too: this command can |
| 13 | +fan out many concurrent sub-reviews, so it is explicit about scope, cost, and what it skips. |
| 14 | + |
| 15 | +**How the fan-out runs — and why it keeps the caller's context fresh.** Stage 1 (scouting the |
| 16 | +work-list) runs inline in the main context; it is small and cheap. Stage 2 (the per-PR reviews) |
| 17 | +runs as a **`Workflow`** — a deterministic multi-agent orchestration — *not* as prose-directed |
| 18 | +subagent dispatch. Each PR's heavy work (a large diff, the three nested `review_pr` review agents, |
| 19 | +the full review body) is confined to a Workflow-spawned subagent's context; only a **small |
| 20 | +structured result per PR** (number, verdict, one-line rationale, review URL) returns to the caller. |
| 21 | +Making the fan-out a `Workflow` means that isolation is a property of the control flow, not |
| 22 | +something an executor must remember to do — so a large queue can't blow the main window on diff |
| 23 | +bodies and agent transcripts. (Caveat: per-PR *summaries* still accrete linearly in the caller, so |
| 24 | +"fresh" ≠ "constant" — a 200-PR run still returns 200 rows.) |
| 25 | + |
| 26 | +## Execution context — interactive operator only |
| 27 | + |
| 28 | +**This is an interactive Claude Code / Cursor command, run by a human operator. It is NOT — and |
| 29 | +cannot be — executed by ABCA's headless agent runtime.** The distinction matters because Stage 2's |
| 30 | +core tool, `Workflow`, is unavailable to headless tasks by construction: |
| 31 | + |
| 32 | +- The runtime hard-blocks `Workflow` via a **deny-list**, which is the load-bearing mechanism. |
| 33 | + `agent/src/runner.py` passes `disallowed_tools=_DISALLOWED_TOOLS` (`= ["Workflow", "Task", "Agent", |
| 34 | + "Monitor", "SendMessage", "CronCreate", "CronDelete", "CronList"]`) to `ClaudeAgentOptions` — the |
| 35 | + incident-driven hardening after a repo-less task launched a detached background `Workflow` and |
| 36 | + finalized on a placeholder. This is the *only* thing that removes the tool from the surface: the |
| 37 | + runtime runs under `permission_mode="bypassPermissions"`, so a tool that is merely **absent from |
| 38 | + the `allowed_tools` auto-approve list is auto-*allowed*, not blocked** (runner.py is emphatic: |
| 39 | + "`disallowed_tools` is the only hard lock … do not rely on this allow-list, nor on Cedar, to remove |
| 40 | + a tool from the surface" — Cedar PreToolUse default-permits on no-match). So the guarantee rests on |
| 41 | + `disallowed_tools`, not on `Workflow` being off the allow-list. Interactive Claude Code / Cursor |
| 42 | + sessions are not gated this way, so `Workflow` is available there — which is where this command runs. |
| 43 | +- The runtime loads project `.claude/` config only when a repo is cloned: |
| 44 | + `_resolve_setting_sources(config)` returns `["project"]` when `config.repo_url` is set and `[]` |
| 45 | + for a repo-less task (defense-in-depth that also stops a stray on-disk skill from being reachable). |
| 46 | + So this command's stub **is** discoverable to a repo-cloning task — which is exactly why the |
| 47 | + `disallowed_tools` block, not mere obscurity, is the guarantee: even though the file is on disk and |
| 48 | + reachable on that path, the tool it calls is off the surface. |
| 49 | +- The read-only review workflow (`coding/pr-review-v1`) cannot be steered into `/review_prs`: it is |
| 50 | + bound to a fixed `pr_review` system prompt (`PR_REVIEW_WORKFLOW` in `agent/src/prompts/pr_review.py`, |
| 51 | + selected by `get_system_prompt` in `agent/src/prompts/__init__.py`) that drives the agent through |
| 52 | + fixed steps — there is no "invoke a slash command" path, and `Workflow` is deny-listed regardless. |
| 53 | + (The `coding/pr-review-v1 → pr_review` entry in `agent/src/workflow/loader.py` is the Cedar |
| 54 | + *principal* audit tag, not the prompt binding.) |
| 55 | + |
| 56 | +In short: operators run `/review_prs`; the ABCA runtime never does, and structurally cannot. |
| 57 | + |
| 58 | +## Arguments |
| 59 | + |
| 60 | +Optional. |
| 61 | + |
| 62 | +- **No arguments** — review every PR that currently requests your review (the default query in |
| 63 | + Stage 1). |
| 64 | +- **Explicit list** — e.g. `#616 #612 590` — review exactly those PRs, skipping Stage-1 derivation. |
| 65 | +- **Natural-language filter** — e.g. "all open non-fork PRs with green CI", "everything P1 and |
| 66 | + above requesting my review" — translate to the appropriate `gh` search and gates in Stage 1. |
| 67 | + |
| 68 | +## Stage 1: Build the work-list |
| 69 | + |
| 70 | +Derive the candidate PRs. The default set is *ready-to-review, not mine, not yet approved by me, |
| 71 | +requesting my review, excluding forks*: |
| 72 | + |
| 73 | +```sh |
| 74 | +gh pr list --repo <owner>/<repo> \ |
| 75 | + --search "is:open is:pr draft:false -author:@me -review:approved review-requested:@me" \ |
| 76 | + --json number,isCrossRepository,headRepositoryOwner \ |
| 77 | + --jq '.[] | select(.isCrossRepository == false) | .number' |
| 78 | +``` |
| 79 | + |
| 80 | +- **Fork exclusion is client-side.** GitHub's issue/PR search has **no fork qualifier** — `is:fork` |
| 81 | + is a *repository*-search qualifier and is silently ignored in a PR search (it returns a superset, |
| 82 | + not an error). Filter on `isCrossRepository == false` (a PR whose head branch lives in a fork is |
| 83 | + cross-repository); `headRepositoryOwner.login` (fetched above) tells you *which* fork — surface it |
| 84 | + when announcing a dropped cross-repo PR (e.g. `#42 — dropped: fork PR from acme-labs`). |
| 85 | +- **Optional CI-green gate (only when the user asks for "green" / "passing" / "CI clean").** Keep a |
| 86 | + PR iff its checks are all terminal **and** all passed — i.e. `gh pr checks <n>` exits `0`. Do |
| 87 | + **not** pass `--json` to `gh pr checks` in the gate: with `--json` it always exits `0` and the |
| 88 | + exit code stops meaning anything (use `--json state,bucket` only for *reporting* the state). The |
| 89 | + gate is opt-in, not the default: a red or still-running PR is often exactly the one that needs a |
| 90 | + "request changes", so gating it out by default would hide the work. |
| 91 | +- **Announce what you drop.** For every PR removed by the CI gate (or any other filter), say so and |
| 92 | + why. Silent truncation reads as "reviewed everything" when it wasn't. |
| 93 | +- **Empty list** — if nothing matches, say so and stop. Never invent PR numbers. |
| 94 | +- **Transient by design** — submitting a review clears `review-requested:@me`, so re-running the |
| 95 | + default query later returns a *different* set. That is correct for a worklist; do not treat it as |
| 96 | + a bug or try to re-review already-reviewed PRs. |
| 97 | + |
| 98 | +Present the resolved list (with the reason any candidate was dropped) before fanning out. |
| 99 | + |
| 100 | +## Stage 2: Fan out via `Workflow` — one full `review_pr` per PR |
| 101 | + |
| 102 | +Run the per-PR reviews as a **`Workflow`**, not as inline subagent dispatch. The `Workflow` tool |
| 103 | +requires explicit opt-in — reaching for it here is warranted because the user invoked a batch |
| 104 | +review, and it is what keeps the diffs and nested-agent transcripts out of the caller's context |
| 105 | +(see the Persona note). For a **single** PR, skip the Workflow and just run `review_pr` directly — |
| 106 | +orchestration overhead isn't worth it for `N == 1`. |
| 107 | + |
| 108 | +**Pre-stage shared git state ONCE, inline, before invoking the Workflow** — worktree/branch |
| 109 | +creation mutates shared `.git` state and must not race across concurrent agents: |
| 110 | + |
| 111 | +1. `git fetch origin main` (single fetch; reason about base freshness against the current tip). |
| 112 | +2. Per PR: fetch its head with a **forced** refspec — `git fetch origin +pull/<n>/head:pr<n>head` |
| 113 | + (the leading `+` overwrites an existing `pr<n>head`, so a re-run after an interrupted or |
| 114 | + uncleaned run doesn't fail on the stale local branch). Add a worktree on that ref, dump |
| 115 | + `gh pr diff <n>` to a scratch file, and capture the head SHA (`gh pr view <n> --json |
| 116 | + headRefOid`) and base branch (`--json baseRefName`). Use a run-scoped scratch dir (e.g. a |
| 117 | + per-invocation temp path) so concurrent or repeated runs don't collide on worktree paths. |
| 118 | + |
| 119 | +Assemble one descriptor per PR — `{ number, worktreePath, diffPath, headSha, baseRef }` — and pass |
| 120 | +the **array** as the Workflow's `args`. Then invoke `Workflow` with a self-contained script that: |
| 121 | + |
| 122 | +- reads the descriptors from `args` and fans out **one agent per PR concurrently** (`parallel()` |
| 123 | + over the descriptors, or `pipeline()` if you add a downstream stage); the tool caps real |
| 124 | + concurrency, so pass all PRs — they queue and drain automatically. |
| 125 | +- has each agent execute the full [`review_pr`](./review_pr.md) workflow for its PR and **submit** |
| 126 | + the review (inline suggestions + an Approve / Comment / Request-changes decision) via the Stage-3 |
| 127 | + mechanism. The agent prompt must include that PR's `worktreePath`, `diffPath`, `headSha` (the |
| 128 | + review `commit_id`), and `baseRef` — and **flag stacked PRs** whose `baseRef != main` (the diff |
| 129 | + is against the base branch, not `main`, and merge order matters). |
| 130 | +- carries the standing `review_pr` governance rule into each agent prompt: the gate is an |
| 131 | + **approved backing issue** (see |
| 132 | + [ADR-003](../../docs/decisions/ADR-003-contribution-governance.md)); a `pr/*` branch name is a |
| 133 | + de-facto-waived nit, **not** a blocker. Each agent must **read existing review threads first** |
| 134 | + and independently verify any prior blocking claim against current code before finalizing. |
| 135 | +- returns a **compact `schema`-validated result per PR** — `{ number, verdict, rationale, |
| 136 | + reviewUrl }` — and nothing larger. The `schema` is what guarantees only the summary crosses back |
| 137 | + to the caller; without it an agent's free-text return could drag a full review body into the main |
| 138 | + context, defeating the freshness goal. The Workflow's return value is the array of these results. |
| 139 | + |
| 140 | +Sketch (illustrative — adapt names/paths): |
| 141 | + |
| 142 | +```js |
| 143 | +export const meta = { |
| 144 | + name: 'review-prs', |
| 145 | + description: 'Review a set of PRs, one review_pr per PR, return compact verdicts', |
| 146 | + phases: [{ title: 'Review' }], |
| 147 | +} |
| 148 | +const RESULT = { type: 'object', required: ['number', 'verdict', 'rationale', 'reviewUrl'], properties: { |
| 149 | + number: { type: 'number' }, |
| 150 | + verdict: { enum: ['APPROVE', 'COMMENT', 'REQUEST_CHANGES'] }, |
| 151 | + rationale: { type: 'string' }, |
| 152 | + reviewUrl: { type: 'string' }, |
| 153 | +} } |
| 154 | +const results = await parallel(args.map((pr) => () => |
| 155 | + agent( |
| 156 | + `Run the /review_pr workflow for PR #${pr.number} and SUBMIT the review with inline ` + |
| 157 | + `suggestions. Worktree: ${pr.worktreePath}. Diff: ${pr.diffPath}. commit_id: ${pr.headSha}. ` + |
| 158 | + `Base: ${pr.baseRef}${pr.baseRef !== 'main' ? ' (STACKED — diff is vs the base branch)' : ''}. ` + |
| 159 | + `Governance: gate is an approved backing issue; a pr/* branch is a waived nit. Read existing ` + |
| 160 | + `review threads first and verify prior blocking claims. Submit via gh api (see Stage 3).`, |
| 161 | + { label: `review:#${pr.number}`, phase: 'Review', schema: RESULT }, |
| 162 | + ))) |
| 163 | +return results.filter(Boolean) |
| 164 | +``` |
| 165 | + |
| 166 | +Scale the depth to the request inside each agent: a routine queue is one agent per PR running |
| 167 | +`review_pr` once; "audit these thoroughly" lets each agent invoke the deeper Stage-3 agent fan-out |
| 168 | +that `review_pr` itself prescribes. |
| 169 | + |
| 170 | +## Stage 3: Submit & report |
| 171 | + |
| 172 | +- **Submission mechanism** — the GitHub MCP `pull_request_review_write` tool may lack the required |
| 173 | + token scope (`Resource not accessible by personal access token`). When it does, submit via the |
| 174 | + `gh` CLI instead: |
| 175 | + |
| 176 | + ```sh |
| 177 | + gh api --method POST repos/<owner>/<repo>/pulls/<n>/reviews --input <review>.json |
| 178 | + ``` |
| 179 | + |
| 180 | + where `<review>.json` has `commit_id`, `event` (`APPROVE` | `COMMENT` | `REQUEST_CHANGES`), |
| 181 | + `body`, and `comments[]`. Inline comments must anchor to a line present on the **RIGHT** side of |
| 182 | + the diff, or the call returns `422` — move any such finding into the review body instead. |
| 183 | + Each Workflow agent submits its own review this way from inside its context — the review bodies |
| 184 | + never return to the caller (only the compact `schema` result does). |
| 185 | +- **Clean up — unconditionally.** Remove every per-PR worktree (`git worktree remove --force`) and |
| 186 | + `pr<n>head` branch created in Stage 2, **whether the Workflow succeeded, threw, or the run was |
| 187 | + interrupted** — not only on the happy path. A partial or failed run still leaves worktrees and |
| 188 | + branches in the shared `.git`, so treat teardown as a `finally`, not a trailing step: bounded |
| 189 | + blast radius applies to the reviewer too. (The forced refspec in Stage 2 also lets a later run |
| 190 | + self-heal if a crash skipped teardown entirely.) |
| 191 | +- **Report** — assemble a summary table from the Workflow's returned results: |
| 192 | + **PR · verdict · one-line rationale · review URL**. Include the count reviewed and the count |
| 193 | + (and reasons) dropped in Stage 1. |
| 194 | + |
| 195 | +## Notes |
| 196 | + |
| 197 | +- This command is a thin orchestrator over `review_pr`. It intentionally does not duplicate the |
| 198 | + review stages, agent-invocation requirements, or output structure — refinements to the review bar |
| 199 | + land in [`review_pr`](./review_pr.md) and are inherited here automatically. |
| 200 | +- The Stage-2 fan-out uses the **`Workflow`** tool so context-freshness is enforced by the control |
| 201 | + flow rather than left to an executor following prose. This command *uses* `Workflow`; it does not |
| 202 | + modify orchestration infrastructure. Git pre-staging (worktrees/branches) stays inline before the |
| 203 | + Workflow because it mutates shared `.git` state and must not race across concurrent agents. |
0 commit comments