Decide the exit code from the payload the digest printed - #529
Merged
Conversation
The code came from the last liveness poll and the digest from a later fetch, so a review landing between the two printed `review_on_head=yes` and returned 40 or 50. A reader resolves that disagreement by believing the code, which drops the review it was just shown, and the same gap at the other exit printed coverage and returned PENDING. Both exits now leave the loop by breaking, and one payload decides the digest and the code together. Coverage wins over a stalled request and over the timeout, because a landed review is the outcome the whole wait is for, however the loop happened to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in scripts/pr_review.py wait where the exit code could be decided from an older poll while the printed digest reflected a newer fetch, leading to contradictory “covered vs stalled/pending” signals for the same run.
Changes:
- Updates
digest()to optionally render from a caller-provided PR payload so output can be tied to a specific read. - Refactors
waitto break out of the loop and then use a single final fetch for digest rendering and exit-code decisions. - Adds regression tests covering reviews landing during the final read near both the stalled and timeout exits.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/pr_review.py | Refactors digest() and wait to reduce/avoid inconsistencies between printed digest and exit codes. |
| scripts/test_pr_review.py | Adds tests to reproduce and lock down the race outcomes described in the PR. |
The exit code was taken off one payload while the stall behind it came from a reading made earlier in the loop, so a request picked up since still reported as picked up by nothing. That is the disagreement this branch exists to close, one layer further in, and the digest re-read the timeline on its own besides, spending two REST calls to reach two possibly different answers. `stall_of` derives the stall from the payload it is reported beside, the exit path passes both to the digest, and a covered head or an absent request settles it with no call at all. An answer now outranks a stall, since the reviewer saying something is worth more to the reader than the reviewer saying nothing, and a wait that broke on a stall the re-read clears reports as pending, which is what it is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes a race the review on the promotion pull request #528 found. That pull request cannot carry the fix, since its head is
developanddeveloptakes changes only through a feature branch, so this is the fix and #528 picks it up when this merges.The race
waitdecided its exit code from the lastQ_LIVEpoll and printed a digest built from a laterQ_FULLfetch. A review landing between the two makes the digest printreview_on_head=yeswhile the code returns 40 or 50, and the same gap at the other exit prints coverage and returns 30.The disagreement is not cosmetic. A reader that has both resolves it by believing the exit code, so the review it was just shown is the thing that gets dropped, and a covered pull request reads as stalled. On this repository's own loops the window is small and the traffic is exactly what fills it: a re-request lands a review within a few minutes of the poll that missed it.
The fix
Both exits leave the loop by breaking, and one payload then decides the digest and the code together. Coverage wins over a stalled request and over the timeout, because a landed review is the outcome the whole wait exists for, however the loop happened to end.
Verification
Two cases, one per exit, each watched failing against a build that decides from the stale poll: they print
review_on_head=yesand return 50 and 30 respectively.scripts/test_pr_review.py61 pass,prose_lint.pyclean over the diff,repo_gate.pyclean.🤖 Generated with Claude Code