Update

[ghstack-poisoned]

Author: jeanschmidt

greenlight/src/greenlight/github_client.py

@@ -308,6 +308,7 @@ def fingerprint_pr(
 REVIEW_EVENT_COMMENT = "COMMENT"
 _REVIEW_EVENTS: frozenset[str] = frozenset({REVIEW_EVENT_APPROVE, REVIEW_EVENT_REQUEST_CHANGES, REVIEW_EVENT_COMMENT})
 _REVIEW_STATE_APPROVED = "APPROVED"
+_REVIEW_STATE_COMMENTED = "COMMENTED"
 
 
 def get_pr(client: VerdictClient, repo: str, number: int) -> VerdictPR:
@@ -365,31 +366,34 @@ def upsert_issue_comment(
     pr.create_issue_comment(body)
 
 
-def _iter_greenlight_approvals(pr: VerdictPR, bot_login: str) -> Iterator[_VerdictReview]:
-    """Yield each live APPROVED review authored by ``bot_login`` -- greenlight's own approvals.
+def _iter_greenlight_reviews(pr: VerdictPR, bot_login: str) -> Iterator[_VerdictReview]:
+    """Yield every review authored by ``bot_login`` (any state) -- greenlight's own reviews.
 
-    The login is passed in (the greenlight GitHub App's ``<slug>[bot]`` account) rather than read
-    via ``get_user``, which an App installation token cannot call. Null-user, empty-login, and
-    non-APPROVED reviews are skipped; the login match is case-insensitive.
+    The login is passed in (the greenlight App's ``<slug>[bot]`` account) since an App token
+    cannot call ``get_user``. Null-user and empty-login reviews are skipped; match is case-insensitive.
     """
     target = bot_login.lower()
     for review in pr.get_reviews():
         user = review.user
-        if user is None or not user.login:
-            continue
-        if user.login.lower() == target and review.state == _REVIEW_STATE_APPROVED:
+        if user is not None and user.login and user.login.lower() == target:
             yield review
 
 
 def has_live_greenlight_approval(pr: VerdictPR, *, bot_login: str) -> bool:
-    """Return True iff ``bot_login`` has at least one live (undismissed) APPROVED review on ``pr``."""
-    return next(_iter_greenlight_approvals(pr, bot_login), None) is not None
+    """Return True iff greenlight's latest non-COMMENTED review on ``pr`` is APPROVED.
+
+    Mirrors trymerge's approver rule (latest non-COMMENTED state per login wins), so a stale
+    APPROVED behind a newer DISMISSED never reads as approved. Assumes ``get_reviews()`` is oldest-first.
+    """
+    states = [r.state for r in _iter_greenlight_reviews(pr, bot_login) if r.state != _REVIEW_STATE_COMMENTED]
+    return bool(states) and states[-1] == _REVIEW_STATE_APPROVED
 
 
 def dismiss_prior_greenlight_approvals(pr: VerdictPR, *, bot_login: str, message: str) -> list[int]:
-    """Dismiss every prior APPROVED review authored by ``bot_login`` (see ``_iter_greenlight_approvals``)."""
+    """Dismiss every prior APPROVED review authored by ``bot_login`` (see ``_iter_greenlight_reviews``)."""
     dismissed: list[int] = []
-    for review in _iter_greenlight_approvals(pr, bot_login):
-        review.dismiss(message)
-        dismissed.append(review.id)
+    for review in _iter_greenlight_reviews(pr, bot_login):
+        if review.state == _REVIEW_STATE_APPROVED:
+            review.dismiss(message)
+            dismissed.append(review.id)
     return dismissed

greenlight/tests/test_github_client.py

@@ -1365,7 +1365,7 @@ def test_has_live_greenlight_approval_false_for_commented_and_dismissed():
     assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is False
 
 
-def test_has_live_greenlight_approval_true_when_any_own_approved_among_many():
+def test_has_live_greenlight_approval_true_when_latest_non_commented_approved():
     reviews = [
         _FakeVerdictReview(1, _FakeActor("alice"), "APPROVED"),
         _FakeVerdictReview(2, _FakeActor("greenlight-app[bot]"), "COMMENTED"),
@@ -1374,3 +1374,52 @@ def test_has_live_greenlight_approval_true_when_any_own_approved_among_many():
     pr = _FakeVerdictPR(reviews=reviews)
 
     assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is True
+
+
+def test_has_live_greenlight_approval_false_when_approved_then_dismissed():
+    reviews = [
+        _FakeVerdictReview(1, _FakeActor("greenlight-app[bot]"), "APPROVED"),
+        _FakeVerdictReview(2, _FakeActor("greenlight-app[bot]"), "DISMISSED"),
+    ]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    # trymerge collapses greenlight to its latest non-COMMENTED state (DISMISSED), so it would
+    # not count greenlight as an approver; an older APPROVED must not read as a live approval.
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is False
+
+
+def test_has_live_greenlight_approval_true_when_dismissed_then_approved():
+    reviews = [
+        _FakeVerdictReview(1, _FakeActor("greenlight-app[bot]"), "DISMISSED"),
+        _FakeVerdictReview(2, _FakeActor("greenlight-app[bot]"), "APPROVED"),
+    ]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is True
+
+
+def test_has_live_greenlight_approval_true_when_commented_after_approved():
+    reviews = [
+        _FakeVerdictReview(1, _FakeActor("greenlight-app[bot]"), "APPROVED"),
+        _FakeVerdictReview(2, _FakeActor("greenlight-app[bot]"), "COMMENTED"),
+    ]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is True
+
+
+def test_has_live_greenlight_approval_false_when_latest_changes_requested():
+    reviews = [
+        _FakeVerdictReview(1, _FakeActor("greenlight-app[bot]"), "APPROVED"),
+        _FakeVerdictReview(2, _FakeActor("greenlight-app[bot]"), "CHANGES_REQUESTED"),
+    ]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is False
+
+
+def test_has_live_greenlight_approval_false_when_only_other_author_approved():
+    reviews = [_FakeVerdictReview(1, _FakeActor("alice"), "APPROVED")]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is False