From 18e659da6f14d27d1f039068d391f9a793bfb5da Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 2 Jun 2026 12:33:01 +0000 Subject: [PATCH] ci: review PRs through per-lens subagents with PR-specific lenses Change the review fan-out from one subagent per commit to one subagent per lens, each reviewing every commit through a single perspective. Four base lenses (correctness/memory safety, lifetimes/concurrency, security, API/style) always run; the orchestrator skims the diff and adds 1-3 PR-specific lenses (e.g. a DNS protocol lens for resolved changes). A single generalist reviewer tended to converge on one finding on large diffs; focused lenses dig deeper. Commits are reviewed in chronological order via a commit-order.txt manifest, since the SHA-named worktree dirs don't sort chronologically. Co-developed-by: Claude Opus 4.8 --- .github/workflows/claude-review.yml | 92 +++++++++++++++++++++++------ 1 file changed, 73 insertions(+), 19 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index b364c0adff5..e53852436a8 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -167,11 +167,15 @@ jobs: PR_NUMBER: ${{ needs.setup.outputs.pr_number }} run: | git fetch origin "pull/${PR_NUMBER}/head" - for sha in $(git log --reverse --format=%H HEAD..FETCH_HEAD); do + # Chronological commit order, oldest first. The worktree dirs are + # SHA-named, so listing them doesn't preserve order — reviewers read + # this manifest to review commits in the order they were authored. + git log --reverse --format=%H HEAD..FETCH_HEAD > commit-order.txt + while read -r sha; do git worktree add "worktrees/$sha" "$sha" git -C "worktrees/$sha" diff HEAD~..HEAD > "worktrees/$sha/commit.patch" git -C "worktrees/$sha" log -1 --format='%B' HEAD > "worktrees/$sha/commit-message.txt" - done + done < commit-order.txt - name: Install sandbox dependencies run: | @@ -258,33 +262,81 @@ jobs: Review this pull request. All required context has been pre-fetched into local files. - ## Phase 1: Review commits + ## Phase 1: Review the PR through multiple lenses - First, list the directories in `worktrees/` and read `review-schema.json`. - Then, spawn exactly one review subagent per worktree directory, all in a - single message so they run concurrently. Do NOT batch or group multiple - commits into a single agent. Do NOT read any other files before spawning — - the subagents will do that themselves. + First, read `review-schema.json` and `commit-order.txt` in the repository + root. `commit-order.txt` lists the commit SHAs in chronological order, + oldest first — this is the order in which commits must be reviewed. Each + worktree at `worktrees//` contains the full source tree checked out at + that commit, plus `commit.patch` (the diff) and `commit-message.txt` (the + commit message). - Each worktree at `worktrees//` contains the full source tree checked - out at that commit, plus `commit.patch` (the diff) and `commit-message.txt` - (the commit message). + You will review the PR through a set of lenses. Every review uses these + four base lenses: + 1. Correctness & memory safety — logic errors, edge cases, mishandled + error paths, use-after-free, double-free, memory/fd leaks, NULL + dereferences, uninitialized reads, integer/buffer overflow, off-by-one. + 2. Resource lifetimes & concurrency — ownership and reference counting, + cleanup and unref paths, reentrancy, event-loop and async behavior, + signal safety, assert/assert_return contracts, and state-machine + invariants. + 3. Security & robustness — handling of untrusted input, parsing, bounds + checks, resource exhaustion / DoS, privilege boundaries, path and + symlink handling, format strings, TOCTOU races. + 4. API design, style & maintainability — interface design, consistency + with existing systemd idioms and CODING_STYLE, naming, error-propagation + conventions, dead code, and needless complexity. - Each reviewer reviews design, code quality, style, potential bugs, and - security implications. + ### Add PR-specific lenses + + Before spawning, briefly inspect what this PR actually changes so you can + add domain-specific lenses. Read each worktree's `commit-message.txt` and + skim the changed file paths and hunk headers in each `commit.patch`, in + chronological order — just enough to understand which subsystems and + problem domains are touched. Do + NOT perform the review yourself or read the full source; that is the + subagents' job. + + Based on that, add 1 to 3 EXTRA lenses targeting the specific concerns of + this PR. Each extra lens needs a short name and a one-sentence description + of what it must check. For example: + - a PR changing DNS logic in resolved → a "DNS protocol conformance" lens + (RFC compliance, packet/label parsing, name/length limits, DNSSEC, + EDNS, caching/TTL semantics). + - a PR changing D-Bus/Varlink interfaces → an "IPC interface + compatibility" lens (backward compatibility, method/signal signatures, + polkit authorization). + - a PR changing unit file parsing → a "unit file format & compatibility" + lens (directive semantics, ordering, backward compatibility). + - a PR touching sd-boot/UEFI → a "boot/firmware safety" lens. + Only add extra lenses genuinely warranted by the diff — for a small or + generic change the four base lenses may be enough, in which case add none. + + ### Spawn the reviewers + + Then spawn exactly one review subagent per lens (the base lenses plus any + extra lenses you derived), all in a single message so they run + concurrently. Each subagent reviews EVERY commit (every worktree + directory), but only through the perspective of its assigned lens. Each subagent must be spawned with `model: "opus"`. Each subagent prompt must include: + - The name and full description of its assigned lens, with an instruction + to report ONLY findings that fall under that lens and to ignore issues + belonging to the other lenses — those are covered by other reviewers. - Instructions to read `pr-context.json` in the repository root for additional context. - The contents of `review-schema.json` (paste it into each prompt so the agent doesn't have to read it separately). - - The worktree path. - - Instructions to read `commit-message.txt` and `commit.patch` in the - worktree for the commit message and diff. - - Instructions to verify every `line` and `start_line` value - against the hunk ranges in `commit.patch` before returning. + - The list of every worktree path in chronological order (oldest commit + first, matching `commit-order.txt`), with an instruction to review the + commits in that order and read each one's `commit-message.txt` and + `commit.patch`. + - Instructions that each finding's `commit` field must be the SHA of the + worktree the finding belongs to, and that every `line` and `start_line` + value must be verified against the hunk ranges in that commit's + `commit.patch` before returning. - Instructions to return ONLY a raw JSON array of findings. No markdown, no explanation, no code fences — just the JSON array. If there are no findings, return `[]`. @@ -311,7 +363,9 @@ jobs: Then: 1. Collect all issues. Merge duplicates across agents (same file, same - problem, lines within 3 of each other). + problem, lines within 3 of each other). Because the lenses overlap, + the same issue is often reported by more than one lens — collapse + these into a single comment, keeping the clearest wording. 2. Drop any issue whose suggestion is to add a code comment, docstring, or documentation (e.g. "add a comment explaining…", "missing docstring", "document this function", "would benefit from a