From a5a7c58300f937c7db6f5cc372c884c3334a26c6 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Wed, 14 Jan 2026 14:38:00 +0100 Subject: [PATCH 1/6] Split revisions --- bot/code_review_bot/github.py | 0 bot/code_review_bot/revisions/__init__.py | 255 ++++++++++++++++++ bot/code_review_bot/revisions/github.py | 20 ++ .../phabricator.py} | 193 +------------ 4 files changed, 284 insertions(+), 184 deletions(-) create mode 100644 bot/code_review_bot/github.py create mode 100644 bot/code_review_bot/revisions/__init__.py create mode 100644 bot/code_review_bot/revisions/github.py rename bot/code_review_bot/{revisions.py => revisions/phabricator.py} (71%) diff --git a/bot/code_review_bot/github.py b/bot/code_review_bot/github.py new file mode 100644 index 000000000..e69de29bb diff --git a/bot/code_review_bot/revisions/__init__.py b/bot/code_review_bot/revisions/__init__.py new file mode 100644 index 000000000..4df62e00b --- /dev/null +++ b/bot/code_review_bot/revisions/__init__.py @@ -0,0 +1,255 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import os +import random +from datetime import timedelta + +import rs_parsepatch +import structlog +from libmozdata.phabricator import PhabricatorAPI + +from code_review_bot import Issue, stats, taskcluster +from code_review_bot.config import ( + settings, +) +from code_review_bot.revisions.github import GithubRevision +from code_review_bot.revisions.phabricator import PhabricatorRevision +from code_review_bot.tasks.base import AnalysisTask + +logger = structlog.get_logger(__name__) + + +class ImprovementPatch: + """ + An improvement patch built by the bot + """ + + def __init__(self, analyzer, patch_name, content): + assert isinstance(analyzer, AnalysisTask) + + # Build name from analyzer and revision + self.analyzer = analyzer + self.name = f"{self.analyzer.name}-{patch_name}.diff" + self.content = content + self.url = None + self.path = None + + def __str__(self): + return f"{self.analyzer.name}: {self.url or self.path or self.name}" + + def write(self): + """ + Write patch on local FS, for dev & tests only + """ + self.path = os.path.join(settings.taskcluster.results_dir, self.name) + with open(self.path, "w") as f: + length = f.write(self.content) + logger.info("Improvement patch saved", path=self.path, length=length) + + def publish(self, days_ttl=30): + """ + Push through Taskcluster API to setup the content-type header + so it displays nicely in browsers + """ + assert ( + not settings.taskcluster.local + ), "Only publish on online Taskcluster tasks" + self.url = taskcluster.upload_artifact( + f"public/patch/{self.name}", + self.content.encode(), + content_type="text/plain; charset=utf-8", # Displays instead of download + ttl=timedelta(days=days_ttl - 1), + ) + logger.info("Improvement patch published", url=self.url) + + +class Revision: + """ + A generic revision to override using provider specific details + """ + + def __init__( + self, + ): + # backend's returned URL to list or create issues linked to the revision in bulk (diff is optional) + self.issues_url = None + + # Patches built later on + self.improvement_patches = [] + + # Patch analysis + self.files = [] + self.lines = {} + + @property + def namespaces(self): + raise NotImplementedError + + @property + def before_after_feature(self): + """ + Randomly run the before/after feature depending on a configured ratio. + All the diffs of a revision must be analysed with or without the feature. + """ + if getattr(self, "id", None) is None: + logger.debug( + "Backend ID must be set to determine if using the before/after feature. Skipping." + ) + return False + # Set random module pseudo-random seed based on the revision ID to + # ensure that successive calls to random.random will return deterministic values + random.seed(self.id) + return random.random() < taskcluster.secrets.get("BEFORE_AFTER_RATIO", 0) + # Reset random module seed to prevent deterministic values after calling that function + random.seed(os.urandom(128)) + + def __repr__(self): + raise NotImplementedError + + def __str__(self): + raise NotImplementedError + + @staticmethod + def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): + """ + Load identifiers from Phabricator, using the remote task description + """ + + # Load build target phid from the task env + code_review = try_task["extra"]["code-review"] + + if "github" in code_review: + return GithubRevision(**code_review["github"]) + else: + return PhabricatorRevision.from_try_task( + code_review, decision_task, phabricator + ) + + def analyze_patch(self): + """ + Analyze loaded patch to extract modified lines + and statistics + """ + assert self.patch is not None, "Missing patch" + assert isinstance(self.patch, str), "Invalid patch type" + + # List all modified lines from current revision changes + patch_stats = rs_parsepatch.get_lines(self.patch) + assert len(patch_stats) > 0, "Empty patch" + + self.lines = {stat["filename"]: stat["added_lines"] for stat in patch_stats} + + # Shortcut to files modified + self.files = self.lines.keys() + + # Report nb of files and lines analyzed + stats.add_metric("analysis.files", len(self.files)) + stats.add_metric( + "analysis.lines", sum(len(line) for line in self.lines.values()) + ) + + def load_file(self, path): + """ + Load a file content at current revision from remote HGMO + """ + raise NotImplementedError + + def has_file(self, path): + """ + Check if the path is in this patch + """ + assert isinstance(path, str) + return path in self.files + + def contains(self, issue): + """ + Check if the issue (path+lines) is in this patch + """ + assert isinstance(issue, Issue) + + # Get modified lines for this issue + modified_lines = self.lines.get(issue.path) + if modified_lines is None: + return False + + # Empty line means full file + if issue.line is None: + return True + + # Detect if this issue is in the patch + lines = set(range(issue.line, issue.line + issue.nb_lines)) + return not lines.isdisjoint(modified_lines) + + @property + def has_clang_files(self): + """ + Check if this revision has any file that might + be a C/C++ file + """ + + def _is_clang(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.cpp_extensions + + return any(_is_clang(f) for f in self.files) + + @property + def has_clang_header_files(self): + """ + Check if this revision has any file that might + be a C/C++ header file + """ + + def _is_clang_header(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.cpp_header_extensions + + return any(_is_clang_header(f) for f in self.files) + + @property + def has_idl_files(self): + """ + Check if this revision has any idl files + """ + + def _is_idl(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.idl_extensions + + return any(_is_idl(f) for f in self.files) + + @property + def is_blacklisted(self): + raise NotImplementedError + + def add_improvement_patch(self, analyzer, content): + """ + Save an improvement patch, and make it available + as a Taskcluster artifact + """ + assert isinstance(content, str) + assert len(content) > 0 + self.improvement_patches.append(ImprovementPatch(analyzer, repr(self), content)) + + def reset(self): + """ + Reset temporary data in BEFORE mode + * improvement patches + """ + self.improvement_patches = [] + + @property + def bugzilla_id(self): + raise NotImplementedError + + @property + def title(self): + raise NotImplementedError + + def as_dict(self): + """ + Outputs a serializable representation of this revision + """ + raise NotImplementedError diff --git a/bot/code_review_bot/revisions/github.py b/bot/code_review_bot/revisions/github.py new file mode 100644 index 000000000..2e68dbacb --- /dev/null +++ b/bot/code_review_bot/revisions/github.py @@ -0,0 +1,20 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +from code_review_bot.revision import Revision + + +class GithubRevision(Revision): + """ + A revision from a github pull-request + """ + + def __init__(self, repo_url, branch, pull_number, pull_head_sha): + self.repo_url = repo_url + self.branch = branch + self.pull_number = pull_number + self.pull_head_sha = pull_head_sha + + def __str__(self): + return f"Github pull request {self.repo_url} #{self.pull_number} ({self.pull_head_sha[:8]})" diff --git a/bot/code_review_bot/revisions.py b/bot/code_review_bot/revisions/phabricator.py similarity index 71% rename from bot/code_review_bot/revisions.py rename to bot/code_review_bot/revisions/phabricator.py index d6b80ef3e..ab44dbe62 100644 --- a/bot/code_review_bot/revisions.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -3,74 +3,27 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. import os -import random import time import urllib.parse -from datetime import timedelta from pathlib import Path import requests -import rs_parsepatch import structlog from libmozdata.phabricator import PhabricatorAPI -from code_review_bot import InvalidRepository, InvalidTrigger, Issue, stats, taskcluster +from code_review_bot import InvalidRepository, InvalidTrigger from code_review_bot.config import ( REPO_AUTOLAND, REPO_MOZILLA_CENTRAL, GetAppUserAgent, settings, ) -from code_review_bot.tasks.base import AnalysisTask +from code_review_bot.revision import Revision logger = structlog.get_logger(__name__) -class ImprovementPatch: - """ - An improvement patch built by the bot - """ - - def __init__(self, analyzer, patch_name, content): - assert isinstance(analyzer, AnalysisTask) - - # Build name from analyzer and revision - self.analyzer = analyzer - self.name = f"{self.analyzer.name}-{patch_name}.diff" - self.content = content - self.url = None - self.path = None - - def __str__(self): - return f"{self.analyzer.name}: {self.url or self.path or self.name}" - - def write(self): - """ - Write patch on local FS, for dev & tests only - """ - self.path = os.path.join(settings.taskcluster.results_dir, self.name) - with open(self.path, "w") as f: - length = f.write(self.content) - logger.info("Improvement patch saved", path=self.path, length=length) - - def publish(self, days_ttl=30): - """ - Push through Taskcluster API to setup the content-type header - so it displays nicely in browsers - """ - assert ( - not settings.taskcluster.local - ), "Only publish on online Taskcluster tasks" - self.url = taskcluster.upload_artifact( - f"public/patch/{self.name}", - self.content.encode(), - content_type="text/plain; charset=utf-8", # Displays instead of download - ttl=timedelta(days=days_ttl - 1), - ) - logger.info("Improvement patch published", url=self.url) - - -class Revision: +class PhabricatorRevision(Revision): """ A Phabricator revision to analyze and report on """ @@ -91,9 +44,11 @@ def __init__( base_repository=None, base_repository_conf=None, phabricator_repository=None, - url=None, patch=None, + url=None, ): + super().__init__() + # Identification self.phabricator_id = phabricator_id self.phabricator_phid = phabricator_phid @@ -121,16 +76,8 @@ def __init__( # the phabricator repository payload for later identification self.phabricator_repository = phabricator_repository - # backend's returned URL to list or create issues linked to the revision in bulk (diff is optional) - self.issues_url = None - - # Patches built later on - self.improvement_patches = [] - # Patch analysis self.patch = patch - self.files = [] - self.lines = {} @property def namespaces(self): @@ -168,24 +115,6 @@ def from_autoland(self): def from_mozilla_central(self): return self.head_repository == REPO_MOZILLA_CENTRAL - @property - def before_after_feature(self): - """ - Randomly run the before/after feature depending on a configured ratio. - All the diffs of a revision must be analysed with or without the feature. - """ - if getattr(self, "id", None) is None: - logger.debug( - "Backend ID must be set to determine if using the before/after feature. Skipping." - ) - return False - # Set random module pseudo-random seed based on the revision ID to - # ensure that successive calls to random.random will return deterministic values - random.seed(self.id) - return random.random() < taskcluster.secrets.get("BEFORE_AFTER_RATIO", 0) - # Reset random module seed to prevent deterministic values after calling that function - random.seed(os.urandom(128)) - def __repr__(self): if self.diff_phid: # Most revisions have a Diff from Phabricator @@ -201,13 +130,12 @@ def __str__(self): return f"Phabricator #{self.diff_id} - {self.diff_phid}" @staticmethod - def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): + def from_try_task( + code_review: dict, decision_task: dict, phabricator: PhabricatorAPI + ): """ Load identifiers from Phabricator, using the remote task description """ - - # Load build target phid from the task env - code_review = try_task["extra"]["code-review"] build_target_phid = code_review.get("phabricator-diff") or code_review.get( "phabricator-build-target" ) @@ -412,29 +340,6 @@ def from_phabricator_trigger(build_target_phid: str, phabricator: PhabricatorAPI repository_try_name=repository.try_name, ) - def analyze_patch(self): - """ - Analyze loaded patch to extract modified lines - and statistics - """ - assert self.patch is not None, "Missing patch" - assert isinstance(self.patch, str), "Invalid patch type" - - # List all modified lines from current revision changes - patch_stats = rs_parsepatch.get_lines(self.patch) - assert len(patch_stats) > 0, "Empty patch" - - self.lines = {stat["filename"]: stat["added_lines"] for stat in patch_stats} - - # Shortcut to files modified - self.files = self.lines.keys() - - # Report nb of files and lines analyzed - stats.add_metric("analysis.files", len(self.files)) - stats.add_metric( - "analysis.lines", sum(len(line) for line in self.lines.values()) - ) - def load_file(self, path): """ Load a file content at current revision from remote HGMO @@ -467,70 +372,6 @@ def load_file(self, path): return content - def has_file(self, path): - """ - Check if the path is in this patch - """ - assert isinstance(path, str) - return path in self.files - - def contains(self, issue): - """ - Check if the issue (path+lines) is in this patch - """ - assert isinstance(issue, Issue) - - # Get modified lines for this issue - modified_lines = self.lines.get(issue.path) - if modified_lines is None: - return False - - # Empty line means full file - if issue.line is None: - return True - - # Detect if this issue is in the patch - lines = set(range(issue.line, issue.line + issue.nb_lines)) - return not lines.isdisjoint(modified_lines) - - @property - def has_clang_files(self): - """ - Check if this revision has any file that might - be a C/C++ file - """ - - def _is_clang(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.cpp_extensions - - return any(_is_clang(f) for f in self.files) - - @property - def has_clang_header_files(self): - """ - Check if this revision has any file that might - be a C/C++ header file - """ - - def _is_clang_header(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.cpp_header_extensions - - return any(_is_clang_header(f) for f in self.files) - - @property - def has_idl_files(self): - """ - Check if this revision has any idl files - """ - - def _is_idl(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.idl_extensions - - return any(_is_idl(f) for f in self.files) - @property def is_blacklisted(self): """Check if the revision author is in the black-list""" @@ -541,22 +382,6 @@ def is_blacklisted(self): logger.info("Revision from a blacklisted user", revision=self, author=author) return True - def add_improvement_patch(self, analyzer, content): - """ - Save an improvement patch, and make it available - as a Taskcluster artifact - """ - assert isinstance(content, str) - assert len(content) > 0 - self.improvement_patches.append(ImprovementPatch(analyzer, repr(self), content)) - - def reset(self): - """ - Reset temporary data in BEFORE mode - * improvement patches - """ - self.improvement_patches = [] - @property def bugzilla_id(self): if self.revision is None: From e11947e27a3c3fb8e9735fe663e5a57ed6ddb0ea Mon Sep 17 00:00:00 2001 From: Valentin Rigal Date: Fri, 16 Jan 2026 11:06:13 +0100 Subject: [PATCH 2/6] Remove github specific code --- bot/code_review_bot/revisions/__init__.py | 252 +--------------------- bot/code_review_bot/revisions/base.py | 237 ++++++++++++++++++++ bot/code_review_bot/revisions/github.py | 20 -- 3 files changed, 238 insertions(+), 271 deletions(-) create mode 100644 bot/code_review_bot/revisions/base.py delete mode 100644 bot/code_review_bot/revisions/github.py diff --git a/bot/code_review_bot/revisions/__init__.py b/bot/code_review_bot/revisions/__init__.py index 4df62e00b..88f6feafb 100644 --- a/bot/code_review_bot/revisions/__init__.py +++ b/bot/code_review_bot/revisions/__init__.py @@ -2,254 +2,4 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this # file, You can obtain one at http://mozilla.org/MPL/2.0/. -import os -import random -from datetime import timedelta - -import rs_parsepatch -import structlog -from libmozdata.phabricator import PhabricatorAPI - -from code_review_bot import Issue, stats, taskcluster -from code_review_bot.config import ( - settings, -) -from code_review_bot.revisions.github import GithubRevision -from code_review_bot.revisions.phabricator import PhabricatorRevision -from code_review_bot.tasks.base import AnalysisTask - -logger = structlog.get_logger(__name__) - - -class ImprovementPatch: - """ - An improvement patch built by the bot - """ - - def __init__(self, analyzer, patch_name, content): - assert isinstance(analyzer, AnalysisTask) - - # Build name from analyzer and revision - self.analyzer = analyzer - self.name = f"{self.analyzer.name}-{patch_name}.diff" - self.content = content - self.url = None - self.path = None - - def __str__(self): - return f"{self.analyzer.name}: {self.url or self.path or self.name}" - - def write(self): - """ - Write patch on local FS, for dev & tests only - """ - self.path = os.path.join(settings.taskcluster.results_dir, self.name) - with open(self.path, "w") as f: - length = f.write(self.content) - logger.info("Improvement patch saved", path=self.path, length=length) - - def publish(self, days_ttl=30): - """ - Push through Taskcluster API to setup the content-type header - so it displays nicely in browsers - """ - assert ( - not settings.taskcluster.local - ), "Only publish on online Taskcluster tasks" - self.url = taskcluster.upload_artifact( - f"public/patch/{self.name}", - self.content.encode(), - content_type="text/plain; charset=utf-8", # Displays instead of download - ttl=timedelta(days=days_ttl - 1), - ) - logger.info("Improvement patch published", url=self.url) - - -class Revision: - """ - A generic revision to override using provider specific details - """ - - def __init__( - self, - ): - # backend's returned URL to list or create issues linked to the revision in bulk (diff is optional) - self.issues_url = None - - # Patches built later on - self.improvement_patches = [] - - # Patch analysis - self.files = [] - self.lines = {} - - @property - def namespaces(self): - raise NotImplementedError - - @property - def before_after_feature(self): - """ - Randomly run the before/after feature depending on a configured ratio. - All the diffs of a revision must be analysed with or without the feature. - """ - if getattr(self, "id", None) is None: - logger.debug( - "Backend ID must be set to determine if using the before/after feature. Skipping." - ) - return False - # Set random module pseudo-random seed based on the revision ID to - # ensure that successive calls to random.random will return deterministic values - random.seed(self.id) - return random.random() < taskcluster.secrets.get("BEFORE_AFTER_RATIO", 0) - # Reset random module seed to prevent deterministic values after calling that function - random.seed(os.urandom(128)) - - def __repr__(self): - raise NotImplementedError - - def __str__(self): - raise NotImplementedError - - @staticmethod - def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): - """ - Load identifiers from Phabricator, using the remote task description - """ - - # Load build target phid from the task env - code_review = try_task["extra"]["code-review"] - - if "github" in code_review: - return GithubRevision(**code_review["github"]) - else: - return PhabricatorRevision.from_try_task( - code_review, decision_task, phabricator - ) - - def analyze_patch(self): - """ - Analyze loaded patch to extract modified lines - and statistics - """ - assert self.patch is not None, "Missing patch" - assert isinstance(self.patch, str), "Invalid patch type" - - # List all modified lines from current revision changes - patch_stats = rs_parsepatch.get_lines(self.patch) - assert len(patch_stats) > 0, "Empty patch" - - self.lines = {stat["filename"]: stat["added_lines"] for stat in patch_stats} - - # Shortcut to files modified - self.files = self.lines.keys() - - # Report nb of files and lines analyzed - stats.add_metric("analysis.files", len(self.files)) - stats.add_metric( - "analysis.lines", sum(len(line) for line in self.lines.values()) - ) - - def load_file(self, path): - """ - Load a file content at current revision from remote HGMO - """ - raise NotImplementedError - - def has_file(self, path): - """ - Check if the path is in this patch - """ - assert isinstance(path, str) - return path in self.files - - def contains(self, issue): - """ - Check if the issue (path+lines) is in this patch - """ - assert isinstance(issue, Issue) - - # Get modified lines for this issue - modified_lines = self.lines.get(issue.path) - if modified_lines is None: - return False - - # Empty line means full file - if issue.line is None: - return True - - # Detect if this issue is in the patch - lines = set(range(issue.line, issue.line + issue.nb_lines)) - return not lines.isdisjoint(modified_lines) - - @property - def has_clang_files(self): - """ - Check if this revision has any file that might - be a C/C++ file - """ - - def _is_clang(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.cpp_extensions - - return any(_is_clang(f) for f in self.files) - - @property - def has_clang_header_files(self): - """ - Check if this revision has any file that might - be a C/C++ header file - """ - - def _is_clang_header(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.cpp_header_extensions - - return any(_is_clang_header(f) for f in self.files) - - @property - def has_idl_files(self): - """ - Check if this revision has any idl files - """ - - def _is_idl(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.idl_extensions - - return any(_is_idl(f) for f in self.files) - - @property - def is_blacklisted(self): - raise NotImplementedError - - def add_improvement_patch(self, analyzer, content): - """ - Save an improvement patch, and make it available - as a Taskcluster artifact - """ - assert isinstance(content, str) - assert len(content) > 0 - self.improvement_patches.append(ImprovementPatch(analyzer, repr(self), content)) - - def reset(self): - """ - Reset temporary data in BEFORE mode - * improvement patches - """ - self.improvement_patches = [] - - @property - def bugzilla_id(self): - raise NotImplementedError - - @property - def title(self): - raise NotImplementedError - - def as_dict(self): - """ - Outputs a serializable representation of this revision - """ - raise NotImplementedError +from code_review_bot.revisions.base import ImprovementPatch, Revision # noqa diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py new file mode 100644 index 000000000..f15eb54a9 --- /dev/null +++ b/bot/code_review_bot/revisions/base.py @@ -0,0 +1,237 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import os +import random +from abc import ABC +from datetime import timedelta + +import rs_parsepatch +import structlog + +from code_review_bot import Issue, stats, taskcluster +from code_review_bot.config import ( + settings, +) +from code_review_bot.tasks.base import AnalysisTask + +logger = structlog.get_logger(__name__) + + +class ImprovementPatch: + """ + An improvement patch built by the bot + """ + + def __init__(self, analyzer, patch_name, content): + assert isinstance(analyzer, AnalysisTask) + + # Build name from analyzer and revision + self.analyzer = analyzer + self.name = f"{self.analyzer.name}-{patch_name}.diff" + self.content = content + self.url = None + self.path = None + + def __str__(self): + return f"{self.analyzer.name}: {self.url or self.path or self.name}" + + def write(self): + """ + Write patch on local FS, for dev & tests only + """ + self.path = os.path.join(settings.taskcluster.results_dir, self.name) + with open(self.path, "w") as f: + length = f.write(self.content) + logger.info("Improvement patch saved", path=self.path, length=length) + + def publish(self, days_ttl=30): + """ + Push through Taskcluster API to setup the content-type header + so it displays nicely in browsers + """ + assert ( + not settings.taskcluster.local + ), "Only publish on online Taskcluster tasks" + self.url = taskcluster.upload_artifact( + f"public/patch/{self.name}", + self.content.encode(), + content_type="text/plain; charset=utf-8", # Displays instead of download + ttl=timedelta(days=days_ttl - 1), + ) + logger.info("Improvement patch published", url=self.url) + + +class Revision(ABC): + """ + A generic revision class to override using provider specific details + """ + + def __init__( + self, + ): + # backend's returned URL to list or create issues linked to the revision in bulk (diff is optional) + self.issues_url = None + + # Patches built later on + self.improvement_patches = [] + + # Patch analysis + self.files = [] + self.lines = {} + + @property + def namespaces(self): + raise NotImplementedError + + @property + def before_after_feature(self): + """ + Randomly run the before/after feature depending on a configured ratio. + All the diffs of a revision must be analysed with or without the feature. + """ + if getattr(self, "id", None) is None: + logger.debug( + "Backend ID must be set to determine if using the before/after feature. Skipping." + ) + return False + # Set random module pseudo-random seed based on the revision ID to + # ensure that successive calls to random.random will return deterministic values + random.seed(self.id) + return random.random() < taskcluster.secrets.get("BEFORE_AFTER_RATIO", 0) + # Reset random module seed to prevent deterministic values after calling that function + random.seed(os.urandom(128)) + + def __repr__(self): + raise NotImplementedError + + def __str__(self): + raise NotImplementedError + + def analyze_patch(self): + """ + Analyze loaded patch to extract modified lines + and statistics + """ + assert self.patch is not None, "Missing patch" + assert isinstance(self.patch, str), "Invalid patch type" + + # List all modified lines from current revision changes + patch_stats = rs_parsepatch.get_lines(self.patch) + assert len(patch_stats) > 0, "Empty patch" + + self.lines = {stat["filename"]: stat["added_lines"] for stat in patch_stats} + + # Shortcut to files modified + self.files = self.lines.keys() + + # Report nb of files and lines analyzed + stats.add_metric("analysis.files", len(self.files)) + stats.add_metric( + "analysis.lines", sum(len(line) for line in self.lines.values()) + ) + + def load_file(self, path): + """ + Load a file content at current revision from remote HGMO + """ + raise NotImplementedError + + def has_file(self, path): + """ + Check if the path is in this patch + """ + assert isinstance(path, str) + return path in self.files + + def contains(self, issue): + """ + Check if the issue (path+lines) is in this patch + """ + assert isinstance(issue, Issue) + + # Get modified lines for this issue + modified_lines = self.lines.get(issue.path) + if modified_lines is None: + return False + + # Empty line means full file + if issue.line is None: + return True + + # Detect if this issue is in the patch + lines = set(range(issue.line, issue.line + issue.nb_lines)) + return not lines.isdisjoint(modified_lines) + + @property + def has_clang_files(self): + """ + Check if this revision has any file that might + be a C/C++ file + """ + + def _is_clang(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.cpp_extensions + + return any(_is_clang(f) for f in self.files) + + @property + def has_clang_header_files(self): + """ + Check if this revision has any file that might + be a C/C++ header file + """ + + def _is_clang_header(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.cpp_header_extensions + + return any(_is_clang_header(f) for f in self.files) + + @property + def has_idl_files(self): + """ + Check if this revision has any idl files + """ + + def _is_idl(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.idl_extensions + + return any(_is_idl(f) for f in self.files) + + @property + def is_blacklisted(self): + raise NotImplementedError + + def add_improvement_patch(self, analyzer, content): + """ + Save an improvement patch, and make it available + as a Taskcluster artifact + """ + assert isinstance(content, str) + assert len(content) > 0 + self.improvement_patches.append(ImprovementPatch(analyzer, repr(self), content)) + + def reset(self): + """ + Reset temporary data in BEFORE mode + * improvement patches + """ + self.improvement_patches = [] + + @property + def bugzilla_id(self): + raise NotImplementedError + + @property + def title(self): + raise NotImplementedError + + def as_dict(self): + """ + Outputs a serializable representation of this revision + """ + raise NotImplementedError diff --git a/bot/code_review_bot/revisions/github.py b/bot/code_review_bot/revisions/github.py deleted file mode 100644 index 2e68dbacb..000000000 --- a/bot/code_review_bot/revisions/github.py +++ /dev/null @@ -1,20 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this -# file, You can obtain one at http://mozilla.org/MPL/2.0/. - -from code_review_bot.revision import Revision - - -class GithubRevision(Revision): - """ - A revision from a github pull-request - """ - - def __init__(self, repo_url, branch, pull_number, pull_head_sha): - self.repo_url = repo_url - self.branch = branch - self.pull_number = pull_number - self.pull_head_sha = pull_head_sha - - def __str__(self): - return f"Github pull request {self.repo_url} #{self.pull_number} ({self.pull_head_sha[:8]})" From 91110740087a71449fad13fb68246ebd7ca140e8 Mon Sep 17 00:00:00 2001 From: Valentin Rigal Date: Fri, 16 Jan 2026 12:48:07 +0100 Subject: [PATCH 3/6] Update code related to Phabricator Revision --- bot/code_review_bot/analysis.py | 16 ++++++------ bot/code_review_bot/cli.py | 9 ++++--- bot/code_review_bot/report/lando.py | 6 +++++ .../report/mail_builderrors.py | 2 +- bot/code_review_bot/revisions/__init__.py | 1 + bot/code_review_bot/revisions/phabricator.py | 14 +++++----- bot/code_review_bot/sources/phabricator.py | 12 +++++---- bot/code_review_bot/workflow.py | 26 ++++++++++++++----- 8 files changed, 56 insertions(+), 30 deletions(-) diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index 570d2c760..f6a9a6a7c 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -17,12 +17,12 @@ ) -class RevisionBuild(PhabricatorBuild): +class PhabricatorRevisionBuild(PhabricatorBuild): """ Convert the bot revision into a libmozevent compatible build """ - def __init__(self, revision, phabricator_api): + def __init__(self, phab_revision, phabricator_api): # State should be updated to Public self.state = PhabricatorBuildState.Queued @@ -32,18 +32,18 @@ def __init__(self, revision, phabricator_api): # Incremented on an unexpected failure during build's push to try self.retries = 0 - # Revision used by Phabricator updates + # PhabricatorRevision used by Phabricator updates # Direct output of Phabricator API (not the object passed here) - self.revision_id = revision.phabricator_id + self.revision_id = phab_revision.phabricator_id self.revision_url = None self.revision = None # Needed to update Phabricator Harbormaster - self.target_phid = revision.build_target_phid + self.target_phid = phab_revision.build_target_phid # Needed to load stack of patches - self.diff = revision.diff - self.diff_id = revision.diff_id + self.diff = phab_revision.diff + self.diff_id = phab_revision.diff_id # Needed to apply patch and communicate on Phabricator self.base_revision = None @@ -166,7 +166,7 @@ def publish_analysis_lando(payload, lando_warnings): Publish result of patch application and push to try on Lando """ mode, build, extras = payload - assert isinstance(build, RevisionBuild), "Not a RevisionBuild" + assert isinstance(build, PhabricatorRevisionBuild), "Not a PhabricatorRevisionBuild" logger.debug("Publishing a Lando build update", mode=mode, build=str(build)) if mode == "fail:general": diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 5e4c2ad7c..1d620e078 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -26,7 +26,7 @@ ) from code_review_bot.config import settings from code_review_bot.report import get_reporters -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision from code_review_bot.tools.libmozdata import setup as setup_libmozdata from code_review_bot.tools.log import init_logger from code_review_bot.workflow import Workflow @@ -160,18 +160,19 @@ def main(): settings.load_user_blacklist(taskcluster.secrets["user_blacklist"], phabricator_api) # Load unique revision + # Only Phabricator revisions are supported for now but it is intended to support generic Revision try: if settings.generic_group_id: - revision = Revision.from_decision_task( + revision = PhabricatorRevision.from_decision_task( queue_service.task(settings.generic_group_id), phabricator_api ) elif settings.phabricator_build_target: - revision = Revision.from_phabricator_trigger( + revision = PhabricatorRevision.from_phabricator_trigger( settings.phabricator_build_target, phabricator_api, ) else: - revision = Revision.from_try_task( + revision = PhabricatorRevision.from_try_task( queue_service.task(settings.try_task_id), queue_service.task(settings.try_group_id), phabricator_api, diff --git a/bot/code_review_bot/report/lando.py b/bot/code_review_bot/report/lando.py index 330bda4ab..21ed73771 100644 --- a/bot/code_review_bot/report/lando.py +++ b/bot/code_review_bot/report/lando.py @@ -6,6 +6,7 @@ from code_review_bot import Level from code_review_bot.report.base import Reporter +from code_review_bot.revisions import PhabricatorRevision logger = structlog.get_logger(__name__) @@ -29,6 +30,11 @@ def publish(self, issues, revision, task_failures, links, reviewers): """ Send an email to administrators """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) + assert ( revision.phabricator_id and revision.phabricator_phid and revision.diff ), "Revision must have a Phabricator ID, a PHID and a diff" diff --git a/bot/code_review_bot/report/mail_builderrors.py b/bot/code_review_bot/report/mail_builderrors.py index 621488264..d1879eb61 100644 --- a/bot/code_review_bot/report/mail_builderrors.py +++ b/bot/code_review_bot/report/mail_builderrors.py @@ -36,7 +36,7 @@ def publish(self, issues, revision, task_failures, links, reviewers): """ assert ( revision.phabricator_id and revision.phabricator_phid - ), "Revision must have a Phabricator ID and PHID" + ), "PhabricatorRevision must have a Phabricator ID and PHID" assert ( "attachments" in revision.diff ), f"Unable to find the commits for revision with phid {revision.phabricator_phid}." diff --git a/bot/code_review_bot/revisions/__init__.py b/bot/code_review_bot/revisions/__init__.py index 88f6feafb..14de3f486 100644 --- a/bot/code_review_bot/revisions/__init__.py +++ b/bot/code_review_bot/revisions/__init__.py @@ -3,3 +3,4 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. from code_review_bot.revisions.base import ImprovementPatch, Revision # noqa +from code_review_bot.revisions.phabricator import PhabricatorRevision # noqa diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py index ab44dbe62..d505aa1c2 100644 --- a/bot/code_review_bot/revisions/phabricator.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -18,7 +18,7 @@ GetAppUserAgent, settings, ) -from code_review_bot.revision import Revision +from code_review_bot.revisions import Revision logger = structlog.get_logger(__name__) @@ -99,7 +99,7 @@ def repo_slug(url): if self.diff_phid: out.append(f"phabricator.diffphid.{self.diff_phid}") - # Revision indexes + # Phabricator revision indexes # Only head changeset is useful to uniquely identify the revision if self.head_repository and self.head_changeset: repo = repo_slug(self.head_repository) @@ -217,7 +217,7 @@ def from_try_task( # Build a revision without repositories as they are retrieved later # when analyzing the full task group - return Revision( + return PhabricatorRevision( phabricator_id=revision["id"], phabricator_phid=phid, diff_id=diff_id, @@ -262,7 +262,7 @@ def from_decision_task(task: dict, phabricator: PhabricatorAPI): head_changeset = task["payload"]["env"]["GECKO_HEAD_REV"] base_changeset = task["payload"]["env"]["GECKO_BASE_REV"] - return Revision( + return PhabricatorRevision( head_changeset=head_changeset, base_changeset=base_changeset, head_repository=head_repository, @@ -325,7 +325,7 @@ def from_phabricator_trigger(build_target_phid: str, phabricator: PhabricatorAPI ) logger.info("Found repository", name=repo_name, phid=repo_phid) - return Revision( + return PhabricatorRevision( phabricator_id=revision["id"], phabricator_phid=revision_phid, diff_id=diff["id"], @@ -379,7 +379,9 @@ def is_blacklisted(self): if author is None: return False - logger.info("Revision from a blacklisted user", revision=self, author=author) + logger.info( + "Phabricator revision from a blacklisted user", revision=self, author=author + ) return True @property diff --git a/bot/code_review_bot/sources/phabricator.py b/bot/code_review_bot/sources/phabricator.py index 56087a12e..1e82b5c20 100644 --- a/bot/code_review_bot/sources/phabricator.py +++ b/bot/code_review_bot/sources/phabricator.py @@ -56,7 +56,7 @@ def __init__(self, request): self.actual_base_revision = None def __str__(self): - return f"Revision {self.revision_id} - {self.target_phid}" + return f"Phabricator revision {self.revision_id} - {self.target_phid}" class PhabricatorActions: @@ -121,17 +121,17 @@ def update_state(self, build): if self.is_visible(build): build.state = PhabricatorBuildState.Public build.revision_url = self.build_revision_url(build) - logger.info("Revision is public", build=str(build)) + logger.info("Phabricator revision is public", build=str(build)) # Check if the build has not expired if self.is_expired_build(build): build.state = PhabricatorBuildState.Expired - logger.info("Revision has expired", build=str(build)) + logger.info("Phabricator revision has expired", build=str(build)) elif retries_left <= 0: # Mark as secured when no retries are left build.state = PhabricatorBuildState.Secured - logger.info("Revision is marked as secure", build=str(build)) + logger.info("Phabricator revision is marked as secure", build=str(build)) else: # Enqueue back to retry later @@ -157,7 +157,9 @@ def is_visible(self, build): if projects.intersection(self.secure_projects): raise Exception("Secure revision") except Exception as e: - logger.info("Revision not accessible", build=str(build), error=str(e)) + logger.info( + "Phabricator revision not accessible", build=str(build), error=str(e) + ) return False return True diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index d4ded08e5..d75d4fc39 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -12,7 +12,7 @@ from code_review_bot import Level, stats from code_review_bot.analysis import ( - RevisionBuild, + PhabricatorRevisionBuild, publish_analysis_lando, publish_analysis_phabricator, ) @@ -20,7 +20,7 @@ from code_review_bot.config import settings from code_review_bot.mercurial import MercurialWorker, Repository, robust_checkout from code_review_bot.report.debug import DebugReporter -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision, Revision from code_review_bot.sources.phabricator import ( PhabricatorActions, PhabricatorBuildState, @@ -275,8 +275,13 @@ def start_analysis(self, revision): url=settings.taskcluster_url, ) + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) + # Initialize Phabricator build using revision - build = RevisionBuild(revision, self.phabricator) + build = PhabricatorRevisionBuild(revision, self.phabricator) # Copy internal Phabricator credentials to setup libmozevent phabricator = PhabricatorActions( @@ -428,7 +433,7 @@ def index(self, revision, **kwargs): """ Index current task on Taskcluster index """ - assert isinstance(revision, Revision) + assert isinstance(revision, Revision), "Must be a Revision instance" if settings.taskcluster.local or self.index_service is None: logger.info("Skipping taskcluster indexing", rev=str(revision), **kwargs) @@ -447,9 +452,10 @@ def index(self, revision, **kwargs): payload["try_task_id"] = settings.try_task_id payload["try_group_id"] = settings.try_group_id - # Always add the repository we are working on + # Add the repository we are working on for Phabricator revisions # This is mainly used by the frontend to list & filter diffs - payload["repository"] = revision.base_repository + if isinstance(revision, PhabricatorRevision): + payload["repository"] = revision.base_repository # Add restartable flag for monitoring payload["monitoring_restart"] = payload["state"] == "error" and payload.get( @@ -664,6 +670,10 @@ def update_status(self, revision, state): """ Update build status on HarborMaster """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) assert isinstance(state, BuildState) if not revision.build_target_phid: logger.info( @@ -684,6 +694,10 @@ def publish_link(self, revision: Revision, slug: str, name: str, url: str): """ Publish a link as a HarborMaster artifact """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) if not revision.build_target_phid: logger.info( "No build target found, skipping HarborMaster link creation", From a8d8adb00c12eb172cd7113f4e99cc2d65800f58 Mon Sep 17 00:00:00 2001 From: Valentin Rigal Date: Fri, 16 Jan 2026 13:11:42 +0100 Subject: [PATCH 4/6] Restore PhabricatorRevision.from_try_task I think the change is related to Github revisions --- bot/code_review_bot/revisions/phabricator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py index d505aa1c2..ef1e5447e 100644 --- a/bot/code_review_bot/revisions/phabricator.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -130,12 +130,12 @@ def __str__(self): return f"Phabricator #{self.diff_id} - {self.diff_phid}" @staticmethod - def from_try_task( - code_review: dict, decision_task: dict, phabricator: PhabricatorAPI - ): + def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): """ Load identifiers from Phabricator, using the remote task description """ + # Load build target phid from the task env + code_review = try_task["extra"]["code-review"] build_target_phid = code_review.get("phabricator-diff") or code_review.get( "phabricator-build-target" ) From a63cf513685c6e702bf6547730124165dec4ce0a Mon Sep 17 00:00:00 2001 From: Valentin Rigal Date: Fri, 16 Jan 2026 13:06:23 +0100 Subject: [PATCH 5/6] Update tests --- bot/tests/conftest.py | 8 +-- bot/tests/test_autoland.py | 4 +- bot/tests/test_index.py | 14 ++-- ...alysis.py => test_phabricator_analysis.py} | 6 +- bot/tests/test_reporter_phabricator.py | 70 ++++++++++++++----- 5 files changed, 69 insertions(+), 33 deletions(-) rename bot/tests/{test_analysis.py => test_phabricator_analysis.py} (97%) diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 30e194e2a..9d75614dc 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -332,10 +332,10 @@ def mock_revision(mock_phabricator, mock_try_task, mock_decision_task, mock_conf """ Mock a mercurial revision """ - from code_review_bot.revisions import Revision + from code_review_bot.revisions import PhabricatorRevision with mock_phabricator as api: - return Revision.from_try_task(mock_try_task, mock_decision_task, api) + return PhabricatorRevision.from_try_task(mock_try_task, mock_decision_task, api) @pytest.fixture @@ -343,10 +343,10 @@ def mock_revision_autoland(mock_phabricator, mock_autoland_task): """ Mock a mercurial revision from autoland repo """ - from code_review_bot.revisions import Revision + from code_review_bot.revisions import PhabricatorRevision with mock_phabricator as api: - return Revision.from_decision_task(mock_autoland_task, api) + return PhabricatorRevision.from_decision_task(mock_autoland_task, api) class Response: diff --git a/bot/tests/test_autoland.py b/bot/tests/test_autoland.py index e7a2275e4..7ca46bc41 100644 --- a/bot/tests/test_autoland.py +++ b/bot/tests/test_autoland.py @@ -2,7 +2,7 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this # file, You can obtain one at http://mozilla.org/MPL/2.0/. -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision def test_revision(mock_autoland_task, mock_phabricator, mock_hgmo): @@ -11,7 +11,7 @@ def test_revision(mock_autoland_task, mock_phabricator, mock_hgmo): """ with mock_phabricator as api: - revision = Revision.from_decision_task(mock_autoland_task, api) + revision = PhabricatorRevision.from_decision_task(mock_autoland_task, api) assert revision.as_dict() == { "bugzilla_id": None, diff --git a/bot/tests/test_index.py b/bot/tests/test_index.py index 277c3b7c3..768bf06d8 100644 --- a/bot/tests/test_index.py +++ b/bot/tests/test_index.py @@ -5,10 +5,10 @@ from unittest import mock from code_review_bot.config import TaskCluster -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision -class MockRevision(Revision): +class MockPhabricatorRevision(PhabricatorRevision): """ Fake revision to easily set properties """ @@ -34,7 +34,7 @@ def test_taskcluster_index(mock_config, mock_workflow, mock_try_task): mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) mock_workflow.index_service = mock.Mock() - rev = MockRevision( + rev = MockPhabricatorRevision( namespaces=["mock.1234"], details={"id": "1234", "someData": "mock", "state": "done"}, repository="test-repo", @@ -84,7 +84,7 @@ def test_index_autoland( """ with mock_phabricator as api: - revision = Revision.from_decision_task(mock_autoland_task, api) + revision = PhabricatorRevision.from_decision_task(mock_autoland_task, api) mock_workflow.index_service = mock.Mock() mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) @@ -134,7 +134,7 @@ def test_index_phabricator( """ with mock_phabricator as api: - revision = Revision.from_phabricator_trigger( + revision = PhabricatorRevision.from_phabricator_trigger( build_target_phid="PHID-HMBT-test", phabricator=api, ) @@ -199,7 +199,9 @@ def test_index_from_try( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) mock_workflow.index_service = mock.Mock() mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) diff --git a/bot/tests/test_analysis.py b/bot/tests/test_phabricator_analysis.py similarity index 97% rename from bot/tests/test_analysis.py rename to bot/tests/test_phabricator_analysis.py index da0a50b7e..9eb499df8 100644 --- a/bot/tests/test_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -7,7 +7,7 @@ from code_review_bot import mercurial from code_review_bot.config import RepositoryConf -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision from code_review_bot.sources.phabricator import PhabricatorActions @@ -18,7 +18,7 @@ def test_revision(mock_phabricator): """ with mock_phabricator as api: - revision = Revision.from_phabricator_trigger( + revision = PhabricatorRevision.from_phabricator_trigger( build_target_phid="PHID-HMBT-test", phabricator=api, ) @@ -93,7 +93,7 @@ def mock_hgrun(cmd): with mock_phabricator as api: mock_workflow.phabricator = api - revision = Revision.from_phabricator_trigger( + revision = PhabricatorRevision.from_phabricator_trigger( build_target_phid="PHID-HMBT-test", phabricator=api, ) diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index 3a9438f75..9fa67ccea 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -13,7 +13,7 @@ from code_review_bot import Level from code_review_bot.report.phabricator import PhabricatorReporter -from code_review_bot.revisions import ImprovementPatch, Revision +from code_review_bot.revisions import ImprovementPatch, PhabricatorRevision from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask from code_review_bot.tasks.clang_tidy import ClangTidyIssue, ClangTidyTask from code_review_bot.tasks.clang_tidy_external import ( @@ -270,7 +270,9 @@ def test_phabricator_clang_tidy( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "another_test.cpp": [41, 42, 43] @@ -306,7 +308,9 @@ def test_phabricator_clang_format( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.cpp": [41, 42, 43], @@ -348,7 +352,9 @@ def test_phabricator_mozlint( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "python/test.py": [41, 42, 43], @@ -437,7 +443,9 @@ def test_phabricator_coverage( Test Phabricator reporter publication on a mock coverage issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -503,7 +511,9 @@ def raise_404(*args, **kwargs): raise HTTPError(response=resp_mock) with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -537,7 +547,9 @@ def test_phabricator_clang_tidy_and_coverage( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -661,7 +673,9 @@ def test_phabricator_analyzers( api.comment = unittest.mock.Mock(return_value=True) # Always use the same setup, only varies the analyzers - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = {"test.cpp": [0, 41, 42, 43], "dom/test.cpp": [42]} revision.id = 52 reporter = PhabricatorReporter( @@ -745,7 +759,9 @@ def test_phabricator_clang_tidy_build_error( from code_review_bot import Level with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.cpp": [41, 42, 43] @@ -803,7 +819,9 @@ def test_full_file( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "xx.cpp": [123, 124, 125] @@ -865,7 +883,9 @@ def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.id = 52 reporter = PhabricatorReporter({"analyzers": ["clang-tidy"]}, api=api) @@ -890,7 +910,9 @@ def test_extra_errors( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = {"path/to/file.py": [1, 2, 3]} revision.files = ["path/to/file.py"] revision.id = 52 @@ -981,7 +1003,9 @@ def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decisio """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.rst": [41, 42, 43], @@ -1029,7 +1053,9 @@ def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.rst": [41, 42, 43], @@ -1065,7 +1091,9 @@ def test_phabricator_external_tidy( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "another_test.cpp": [41, 42, 43] @@ -1116,7 +1144,9 @@ def test_phabricator_newer_diff( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -1194,7 +1224,9 @@ def test_phabricator_former_diff_comparison( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -1342,7 +1374,9 @@ def test_phabricator_before_after_comment( mock_taskcluster_config.secrets = {"BEFORE_AFTER_RATIO": 1} with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], From b785ec125b6555d31e8b4e3d6e32a10584e64b01 Mon Sep 17 00:00:00 2001 From: Valentin Rigal Date: Fri, 16 Jan 2026 15:22:32 +0100 Subject: [PATCH 6/6] Suggestions --- bot/code_review_bot/cli.py | 8 +++++--- bot/code_review_bot/revisions/base.py | 20 ++++++++++++++++++++ bot/code_review_bot/revisions/phabricator.py | 2 +- bot/code_review_bot/workflow.py | 13 ++++++++----- 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 1d620e078..6e2a9a875 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -26,7 +26,7 @@ ) from code_review_bot.config import settings from code_review_bot.report import get_reporters -from code_review_bot.revisions import PhabricatorRevision +from code_review_bot.revisions import PhabricatorRevision, Revision from code_review_bot.tools.libmozdata import setup as setup_libmozdata from code_review_bot.tools.log import init_logger from code_review_bot.workflow import Workflow @@ -160,23 +160,25 @@ def main(): settings.load_user_blacklist(taskcluster.secrets["user_blacklist"], phabricator_api) # Load unique revision - # Only Phabricator revisions are supported for now but it is intended to support generic Revision try: if settings.generic_group_id: + # Only Phabricator revisions is supported from decision task revision = PhabricatorRevision.from_decision_task( queue_service.task(settings.generic_group_id), phabricator_api ) elif settings.phabricator_build_target: + # Only Phabricator revisions is supported from build target revision = PhabricatorRevision.from_phabricator_trigger( settings.phabricator_build_target, phabricator_api, ) else: - revision = PhabricatorRevision.from_try_task( + revision = Revision.from_try_task( queue_service.task(settings.try_task_id), queue_service.task(settings.try_group_id), phabricator_api, ) + except InvalidTrigger as e: logger.info("Early stop analysis due to invalid trigger", error=str(e)) diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py index f15eb54a9..032f7dd70 100644 --- a/bot/code_review_bot/revisions/base.py +++ b/bot/code_review_bot/revisions/base.py @@ -9,6 +9,7 @@ import rs_parsepatch import structlog +from libmozdata.phabricator import PhabricatorAPI from code_review_bot import Issue, stats, taskcluster from code_review_bot.config import ( @@ -235,3 +236,22 @@ def as_dict(self): Outputs a serializable representation of this revision """ raise NotImplementedError + + @staticmethod + def from_try_task( + try_task: dict, decision_task: dict, phabricator: PhabricatorAPI = None + ): + """ + Load identifiers from Phabricator, using the remote task description + """ + from code_review_bot.revisions.phabricator import PhabricatorRevision + + # Load build target phid from the task env + code_review = try_task["extra"]["code-review"] + + if phabricator: + return PhabricatorRevision.from_try_task( + code_review, decision_task, phabricator + ) + else: + raise NotImplementedError diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py index ef1e5447e..8970e19ae 100644 --- a/bot/code_review_bot/revisions/phabricator.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -18,7 +18,7 @@ GetAppUserAgent, settings, ) -from code_review_bot.revisions import Revision +from code_review_bot.revisions.base import Revision logger = structlog.get_logger(__name__) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index d75d4fc39..260a638e3 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -160,6 +160,10 @@ def ingest_revision(self, revision, group_id): """ Simpler workflow to ingest a revision """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) assert ( revision.from_autoland or revision.from_mozilla_central ), "Need a revision from autoland or mozilla-central" @@ -248,6 +252,10 @@ def start_analysis(self, revision): Apply a patch on a local clone and push to try to trigger a new Code review analysis """ logger.info("Starting revision analysis", revision=revision) + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) # Index ASAP Taskcluster task for this revision self.index(revision, state="analysis") @@ -275,11 +283,6 @@ def start_analysis(self, revision): url=settings.taskcluster_url, ) - if not isinstance(revision, PhabricatorRevision): - raise NotImplementedError( - "Only Phabricator revisions are supported for now" - ) - # Initialize Phabricator build using revision build = PhabricatorRevisionBuild(revision, self.phabricator)