Repository navigation
Conversation
askalf
requested review from
justaugustus and
spencerschrock
and removed request for
a team
September 14, 2026 19:59
|
This pull request has been marked stale because it has been open for 10 days with no activity |
Author
|
Still relevant. The branch is current with main with no conflicts, and DCO and CI are green at the current head. Ready for review whenever a maintainer has a moment. |
askalf
force-pushed
the
fix/code-review-prow-label-self-merge
branch
from
September 29, 2026 11:47
e78ccd9 to
ee8b468
Compare
detectCommitRevisionInfo checks for Prow before GitHub, so any merged pull request carrying an "lgtm" or "approved" label is classified as ReviewPlatformProw. getChangesets then only copied Reviews and Author for GitHub changesets, and codeApproved.approved() returned true for Prow before the review loop ran. A pull request opened, labelled and merged by the same account with no reviews therefore scored as reviewed, while the same pull request without the label was correctly caught by the reviewer != author comparison. Prow changesets are detected from the labels of a merged GitHub pull request, so the review data is already in hand. Populate Reviews and Author for them the same way as for GitHub, and let approved() run the existing comparison. Tide merges keep passing through the implicit MergedBy review when the merger is not the author. Gerrit, Phabricator and Piper are detected from commit messages with no review data attached and keep their existing behaviour. Fixes ossf#5206 Signed-off-by: askalf <263217947+askalf@users.noreply.github.com>
Adversarial verification of the prow self-merge fix rebuilt the boundary ledger from the diff and found reachable rows with no test behind them. checks/raw: the approved label (the second prow trigger alongside lgtm), an unsquashed prow merge and the reverse commit order for it, a prow changeset carrying an explicit review next to the implicit MergedBy one, a mixed prow + GitHub commit list, and an unknown-platform commit. probes/codeApproved: a prow changeset with no review data, one reviewed without an approval, and a bot-authored one, plus the gerrit, phabricator and piper arms of the switch the fix left in place. Two cases pass on the base commit by construction and are named controls: the piper changeset, which never reaches the changed predicate, and a labelled but unmerged pull request, which is not classified as prow. Signed-off-by: askalf <263217947+askalf@users.noreply.github.com>
One case pins that a self-merged Prow changeset keeps its author and reviews, and one that it is not approved. Both fail without the fix. Signed-off-by: askalf <263217947+askalf@users.noreply.github.com>
askalf
force-pushed
the
fix/code-review-prow-label-self-merge
branch
from
October 19, 2026 13:03
90c0686 to
9f4c627
Compare
askalf
had a problem deploying
to
integration-test
October 19, 2026 14:42 — with 
GitHub Actions
Failure
GitHub Actions
Failure
This branch had an error being deployed
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.
Fixes #5206
Summary
detectCommitRevisionInfotests for Prow before GitHub, so any merged pull request carrying a label namedlgtmorapprovedis classifiedReviewPlatformProw. Two independent places then discard the GitHub review data that is already in hand.getChangesets(checks/raw/code_review.go:164) only populatedReviewsandAuthorwhenrev.Platform == checker.ReviewPlatformGitHub, so a Prow-classified changeset carriedReviews: nil, Author: {Login: ""}.codeApproved.approved()(probes/codeApproved/impl.go:119) returnedtrueforReviewPlatformProwunconditionally, before thereview.State == "APPROVED" && review.Author.Login != c.Author.Loginloop ever ran.approved/lgtmand merged by the same account with zero reviews scores as reviewed; the identical PR without the label is correctly caught by the self-merge comparison. The label removes a check instead of adding information.Reviews/Authorfor Prow the same way as for GitHub, and dropReviewPlatformProwfrom the short-circuit inapproved(). Prow detection and changeset grouping are unchanged; Gerrit/Phabricator/Piper are unchanged.Tests
One case in
Test_getChangesets(a self-merged PR labelledlgtmkeeps its author and reviews) and one inTestProbeCodeApproved(a self-merged Prow changeset is not approved). Both fail on main and pass with the fix.gofmt,go vetandgo teston the touched packages are clean.Genuine Prow projects keep their scores. In a Tide merge the merger is the Prow bot, not the author, so the implicit merge approval still counts. Only author == merger with no other approving review changes.
AI assistance: this bug was found and the fix and tests were drafted with AI tooling in my workflow; the tests and checks above were run as described. I'm responsible for the change and will handle review feedback.