Repository navigation
[No QA] Skip AI reviews when they already ran for the PR head commit - #98016
Conversation
neil-marcellini
changed the title
neil-marcellini
changed the title
Resolves the claude-review.yml conflict by keeping the skip gate and the gh-based path filter alongside main's rules_directory input, and points both workflows at the toolkit commit that carries the merged scripts. shouldSkipReview.sh builds the status context by appending the PR number, so recordReviewComplete.sh is now handed that step's context output instead of the bare reviewer name. codex-review.yml passes rules_directory because the toolkit's default path does not exist in this repo.
gh pr view --json files runs a GraphQL query capped at 100 files and does not paginate, so on a PR with more changed files the paths that decide whether the code or docs review runs could fall outside the list and the review would be skipped with no error. The REST files endpoint under gh api --paginate returns the full list. Also writes the list to RUNNER_TEMP instead of the checked-out working tree.
This comment has been minimized.
This comment has been minimized.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 477efcfa00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcb7674660
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Julesssss
left a comment
Julesssss
left a comment
There was a problem hiding this comment.
LGTM, one NAB comment: https://github.com/Expensify/App/pull/98016/changes#r4149660883
|
@Julesssss I updated this based on your non-blocking comment, but it was actually kind of blocking because you did not approve it last time 😄 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.5.2-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |
3 participants
Explanation of Change
(Neil's AI agent)
Today, marking a PR ready for review runs the Claude reviewer again on the exact same commit it already reviewed, reposting identical findings. This gates the reviewer on whether it has already completed for the PR's current head commit.
The Codex review workflow is left unchanged because it is disabled in this repository and is being deleted in #102445.
The marker is a commit status on the head SHA —
ai-review-completed/claude/pr-<number>— which is the only durable record keyed by commit. The PR number is part of the name because two PRs can share a head SHA. 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). The workflow gains 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".
Three knock-on changes in
claude-review.yml, all required by the new comment trigger:@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. A comment-triggered run never skips, since asking for a review by comment is a deliberate request to repeat it.authorizejob. Anyone can comment@claude review, so the concurrency group (withcancel-in-progress) now sits on thereviewjob, which only runs once the commenter is authorized. An unauthorized comment can't cancel a running review.dorny/paths-filterwith a paginatedgh api .../pulls/<number>/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.
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)
pull_request_targetandissue_commentworkflows always run from the default branch, so this can't be tested on a PR in this repository before it merges. I tested it on a fork (neil-marcellini/App) instead, set up like this:mainis App'smainwith this PR merged in atded2c3a. Later commits on this PR restorecodex-review.ymland move the authorization check into its own job; the fork run below predates that second change.run: echo stub, keeping theiridandif, so the steps around them behave as in production without an API key. "Post code review results" and "Setup Node" are removed because the stub needs neither.runs-onisubuntu-latestbecause the fork has no Blacksmith runners. All other workflows are deleted.OS_BOTIFY_TOKENis set to a dummy value. The fork owner passes the authorization check asOWNER, and the team-membership lookup only logs a warning when the token can't read the org.Steps, run on two draft PRs in the fork: #1 changes a file under
src/, and #2 only changes adocs/**/*.mdfile.@claude review.ai-review-completed/claude/pr-1status with statesuccesson the head commit, linking to the run.ai-review-completed/claude/pr-1 already completed for <sha>, skipping reviewand that every step after "Check for an existing review of this commit" is skipped.@claude reviewon that already-reviewed commit.Review requested by comment, running it even if ...and runs the review./codex-review.@claude review.src/. This checks that the new path filter picks the same reviewer asdorny/paths-filterdid.Results (all passed)
@claude reviewon75a3f4175a3f415796811@claude reviewon5796811/codex-reviewcommentStep 6, ready for review on an already-reviewed commit:
Steps 8 and 10, a new commit and a manual re-request, both run the full review:
Step 14, docs-only PR:
Commit statuses at the end:
On
5796811, the status first pointed to run 36468303309 and was replaced by the step 10 run, because GitHub keeps one status per context per commit.Offline tests
N/A — CI configuration only.
QA Steps
No QA — this changes GitHub Actions workflows only and has no staging or production surface.