From e1472bf0752bad9c8d101fd3e409154b313ac785 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Fri, 22 May 2026 11:23:22 +0200 Subject: [PATCH 1/2] Dismiss all stale Github reviews --- bot/code_review_bot/report/github.py | 29 ++++++++++++---------- bot/code_review_bot/sources/github.py | 35 ++++++++++++++++++++++----- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/bot/code_review_bot/report/github.py b/bot/code_review_bot/report/github.py index 735ee807e..b2634b600 100644 --- a/bot/code_review_bot/report/github.py +++ b/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,19 @@ 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 + 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 - - self.github_client.publish_review( - issues=publishable_issues, - revision=revision, - message=message, - event=event, - ) + logger.info( + "No publishable issue, no review nor comment published on Github." + ) diff --git a/bot/code_review_bot/sources/github.py b/bot/code_review_bot/sources/github.py index 84e2d0130..d2d07b768 100644 --- a/bot/code_review_bot/sources/github.py +++ b/bot/code_review_bot/sources/github.py @@ -70,12 +70,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 +90,31 @@ 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) + + for review in pr.get_reviews(): + if review.user.login != "mozilla-code-review[bot]": + 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, + ) + 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 From 32e32045bc36819e80c93d0f219b791e3cdbc85c Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Fri, 22 May 2026 11:47:13 +0200 Subject: [PATCH 2/2] Update unit test --- bot/tests/test_reporter_github.py | 51 +++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/bot/tests/test_reporter_github.py b/bot/tests/test_reporter_github.py index 1160cefe2..97ca9eed9 100644 --- a/bot/tests/test_reporter_github.py +++ b/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,21 @@ 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={}, ) @@ -149,15 +182,9 @@ 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/reviews"), ] - review_creation = responses.calls[-1] - assert json.loads(review_creation.request.body) == { - "comments": [], - "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "event": "APPROVE", - }