Skip to content

maintainer-approval uses historical APPROVED reviews instead of latest review state per reviewer #5322

@fallintoplace

Description

@fallintoplace

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

  1. Open a PR that requires approval from a maintainer or a path owner
  2. Have reviewer X submit APPROVED
  3. Have the same reviewer X later submit CHANGES_REQUESTED
  4. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions