[HOLD Github-Actions 97] [No QA] Skip AI reviews when they already ran for the PR head commit - #98016
Draft
neil-marcellini wants to merge 2 commits into
Draft
[HOLD Github-Actions 97] [No QA] Skip AI reviews when they already ran for the PR head commit#98016neil-marcellini wants to merge 2 commits into
neil-marcellini wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
(Neil's AI agent)
Today, marking a PR ready for review runs both AI reviewers again on the exact same commit they already reviewed, reposting identical findings. This gates each reviewer on whether it has already completed for the PR's current head commit.
The marker is a commit status on the head SHA —
ai-review-completed/claudeandai-review-completed/codex— which is the only durable record keyed by commit. Reactions and comments are not tied to a commit, and workflow-run history is not either (comment-triggered runs recordgithub.shaas the base branch head, not the PR head). Both workflows gain two steps around the existing ones:skip=trueif asuccessstatus with that context is already on it. Every step that costs money or posts to the PR is gated onskip != 'true'.The status is named
ai-review-completed/*rather thanai-review/*because it sits directly beside the review job in the PR's checks list. A neighbouring green row calledai-review/claudereads like a second review verdict;ai-review-completed/claudereads as the record of a past run, which is what it is.The status is recorded whether or not the review found anything — re-running on an unchanged commit would only repost identical comments, so "already completed" is the right skip condition rather than "already passed".
Two knock-on changes in
claude-review.yml, both required by the skip:@claude reviewcomment trigger. Auth and Web-Expensify already have it; App did not. A skip with no manual override would leave no way to force a re-review, so this is a prerequisite rather than a nice-to-have. It also closes the on-demand inconsistency called out in the issue. Codex already had/codex-review.dorny/paths-filterwith agh pr view --json filesfilter.paths-filterreads thepull_requestevent payload and has no PR context on anissue_commentevent, so it cannot survive the new trigger without being handed an explicit base and ref (which in turn needs a full fetch of the PR head thatpull_request_targetdeliberately does not do). One API call gives the same file list for both event types.The shared scripts live in the toolkit action so Auth and Web-Expensify use the same implementation: Expensify/GitHub-Actions#97. This PR must not merge until that one does — the toolkit refs here point at that PR's branch commit and need re-pinning to the merged SHA first.
Sibling PRs: https://github.com/Expensify/Auth/pull/23558, https://github.com/Expensify/Web-Expensify/pull/55193
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/669298
PROPOSAL: N/A
Tests
(Neil's AI agent)
CI-only change with no product surface, so there is nothing to test in the app and no unit tests apply.
actionlintpasses on both workflows (only the pre-existing unknown-blacksmith-*-label warning), andoxfmtreports both formatted.Not yet run end to end — the toolkit PR is not merged. Planned, on this PR itself:
@claude review; wait for the review to finish.ai-review-completed/claudestatus appears on the head commit in the checks list, linking to the run.reviewandcodex_reviewshould run and skip their review steps (Codex's own status is recorded when/codex-reviewruns, so verify it independently in step 6).@claude reviewon the already-reviewed commit — the review should run, confirming the manual override still works./codex-reviewtwice on the same commit — the second should skip.docs/**/*.md, confirm the docs reviewer still runs (verifying the replacement path filter).Offline tests
N/A — CI configuration only.
QA Steps
No QA — this changes GitHub Actions workflows only and has no staging or production surface.