Dismiss all stale Github reviews

bot/code_review_bot/report/github.py

@@ -42,7 +42,10 @@ def publish(self, issues, revision, task_failures, notices, reviewers):
             return
 
         if reviewers:
-            raise NotImplementedError
+            logger.warn(
+                f"These reviewer groups should be assigned, but it's not yet possible on Github: {', '.join(reviewers)}"
+            )
+
         # Avoid publishing a patch from a de-activated analyzer
         publishable_issues = [
             issue
@@ -51,19 +54,29 @@ def publish(self, issues, revision, task_failures, notices, reviewers):
             and issue.analyzer.name not in self.analyzers_skipped
         ]
 
+        # Remove any earlier review to get a clean state
+        nb_dismissed = self.github_client.cleanup_pr(revision)
+
         if publishable_issues:
             # Publish a review summarizing detected, unresolved and closed issues
-            message = f"{len(issues)} issues have been found in this revision"
-            event = ReviewEvent.RequestChanges
+            self.github_client.publish_review(
+                issues=publishable_issues,
+                revision=revision,
+                message=f"{len(publishable_issues)} issues have been found in this revision",
+                event=ReviewEvent.RequestChanges,
+            )
         else:
-            # Simply approve the pull request
-            logger.info("No publishable issue, approving the pull request")
-            message = None
-            event = ReviewEvent.Approved
+            # Publish a comment, mentioning if previous issues were cleared up
+            logger.info(
+                "No publishable issue, posting a standalone comment",
+                nb_dismissed=nb_dismissed,
+            )
+            if nb_dismissed > 0:
+                message = "Previous issues have been fixed. This pull request is :ok:"
+            else:
+                message = "No new issues detected. This pull request is :ok:"
 
-        self.github_client.publish_review(
-            issues=publishable_issues,
-            revision=revision,
-            message=message,
-            event=event,
-        )
+            self.github_client.publish_comment(
+                revision=revision,
+                message=message,
+            )

bot/code_review_bot/sources/github.py

@@ -5,6 +5,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import enum
+from functools import lru_cache
 
 import structlog
 from github import Auth, GithubIntegration
@@ -49,6 +50,7 @@ def __init__(self, client_id: str, private_key: str, installation_id: str):
 
         self.review_comments = []
 
+    @lru_cache
     def get_pull_request(self, revision: GithubRevision):
         repo = self.api.get_repo(revision.repo_name)
         return repo.get_pull(revision.pull_number)
@@ -60,6 +62,19 @@ def _build_review_comment(self, issue):
             body=issue.message,
         )
 
+    def publish_comment(
+        self,
+        revision: GithubRevision,
+        message: str | None = None,
+    ):
+        """
+        Publish a comment on a pull request
+        """
+        assert isinstance(revision, GithubRevision), "Only for github revisions"
+
+        pull_request = self.get_pull_request(revision)
+        pull_request.create_issue_comment(body=message)
+
     def publish_review(
         self,
         issues: list[Issue],
@@ -70,12 +85,7 @@ def publish_review(
         """
         Publish a review from a list of publishable issues, requesting changes to the author.
         """
-
-        if not isinstance(revision, GithubRevision):
-            logger.warning(
-                f"Revision must originate from Github in order to publish a review, skipping {revision}."
-            )
-            return
+        assert isinstance(revision, GithubRevision), "Only for github revisions"
 
         repo = self.api.get_repo(revision.repo_name)
         pull_request = repo.get_pull(revision.pull_number)
@@ -95,3 +105,40 @@ def publish_review(
             event=event.value,
             **attrs,
         )
+
+    def cleanup_pr(self, revision: GithubRevision):
+        """
+        Dismiss previous reviews from the bot
+        """
+        assert isinstance(revision, GithubRevision), "Only for github revisions"
+
+        pr = self.get_pull_request(revision)
+
+        nb = 0
+        for review in pr.get_reviews():
+            # Only process our own reviews
+            if review.user.login != "mozilla-code-review[bot]":
+                continue
+
+            # Only process active reviews
+            if review.state == "DISMISSED":
+                continue
+
+            try:
+                review.dismiss("This review is now deprecated.")
+                logger.info(
+                    "Dismissed previous Github review from the bot",
+                    review=review.id,
+                    submitted=review.submitted_at,
+                )
+                nb += 1
+            except Exception as e:
+                logger.warn(
+                    "Failed to dismiss previous Github review from the bot",
+                    review=review.id,
+                    submitted=review.submitted_at,
+                    error=e,
+                )
+                raise  # trashme
+
+        return nb

bot/tests/conftest.py

@@ -333,7 +333,10 @@ def mock_github(mock_config):
     responses.add(
         responses.GET,
         "https://api.github.com:443/repos/owner/repo-name/pulls/1",
-        json={"url": "https://api.github.com/repos/owner/repo-name/pulls/1"},
+        json={
+            "url": "https://api.github.com/repos/owner/repo-name/pulls/1",
+            "issue_url": "https://api.github.com/repos/owner/repo-name/pulls/1",
+        },
     )
     responses.add(
         responses.GET,

bot/tests/test_reporter_github.py

@@ -68,12 +68,20 @@ def test_github_review(
     )
     assert issue_coverage.is_publishable()
 
+    # Mock to publish a new review
     responses.add(
         responses.POST,
         "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
         json={},
     )
 
+    # Mock to list existing reviews
+    responses.add(
+        responses.GET,
+        "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
+        json=[],
+    )
+
     reporter.publish([issue_clang_tidy, issue_coverage], revision, [], [], [])
     assert [(call.request.method, call.request.url) for call in responses.calls] == [
         ("GET", "https://github.com/owner/repo-name/pull/1.diff"),
@@ -84,6 +92,18 @@ def test_github_review(
         ),
         ("GET", "https://api.github.com:443/repos/owner/repo-name"),
         ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1"),
+        (
+            "GET",
+            "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
+        ),
+        (
+            "GET",
+            "https://api.github.com:443/repos/owner/repo-name",
+        ),
+        (
+            "GET",
+            "https://api.github.com:443/repos/owner/repo-name/pulls/1",
+        ),
         (
             "GET",
             "https://api.github.com:443/repos/owner/repo-name/commits/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
@@ -110,7 +130,7 @@ def test_github_review(
     }
 
 
-def test_github_review_approve(
+def test_github_review_cleanup(
     monkeypatch,
     mock_github,
     mock_config,
@@ -120,7 +140,7 @@ def test_github_review_approve(
     mock_task,
     mock_backend_secret,
 ):
-    """In case no issue is found, the pull request is approved"""
+    """In case no issue is found, previous reviews are dismissed"""
     revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None)
     revision.lines = {}
     revision.files = ["test.txt", "test.cpp", "another_test.cpp"]
@@ -134,8 +154,27 @@ def test_github_review_approve(
     )
 
     responses.add(
-        responses.POST,
+        responses.GET,
         "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews",
+        json=[
+            {"id": 1, "user": {"login": "a-moz-developer"}},
+            {
+                "id": 2,
+                "user": {"login": "mozilla-code-review[bot]"},
+                "pull_request_url": "https://api.github.com/repos/owner/repo-name/pulls/2",
+            },
+        ],
+    )
+
+    responses.add(
+        responses.PUT,
+        "https://api.github.com:443/repos/owner/repo-name/pulls/2/reviews/2/dismissals",
+        json={},
+    )
+
+    responses.add(
+        responses.POST,
+        "https://api.github.com:443/repos/owner/repo-name/pulls/1/comments",
         json={},
     )
 
@@ -149,15 +188,18 @@ def test_github_review_approve(
         ),
         ("GET", "https://api.github.com:443/repos/owner/repo-name"),
         ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1"),
+        ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews"),
         (
-            "GET",
-            "https://api.github.com:443/repos/owner/repo-name/commits/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+            "PUT",
+            "https://api.github.com:443/repos/owner/repo-name/pulls/2/reviews/2/dismissals",
+        ),
+        (
+            "POST",
+            "https://api.github.com:443/repos/owner/repo-name/pulls/1/comments",
         ),
-        ("POST", "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews"),
     ]
-    review_creation = responses.calls[-1]
-    assert json.loads(review_creation.request.body) == {
-        "comments": [],
-        "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
-        "event": "APPROVE",
+
+    # Check published comment
+    assert json.loads(responses.calls[-1].request.body) == {
+        "body": "Previous issues have been fixed. This pull request is :ok:"
     }