Add shared scripts to skip AI reviews already completed for a commit - #97
Add shared scripts to skip AI reviews already completed for a commit#97neil-marcellini wants to merge 6 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![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: f80a5ac948
ℹ️ 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".
Commit statuses hang off a repository commit rather than a PR, so two PRs sharing a head SHA could read each other's completion marker and skip a review of a different diff. shouldSkipReview.sh now appends the PR number to the context it was given and publishes the result as a "context" output for recordReviewComplete.sh to record against. The combined status endpoint pages at 30 statuses, so add --paginate to keep the marker visible on commits carrying many other statuses.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a4fa52bc5
ℹ️ 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".
An issue_comment run is someone asking for a review by hand, so it is a deliberate request to review a commit the gate has most likely already marked complete - the one case where repeating the review is the point. The README claimed this already worked; now it does. Also drop the workflow walkthrough from the README in favour of a high-level description, and document the token scopes the gate needs.
|
Sorry for the delay on this one, reviewing now. |
|
|
||
| Run the gate first and skip the reviewer when its `skip` output is `true`. Once the review succeeds, record completion against the gate's `head_sha` and `context` outputs rather than re-deriving either at the end — `head_sha` is the SHA captured before the review started, so a push mid-review leaves the marker on the commit that was actually reviewed and the next event triggers a fresh one. Record the status whether or not the review found anything, so re-running on an unchanged commit never reposts the same findings. | ||
|
|
||
| An `issue_comment` run never skips, so a comment trigger (`@claude review`, `/codex-review`) is the way to force a re-review of a commit already marked complete. |
There was a problem hiding this comment.
will this work for codex too?
2 participants
Details
(Neil's AI agent)
When a PR is reviewed by an AI reviewer while it is a draft and then marked ready for review, the reviewer runs again on the exact same commit and reposts the same findings. This adds the shared plumbing to skip that duplicate run; the consumer workflows in Auth, Web-Expensify, and App are updated in the linked PRs below.
The marker is a commit status on the PR head SHA — the only durable record that is keyed by commit. Reactions and comments are not tied to a commit, and workflow-run history is not either (comment-triggered runs record
github.shaas the base branch head, not the PR head).Two new scripts on the toolkit's
PATH:shouldSkipReview.sh <PR_NUMBER> <CONTEXT>— resolves the head SHA and writeshead_shaandskip=true|falseto$GITHUB_OUTPUT.skipistruewhen asuccessstatus with that context already exists on the SHA.recordReviewComplete.sh <HEAD_SHA> <CONTEXT> [DESCRIPTION]— sets that status, linking back to the workflow run.The README tells callers to name the context
ai-review-completed/<reviewer>rather thanai-review/<reviewer>. The status lands beside the review job in the PR's checks list, and a neighbouring green row calledai-review/claudereads like a second review verdict instead of a record that one already happened.Two design points, both documented in the README:
@claude review,/codex-review) bypass the gate and are the way to force a re-review.Related Issues
https://github.com/Expensify/Expensify/issues/669298
Manual Tests
(Neil's AI agent)
shellcheckpasses for both new scripts (./scripts/shellCheck.sh); the existing toolkit scripts have no unit tests.The gate itself was verified end to end on a throwaway PR, #98. It carries this branch plus a
testSkipGate.ymlworkflow that stands in for a reviewer: it callsshouldSkipReview.sh, echoes the decision as a run annotation, and callsrecordReviewComplete.shunder the samesteps.code-review.outcome == 'success'condition the consumer workflows use. No real reviewer runs, so what is under test is only the two scripts here.Two deliberate choices in that harness:
pull_requestrather thanpull_request_target, so GitHub reads the workflow definition from the branch head.pull_request_targetandissue_commentboth read it from the base branch, which is why the linked consumer PRs cannot exercise their own changes and have to be verified after this merges.ai-review-completed/test, so it cannot collide with a real reviewer's status on any repo.Results, all on PR 98:
opened(as a draft)skip=false, review runs, status recordedskip=false,ai-review-completed/test= success ond30e911skip=true, review skippedalready completed for d30e911, skipping reviewready_for_review, same SHAskip=true, review skippedskip=true— this is the duplicate-review case the PR exists to fixsynchronize(new commit pushed)skip=false, review runs againskip=false, new status on1945767In the skip cases both the stand-in review step and the record step report
skipped, confirming the gate suppresses the re-record as well as the review.PR 98 is closed without merging; the harness workflow only ever existed on its branch.
Linked PRs
This must merge first; the consumer PRs pin the toolkit to this branch's commit and need re-pinning to the merged SHA.
@claude reviewtrigger App was missing, since a skip needs a manual override)