From b6a15d5cb2597913873ef47992a86c09797cd29b Mon Sep 17 00:00:00 2001 From: Ben Hearsum Date: Thu, 17 Sep 2026 15:10:16 -0400 Subject: [PATCH 1/5] refactor: put revision loading and workflow handling calls next to one another build/test workflows will load revisions the same way that linting workflows do, but they will need to call a separate `Workflow` method. We'll be able to distinguish between the two by looking at the decision task parameters of the `try_group_id`. By putting these next to each other, we can call the right method without needing to leak the parameters out of this block into areas where they aren't appropriate. --- bot/code_review_bot/cli.py | 64 ++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 73e00a412..35961a7ec 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -171,6 +171,19 @@ def main(): # We need Phabricator API to list black-listed users settings.load_user_blacklist(taskcluster.secrets["user_blacklist"], phabricator_api) + # Run workflow according to source + w = Workflow( + reporters, + index_service, + queue_service, + phabricator_api, + taskcluster.secrets["ZERO_COVERAGE_ENABLED"], + # Update build status only when phabricator reporting is enabled + update_build=phabricator_reporting_enabled, + task_failures_ignored=taskcluster.secrets["task_failures_ignored"], + ) + + revision = None # Load unique revision try: if settings.generic_group_id: @@ -178,6 +191,7 @@ def main(): revision = PhabricatorRevision.from_decision_task( queue_service.task(settings.generic_group_id), phabricator_api ) + w.ingest_revision(revision, settings.generic_group_id) elif settings.phabricator_build_target: # Only Phabricator revisions is supported from build target revision = PhabricatorRevision.from_phabricator_trigger( @@ -186,12 +200,14 @@ def main(): ) if revision is None: return 0 + w.start_analysis(revision) else: revision = Revision.from_try_task( queue_service.task(settings.try_task_id), queue_service.task(settings.try_group_id), phabricator_api, ) + w.run(revision) except InvalidTrigger as e: logger.info("Early stop analysis due to invalid trigger", error=str(e)) @@ -204,40 +220,22 @@ def main(): # Stop cleanly as we just want to ignore that case, but report on sentry through warning return 0 except Exception as e: - # Report revision loading failure on production only - # On testing or dev instances, we can use different Phabricator - # configuration that do not match all the pulse messages sent - if settings.on_production: - raise - - else: - logger.info( - "Failed to load revision", - task=settings.try_task_id, - error=str(e), - phabricator=phabricator["url"], - ) - return 1 + if not revision: + # Report revision loading failure on production only + # On testing or dev instances, we can use different Phabricator + # configuration that do not match all the pulse messages sent + if settings.on_production: + raise + + else: + logger.info( + "Failed to load revision", + task=settings.try_task_id, + error=str(e), + phabricator=phabricator["url"], + ) + return 1 - # Run workflow according to source - w = Workflow( - reporters, - index_service, - queue_service, - phabricator_api, - taskcluster.secrets["ZERO_COVERAGE_ENABLED"], - # Update build status only when phabricator reporting is enabled - update_build=phabricator_reporting_enabled, - task_failures_ignored=taskcluster.secrets["task_failures_ignored"], - ) - try: - if settings.generic_group_id: - w.ingest_revision(revision, settings.generic_group_id) - elif settings.phabricator_build_target: - w.start_analysis(revision) - else: - w.run(revision) - except Exception as e: # Log errors to papertrail logger.error( "Static analysis failure", revision=revision, error=e, exc_info=True From 16d47535d99abb4b0f519835481ed1cd6c7226ae Mon Sep 17 00:00:00 2001 From: Ben Hearsum Date: Thu, 17 Sep 2026 15:27:23 -0400 Subject: [PATCH 2/5] feat: fetch build target phids from decision task parameters While these are available in the `code-review` task metadata for the existing linting tasks, upcoming work to add builds and tests on phabricator revisions will not have such tasks. Luckily, all reviewbot pushes to Try already have `phabricator_diff` in their parameters, and we can fetch it from there. Note that while this makes `try_task_id` unneeded in `cli.py`, it's still needed in `workflow.py` to find the tasks that need to be analyzed. --- bot/code_review_bot/__init__.py | 3 + bot/code_review_bot/cli.py | 13 +++- bot/code_review_bot/revisions/base.py | 6 +- bot/code_review_bot/revisions/phabricator.py | 6 +- bot/tests/conftest.py | 18 ++--- bot/tests/test_index.py | 5 +- bot/tests/test_reporter_github.py | 6 +- bot/tests/test_reporter_phabricator.py | 69 ++++++++------------ 8 files changed, 56 insertions(+), 70 deletions(-) diff --git a/bot/code_review_bot/__init__.py b/bot/code_review_bot/__init__.py index 7a99afde1..47c06447d 100644 --- a/bot/code_review_bot/__init__.py +++ b/bot/code_review_bot/__init__.py @@ -11,6 +11,9 @@ import requests import structlog + +# Workaround https://github.com/taskcluster/taskcluster/issues/9172 +import taskcluster.download from libmozdata.phabricator import LintResult, UnitResult, UnitResultState from taskcluster.helper import TaskclusterConfig diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 35961a7ec..c6de18d25 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -16,6 +16,7 @@ UnitResult, UnitResultState, ) +from taskcluster.download import downloadArtifactToBuf from code_review_bot import ( AnalysisException, @@ -202,11 +203,19 @@ def main(): return 0 w.start_analysis(revision) else: + decision_task = queue_service.task(settings.try_group_id) + rawParams, _ = downloadArtifactToBuf( + taskId=settings.try_group_id, + name="public/parameters.yml", + queueService=queue_service, + ) + parameters = yaml.safe_load(bytes(rawParams)) revision = Revision.from_try_task( - queue_service.task(settings.try_task_id), - queue_service.task(settings.try_group_id), + decision_task, + parameters["phabricator_diff"], phabricator_api, ) + w.run(revision) except InvalidTrigger as e: diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py index e59e70290..b75e6cdad 100644 --- a/bot/code_review_bot/revisions/base.py +++ b/bot/code_review_bot/revisions/base.py @@ -275,7 +275,9 @@ def serialize(self): raise NotImplementedError @staticmethod - def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorAPI): + def from_try_task( + decision_task: dict, build_target_phid: str, phabricator: PhabricatorAPI + ): """ Load identifiers from Phabricator or Github, using the remote task description """ @@ -294,5 +296,5 @@ def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorA ) else: return PhabricatorRevision.from_try_task( - try_task["extra"]["code-review"], decision_task, phabricator + decision_task, build_target_phid, phabricator ) diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py index 4b5d5cef1..70a37a820 100644 --- a/bot/code_review_bot/revisions/phabricator.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -133,15 +133,11 @@ def __str__(self): @staticmethod def from_try_task( - code_review: dict, decision_task: dict, phabricator: PhabricatorAPI + decision_task: dict, build_target_phid: str, phabricator: PhabricatorAPI ): """ Load identifiers from Phabricator, using the remote task description """ - # Load build target phid from the task env - build_target_phid = code_review.get("phabricator-diff") or code_review.get( - "phabricator-build-target" - ) assert ( build_target_phid is not None ), "Missing phabricator-build-target or phabricator-diff declaration" diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 9faff79f2..8a1a7e98b 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -381,14 +381,6 @@ def mock_github(mock_config): ) -@pytest.fixture -def mock_try_task(): - """ - Mock a remote Try task definition - """ - return {"extra": {"code-review": {"phabricator-diff": "PHID-HMBT-test"}}} - - @pytest.fixture def mock_github_decision_task(): """ @@ -446,14 +438,14 @@ def mock_autoland_task(): @pytest.fixture -def mock_revision(mock_phabricator, mock_try_task, mock_decision_task, mock_config): +def mock_revision(mock_phabricator, mock_decision_task, mock_config): """ Mock a mercurial revision """ from code_review_bot.revisions import PhabricatorRevision, Revision with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) return revision @@ -470,15 +462,13 @@ def mock_revision_autoland(mock_phabricator, mock_autoland_task): @pytest.fixture -def mock_github_revision( - mock_github, mock_try_task, mock_github_decision_task, mock_config -): +def mock_github_revision(mock_github, mock_github_decision_task, mock_config): """ Mock a github revision """ from code_review_bot.revisions import GithubRevision, Revision - revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None) + revision = Revision.from_try_task(mock_github_decision_task, "PHID-HMBT-test", None) assert isinstance(revision, GithubRevision) return revision diff --git a/bot/tests/test_index.py b/bot/tests/test_index.py index d0e0df4da..89c49dcde 100644 --- a/bot/tests/test_index.py +++ b/bot/tests/test_index.py @@ -26,7 +26,7 @@ def as_dict(self): return self._details -def test_taskcluster_index(mock_config, mock_workflow, mock_try_task): +def test_taskcluster_index(mock_config, mock_workflow): """ Test the Taskcluster indexing API by mocking an online taskcluster state @@ -188,7 +188,6 @@ def test_index_phabricator( def test_index_from_try( mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_workflow, mock_config, @@ -199,7 +198,7 @@ def test_index_from_try( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) mock_workflow.index_service = mock.Mock() diff --git a/bot/tests/test_reporter_github.py b/bot/tests/test_reporter_github.py index 64465adb2..75bee8337 100644 --- a/bot/tests/test_reporter_github.py +++ b/bot/tests/test_reporter_github.py @@ -22,7 +22,6 @@ def test_github_review( mock_github, mock_config, phab, - mock_try_task, mock_github_decision_task, mock_task, mock_backend_secret, @@ -30,7 +29,7 @@ def test_github_review( """ Report 2 clang tidy issues by pushing a review to a Github pull request """ - revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None) + revision = Revision.from_try_task(mock_github_decision_task, "PHID-HMBT-test", None) assert isinstance(revision, GithubRevision) revision.lines = { # Add dummy lines diff @@ -169,13 +168,12 @@ def test_github_review_cleanup( mock_github, mock_config, phab, - mock_try_task, mock_github_decision_task, mock_task, mock_backend_secret, ): """In case no issue is found, previous reviews are dismissed""" - revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None) + revision = Revision.from_try_task(mock_github_decision_task, "PHID-HMBT-test", None) revision.lines = {} revision.files = ["test.txt", "test.cpp", "another_test.cpp"] revision.id = 52 diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index 2e242c78c..9a251e027 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -262,15 +262,13 @@ """ -def test_phabricator_clang_tidy( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task -): +def test_phabricator_clang_tidy(mock_phabricator, phab, mock_decision_task, mock_task): """ Test Phabricator reporter publication on a mock clang-tidy issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -300,14 +298,14 @@ def test_phabricator_clang_tidy( def test_phabricator_clang_format( - mock_config, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_config, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication on a mock clang-format issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -342,7 +340,7 @@ def test_phabricator_clang_format( def test_phabricator_mozlint( - mock_config, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_config, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication on two mock mozlint issues @@ -350,7 +348,7 @@ def test_phabricator_mozlint( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -432,7 +430,6 @@ def test_phabricator_coverage( mock_config, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, ): @@ -440,7 +437,7 @@ 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 = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -493,7 +490,6 @@ def test_phabricator_no_coverage_on_deleted_file( mock_config, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, ): @@ -507,7 +503,7 @@ 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 = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -533,7 +529,6 @@ def test_phabricator_clang_tidy_and_coverage( mock_config, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, ): @@ -542,7 +537,7 @@ 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 = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -654,7 +649,6 @@ def test_phabricator_analyzers( valid_patches, mock_config, mock_phabricator, - mock_try_task, mock_decision_task, mock_task, ): @@ -667,7 +661,7 @@ 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 = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = {"test.cpp": [0, 41, 42, 43], "dom/test.cpp": [42]} revision.id = 52 @@ -743,7 +737,7 @@ def test_phabricator_analyzers( def test_phabricator_clang_tidy_build_error( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator Lint for a ClangTidyIssue with build error @@ -752,7 +746,7 @@ 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 = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -803,15 +797,13 @@ def test_phabricator_clang_tidy_build_error( assert phab.comments[51] == [VALID_BUILD_ERROR_MESSAGE] -def test_full_file( - mock_config, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task -): +def test_full_file(mock_config, mock_phabricator, phab, mock_decision_task, mock_task): """ Test Phabricator reporter supports an issue on a full file """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -868,13 +860,13 @@ def test_full_file( ] -def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task): +def test_task_failures(mock_phabricator, phab, mock_decision_task): """ Test Phabricator reporter publication with some task failures """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.id = 52 reporter = PhabricatorReporter({"analyzers": ["clang-tidy"]}, api=api) @@ -892,15 +884,13 @@ def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task assert phab.comments[51] == [VALID_TASK_FAILURES_MESSAGE] -def test_extra_errors( - mock_phabricator, mock_try_task, mock_decision_task, phab, mock_task -): +def test_extra_errors(mock_phabricator, mock_decision_task, phab, mock_task): """ Test Phabricator reporter publication with some errors outside of patch """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = {"path/to/file.py": [1, 2, 3]} revision.files = ["path/to/file.py"] @@ -986,13 +976,13 @@ def test_extra_errors( assert phab.comments[51] == [VALID_MOZLINT_MESSAGE] -def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decision_task): +def test_phabricator_notices(mock_phabricator, phab, mock_decision_task): """ Test Phabricator reporter publication on a mock clang-format issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1035,13 +1025,13 @@ def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decisio ] -def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision_task): +def test_phabricator_tgdiff(mock_phabricator, phab, mock_decision_task): """ Test Phabricator reporter publication on a mock clang-format issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1066,14 +1056,14 @@ def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision def test_phabricator_external_tidy( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication on a mock external-tidy issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1118,14 +1108,14 @@ def test_phabricator_external_tidy( def test_phabricator_newer_diff( - monkeypatch, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + monkeypatch, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication won't be called when a newer diff exists for the patch """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1194,7 +1184,7 @@ def test_phabricator_newer_diff( def test_phabricator_former_diff_comparison( - monkeypatch, mock_phabricator, phab, mock_try_task, mock_decision_task, mock_task + monkeypatch, mock_phabricator, phab, mock_decision_task, mock_task ): """ Test Phabricator reporter publication shows the number of unresolved @@ -1204,7 +1194,7 @@ def test_phabricator_former_diff_comparison( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff @@ -1338,7 +1328,6 @@ def test_phabricator_before_after_comment( monkeypatch, mock_phabricator, phab, - mock_try_task, mock_decision_task, mock_task, mock_taskcluster_config, @@ -1353,7 +1342,7 @@ 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 = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) assert isinstance(revision, PhabricatorRevision) revision.lines = { # Add dummy lines diff From c9758020b6a9b32f6ca5ec9a38085d7acdb7b318 Mon Sep 17 00:00:00 2001 From: Ben Hearsum Date: Thu, 17 Sep 2026 15:27:23 -0400 Subject: [PATCH 3/5] don't require TRY_TASK_ID to enter publication mode The existing publication mode for linting checks requires this, but this won't exist when we start dealing with builds & tests (we'll be looking at the entire `try_group_id` instead). Given this, we can't require it to be there to have `try_group_id` set in the settings. --- bot/code_review_bot/config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index 8fe6aeb25..1dac29ace 100644 --- a/bot/code_review_bot/config.py +++ b/bot/code_review_bot/config.py @@ -90,8 +90,9 @@ def setup( taskcluster_parallel_requests=None, ): # Detect source from env - if "TRY_TASK_ID" in os.environ and "TRY_TASK_GROUP_ID" in os.environ: + if "TRY_TASK_ID" in os.environ: self.try_task_id = os.environ["TRY_TASK_ID"] + if "TRY_TASK_GROUP_ID" in os.environ: self.try_group_id = os.environ["TRY_TASK_GROUP_ID"] elif "GENERIC_TASK_GROUP_ID" in os.environ: self.generic_group_id = os.environ["GENERIC_TASK_GROUP_ID"] From 0a133d2df1400b1ddfa3e7b6bbe1375d89a6381b Mon Sep 17 00:00:00 2001 From: Ben Hearsum Date: Fri, 18 Sep 2026 13:15:56 -0400 Subject: [PATCH 4/5] refactor: wire in support for kicking off additional analysis modes This requires two main things: 1) The ability to signal a mode when the bot starts 2) Raising the currently hardcoded taskgraph parameters up to `start_analysis`, to allow them to be set differently for different modes This will allow us to start additional analyses for builds, tests, etc. by setting the appropriate parameters when pushing to Try. --- bot/code_review_bot/analysis.py | 5 +++++ bot/code_review_bot/cli.py | 2 +- bot/code_review_bot/config.py | 7 +++++++ bot/code_review_bot/vcs.py | 14 ++++++------- bot/code_review_bot/workflow.py | 16 +++++++++++++-- bot/tests/test_mercurial.py | 28 ++++++++++++++++---------- bot/tests/test_phabricator_analysis.py | 5 +++-- bot/tests/test_testing_policy.py | 8 ++++---- 8 files changed, 58 insertions(+), 27 deletions(-) diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index e6b33427d..f859b2d3a 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -1,3 +1,4 @@ +import enum from functools import cached_property import structlog @@ -17,6 +18,10 @@ ) +class AnalysisMode(enum.Enum): + Lint = 1 + + class PhabricatorRevisionBuild(PhabricatorBuild): """ Convert the bot revision into a libmozevent compatible build diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index c6de18d25..9bb4be260 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -201,7 +201,7 @@ def main(): ) if revision is None: return 0 - w.start_analysis(revision) + w.start_analysis(revision, settings.analysis_mode) else: decision_task = queue_service.task(settings.try_group_id) rawParams, _ = downloadArtifactToBuf( diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index 1dac29ace..73a98c398 100644 --- a/bot/code_review_bot/config.py +++ b/bot/code_review_bot/config.py @@ -14,6 +14,8 @@ import structlog +from code_review_bot.analysis import AnalysisMode + REPO_MOZILLA_CENTRAL = "https://hg.mozilla.org/mozilla-central" REPO_AUTOLAND = "https://hg.mozilla.org/integration/autoland" @@ -47,6 +49,7 @@ def __init__(self): self.try_group_id = None self.generic_group_id = None self.phabricator_build_target = None + self.analysis_mode = None self.repositories = [] self.decision_env_prefixes = [] @@ -102,6 +105,10 @@ def setup( assert self.phabricator_build_target.startswith( "PHID-HMBT" ), f"Not a phabrication build target PHID: {self.phabricator_build_target}" + # TODO: remove the default after we can be certain it will always be + # present + mode = os.environ.get("ANALYSIS_MODE", "Lint") + self.analysis_mode = AnalysisMode[mode] else: raise Exception("Only TRY mode is supported") diff --git a/bot/code_review_bot/vcs.py b/bot/code_review_bot/vcs.py index 3dd5d9d8e..f669be19c 100644 --- a/bot/code_review_bot/vcs.py +++ b/bot/code_review_bot/vcs.py @@ -162,7 +162,7 @@ def apply_build(self, build): logger.info("Applying patch", phid=patch.phid, message=message) self.apply_patch(patch, message, commit) - def add_try_commit(self, build): + def add_try_commit(self, build, extra_parameters): """ Build and commit the file configuring try with try_task_config.json and the code-review workflow parameters in JSON @@ -171,11 +171,11 @@ def add_try_commit(self, build): config = { "version": 2, "parameters": { - "target_tasks_method": "codereview", - "optimize_target_tasks": True, "phabricator_diff": build.target_phid, }, } + config["parameters"].update(extra_parameters) + diff_phid = build.stack[-1].phid if build.revision_url: @@ -213,7 +213,7 @@ def __init__( ): self.skippable_files = skippable_files - def run(self, repository, build): + def run(self, repository, build, extra_parameters): """ Apply the stack of patches from the build, handling retries in case of try server errors @@ -228,7 +228,7 @@ def run(self, repository, build): ) try: - return self.handle_build(repository, build) + return self.handle_build(repository, build, extra_parameters) except RetryNeeded: build.retries += 1 @@ -289,7 +289,7 @@ def format_error(self, error): """Extract a readable error log from a VCS exception""" raise NotImplementedError - def handle_build(self, repository, build): + def handle_build(self, repository, build, extra_parameters): """ Try to load and apply a diff on local clone If successful, push to try and send a treeherder link @@ -321,7 +321,7 @@ def handle_build(self, repository, build): ) # Configure the try task - repository.add_try_commit(build) + repository.add_try_commit(build, extra_parameters) # Then push that stack on try tip = repository.push_to_try() diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 38a3f262d..8f20d38c6 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -16,6 +16,7 @@ from code_review_bot import Level, stats from code_review_bot.analysis import ( + AnalysisMode, PhabricatorRevisionBuild, publish_analysis_lando, publish_analysis_phabricator, @@ -273,7 +274,9 @@ def _build_tasks(tasks): # Publish issues in the backend self.backend_api.publish_issues(issues, revision) - def start_analysis(self, revision: PhabricatorRevision) -> None: + def start_analysis( + self, revision: PhabricatorRevision, analysis_mode: AnalysisMode + ): """ Apply a patch on a local clone and push to try to trigger a new Code review analysis """ @@ -365,9 +368,18 @@ def start_analysis(self, revision: PhabricatorRevision) -> None: # We'll clone the required repository repository.clone() + parameters = {} + if analysis_mode == AnalysisMode.Lint: + parameters.update( + { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + } + ) + # Apply the stack of patches and push to try worker = MercurialWorker() - output = worker.run(repository, build) + output = worker.run(repository, build, parameters) # Cancel any in-progress tasks from an earlier update # This is done after pushing to try to avoid delaying runs of the diff --git a/bot/tests/test_mercurial.py b/bot/tests/test_mercurial.py index 890ce892f..28b215153 100644 --- a/bot/tests/test_mercurial.py +++ b/bot/tests/test_mercurial.py @@ -73,6 +73,12 @@ def test_robustcheckout(monkeypatch): ] +LINT_EXTRA_PARAMS = { + "optimize_target_tasks": True, + "target_tasks_method": "codereview", +} + + def test_push_to_try(PhabricatorMock, mock_mc, responses): """ Run mercurial worker on a single diff @@ -108,7 +114,7 @@ def test_push_to_try(PhabricatorMock, mock_mc, responses): ) worker = mercurial.MercurialWorker() - result = worker.run(mock_mc, build) + result = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was generated tip = mock_mc.repo.tip() @@ -209,7 +215,7 @@ def _readme(content): assert not os.path.exists(config) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_mc.repo.tip() @@ -315,7 +321,7 @@ def test_dont_push_skippable_files_to_try(PhabricatorMock, mock_mc): worker = mercurial.MercurialWorker( skippable_files=["test.txt"], ) - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_mc.repo.tip() @@ -386,7 +392,7 @@ def test_treeherder_link(PhabricatorMock, mock_mc): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_mc.repo.tip() @@ -436,7 +442,7 @@ def boom(*args): mock_mc.apply_build = boom worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the unit result was published @@ -485,7 +491,7 @@ def test_failure_mercurial(PhabricatorMock, mock_config, mock_mc): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued assert mode == "fail:mercurial" @@ -558,7 +564,7 @@ def test_push_to_try_nss(PhabricatorMock, mock_nss): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_nss, build) + mode, out_build, details = worker.run(mock_nss, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued tip = mock_nss.repo.tip() @@ -638,7 +644,7 @@ def test_crash_utf8_author(PhabricatorMock, mock_mc): # Run the mercurial worker on that patch only worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(mock_mc, build) + mode, out_build, details = worker.run(mock_mc, build, LINT_EXTRA_PARAMS) # Check we have the patch with utf-8 author properly applied assert [(c.author, c.desc) for c in mock_mc.repo.log()] == [ @@ -708,7 +714,7 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): repository_mock.retries = 0 worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(repository_mock, build) + mode, out_build, details = worker.run(repository_mock, build, LINT_EXTRA_PARAMS) assert mode == "success" assert out_build == build @@ -765,7 +771,7 @@ def test_push_failure_max_retries(PhabricatorMock, mock_mc, monkeypatch): ) worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(repository_mock, build) + mode, out_build, details = worker.run(repository_mock, build, LINT_EXTRA_PARAMS) # Check the treeherder link was queued assert build.retries == 3 @@ -836,7 +842,7 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): worker = mercurial.MercurialWorker() - mode, out_build, details = worker.run(repository_mock, build) + mode, out_build, details = worker.run(repository_mock, build, LINT_EXTRA_PARAMS) assert repository_mock.push_to_try.call_count == 2 assert mode == "success" diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index 8aa95c31a..965048d94 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -12,6 +12,7 @@ from code_review_bot import mercurial from code_review_bot.analysis import ( + AnalysisMode, publish_analysis_phabricator, ) from code_review_bot.config import RepositoryConf @@ -116,7 +117,7 @@ def test_workflow_private_build( phabricator=api, ) - assert mock_workflow.start_analysis(revision) is None + assert mock_workflow.start_analysis(revision, AnalysisMode.Lint) is None # No clone nor push happened assert hgrun_calls == [] @@ -172,7 +173,7 @@ def mock_hgrun(cmd): phabricator=api, ) - mock_workflow.start_analysis(revision) + mock_workflow.start_analysis(revision, AnalysisMode.Lint) # Check hgrun initial call to clone mozilla central through robust checkout assert hgrun_calls == [ diff --git a/bot/tests/test_testing_policy.py b/bot/tests/test_testing_policy.py index a3c67dfbe..93b638b29 100644 --- a/bot/tests/test_testing_policy.py +++ b/bot/tests/test_testing_policy.py @@ -373,7 +373,7 @@ def test_api_failure_is_not_fatal(api, revision): def test_phabricator_reporter_sets_tag( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_backend_secret + mock_phabricator, phab, mock_decision_task, mock_backend_secret ): """ The Phabricator reporter sets the tag on a documentation-only revision @@ -382,7 +382,7 @@ def test_phabricator_reporter_sets_tag( from code_review_bot.revisions import Revision with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) revision.lines = {"docs/index.rst": [1, 2], "dom/base/test/test_foo.html": [3]} revision.files = list(revision.lines.keys()) revision.id = 52 @@ -397,7 +397,7 @@ def test_phabricator_reporter_sets_tag( def test_phabricator_reporter_skips_code_changes( - mock_phabricator, phab, mock_try_task, mock_decision_task, mock_backend_secret + mock_phabricator, phab, mock_decision_task, mock_backend_secret ): """ The Phabricator reporter does not tag a revision modifying code @@ -406,7 +406,7 @@ def test_phabricator_reporter_skips_code_changes( from code_review_bot.revisions import Revision with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = Revision.from_try_task(mock_decision_task, "PHID-HMBT-test", api) revision.lines = {"docs/index.rst": [1, 2], "dom/base/nsDocument.cpp": [3]} revision.files = list(revision.lines.keys()) revision.id = 52 From cfb49991186f56bf0ce6f42b9c12af037f25bed2 Mon Sep 17 00:00:00 2001 From: Ben Hearsum Date: Fri, 18 Sep 2026 13:36:53 -0400 Subject: [PATCH 5/5] refactor: wire in support for publishing results for additional analysis modes This is similar to the previous commit in that we need a signal to the bot to indicate which mode to use, and to handle it. The details differ, however: * Our best signal for which mode to operate in comes from the decision task parameters, so that choice is deferred to cli.py. This is because the only way to process non-lint groups is to fire on task group completion (there will be no `code-review` task for these, and thus no way to feed that signal into the code review task payload itself). * `Workflow.run` is very clearly oriented specifically towards linting, and we'll need a substantially different version of it to process build and test results. For this reason, it's been moved entirely to its own method, with the intention of providing a new method when support for build and test results is added. This is different than `start_analysis`, where the only difference is in the parameters that get fed into the try push. --- bot/code_review_bot/cli.py | 10 +++++++++- bot/code_review_bot/workflow.py | 6 +++++- bot/tests/test_default.py | 3 ++- bot/tests/test_remote.py | 31 ++++++++++++++++--------------- bot/tests/test_workflow.py | 3 ++- 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 9bb4be260..e86eb1798 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -25,6 +25,7 @@ stats, taskcluster, ) +from code_review_bot.analysis import AnalysisMode from code_review_bot.config import settings from code_review_bot.report import get_reporters from code_review_bot.revisions import PhabricatorRevision, Revision @@ -216,7 +217,14 @@ def main(): phabricator_api, ) - w.run(revision) + analysis_mode = None + if parameters["target_tasks_method"] == "codereview": + analysis_mode = AnalysisMode.Lint + + if not analysis_mode: + raise Exception("Cannot detect analysis mode; cannot proceed!") + + w.run(revision, analysis_mode) except InvalidTrigger as e: logger.info("Early stop analysis due to invalid trigger", error=str(e)) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 8f20d38c6..572421cd4 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -105,10 +105,14 @@ def __init__( # Is local clone already setup ? self.clone_available = False - def run(self, revision): + def run(self, revision, analysis_mode: AnalysisMode): """ Find all issues on remote tasks and publish them """ + if analysis_mode == AnalysisMode.Lint: + return self._run_lint(revision) + + def _run_lint(self, revision): # Index ASAP Taskcluster task for this revision self.index(revision, state="started") diff --git a/bot/tests/test_default.py b/bot/tests/test_default.py index 404ab4377..bf0dc54fa 100644 --- a/bot/tests/test_default.py +++ b/bot/tests/test_default.py @@ -5,6 +5,7 @@ import pytest from code_review_bot import Level +from code_review_bot.analysis import AnalysisMode from code_review_bot.tasks.default import DefaultIssue, DefaultTask @@ -65,7 +66,7 @@ def test_parser(mock_workflow, mock_revision, mock_hgmo, mock_backend): mock_workflow.backend_api.publish_revision = lambda rev: {} mock_revision.id = 1337 - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues.pop() diff --git a/bot/tests/test_remote.py b/bot/tests/test_remote.py index 8af91b1a0..baa37007a 100644 --- a/bot/tests/test_remote.py +++ b/bot/tests/test_remote.py @@ -7,6 +7,7 @@ from libmozdata.phabricator import BuildState from code_review_bot import stats +from code_review_bot.analysis import AnalysisMode @pytest.fixture @@ -52,7 +53,7 @@ def test_no_deps( ) with pytest.raises(AssertionError) as e: - mock_workflow.run(mock_revision) + mock_workflow.run(mock_revision, AnalysisMode.Lint) assert str(e.value) == "No task dependencies to analyze" @@ -112,7 +113,7 @@ def test_baseline( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 issue = issues[0] @@ -168,7 +169,7 @@ def test_no_failed( "extra-task": {}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -196,13 +197,13 @@ def test_no_issues( "extra-task": {}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Fail # Now mark that task failure as ignorable mock_workflow.task_failures_ignored = ["source-test-mozlint-flake8"] - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -248,7 +249,7 @@ def test_build_status_fail_on_error( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 assert mock_revision._state == BuildState.Fail @@ -294,7 +295,7 @@ def test_build_status_pass_on_warning( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 assert mock_revision._state == BuildState.Pass @@ -320,7 +321,7 @@ def test_unsupported_analyzer( "extra-task": {}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -357,7 +358,7 @@ def test_mozlint_task( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues[0] assert isinstance(issue, MozLintIssue) @@ -428,7 +429,7 @@ def test_clang_tidy_task( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 2 issue = issues[0] assert isinstance(issue, ClangTidyIssue) @@ -509,7 +510,7 @@ def test_clang_format_task( } mock_workflow.setup_mock_tasks(tasks) assert len(mock_revision.improvement_patches) == 0 - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues[0] assert isinstance(issue, ClangFormatIssue) @@ -576,7 +577,7 @@ def test_no_tasks( "remoteTryTask": {"dependencies": ["decision", "someOtherDockerbuild"]}, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass @@ -612,12 +613,12 @@ def test_zero_coverage_option( ) mock_workflow.zero_coverage_enabled = False - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 0 assert mock_revision._state == BuildState.Pass mock_workflow.zero_coverage_enabled = True - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 assert isinstance(issues[0], CoverageIssue) assert mock_revision._state == BuildState.Pass @@ -661,7 +662,7 @@ def test_external_tidy_task( }, } ) - issues = mock_workflow.run(mock_revision) + issues = mock_workflow.run(mock_revision, AnalysisMode.Lint) assert len(issues) == 1 issue = issues[0] assert isinstance(issue, ExternalTidyIssue) diff --git a/bot/tests/test_workflow.py b/bot/tests/test_workflow.py index 29eb99f21..3f901a8fc 100644 --- a/bot/tests/test_workflow.py +++ b/bot/tests/test_workflow.py @@ -12,6 +12,7 @@ import responses from libmozdata.phabricator import ConduitError +from code_review_bot.analysis import AnalysisMode from code_review_bot.config import Settings, TaskCluster from code_review_bot.revisions import PhabricatorRevision from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask @@ -238,7 +239,7 @@ def test_before_after(mock_taskcluster_config, mock_workflow, mock_task, mock_re # Set backend ID as the publication is disabled for tests mock_revision.id = 1337 assert mock_revision.before_after_feature is True - mock_workflow.run(mock_revision) + mock_workflow.run(mock_revision, AnalysisMode.Lint) assert mock_workflow.publish.call_args_list == [ mock.call( mock_revision,