feat: two-pass Claude review (find, then filter) to cut false positives - behind feature flag by MounirDhahri · Pull Request #110 · artsy/duchamp

MounirDhahri · 2026-07-31T07:15:57Z

Summary

Since moving to Opus 5 (#108), reviews have gotten chattier — more findings per PR, including more that don't hold up under scrutiny. This splits the single review pass into two:

Find (new, read-only): explores the diff/codebase and lists every candidate issue without filtering for confidence.
Filter (existing step, prompt extended): reads the find pass's raw list, verifies each claim against the diff/codebase, drops what doesn't hold up or doesn't matter, and posts only what survives — same posting behavior as today.

This change is behind a feature flag active only for repos who opt-in, currently, that's Volt and Eigen

Output when I asked claude to review this compared to multiple repos

Test plan

yarn test scripts/build-review-prompt.test.ts — 24 tests, including new coverage for buildFinderPrompt, pass1FindingsPath, and buildPrompt's pass-1-findings prepending (with/without config, with/without a custom prompt: override)
yarn test (full suite) — 143 tests pass
Workflow YAML validated with a syntax parse
Watch the first real PR review under this workflow to confirm the two-step timing/output wiring behaves as expected in CI (I don't have a way to dry-run claude-code-action locally)

Generated by Claude Code

The reviewer's single pass has to both find issues and judge which ones are worth reporting at once, which produces noisier reviews under models that follow "report everything, be thorough" instructions more literally. Splits the review job into two claude-code-action steps: a read-only "find" pass with no GitHub-posting tools in its allowlist (so it cannot comment on the PR regardless of prompt behavior) that lists every candidate issue without filtering, then the existing "filter" pass verifies each claim against the diff/codebase and posts only what holds up. Findings pass between the two via a file in the runner's temp dir, so no cleanup step is needed and nothing shows up in git status. No change needed for repos that call claude-review.yml@main - this is entirely internal to the reusable workflow.

- Grant pass 1 --add-dir "${{ runner.temp }}" so it can actually reach the findings file: Claude Code's file-tool sandbox is scoped to the working directory independent of --allowedTools, and $RUNNER_TEMP sits outside it. Without this, pass 1's Write would be refused and the flow would silently fall back to single-pass with nothing in the logs saying so. - Make pass 1 best-effort: continue-on-error + a 20-minute step cap, so a slow or failed finder degrades to today's single-pass review instead of taking pass 2 (the only step that can post) down with it. - Log when no pass-1 findings file is found, instead of silently proceeding. - Treat pass 1's literal "NONE" output as no findings, not as a findings section to prepend. - Fix a test cleanup bug: assigning `undefined` to process.env.RUNNER_TEMP coerces to the string "undefined" instead of unsetting it. - Reword "read-only" to the claim that's actually true and enforced: pass 1 cannot post to GitHub (no allowlisted tool reaches it), not that it can't write anywhere - Write is granted, scoped by instruction to the findings file only.

github-actions · 2026-07-31T07:41:29Z

Detected new or modified files in .github/workflows/. Please ensure you update the relevant documentation alongside these workflow changes.

So future Claude Code sessions run by Mounir automatically add him as Co-authored-by on commits in this repo, without misattributing other engineers' sessions. Co-authored-by: MounirDhahri <soussouheros@gmail.com>

This reverts commit f524293.

Flips the default back to today's single-pass review (legacy), and moves the two-pass find+filter flow behind a new opt-in workflow input, compare-two-pass-review (default false), set the same way callers already set model/timeout-minutes. When the flag is off (default): behavior is unchanged from production - one review comment, one model call. When the flag is on: the legacy review still runs and posts as normal, and the find+filter pass also runs, posting a second, separately-labeled comment ("Single-pass review (baseline)" vs "Two-pass review (candidate)") on the same PR - so the two approaches can be compared side by side before deciding whether to make two-pass the default. Roughly 3x model calls while enabled. Added scripts/build-review-prompt.ts --mode=legacy, which builds the plain review prompt and never reads a pass-1 findings file - so the baseline stays uncontaminated by the comparison run even when both execute in the same job.

claude · 2026-07-31T14:49:08Z

            await cleanupPreviousAIReviews({ github, context, core });

-      - name: Run Claude Code Review
+      - name: Run Claude Code Review (legacy - single-pass)


The legacy step has no timeout-minutes, so it can eat most of the job's 45-minute cap before pass 1 and pass 2 even start.

All three passes run sequentially in this one job (timeout-minutes: ${{ inputs.timeout-minutes }}, line 164). Pass 1 is capped at 20 minutes. If legacy takes 20 and pass 1 takes its full 20, pass 2 has 5 minutes and gets killed by the job cap. Unlike pass 1, pass 2 has no continue-on-error, so the job goes red and the 🅱️ comment never posts.

docs/actions.md:214 says the flag "roughly triples ... wall-clock time", but the default only went from 30 to 45 (1.5x). Either cap the legacy step (timeout-minutes: 15) so the budget is bounded, or raise the default further when the comparison is on.

claude · 2026-07-31T14:49:15Z

+        uses: anthropics/claude-code-action@v1
+        with:
+          anthropic_api_key: ${{ secrets.anthropic-api-key }}
+          claude_args: '--model ${{ inputs.model }} --add-dir "${{ runner.temp }}" --allowedTools "Read,Glob,Grep,Bash(git diff:*),Bash(git log:*),Bash(git show:*),Write"'


Write is unscoped, so the only thing keeping pass 1 out of the checkout is prompt text (FINDER_PROMPT: "Do not modify any files").

One concrete path if it ignores that: pass 1 writes .claude-review.yml, then the next step (Build review prompt (pass 2 - filter), line 288) calls loadRepoConfig(), which reads path.join(process.cwd(), ".claude-review.yml") — and buildPrompt returns repoConfig.prompt verbatim when a full prompt: key is set (build-review-prompt.ts:223). Pass 2's entire review prompt gets replaced.

Low likelihood, cheap to close: either scope the write to the temp dir in --allowedTools, or add a step between the two passes that fails on git status --porcelain output.

claude · 2026-07-31T14:49:23Z

+const main = (): void => {
+  const isFinderMode = process.argv.includes("--mode=finder")
+  const isLegacyMode = process.argv.includes("--mode=legacy")
+


A typo'd flag silently degrades instead of failing. --mode=finderr matches neither branch, falls through to the pass-2 path, finds no findings file, logs a warning, and emits an ordinary single-pass prompt. The workflow step still succeeds, so the only signal is a console.warn buried in the step log — which is the same warning you get from a legitimately-failed pass 1.

main is also not exported, so none of the 150 new test lines cover the mode dispatch or the findings-file read; they all test the pure builders. Extracting resolveMode(argv) (throwing on an unrecognized --mode= value) would make both problems go away at once.

claude · 2026-07-31T14:50:15Z

Summary

Adds an opt-in compare-two-pass-review flag to claude-review.yml. When on, the job runs two extra Claude steps alongside the existing review: a "find" pass that dumps every candidate issue to a file in RUNNER_TEMP, and a "filter" pass that reads those findings, verifies them, and posts a separately-labeled 🅱️ comment next to the 🅰️ baseline. build-review-prompt.ts gains --mode=finder / --mode=legacy dispatch and a withPass1Findings prepend step.

The default-off gating is done well. --mode=legacy provably ignores the findings file, so the baseline everyone gets today cannot drift, and pass 1 genuinely has no GitHub-posting tool in its --allowedTools — the comments claiming that are accurate, not aspirational.

Issues Found

🟡 The 45-minute job cap does not fit three sequential passes — .github/workflows/claude-review.yml:216

All passes share one job. Pass 1 is capped at 20 minutes; the legacy step is uncapped. Legacy at 20 + pass 1 at 20 leaves pass 2 five minutes, and pass 2 has no continue-on-error, so the job goes red and 🅱️ never posts. docs/actions.md:214 says the flag "roughly triples ... wall-clock time" while the default went 30 → 45 (1.5x). Cap the legacy step, or raise the default when the comparison is on. Inline comment has details.

🟢 Pass 1's Write is unscoped — .github/workflows/claude-review.yml:272

Only prompt text keeps pass 1 out of the checkout. If it strays, one path is real: a write to .claude-review.yml is picked up by the very next step's loadRepoConfig(), and a full prompt: key replaces pass 2's entire prompt (build-review-prompt.ts:223). Scope the write path, or gate pass 2 on a clean git status --porcelain.

🟢 A typo'd --mode= value silently produces a single-pass prompt — scripts/build-review-prompt.ts:253

--mode=finderr matches neither branch and falls through to the pass-2 path, warning about a missing findings file and emitting an ordinary prompt. The step still exits 0. main is not exported, so the mode dispatch and the findings-file read are the only new logic with no test coverage. Extracting resolveMode(argv) that throws on an unknown mode fixes both.

🟢 docs/actions.md:214 is one 8-sentence bullet. Split the mechanics, the safety property, and the cost warning into separate bullets. The cost warning is the part a reader most needs to catch, and right now it is the last clause of a wall of text.

Areas Reviewed

Architecture & Design. Reusing one script with a mode flag over a second script is the right call. appendConfigSections keeps focus areas and ignore paths applied to both prompts, and the finder deliberately skips the prompt: override so pass 1 stays generic. Both choices are covered by tests.

Security. Pass 1's findings are model-generated text that flows into GITHUB_OUTPUT via a heredoc and then into pass 2's prompt. The heredoc delimiter is timestamp-based, so a collision is not a practical concern, and the check job already gates the whole workflow on the PR author having write access, which bounds who can influence the diff pass 1 reads. No new exposure.

Testing. 149 new lines, and they cover the parts worth covering: the empty / whitespace / NONE short-circuits, ordering of the prepend relative to the review prompt, the custom-prompt interaction in both directions, and RUNNER_TEMP fallback with proper env restore in afterEach. Only gap is main (above).

Bugs & Edge Cases. RUNNER_TEMP is consistent across steps in a job, so the path baked into FINDER_PROMPT at import time matches what the pass-2 build step reads. The inputs.compare-two-pass-review && '...' || '' guard evaluates to an empty string when off, so the 🅰️ header instruction stays out of the default path.

Questions for Author

The Cleanup previous AI review comments step runs once, before the legacy review, and both passes post after it. That works today. Is the plan to keep it that way once a winner is picked, or would cleanup need to become pass-aware if the two reviews ever move into parallel jobs?

MounirDhahri · 2026-07-31T14:52:47Z

Merging this since it's gated behind a feature flag to try out for a week

@main

Trial: point run-claude-review.yml at the two-pass-review branch of duchamp and turn on compare-two-pass-review, so PRs here get both the standard single-pass review and a second, separately-labeled two-pass (find + filter) review for side-by-side comparison. Temporary - once artsy/duchamp#110 merges, revert the `uses:` ref to @main. Co-authored-by: Mounir Dhahri <noreply@anthropic.com>