Update

[ghstack-poisoned]

Author: jeanschmidt

greenlight/src/greenlight/github_client.py

@@ -32,6 +32,7 @@
         _PRComment,
         _PRReview,
         _RepoClient,
+        _VerdictReview,
     )
 
 
@@ -364,20 +365,31 @@ def upsert_issue_comment(
     pr.create_issue_comment(body)
 
 
-def dismiss_prior_greenlight_approvals(pr: VerdictPR, *, bot_login: str, message: str) -> list[int]:
-    """Dismiss every prior APPROVED review authored by ``bot_login``.
+def _iter_greenlight_approvals(pr: VerdictPR, bot_login: str) -> Iterator[_VerdictReview]:
+    """Yield each live APPROVED review authored by ``bot_login`` -- greenlight's own approvals.
 
-    The login is passed in (the greenlight GitHub App's ``<slug>[bot]`` account) rather
-    than read via ``get_user``, which is not available on an App installation token; only
-    that account's own approvals are ever dismissed.
+    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.
     """
     target = bot_login.lower()
-    dismissed: list[int] = []
     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:
-            review.dismiss(message)
-            dismissed.append(review.id)
+            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
+
+
+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``)."""
+    dismissed: list[int] = []
+    for review in _iter_greenlight_approvals(pr, bot_login):
+        review.dismiss(message)
+        dismissed.append(review.id)
     return dismissed

greenlight/src/greenlight/verdict.py

@@ -12,8 +12,9 @@
 updates GitHub with a defanged copy of the message. Both LAND and NO_LAND upsert one
 canonical verdict comment -- edited in place across runs, found by a hidden marker and
 restricted to greenlight's own account (``bot_login``); LAND additionally posts an approving
-review, and NO_LAND additionally dismisses greenlight's own prior approval (matched by
-``bot_login``). The row is authoritative, so the comment upsert is best-effort on every path (a
+review unless greenlight already holds a live approval on the PR, and NO_LAND additionally
+dismisses greenlight's own prior approval (both matched by ``bot_login``). The row is
+authoritative, so the comment upsert is best-effort on every path (a
 failed write is logged and swallowed); the LAND approving review and the NO_LAND dismissal are the
 merge gate and stay load-bearing -- they raise on failure. MARKER statuses (CANCELLED / FAILED /
 AI_REVIEW_STARTED) always emit the row and, when a ``bot_login`` and token are both present,
@@ -303,8 +304,11 @@ def _run_full(
     job_url = request.agent_job_url or request.eval_job_url
     body = comment_format.verdict_body(status, reason, message, job_url, request.run_id)
     if status == STATUS_LAND:
-        github_client.post_review(pr, event=github_client.REVIEW_EVENT_APPROVE, body=_LAND_REVIEW_BODY)
-        logger.info("approved %s#%d", request.repo, request.pr_number)
+        if github_client.has_live_greenlight_approval(pr, bot_login=request.bot_login):
+            logger.info("already approved %s#%d; skipping re-approval", request.repo, request.pr_number)
+        else:
+            github_client.post_review(pr, event=github_client.REVIEW_EVENT_APPROVE, body=_LAND_REVIEW_BODY)
+            logger.info("approved %s#%d", request.repo, request.pr_number)
         _best_effort_upsert(request, config, body, build_github=build_github, pr=pr)
         return
     dismissed = github_client.dismiss_prior_greenlight_approvals(

greenlight/tests/test_github_client.py

@@ -1323,3 +1323,54 @@ def test_dismiss_prior_greenlight_approvals_when_none_match():
 
     assert dismissed == []
     assert reviews[0].dismissed_with is None
+
+
+def test_has_live_greenlight_approval_true_for_own_approved():
+    pr = _FakeVerdictPR(reviews=[_FakeVerdictReview(1, _FakeActor("greenlight-app[bot]"), "APPROVED")])
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is True
+
+
+def test_has_live_greenlight_approval_false_when_no_reviews():
+    pr = _FakeVerdictPR(reviews=[])
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is False
+
+
+def test_has_live_greenlight_approval_matches_login_case_insensitively():
+    pr = _FakeVerdictPR(reviews=[_FakeVerdictReview(1, _FakeActor("GreenLight-App[Bot]"), "APPROVED")])
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is True
+
+
+def test_has_live_greenlight_approval_skips_null_user_and_empty_login():
+    reviews = [
+        _FakeVerdictReview(1, None, "APPROVED"),
+        _FakeVerdictReview(2, _FakeActor(""), "APPROVED"),
+    ]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    # A null-user or empty-login APPROVED review cannot be attributed to greenlight.
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is False
+
+
+def test_has_live_greenlight_approval_false_for_commented_and_dismissed():
+    reviews = [
+        _FakeVerdictReview(1, _FakeActor("greenlight-app[bot]"), "COMMENTED"),
+        _FakeVerdictReview(2, _FakeActor("greenlight-app[bot]"), "DISMISSED"),
+    ]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    # Only APPROVED is a live approval; a dismissed review is no longer live.
+    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():
+    reviews = [
+        _FakeVerdictReview(1, _FakeActor("alice"), "APPROVED"),
+        _FakeVerdictReview(2, _FakeActor("greenlight-app[bot]"), "COMMENTED"),
+        _FakeVerdictReview(3, _FakeActor("greenlight-app[bot]"), "APPROVED"),
+    ]
+    pr = _FakeVerdictPR(reviews=reviews)
+
+    assert github_client.has_live_greenlight_approval(pr, bot_login="greenlight-app[bot]") is True

greenlight/tests/test_verdict.py

@@ -697,6 +697,74 @@ def test_full_land_approves_with_empty_body(make_config, tmp_path):
     assert pr.created_reviews == [("APPROVE", "")]
 
 
+def test_full_land_skips_reapproval_when_already_approved(make_config, tmp_path, caplog):
+    rec = _Recorder()
+    emit = _FakeEmit(rec)
+    reviews = [_FakeReview(1, _BOT, "APPROVED", rec)]
+    pr = _FakePR("h", rec, reviews=reviews)
+    gh = _FakeGithub(_FakeRepo(pr))
+    vf = _write_verdict(tmp_path, status="LAND", reason="clean", message="LGTM")
+    req = VerdictRequest(repo="r", pr_number=40, head_sha="h", eval_hash=_HASH, verdict_file=vf, bot_login=_BOT)
+
+    with caplog.at_level(logging.INFO, logger="greenlight"):
+        verdict.run(req, make_config(github_token="tok"), build_github=lambda t: gh, emit=emit, now=lambda: _FIXED)
+
+    # A live greenlight approval already exists: no second APPROVE review is posted (the prior one is
+    # left intact), yet the canonical comment is still upserted and the skip is logged.
+    assert pr.created_reviews == []
+    assert reviews[0].dismissed_with is None
+    assert rec.events == ["emit", "comment"]
+    assert pr.comments[0].startswith(comment_format.COMMENT_MARKER)
+    assert f"**{comment_format.LAND_HEADLINE}**" in pr.comments[0]
+    assert "LGTM" in pr.comments[0]
+    assert any("skipping re-approval" in record.getMessage() for record in caplog.records)
+
+
+def test_full_land_reapproves_when_prior_bot_approval_dismissed(make_config, tmp_path):
+    rec = _Recorder()
+    emit = _FakeEmit(rec)
+    pr = _FakePR("h", rec, reviews=[_FakeReview(1, _BOT, "DISMISSED", rec)])
+    gh = _FakeGithub(_FakeRepo(pr))
+    vf = _write_verdict(tmp_path, status="LAND", reason="clean", message="LGTM")
+    req = VerdictRequest(repo="r", pr_number=41, head_sha="h", eval_hash=_HASH, verdict_file=vf, bot_login=_BOT)
+
+    verdict.run(req, make_config(github_token="tok"), build_github=lambda t: gh, emit=emit, now=lambda: _FIXED)
+
+    # A prior greenlight approval that was dismissed is not live, so LAND re-approves.
+    assert pr.created_reviews == [("APPROVE", "")]
+    assert rec.events == ["emit", "review:APPROVE", "comment"]
+
+
+def test_full_land_approves_when_only_other_author_approved(make_config, tmp_path):
+    rec = _Recorder()
+    emit = _FakeEmit(rec)
+    pr = _FakePR("h", rec, reviews=[_FakeReview(1, "alice", "APPROVED", rec)])
+    gh = _FakeGithub(_FakeRepo(pr))
+    vf = _write_verdict(tmp_path, status="LAND", reason="clean", message="LGTM")
+    req = VerdictRequest(repo="r", pr_number=42, head_sha="h", eval_hash=_HASH, verdict_file=vf, bot_login=_BOT)
+
+    verdict.run(req, make_config(github_token="tok"), build_github=lambda t: gh, emit=emit, now=lambda: _FIXED)
+
+    # A human's approval is not greenlight's own, so LAND still posts greenlight's approval.
+    assert pr.created_reviews == [("APPROVE", "")]
+    assert rec.events == ["emit", "review:APPROVE", "comment"]
+
+
+def test_full_land_approves_when_only_bot_commented_review(make_config, tmp_path):
+    rec = _Recorder()
+    emit = _FakeEmit(rec)
+    pr = _FakePR("h", rec, reviews=[_FakeReview(1, _BOT, "COMMENTED", rec)])
+    gh = _FakeGithub(_FakeRepo(pr))
+    vf = _write_verdict(tmp_path, status="LAND", reason="clean", message="LGTM")
+    req = VerdictRequest(repo="r", pr_number=43, head_sha="h", eval_hash=_HASH, verdict_file=vf, bot_login=_BOT)
+
+    verdict.run(req, make_config(github_token="tok"), build_github=lambda t: gh, emit=emit, now=lambda: _FIXED)
+
+    # A COMMENTED review from greenlight is not an approval, so LAND posts one.
+    assert pr.created_reviews == [("APPROVE", "")]
+    assert rec.events == ["emit", "review:APPROVE", "comment"]
+
+
 class _CommentBoomPR(_FakePR):
     """A PR whose issue-comment create always fails, to prove the cosmetic write is best-effort."""