Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions bugbug/tools/code_review/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down
9 changes: 9 additions & 0 deletions bugbug/tools/core/platforms/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down
59 changes: 59 additions & 0 deletions bugbug/tools/core/platforms/phabricator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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"])
Expand Down
84 changes: 84 additions & 0 deletions tests/test_code_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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=("<external_context/>", [{"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=("<external_context/>", [])),
) 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()
81 changes: 81 additions & 0 deletions tests/test_phabricator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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