Repository navigation
Hold follow-up reviews until the PR head settles - #177
Merged
Merged
Conversation
A PR the bot has never reviewed is still reviewed on the first tick that sees it. Once a review exists, the next one waits until the current head SHA has been observed unchanged for REVIEWER_FOLLOWUP_SETTLE_SECONDS (default 300, 0 disables, re-requested reviews bypass it), so a burst of pushes coalesces into one review of the final head instead of an agy call per push that the post-flight staleness guard then discards. Head stability is measured from the daemon's own first sighting of each (PR, head SHA), recorded in head_first_seen.json under REVIEWER_STATE: commit and push dates are rewritten by rebases, amends and force-pushes, so they cannot answer "how long has this head been stable". The file is recreated when absent or corrupt, written temp-file + mv at mode 0600, and pruned each tick to the live open-PR heads with a 30-day TTL floor.
Both branches bumped the pinned assertion count and rewrote the same reviewer.sh architecture bullet. Keeps both: 694 assertions (633 + 17 for the posted-trace flag + 44 for the settle window), and one bullet naming both the settle window and REVIEWER_POST_REVIEW_TRACE.
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
1 participant
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.
On an actively-worked PR the author pushes every 1–3 minutes, and the daemon starts a ~2 minute review on each new head. Some get thrown away by the existing post-flight staleness guard (quota already spent), and the author receives a high volume of reviews. Observed live: 13 commits produced 7 reviews in 45 minutes, plus 2 discarded runs.
REVIEWER_FOLLOWUP_SETTLE_SECONDS(default 300,0disables)."Observed unchanged" is measured from the daemon's own first sighting of
(PR, head SHA), recorded in$REVIEWER_STATE/head_first_seen.json— never from a commit or push date, since rebases, amends and force-pushes rewrite those arbitrarily. A push changes the SHA, so the timer resets by construction and a burst of pushes coalesces into one review of the final head.The gate runs before the attempt budget and before any GitHub side effect, so a held PR cannot spend
REVIEWER_MAX_ATTEMPTSand starve the PRs behind it. Dry-run and render-only ticks are exempt and record nothing. The pre-existing post-flight guard is untouched — this is complementary: the settle window stops a doomed run from starting, the guard catches what still slips through.State is pruned two ways: an end-of-tick prune to the live open-PR heads (inside the same
ONLY_PR/DRY_RUN/RENDER_PROMPT_ONLYguard as the existing worktree prune, so single-PR runs cannot evict other PRs' timers), plus a 30-day TTL on write as a backstop.Deviation worth reviewing
An explicitly re-requested review bypasses the wait. This was not planned — an existing fixture went red, because a re-request is by definition a follow-up on a head the daemon may have only just seen. Making a human who clicks "re-request review" wait 5 minutes is a regression, and re-requests already bypass the reviewed-SHA skip one branch above.
Verification
677 fixture assertions, green (633 before). Covers: fresh PR reviewed on first sighting; unsettled follow-up skipped before the CI gate with no post; settled follow-up reviewed;
=0disables; a new push does not inherit the previous head's timer; re-request bypass. Time is seeded relative todate +%srather than slept, matching the existing backoff fixtures.Not verified: no live run against a real repo — everything is fixture-level. The
head_first_seen.jsonread-modify-write is not atomic; the daemon holdsflockfor the whole tick, so this only matters if that invariant breaks.Note: conflicts with #178 on the pinned
EXPECTED_ASSERTIONSline — both branches bump it from 633. Trivial to resolve, but guaranteed for whichever merges second.