diff --git a/bugbug/tools/code_review/agent.py b/bugbug/tools/code_review/agent.py index 0520f6b1a8..d6f6233399 100644 --- a/bugbug/tools/code_review/agent.py +++ b/bugbug/tools/code_review/agent.py @@ -226,15 +226,21 @@ async def generate_review_comments( ) -> tuple[list[GeneratedReviewComment], list[dict]]: external_context = "" manifest: list[dict] = [] - if self._review_context_repo: + review_context_repo = self._review_context_repo + review_context_branch = self._review_context_branch + if review_context_repo is None: + repo_ref = await patch.github_repo_ref() + if repo_ref: + review_context_repo, review_context_branch = repo_ref + if review_context_repo: from bugbug.tools.code_review.review_context import ( load_external_context_for_review, ) external_context, manifest = await load_external_context_for_review( patch, - self._review_context_repo, - review_context_branch=self._review_context_branch, + review_context_repo, + review_context_branch=review_context_branch, extra_context_toml=self._extra_context_toml, content_overrides=self._content_overrides, ) diff --git a/bugbug/tools/core/platforms/base.py b/bugbug/tools/core/platforms/base.py index ec89451715..a147a59224 100644 --- a/bugbug/tools/core/platforms/base.py +++ b/bugbug/tools/core/platforms/base.py @@ -77,6 +77,15 @@ async def bug_component(self) -> Optional[str]: """Return 'Product::Component' for the associated bug, or None.""" return None + async def github_repo_ref(self) -> Optional[tuple[str, str]]: + """Return the ('org/project', branch) of the patch's GitHub mirror, or None. + + The branch is the one holding review-context.toml. Used as the + default `review_context_repo`/`review_context_branch` when the + caller doesn't pass them explicitly. + """ + return None + @abstractmethod async def get_base_revision(self) -> Optional[str]: """Return the VCS revision the patch was written against, or None.""" diff --git a/bugbug/tools/core/platforms/phabricator.py b/bugbug/tools/core/platforms/phabricator.py index 39baabd645..4b99842d85 100644 --- a/bugbug/tools/core/platforms/phabricator.py +++ b/bugbug/tools/core/platforms/phabricator.py @@ -33,6 +33,32 @@ # Mozilla-operated bot accounts that should be treated as trusted TRUSTED_BOT_PHIDS = {REVIEWBOT_PHID, REVIEWHELPER_PHID} +# Maps Phabricator repository callsigns to the (GitHub mirror, branch) where +# that repo's review-context.toml lives. Repos not listed here have no known +# GitHub mirror, so no external review context is loaded by default. Some of +# these review-context.toml files don't exist yet Once one is +# added the corresponding repo starts working automatically. +PHABRICATOR_REPO_TO_GITHUB = { + "FIREFOXAUTOLAND": ("mozilla-firefox/firefox", "autoland"), + "FIREFOXBETA": ("mozilla-firefox/firefox", "beta"), + "FIREFOXRELEASE": ("mozilla-firefox/firefox", "release"), + "FIREFOXESRONEFOURZERO": ("mozilla-firefox/firefox", "esr140"), + "FIREFOXESRONEFIVETHREE": ("mozilla-firefox/firefox", "esr153"), + "FIREFOXESRONEONEFIVE": ("mozilla-firefox/firefox", "esr115"), + "THUNDERBIRDDESKTOPMAIN": ("thunderbird/thunderbird-desktop", "main"), + "THUNDERBIRDDESKTOPBETA": ("thunderbird/thunderbird-desktop", "beta"), + "THUNDERBIRDDESKTOPRELEASE": ("thunderbird/thunderbird-desktop", "release"), + "THUNDERBIRDDESKTOPESRONEFOURZERO": ( + "thunderbird/thunderbird-desktop", + "esr140", + ), + "THUNDERBIRDDESKTOPESRONEFIVETHRE": ( + "thunderbird/thunderbird-desktop", + "esr153", + ), + "NSS": ("mozilla/nss", "master"), +} + # Messages used when redacting untrusted content UNTRUSTED_CONTENT_REDACTED = "[Content from untrusted user removed for security]" REDACTED_TITLE = "[Unvalidated revision title redacted for security]" @@ -87,6 +113,23 @@ def resolve_project_phid(slug: str) -> Optional[str]: return data[0]["phid"] +@cache +def _repo_callsign(repository_phid: str) -> Optional[str]: + """Resolve a Phabricator repository PHID to its callsign. + + Cached for the process lifetime; callsigns are effectively static. + """ + phabricator = get_phabricator_client() + response = phabricator.request( + "diffusion.repository.search", + constraints={"phids": [repository_phid]}, + ) + data = response.get("data") or [] + if not data: + return None + return data[0]["fields"].get("callsign") + + @cache def get_project_members(project_phid: str) -> frozenset[str]: """Return the set of user PHIDs that are members of a Phabricator project. @@ -524,6 +567,22 @@ def _fetch() -> Optional[str]: except Exception: return None + @alru_cache + async def github_repo_ref(self) -> Optional[tuple[str, str]]: + repository_phid = self._revision_metadata["fields"].get("repositoryPHID") + if not repository_phid: + return None + + import asyncio + + callsign = await asyncio.get_event_loop().run_in_executor( + None, _repo_callsign, repository_phid + ) + if not callsign: + return None + + return PHABRICATOR_REPO_TO_GITHUB.get(callsign) + @property def bug_id(self) -> int: return int(self._revision_metadata["fields"]["bugzilla.bug-id"]) diff --git a/tests/test_code_review.py b/tests/test_code_review.py index 8a533766da..ffe63b4468 100644 --- a/tests/test_code_review.py +++ b/tests/test_code_review.py @@ -1352,3 +1352,87 @@ def test_run_appends_scope_suggestion_last(): last = result.review_comments[-1] assert last.content == "Split this patch into smaller pieces" assert last.order == 2 + + +# --------------------------------------------------------------------------- +# review_context_repo default: falls back to patch.github_repo_ref() +# --------------------------------------------------------------------------- + + +def _make_review_tool(review_context_repo=None): + """Build a CodeReviewTool without running __init__ (avoids create_agent).""" + pytest.importorskip("langchain") + from bugbug.tools.code_review.agent import AgentResponse, CodeReviewTool + + tool = CodeReviewTool.__new__(CodeReviewTool) + tool.target_software = "Mozilla Firefox" + tool.is_experiment_env = False + tool.review_comments_db = None + tool.show_patch_example = False + tool._review_context_repo = review_context_repo + tool._review_context_branch = "main" + tool._extra_context_toml = None + tool._content_overrides = None + + async def fake_astream(*args, **kwargs): + yield {"structured_response": AgentResponse(comments=[])} + + tool.agent = SimpleNamespace(astream=fake_astream) + return tool + + +def _make_review_patch(github_repo_ref_return=None): + patch_set = PatchSet.from_string("--- a/f.txt\n+++ b/f.txt\n@@ -0,0 +1,1 @@\n+a\n") + return SimpleNamespace( + raw_diff="diff", + patch_set=patch_set, + github_repo_ref=AsyncMock(return_value=github_repo_ref_return), + ) + + +def test_generate_review_comments_falls_back_to_patch_github_repo(): + tool = _make_review_tool(review_context_repo=None) + fake_patch = _make_review_patch( + github_repo_ref_return=("mozilla-firefox/firefox", "autoland"), + ) + + with patch( + "bugbug.tools.code_review.review_context.load_external_context_for_review", + new=AsyncMock(return_value=("", [{"name": "x"}])), + ) as loader: + asyncio.run(tool.generate_review_comments(fake_patch, "summary")) + + fake_patch.github_repo_ref.assert_awaited_once() + loader.assert_awaited_once() + assert loader.await_args.args[1] == "mozilla-firefox/firefox" + assert loader.await_args.kwargs["review_context_branch"] == "autoland" + + +def test_generate_review_comments_explicit_repo_skips_patch_lookup(): + tool = _make_review_tool(review_context_repo="explicit/repo") + fake_patch = _make_review_patch( + github_repo_ref_return=("mozilla-firefox/firefox", "main") + ) + + with patch( + "bugbug.tools.code_review.review_context.load_external_context_for_review", + new=AsyncMock(return_value=("", [])), + ) as loader: + asyncio.run(tool.generate_review_comments(fake_patch, "summary")) + + fake_patch.github_repo_ref.assert_not_awaited() + assert loader.await_args.args[1] == "explicit/repo" + + +def test_generate_review_comments_no_repo_configured_or_known(): + tool = _make_review_tool(review_context_repo=None) + fake_patch = _make_review_patch(github_repo_ref_return=None) + + with patch( + "bugbug.tools.code_review.review_context.load_external_context_for_review", + new=AsyncMock(return_value=("", [])), + ) as loader: + asyncio.run(tool.generate_review_comments(fake_patch, "summary")) + + fake_patch.github_repo_ref.assert_awaited_once() + loader.assert_not_awaited() diff --git a/tests/test_phabricator.py b/tests/test_phabricator.py index 5fe1e72a62..57cb3ab1dd 100644 --- a/tests/test_phabricator.py +++ b/tests/test_phabricator.py @@ -6,6 +6,8 @@ from datetime import timedelta from unittest.mock import MagicMock +import pytest + from bugbug import phabricator from bugbug.tools.core.platforms import phabricator as phab_platform @@ -449,3 +451,82 @@ def _get_transactions(self): patch = FakePatch() # Degrades to the current snapshot rather than raising. assert patch.historical_reviewer_project_phids == ["PHID-PROJ-current"] + + +# --------------------------------------------------------------------------- +# PhabricatorPatch.github_repo_ref() -> review_context_repo/branch defaults +# --------------------------------------------------------------------------- + + +def test_repo_callsign(monkeypatch) -> None: + phab_platform._repo_callsign.cache_clear() + response = { + "diffusion.repository.search": { + "data": [{"fields": {"callsign": "FIREFOXAUTOLAND"}}] + } + } + monkeypatch.setattr( + phab_platform, "get_phabricator_client", lambda: _fake_client(response) + ) + assert phab_platform._repo_callsign("PHID-REPO-autoland") == "FIREFOXAUTOLAND" + phab_platform._repo_callsign.cache_clear() + + +def test_repo_callsign_not_found(monkeypatch) -> None: + phab_platform._repo_callsign.cache_clear() + monkeypatch.setattr( + phab_platform, + "get_phabricator_client", + lambda: _fake_client({"diffusion.repository.search": {"data": []}}), + ) + assert phab_platform._repo_callsign("PHID-REPO-missing") is None + phab_platform._repo_callsign.cache_clear() + + +class _FakePatchWithRepo(phab_platform.PhabricatorPatch): + def __init__(self, repository_phid=None): + self._repository_phid = repository_phid + + @property + def _revision_metadata(self): + return {"fields": {"repositoryPHID": self._repository_phid}} + + +@pytest.mark.asyncio +async def test_github_repo_known_callsign(monkeypatch) -> None: + phab_platform._repo_callsign.cache_clear() + response = { + "diffusion.repository.search": { + "data": [{"fields": {"callsign": "FIREFOXAUTOLAND"}}] + } + } + monkeypatch.setattr( + phab_platform, "get_phabricator_client", lambda: _fake_client(response) + ) + + patch = _FakePatchWithRepo("PHID-REPO-autoland") + assert await patch.github_repo_ref() == ("mozilla-firefox/firefox", "autoland") + phab_platform._repo_callsign.cache_clear() + + +@pytest.mark.asyncio +async def test_github_repo_unmapped_callsign(monkeypatch) -> None: + phab_platform._repo_callsign.cache_clear() + response = { + "diffusion.repository.search": { + "data": [{"fields": {"callsign": "COMMCENTRAL"}}] + } + } + monkeypatch.setattr( + phab_platform, "get_phabricator_client", lambda: _fake_client(response) + ) + + patch = _FakePatchWithRepo("PHID-REPO-comm") + assert await patch.github_repo_ref() is None + phab_platform._repo_callsign.cache_clear() + + +@pytest.mark.asyncio +async def test_github_repo_no_repository_phid() -> None: + patch = _FakePatchWithRepo(None) + assert await patch.github_repo_ref() is None