Describe the issue
The maintainer-approval workflow appears to treat review approval as "has this user ever left an APPROVED review on the PR" rather than "is this user's latest review state currently APPROVED".
That means an older approval can still satisfy the workflow even if the same reviewer later changes their review to CHANGES_REQUESTED.
Why this looks incorrect
The workflow currently checks raw review history in three places:
- maintainer approval path uses
reviews.find(...) with state === "APPROVED"
- maintainer-authored PR path uses
reviews.some(...) with state === "APPROVED"
- per-path approval builds
approverLogins from all reviews with state === "APPROVED"
It does not first reduce reviews to the latest state per reviewer.
Repro
- Open a PR that requires approval from a maintainer or a path owner
- Have reviewer
X submit APPROVED
- Have the same reviewer
X later submit CHANGES_REQUESTED
- Wait for
maintainer-approval to re-run on the review event
Expected behavior
Only the latest review state per reviewer should count.
If reviewer X later changes their review to CHANGES_REQUESTED, their earlier approval should no longer satisfy the workflow.
Actual behavior
An older APPROVED review can still satisfy the workflow because the script checks for any matching approval record in the full review list.
Relevant context
This behavior seems to date back to the initial workflow in #4912 and was preserved when per-path approval was added in #4918.
#4918 also added a test that CHANGES_REQUESTED does not count as approval, but that test only covers a PR with a single CHANGES_REQUESTED review. It does not cover the more important case where the same reviewer first approves and later requests changes.
Suggested fix direction
Collapse the review list to the latest review state per normalized login before evaluating:
- maintainer approval
- maintainer-authored PR approval
- per-path owner approval
That would make the workflow reflect current approval state rather than historical approval existence.
Describe the issue
The
maintainer-approvalworkflow appears to treat review approval as "has this user ever left anAPPROVEDreview on the PR" rather than "is this user's latest review state currentlyAPPROVED".That means an older approval can still satisfy the workflow even if the same reviewer later changes their review to
CHANGES_REQUESTED.Why this looks incorrect
The workflow currently checks raw review history in three places:
reviews.find(...)withstate === "APPROVED"reviews.some(...)withstate === "APPROVED"approverLoginsfrom all reviews withstate === "APPROVED"It does not first reduce reviews to the latest state per reviewer.
Repro
XsubmitAPPROVEDXlater submitCHANGES_REQUESTEDmaintainer-approvalto re-run on the review eventExpected behavior
Only the latest review state per reviewer should count.
If reviewer
Xlater changes their review toCHANGES_REQUESTED, their earlier approval should no longer satisfy the workflow.Actual behavior
An older
APPROVEDreview can still satisfy the workflow because the script checks for any matching approval record in the full review list.Relevant context
This behavior seems to date back to the initial workflow in
#4912and was preserved when per-path approval was added in#4918.#4918also added a test thatCHANGES_REQUESTEDdoes not count as approval, but that test only covers a PR with a singleCHANGES_REQUESTEDreview. It does not cover the more important case where the same reviewer first approves and later requests changes.Suggested fix direction
Collapse the review list to the latest review state per normalized login before evaluating:
That would make the workflow reflect current approval state rather than historical approval existence.