Skip to content

fix(pr-review): anchor requested-changes expiry on the blocker, not our own sweep event#5471

Merged
matthewevans merged 2 commits into
mainfrom
ship/pr-review-expiry-anchor
Jul 10, 2026
Merged

fix(pr-review): anchor requested-changes expiry on the blocker, not our own sweep event#5471
matthewevans merged 2 commits into
mainfrom
ship/pr-review-expiry-anchor

Conversation

@matthewevans

Copy link
Copy Markdown
Member

requested_changes_expiry_state took blocker_timestamp from
local_current_event — the newest event for the head. Every sweep appends a
fresh blocked row (event_id hashes the timestamp, so it is never a dedup
no-op), so the loop reset its own clock on each pass. blocker_age was
permanently ~0 and warning_due could never fire for any PR the loop had ever
blocked: warn/close were dead code.

Anchor on the blocker instead of on our observation of it. Prefer GitHub's
formal CHANGES_REQUESTED on the current head (a newer review correctly restarts
the window); fall back to the earliest local block for that head, never the
latest.

Routing already treated review_blocked / reviewed_request_changes as
blocking while a second inline set did not. Extract is_block_event() as the
single authority so the expiry anchor and the routing decision cannot drift.

On the live event log this surfaces five PRs whose expiry was suppressed
(#4132, #4510, #4628, #4662, #4805). #4512/#4514 correctly stay silent — they
already carry a warning and are counting down to close.

Two of the new tests fail on the old anchor and pass on the new one.

…ur own sweep event

`requested_changes_expiry_state` took `blocker_timestamp` from
`local_current_event` — the newest event for the head. Every sweep appends a
fresh `blocked` row (`event_id` hashes the timestamp, so it is never a dedup
no-op), so the loop reset its own clock on each pass. `blocker_age` was
permanently ~0 and `warning_due` could never fire for any PR the loop had ever
blocked: warn/close were dead code.

Anchor on the blocker instead of on our observation of it. Prefer GitHub's
formal CHANGES_REQUESTED on the current head (a newer review correctly restarts
the window); fall back to the earliest local block for that head, never the
latest.

Routing already treated `review_blocked` / `reviewed_request_changes` as
blocking while a second inline set did not. Extract `is_block_event()` as the
single authority so the expiry anchor and the routing decision cannot drift.

On the live event log this surfaces five PRs whose expiry was suppressed
(#4132, #4510, #4628, #4662, #4805). #4512/#4514 correctly stay silent — they
already carry a warning and are counting down to close.

Two of the new tests fail on the old anchor and pass on the new one.
@matthewevans matthewevans enabled auto-merge July 10, 2026 01:24

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the PR review automation script to anchor the requested-changes expiry clock on the earliest recorded local blocking event (or formal GitHub review) for a given PR head, rather than the most recent observation. This prevents repeated sweep runs from resetting the timer and disabling stale warnings or closures. It introduces helper functions to identify blocking events and retrieve the first block event per PR head, integrates these into the review context and packet creation, and adds comprehensive unit tests to verify the new behavior. No review comments were provided, so there is no feedback to evaluate.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

…rns about

Same root cause as the expiry anchor: state was derived from the LATEST event on
the head, so a later event erased an earlier one.

`active` came from `local_block`, which inspects only the newest event. Once the
stale-changes warning is recorded it becomes the newest event, so a head blocked
only in the local log — no formal CHANGES_REQUESTED, e.g. #4805 whose review sits
on an older commit — flipped `active` to False the moment it was warned. Its
warning was orphaned and `close_due` could never fire.

"Has this head ever been blocked?" is a property of the log's history, not of its
last row. Consult the first block recorded for the head (already plumbed for the
anchor fix) instead of the newest event.

The new test fails on the old `active` and passes on the new one.
@matthewevans matthewevans added this pull request to the merge queue Jul 10, 2026
Merged via the queue into main with commit f94c049 Jul 10, 2026
13 checks passed
@matthewevans matthewevans deleted the ship/pr-review-expiry-anchor branch July 10, 2026 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant