From 755fcd9dbbc5b196627246e70e8d43a1b24ecb67 Mon Sep 17 00:00:00 2001 From: Paul Adenot Date: Mon, 3 Aug 2026 14:26:01 +0200 Subject: [PATCH 1/2] code_review: default review_context_repo from the patch's own repo Previously review_context_repo had to be passed explicitly to every review. Add Patch.github_repo()/github_repo_branch() hooks (default None/"main") so each platform can answer this from what it already knows. PhabricatorPatch resolves it by mapping the revision's repository callsign to its GitHub mirror and branch. The agent only consults this when the caller didn't pass review_context_repo explicitly, so existing callers are unaffected. --- bugbug/tools/code_review/agent.py | 12 ++- bugbug/tools/core/platforms/base.py | 15 ++++ bugbug/tools/core/platforms/phabricator.py | 67 +++++++++++++++++ tests/test_code_review.py | 86 ++++++++++++++++++++++ tests/test_phabricator.py | 84 +++++++++++++++++++++ 5 files changed, 261 insertions(+), 3 deletions(-) diff --git a/bugbug/tools/code_review/agent.py b/bugbug/tools/code_review/agent.py index 0520f6b1a8..735997cb58 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: + review_context_repo = await patch.github_repo() + if review_context_repo: + review_context_branch = await patch.github_repo_branch() + 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..a793e84f84 100644 --- a/bugbug/tools/core/platforms/base.py +++ b/bugbug/tools/core/platforms/base.py @@ -77,6 +77,21 @@ async def bug_component(self) -> Optional[str]: """Return 'Product::Component' for the associated bug, or None.""" return None + async def github_repo(self) -> Optional[str]: + """Return the 'org/project' GitHub mirror of the patch's source repo, or None. + + Used as the default `review_context_repo` when the caller doesn't + pass one explicitly. + """ + return None + + async def github_repo_branch(self) -> str: + """Return the branch of `github_repo()` holding review-context.toml. + + Only consulted when `github_repo()` returns non-None. + """ + return "main" + @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..f138d6110b 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,30 @@ def _fetch() -> Optional[str]: except Exception: return None + @alru_cache + async def _github_mirror(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) + + async def github_repo(self) -> Optional[str]: + mirror = await self._github_mirror() + return mirror[0] if mirror else None + + async def github_repo_branch(self) -> str: + mirror = await self._github_mirror() + return mirror[1] if mirror else "main" + @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..c25d67c13b 100644 --- a/tests/test_code_review.py +++ b/tests/test_code_review.py @@ -1352,3 +1352,89 @@ 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() +# --------------------------------------------------------------------------- + + +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_return=None, github_repo_branch_return="main"): + 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=AsyncMock(return_value=github_repo_return), + github_repo_branch=AsyncMock(return_value=github_repo_branch_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_return="mozilla-firefox/firefox", + github_repo_branch_return="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.assert_awaited_once() + fake_patch.github_repo_branch.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_return="mozilla-firefox/firefox") + + 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.assert_not_awaited() + fake_patch.github_repo_branch.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_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.assert_awaited_once() + loader.assert_not_awaited() diff --git a/tests/test_phabricator.py b/tests/test_phabricator.py index 5fe1e72a62..d756aaacd6 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,85 @@ 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() -> review_context_repo default +# --------------------------------------------------------------------------- + + +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() == "mozilla-firefox/firefox" + assert await patch.github_repo_branch() == "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() is None + assert await patch.github_repo_branch() == "main" + 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() is None + assert await patch.github_repo_branch() == "main" From b5ba7e8454111ab5eacefe9c60019438ff6f329d Mon Sep 17 00:00:00 2001 From: Paul Adenot Date: Tue, 4 Aug 2026 14:00:54 +0200 Subject: [PATCH 2/2] Tie github_repo and github_repo_branch together to convey the fact that it's all or nothing --- bugbug/tools/code_review/agent.py | 6 +++--- bugbug/tools/core/platforms/base.py | 16 +++++---------- bugbug/tools/core/platforms/phabricator.py | 10 +-------- tests/test_code_review.py | 24 ++++++++++------------ tests/test_phabricator.py | 11 ++++------ 5 files changed, 24 insertions(+), 43 deletions(-) diff --git a/bugbug/tools/code_review/agent.py b/bugbug/tools/code_review/agent.py index 735997cb58..d6f6233399 100644 --- a/bugbug/tools/code_review/agent.py +++ b/bugbug/tools/code_review/agent.py @@ -229,9 +229,9 @@ async def generate_review_comments( review_context_repo = self._review_context_repo review_context_branch = self._review_context_branch if review_context_repo is None: - review_context_repo = await patch.github_repo() - if review_context_repo: - review_context_branch = await patch.github_repo_branch() + 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, diff --git a/bugbug/tools/core/platforms/base.py b/bugbug/tools/core/platforms/base.py index a793e84f84..a147a59224 100644 --- a/bugbug/tools/core/platforms/base.py +++ b/bugbug/tools/core/platforms/base.py @@ -77,21 +77,15 @@ async def bug_component(self) -> Optional[str]: """Return 'Product::Component' for the associated bug, or None.""" return None - async def github_repo(self) -> Optional[str]: - """Return the 'org/project' GitHub mirror of the patch's source repo, or None. + async def github_repo_ref(self) -> Optional[tuple[str, str]]: + """Return the ('org/project', branch) of the patch's GitHub mirror, or None. - Used as the default `review_context_repo` when the caller doesn't - pass one explicitly. + 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 - async def github_repo_branch(self) -> str: - """Return the branch of `github_repo()` holding review-context.toml. - - Only consulted when `github_repo()` returns non-None. - """ - return "main" - @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 f138d6110b..4b99842d85 100644 --- a/bugbug/tools/core/platforms/phabricator.py +++ b/bugbug/tools/core/platforms/phabricator.py @@ -568,7 +568,7 @@ def _fetch() -> Optional[str]: return None @alru_cache - async def _github_mirror(self) -> Optional[tuple[str, str]]: + async def github_repo_ref(self) -> Optional[tuple[str, str]]: repository_phid = self._revision_metadata["fields"].get("repositoryPHID") if not repository_phid: return None @@ -583,14 +583,6 @@ async def _github_mirror(self) -> Optional[tuple[str, str]]: return PHABRICATOR_REPO_TO_GITHUB.get(callsign) - async def github_repo(self) -> Optional[str]: - mirror = await self._github_mirror() - return mirror[0] if mirror else None - - async def github_repo_branch(self) -> str: - mirror = await self._github_mirror() - return mirror[1] if mirror else "main" - @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 c25d67c13b..ffe63b4468 100644 --- a/tests/test_code_review.py +++ b/tests/test_code_review.py @@ -1355,7 +1355,7 @@ def test_run_appends_scope_suggestion_last(): # --------------------------------------------------------------------------- -# review_context_repo default: falls back to patch.github_repo() +# review_context_repo default: falls back to patch.github_repo_ref() # --------------------------------------------------------------------------- @@ -1381,21 +1381,19 @@ async def fake_astream(*args, **kwargs): return tool -def _make_review_patch(github_repo_return=None, github_repo_branch_return="main"): +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=AsyncMock(return_value=github_repo_return), - github_repo_branch=AsyncMock(return_value=github_repo_branch_return), + 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_return="mozilla-firefox/firefox", - github_repo_branch_return="autoland", + github_repo_ref_return=("mozilla-firefox/firefox", "autoland"), ) with patch( @@ -1404,8 +1402,7 @@ def test_generate_review_comments_falls_back_to_patch_github_repo(): ) as loader: asyncio.run(tool.generate_review_comments(fake_patch, "summary")) - fake_patch.github_repo.assert_awaited_once() - fake_patch.github_repo_branch.assert_awaited_once() + 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" @@ -1413,7 +1410,9 @@ def test_generate_review_comments_falls_back_to_patch_github_repo(): 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_return="mozilla-firefox/firefox") + 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", @@ -1421,14 +1420,13 @@ def test_generate_review_comments_explicit_repo_skips_patch_lookup(): ) as loader: asyncio.run(tool.generate_review_comments(fake_patch, "summary")) - fake_patch.github_repo.assert_not_awaited() - fake_patch.github_repo_branch.assert_not_awaited() + 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_return=None) + fake_patch = _make_review_patch(github_repo_ref_return=None) with patch( "bugbug.tools.code_review.review_context.load_external_context_for_review", @@ -1436,5 +1434,5 @@ def test_generate_review_comments_no_repo_configured_or_known(): ) as loader: asyncio.run(tool.generate_review_comments(fake_patch, "summary")) - fake_patch.github_repo.assert_awaited_once() + 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 d756aaacd6..57cb3ab1dd 100644 --- a/tests/test_phabricator.py +++ b/tests/test_phabricator.py @@ -454,7 +454,7 @@ def _get_transactions(self): # --------------------------------------------------------------------------- -# PhabricatorPatch.github_repo() -> review_context_repo default +# PhabricatorPatch.github_repo_ref() -> review_context_repo/branch defaults # --------------------------------------------------------------------------- @@ -505,8 +505,7 @@ async def test_github_repo_known_callsign(monkeypatch) -> None: ) patch = _FakePatchWithRepo("PHID-REPO-autoland") - assert await patch.github_repo() == "mozilla-firefox/firefox" - assert await patch.github_repo_branch() == "autoland" + assert await patch.github_repo_ref() == ("mozilla-firefox/firefox", "autoland") phab_platform._repo_callsign.cache_clear() @@ -523,13 +522,11 @@ async def test_github_repo_unmapped_callsign(monkeypatch) -> None: ) patch = _FakePatchWithRepo("PHID-REPO-comm") - assert await patch.github_repo() is None - assert await patch.github_repo_branch() == "main" + 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() is None - assert await patch.github_repo_branch() == "main" + assert await patch.github_repo_ref() is None