Match review-context rules on reviewer groups

Implements the previously-stubbed `review` predicate in the code-review external-content engine so a review-context.toml rule can target a reviewer group (the capability upstream left unimplemented):

- review_context.py: implement _review_matches for `review.reviewer`,
  `review.blocking_reviewer`, and `review.author`. A predicate matches when
  every facet it specifies matches; reviewer groups are matched by Phabricator
  project slug (resolved via resolve_project_phid) against the revision's
  requested reviewers. Thread a ReviewInfo (author + reviewer/blocking group
  PHIDs) through rule_matches/predicate_matches/collect_actions, built from the
  patch in load_external_content_for_diff. Fails closed when no review info is
  available (e.g. a GitHub diff).
- phabricator.py: expose blocking_reviewer_project_phids and author_username,
  refactoring the reviewers fetch into a shared _reviewers cached_property.
- docs/code-review-skills.md: document the now-working `review` predicate.

This replaces the earlier bespoke per-group skill mechanism: teams attach
area guidance by reviewer group through the existing review-context.toml,
with no second skill system. Depends on the reviewer-group plumbing from the
PhabricatorPatch membership change.

Author: msujaws

bugbug/tools/code_review/review_context.py

@@ -150,10 +150,42 @@ def parse_diff_files(diff: str) -> set[str]:
         return files
 
 
+@dataclass(frozen=True)
+class ReviewInfo:
+    """Reviewer-related facts about a revision, used by the ``review`` predicate."""
+
+    author: str | None = None
+    # Reviewer-group (Phabricator project) PHIDs requested on the revision.
+    reviewer_groups: frozenset[str] = frozenset()
+    # The subset of reviewer groups that are *blocking* reviewers.
+    blocking_reviewer_groups: frozenset[str] = frozenset()
+
+
+async def build_review_info(patch) -> ReviewInfo:
+    """Build a ReviewInfo from a patch, tolerating patches that lack the fields."""
+    try:
+        reviewer_groups = frozenset(getattr(patch, "reviewer_project_phids", []) or [])
+        blocking = frozenset(
+            getattr(patch, "blocking_reviewer_project_phids", []) or []
+        )
+        author = getattr(patch, "author_username", None)
+    except Exception:
+        logger.exception("Could not build review info for the patch")
+        return ReviewInfo()
+    return ReviewInfo(
+        author=author,
+        reviewer_groups=reviewer_groups,
+        blocking_reviewer_groups=blocking,
+    )
+
+
 def rule_matches(
-    rule: Rule, changed_files: set[str], bug_component: str | None = None
+    rule: Rule,
+    changed_files: set[str],
+    bug_component: str | None = None,
+    review: ReviewInfo | None = None,
 ) -> bool:
-    return predicate_matches(rule.when, changed_files, bug_component)
+    return predicate_matches(rule.when, changed_files, bug_component, review)
 
 
 def _normalise_path(path: str) -> str:
@@ -188,8 +220,51 @@ def _bugzilla_matches(
     return False
 
 
-def _review_matches(predicate: ReviewPredicate) -> bool:
-    return False
+def _resolve_group_slugs(slugs: list[str]) -> set[str]:
+    """Resolve reviewer-group slugs to project PHIDs (cached, best-effort)."""
+    from bugbug.tools.core.platforms import phabricator as phab
+
+    resolved = set()
+    for slug in slugs:
+        phid = phab.resolve_project_phid(slug)
+        if phid:
+            resolved.add(phid)
+    return resolved
+
+
+def _review_matches(
+    predicate: ReviewPredicate, review: ReviewInfo | None = None
+) -> bool:
+    """Match a revision's reviewers/author against a ReviewPredicate.
+
+    Each specified facet (``author``, ``reviewer``, ``blocking_reviewer``) must
+    match — mirroring FilePredicate's include/exclude/ext conjunction. A
+    predicate with no facets, or a revision with no review info, never matches.
+    """
+    if review is None:
+        return False
+
+    matched_any_facet = False
+
+    if predicate.author:
+        if review.author not in predicate.author:
+            return False
+        matched_any_facet = True
+
+    if predicate.reviewer:
+        if not (review.reviewer_groups & _resolve_group_slugs(predicate.reviewer)):
+            return False
+        matched_any_facet = True
+
+    if predicate.blocking_reviewer:
+        if not (
+            review.blocking_reviewer_groups
+            & _resolve_group_slugs(predicate.blocking_reviewer)
+        ):
+            return False
+        matched_any_facet = True
+
+    return matched_any_facet
 
 
 def _patch_matches(predicate: PatchPredicate) -> bool:
@@ -200,20 +275,21 @@ def predicate_matches(
     predicate: Predicate,
     changed_files: set[str],
     bug_component: str | None = None,
+    review: ReviewInfo | None = None,
 ) -> bool:
     match predicate:
         case AllPredicate(children=children):
             return all(
-                predicate_matches(child, changed_files, bug_component)
+                predicate_matches(child, changed_files, bug_component, review)
                 for child in children
             )
         case AnyPredicate(children=children):
             return any(
-                predicate_matches(child, changed_files, bug_component)
+                predicate_matches(child, changed_files, bug_component, review)
                 for child in children
             )
         case NotPredicate(child=child):
-            return not predicate_matches(child, changed_files, bug_component)
+            return not predicate_matches(child, changed_files, bug_component, review)
         case AnyFilePredicate(predicate=file_predicate):
             return any(_file_matches(file_predicate, f) for f in changed_files)
         case AllFilesPredicate(predicate=file_predicate):
@@ -223,7 +299,7 @@ def predicate_matches(
         case BugzillaPredicate():
             return _bugzilla_matches(predicate, bug_component)
         case ReviewPredicate():
-            return _review_matches(predicate)
+            return _review_matches(predicate, review)
         case PatchPredicate():
             return _patch_matches(predicate)
     raise TypeError(f"Unknown predicate type: {type(predicate).__name__}")
@@ -356,6 +432,7 @@ def collect_actions(
     config: ReviewContextConfig,
     bug_component: str | None = None,
     extra_context_toml: str | None = None,
+    review: ReviewInfo | None = None,
 ) -> list[MatchedAction]:
     """Return deduplicated actions matched from config against the diff.
 
@@ -386,7 +463,7 @@ def collect_actions(
         enumerate(config.rules), key=lambda item: (-item[1].priority, item[0])
     )
     for _, rule in ordered_rules:
-        if not rule_matches(rule, changed_files, bug_component):
+        if not rule_matches(rule, changed_files, bug_component, review):
             continue
         rule_name = rule.name
         new_actions = []
@@ -530,11 +607,15 @@ async def load_external_content_for_diff(
         return []
 
     bug_component: str | None = None
+    review: ReviewInfo | None = None
     if patch is not None:
         bug_component = await patch.bug_component()
+        review = await build_review_info(patch)
 
     try:
-        actions = collect_actions(diff, config, bug_component, extra_context_toml)
+        actions = collect_actions(
+            diff, config, bug_component, extra_context_toml, review
+        )
     except (tomllib.TOMLDecodeError, ReviewContextValidationError):
         logger.exception("Could not parse extra review context")
         return []

bugbug/tools/core/platforms/phabricator.py

@@ -561,11 +561,12 @@ def author_phid(self) -> str:
         return self._revision_metadata["fields"]["authorPHID"]
 
     @cached_property
-    def reviewer_phids(self) -> list[str]:
-        """PHIDs of the revision's reviewers (a mix of users and projects).
+    def _reviewers(self) -> list[dict]:
+        """Raw reviewer entries from the revision's reviewers attachment.
 
         The reviewers attachment is not part of the default revision metadata,
         so we fetch it with a dedicated ``differential.revision.search`` call.
+        Each entry carries ``reviewerPHID`` and ``isBlocking``.
         """
         phabricator = get_phabricator_client()
         response = phabricator.request(
@@ -576,12 +577,12 @@ def reviewer_phids(self) -> list[str]:
         data = response.get("data") or []
         if not data:
             return []
-        reviewers = data[0].get("attachments", {}).get("reviewers", {})
-        return [
-            r["reviewerPHID"]
-            for r in reviewers.get("reviewers", [])
-            if r.get("reviewerPHID")
-        ]
+        return data[0].get("attachments", {}).get("reviewers", {}).get("reviewers", [])
+
+    @property
+    def reviewer_phids(self) -> list[str]:
+        """PHIDs of the revision's reviewers (a mix of users and projects)."""
+        return [r["reviewerPHID"] for r in self._reviewers if r.get("reviewerPHID")]
 
     @property
     def reviewer_project_phids(self) -> list[str]:
@@ -633,6 +634,21 @@ def _add(phid: str) -> None:
 
         return phids
 
+    @property
+    def blocking_reviewer_project_phids(self) -> list[str]:
+        """PHIDs of the revision's *blocking* reviewer groups."""
+        return [
+            r["reviewerPHID"]
+            for r in self._reviewers
+            if r.get("isBlocking")
+            and r.get("reviewerPHID", "").startswith("PHID-PROJ-")
+        ]
+
+    @property
+    def author_username(self) -> str | None:
+        """The revision author's Phabricator username (email), if resolvable."""
+        return self._users_info.get(self.author_phid, {}).get("email")
+
     @property
     def diff_author_phid(self) -> str:
         return self._diff_metadata["authorPHID"]

docs/code-review-skills.md

@@ -83,9 +83,16 @@ The patch's associated bug ID is read from `patch.bug_id` (available on
 component string compared against the rule is `"Product::Component"`.
 
 The schema accepts `bugzilla`, `review`, and `patch` predicates. Runtime
-matching currently implements `bugzilla.component`; `bugzilla.product`,
-`bugzilla.keywords`, `bugzilla.severity`, `review.*`, and `patch.*` are parsed
-and validated but fail closed until trusted metadata is wired into the matcher.
+matching implements `bugzilla.component` and the `review` predicate
+(`review.reviewer`, `review.blocking_reviewer`, `review.author`); a `review`
+predicate matches when every facet it specifies matches, so a rule can target a
+reviewer group (e.g. `review = { reviewer = ["ip-protection-reviewers"] }`).
+Reviewer groups are matched by Phabricator project slug, resolved to the
+revision's requested reviewers. `bugzilla.product`, `bugzilla.keywords`,
+`bugzilla.severity`, and `patch.*` are parsed and validated but fail closed
+until the corresponding metadata is wired into the matcher. When the platform
+provides no reviewer information (e.g. a plain GitHub diff), `review` predicates
+fail closed.
 
 ### `fetch_revision`
 

tests/test_code_review.py

@@ -25,7 +25,9 @@
     search_text,
 )
 from bugbug.tools.code_review.review_context import (
+    ReviewInfo,
     _merge_rules,
+    build_review_info,
     external_content_manifest,
     format_external_content,
     github_repo_allowed,
@@ -39,6 +41,7 @@
     BugzillaPredicate,
     FilePredicate,
     ReviewContextValidationError,
+    ReviewPredicate,
     Rule,
 )
 from bugbug.tools.code_review.review_context_schema import (
@@ -788,6 +791,87 @@ def test_rule_bugzilla_component_matches():
     )
 
 
+def _patch_resolver(monkeypatch, slug_to_phid):
+    monkeypatch.setattr(
+        "bugbug.tools.core.platforms.phabricator.resolve_project_phid",
+        lambda slug: slug_to_phid.get(slug),
+    )
+
+
+def test_rule_review_reviewer_group_matches(monkeypatch):
+    _patch_resolver(monkeypatch, {"ip-protection-reviewers": "PHID-PROJ-ip"})
+    rule = Rule(
+        name="ip",
+        when=ReviewPredicate(reviewer=["ip-protection-reviewers"]),
+        load=[],
+    )
+    review = ReviewInfo(reviewer_groups=frozenset({"PHID-PROJ-ip"}))
+    assert rule_matches(rule, {"any/file.cpp"}, review=review)
+
+
+def test_rule_review_reviewer_group_no_match(monkeypatch):
+    _patch_resolver(monkeypatch, {"ip-protection-reviewers": "PHID-PROJ-ip"})
+    rule = Rule(
+        name="ip",
+        when=ReviewPredicate(reviewer=["ip-protection-reviewers"]),
+        load=[],
+    )
+    review = ReviewInfo(reviewer_groups=frozenset({"PHID-PROJ-other"}))
+    assert not rule_matches(rule, {"any/file.cpp"}, review=review)
+
+
+def test_rule_review_fails_closed_without_review_info(monkeypatch):
+    _patch_resolver(monkeypatch, {"ip-protection-reviewers": "PHID-PROJ-ip"})
+    rule = Rule(
+        name="ip",
+        when=ReviewPredicate(reviewer=["ip-protection-reviewers"]),
+        load=[],
+    )
+    # No review info (e.g. a GitHub diff) → never matches.
+    assert not rule_matches(rule, {"any/file.cpp"})
+
+
+def test_rule_review_blocking_reviewer(monkeypatch):
+    _patch_resolver(monkeypatch, {"security-reviewers": "PHID-PROJ-sec"})
+    rule = Rule(
+        name="sec",
+        when=ReviewPredicate(blocking_reviewer=["security-reviewers"]),
+        load=[],
+    )
+    # Present as a reviewer but not blocking → no match.
+    non_blocking = ReviewInfo(reviewer_groups=frozenset({"PHID-PROJ-sec"}))
+    assert not rule_matches(rule, {"f.cpp"}, review=non_blocking)
+    # Blocking → match.
+    blocking = ReviewInfo(
+        reviewer_groups=frozenset({"PHID-PROJ-sec"}),
+        blocking_reviewer_groups=frozenset({"PHID-PROJ-sec"}),
+    )
+    assert rule_matches(rule, {"f.cpp"}, review=blocking)
+
+
+def test_rule_review_author():
+    rule = Rule(name="a", when=ReviewPredicate(author=["alice@mozilla.com"]), load=[])
+    assert rule_matches(rule, {"f.cpp"}, review=ReviewInfo(author="alice@mozilla.com"))
+    assert not rule_matches(
+        rule, {"f.cpp"}, review=ReviewInfo(author="bob@mozilla.com")
+    )
+
+
+@pytest.mark.asyncio
+async def test_build_review_info_from_patch():
+    from types import SimpleNamespace
+
+    patch = SimpleNamespace(
+        reviewer_project_phids=["PHID-PROJ-ip", "PHID-PROJ-newtab"],
+        blocking_reviewer_project_phids=["PHID-PROJ-ip"],
+        author_username="alice@mozilla.com",
+    )
+    info = await build_review_info(patch)
+    assert info.author == "alice@mozilla.com"
+    assert info.reviewer_groups == frozenset({"PHID-PROJ-ip", "PHID-PROJ-newtab"})
+    assert info.blocking_reviewer_groups == frozenset({"PHID-PROJ-ip"})
+
+
 @pytest.mark.asyncio
 async def test_patch_bug_component():
     from bugbug.tools.core.platforms.phabricator import PhabricatorPatch