mirror of
https://github.com/systemd/systemd.git
synced 2026-06-30 19:57:29 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
committed by
Daan De Meyer
parent
0bb9142bad
commit
18e659da6f
92
.github/workflows/claude-review.yml
vendored
92
.github/workflows/claude-review.yml
vendored
@@ -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/<sha>/` 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/<sha>/` 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
|
||||
|
||||
Reference in New Issue
Block a user