Skip to content

bug(process): an unaddressed /review finding is invisible to the PR queue, so ejected PRs get re-validated from scratch on the next train #2815

Description

@vybe

What happened

Today's merge train validated nine PRs and merged five. Three of the four held back failed the same way, and it is worth naming once rather than in three separate PR threads that three authors read separately.

The shape: a /review finding from 2026-09-14 sat unanswered, and the PR read as Ready again anyway.

Both ejected PRs were re-validated from scratch at full lane depth before anyone noticed the findings were the same ones. That is roughly an hour of agent time per PR, spent twice, to rediscover what was already written down.

Why it happens

reviewDecision only moves when a reviewer submits a new review. A /review posted as a comment never touches it, so a PR carrying unaddressed findings is byte-identical, in every field the queue view reads, to one nobody has looked at. /open-pull-requests showed all three under ✅ Ready this morning, correctly by its own rules.

The merge train catches it, but only by paying full validation cost first.

Suggested change

Cheapest fix that would have worked: when /review or /validate-pr returns NEEDS-FIX or EJECT on a PR, the finding is posted and the PR gets a label — status-needs-fix or similar — that /open-pull-requests sorts out of ✅ Ready and /merge-train Phase 0 treats like a hold. The author's next push removes it.

That makes "a finding is outstanding" a queue-visible fact rather than something a reader has to reconstruct by reading comment threads in date order against the branch head timestamp.

Alternative, if a label is too heavy: have Phase 0's hold grep also compare the newest /review comment's timestamp against the branch head date, which is the derivation the train already does by hand for CHANGES_REQUESTED in Step 3.0 (responded). Extending that to comment-shaped reviews is a small change to an existing rule.

Acceptance criteria

  • A PR with an unaddressed NEEDS-FIX finding does not appear under ✅ Ready in /open-pull-requests.
  • /merge-train Phase 0 excludes it without spending Phase 2 validation on it.
  • A push by the author clears the state automatically — no human bookkeeping step.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions