Update (base update)

[ghstack-poisoned]

Author: jeanschmidt

greenlight/src/greenlight/github_client.py

@@ -11,12 +11,12 @@
 from typing import TYPE_CHECKING
 
 from greenlight import constants
-from greenlight.pr_hash import HumanEvent, PRFingerprint, compute_pr_hash, is_bot
+from greenlight.pr_hash import HumanEvent, PRFingerprint, compute_pr_hash, is_bot, is_bot_command
 from greenlight.review_gate import ReviewSkip, human_review_skip_reason
 from greenlight.state import naive_utc
 
 if TYPE_CHECKING:
-    from collections.abc import Iterable
+    from collections.abc import Iterable, Iterator
     from datetime import datetime
 
     from github import Github
@@ -29,6 +29,7 @@
         _AuthorClient,
         _FingerprintPR,
         _PRActor,
+        _PRComment,
         _PRReview,
         _RepoClient,
     )
@@ -213,6 +214,26 @@ def _actor_login(
     return login
 
 
+def _fingerprint_events(
+    events: Iterable[_PRComment | _PRReview],
+    self_login: str | None,
+    authorized_logins: frozenset[str] | None,
+) -> Iterator[HumanEvent]:
+    """Yield a ``HumanEvent`` for each event passing BOTH fingerprint filters.
+
+    Shared across all three sources so every source filters identically: an event is kept only
+    when ``_actor_login`` attributes it (non-ghost, non-bot, non-self, authorized) AND its body
+    is not a bot command (``is_bot_command``), so a trusted author's ``@pytorchbot merge`` never
+    enters the fingerprint.
+    """
+    for event in events:
+        if _actor_login(event.user, self_login, authorized_logins) is None:
+            continue
+        if is_bot_command(event.body):
+            continue
+        yield HumanEvent(id=event.id, body=event.body)
+
+
 def build_pr_fingerprint(
     pr: _FingerprintPR,
     *,
@@ -235,25 +256,15 @@ def build_pr_fingerprint(
     the verifier MUST pass the identically-resolved set or their digests diverge.
 
     Coverage: the fingerprint covers ``head_sha`` and the ``id`` and ``body`` of
-    non-bot, non-self human events (issue comments, review comments, and reviews). It
-    deliberately EXCLUDES the changed files, event kind/author/state/timestamp, and the
-    PR title/body.
+    non-bot, non-self human events (issue comments, review comments, and reviews),
+    EXCLUDING any whose body is a bot command (contains a bot-command @-mention such as
+    ``@pytorchbot merge``; see ``is_bot_command``). It also deliberately EXCLUDES the
+    changed files, event kind/author/state/timestamp, and the PR title/body.
     """
     human_events: list[HumanEvent] = []
-    for comment in pr.get_issue_comments():
-        if _actor_login(comment.user, self_login, authorized_logins) is None:
-            continue
-        human_events.append(HumanEvent(id=comment.id, body=comment.body))
-
-    for review_comment in pr.get_review_comments():
-        if _actor_login(review_comment.user, self_login, authorized_logins) is None:
-            continue
-        human_events.append(HumanEvent(id=review_comment.id, body=review_comment.body))
-
-    for review in reviews:
-        if _actor_login(review.user, self_login, authorized_logins) is None:
-            continue
-        human_events.append(HumanEvent(id=review.id, body=review.body))
+    human_events.extend(_fingerprint_events(pr.get_issue_comments(), self_login, authorized_logins))
+    human_events.extend(_fingerprint_events(pr.get_review_comments(), self_login, authorized_logins))
+    human_events.extend(_fingerprint_events(reviews, self_login, authorized_logins))
 
     return PRFingerprint(
         head_sha=pr.head.sha,

greenlight/src/greenlight/pr_hash.py

@@ -16,6 +16,12 @@
 ``trymerge.py`` authorization check: that is case-sensitive and scoped to the rules whose
 file patterns match a single PR, so its set diverges from this full lowercased union and
 yields a different digest -- refusing every land.
+
+Bot-command comments/reviews are excluded too: any comment or review whose body contains a
+bot-command @-mention (see ``BOT_COMMAND_MENTIONS`` / ``is_bot_command``) is dropped whole, so a
+trusted author's ``@pytorchbot merge`` never perturbs the digest. This is part of the same
+byte-identical cross-process contract -- the land-time verifier MUST import ``is_bot_command`` and
+MUST NOT reimplement it, or its digest diverges and it refuses every land.
 """
 
 from __future__ import annotations
@@ -25,7 +31,7 @@
 from dataclasses import asdict, dataclass
 from typing import Any
 
-HASH_SCHEME_VERSION = 5
+HASH_SCHEME_VERSION = 6
 
 BOT_LOGINS: frozenset[str] = frozenset(
     {
@@ -44,6 +50,16 @@
     }
 )
 
+# Lowercase; ``@pytorchbot`` is NOT a substring of ``@pytorchmergebot``, so both are listed.
+BOT_COMMAND_MENTIONS: frozenset[str] = frozenset(
+    {
+        "@pytorchbot",
+        "@pytorchmergebot",
+        "@claude",
+        "@greenlight",
+    }
+)
+
 
 def is_bot(login: str | None, user_type: str | None = None) -> bool:
     if user_type is not None and user_type.lower() == "bot":
@@ -56,6 +72,13 @@ def is_bot(login: str | None, user_type: str | None = None) -> bool:
     return normalized_login in BOT_LOGINS
 
 
+def is_bot_command(body: str | None) -> bool:
+    if not body:
+        return False
+    normalized_body = body.lower()
+    return any(mention in normalized_body for mention in BOT_COMMAND_MENTIONS)
+
+
 @dataclass(frozen=True, slots=True)
 class HumanEvent:
     id: int

greenlight/tests/test_github_client.py

@@ -690,6 +690,89 @@ def test_build_pr_fingerprint_self_login_excluded_even_when_authorized():
     assert fingerprint.human_events == (HumanEvent(id=1, body="authorized other"),)
 
 
+def test_build_pr_fingerprint_excludes_bot_command_bodies_across_all_sources():
+    pr = _FakePR(
+        issue_comments=[
+            _FakeComment(1, _FakeActor("alice", "User"), "please fix"),
+            _FakeComment(2, _FakeActor("alice", "User"), "@pytorchbot merge"),
+        ],
+        review_comments=[
+            _FakeComment(3, _FakeActor("bob", "User"), "nit: rename this"),
+            _FakeComment(4, _FakeActor("bob", "User"), "@pytorchmergebot rebase"),
+        ],
+        reviews=[
+            _FakeReview(5, _FakeActor("carol", "User"), "lgtm"),
+            _FakeReview(6, _FakeActor("carol", "User"), "@claude take a look"),
+        ],
+    )
+
+    fingerprint = _build_fp(pr)
+
+    # One bot-command body per source is dropped whole; the three normal human comments survive, so a
+    # trusted author's '@pytorchbot merge' never perturbs the digest.
+    assert fingerprint.human_events == (
+        HumanEvent(id=1, body="please fix"),
+        HumanEvent(id=3, body="nit: rename this"),
+        HumanEvent(id=5, body="lgtm"),
+    )
+
+
+def test_build_pr_fingerprint_bot_command_excluded_on_top_of_authorized_filter():
+    pr = _FakePR(
+        issue_comments=[
+            _FakeComment(1, _FakeActor("alice", "User"), "genuine review note"),
+            _FakeComment(2, _FakeActor("alice", "User"), "@greenlight status"),
+        ],
+        review_comments=[],
+        reviews=[],
+    )
+
+    # alice is authorized, so the author filter keeps both comments; the bot-command body is still
+    # dropped, proving is_bot_command runs in addition to (not instead of) the author filter.
+    fingerprint = _build_fp(pr, authorized_logins=frozenset({"alice"}))
+
+    assert fingerprint.human_events == (HumanEvent(id=1, body="genuine review note"),)
+
+
+def test_build_pr_fingerprint_bot_command_does_not_change_eval_hash():
+    authorized = frozenset({"alice"})
+    base = _FakePR(
+        issue_comments=[
+            _FakeComment(1, _FakeActor("alice", "User"), "please fix"),
+            _FakeComment(2, _FakeActor("alice", "User"), "one more thing"),
+        ],
+        review_comments=[],
+        reviews=[],
+    )
+    base_hash = compute_pr_hash(_build_fp(base, authorized_logins=authorized))
+
+    with_bot_command = _FakePR(
+        issue_comments=[
+            _FakeComment(1, _FakeActor("alice", "User"), "please fix"),
+            _FakeComment(2, _FakeActor("alice", "User"), "one more thing"),
+            _FakeComment(3, _FakeActor("alice", "User"), "@pytorchbot merge"),
+        ],
+        review_comments=[],
+        reviews=[],
+    )
+    # A '@pytorchbot merge' from the same authorized author is dropped whole, so the eval_hash is
+    # unchanged and the scan does not re-dispatch a review over it.
+    assert compute_pr_hash(_build_fp(with_bot_command, authorized_logins=authorized)) == base_hash
+
+    with_normal_comment = _FakePR(
+        issue_comments=[
+            _FakeComment(1, _FakeActor("alice", "User"), "please fix"),
+            _FakeComment(2, _FakeActor("alice", "User"), "one more thing"),
+            _FakeComment(3, _FakeActor("alice", "User"), "actually, rename this"),
+        ],
+        review_comments=[],
+        reviews=[],
+    )
+    # Control: swapping only that one body for a non-command comment moves the eval_hash, proving
+    # the equality above is not vacuous.
+    assert compute_pr_hash(_build_fp(with_normal_comment, authorized_logins=authorized)) != base_hash
+
+
 def test_fingerprint_pr_threads_authorized_logins():
     pr = _FakePR(
         issue_comments=[
@@ -735,12 +818,12 @@ def _golden_pr() -> _FakePR:
     )
 
 
-def test_build_pr_fingerprint_golden_hash_scheme_v5():
-    """End-to-end golden: build_pr_fingerprint -> compute_pr_hash pins the scheme-v5 digest.
+def test_build_pr_fingerprint_golden_hash_scheme_v6():
+    """End-to-end golden: build_pr_fingerprint -> compute_pr_hash pins the current-scheme (v6) digest.
 
-    Guards against drift in is_bot / BOT_LOGINS / self_login exclusion and the
-    PR-field mapping. Uses the default scheme_version (5); a future
-    HASH_SCHEME_VERSION bump regenerates this literal.
+    Guards against drift in is_bot / BOT_LOGINS / is_bot_command / self_login exclusion and the
+    PR-field mapping. Uses the default scheme_version (6); a future HASH_SCHEME_VERSION bump
+    regenerates this literal.
     """
     pr = _golden_pr()
     fingerprint = _build_fp(pr, self_login="greenlight")
@@ -749,7 +832,7 @@ def test_build_pr_fingerprint_golden_hash_scheme_v5():
         HumanEvent(id=1, body="please fix"),
         HumanEvent(id=6, body="lgtm"),
     )
-    assert compute_pr_hash(fingerprint) == "9d0506bd3e887a00d858f49e653cab9f913f185674a475582706cc83fcae70d4"
+    assert compute_pr_hash(fingerprint) == "51a4e58faef3da88e3fe8506437a21d0d7e8060256a2428c410d37a2e8122926"
 
 
 @pytest.mark.parametrize("null_login", [None, ""])
@@ -904,18 +987,19 @@ def test_fingerprint_pr_allow_skip_no_decision_builds_fingerprint_fetching_revie
     assert "get_review_comments" in pr.calls
 
 
-def test_fingerprint_pr_hash_is_byte_identical_to_pre_skip_scheme():
-    """Characterization: a non-skipped PR's eval_hash equals the pre-refactor digest.
+def test_fingerprint_pr_golden_hash_scheme_v6():
+    """End-to-end golden for the current scheme (v6) through the fingerprint_pr entry point.
 
-    Pins the digest produced before reviews were threaded through build_pr_fingerprint,
-    proving the fingerprint payload is unchanged for PRs that are still fingerprinted.
+    fingerprint_pr leaves self_login unset, so (unlike test_build_pr_fingerprint_golden_hash_scheme_v6)
+    the greenlight self-note survives and this pins a distinct digest. A HASH_SCHEME_VERSION bump
+    regenerates this literal.
     """
     pr = _golden_pr()
     client = _FakeScanClient(_FakeScanRepo(pr))
 
     result = github_client.fingerprint_pr(client, "pytorch/pytorch", 9)
 
-    assert result == ("head-sha", "bcf6a1d21566873cd1da85fa724bd1e9996e213b869ed28544751c7e4e06a0f4")
+    assert result == ("head-sha", "fed90fac1ad308b7149a622b6d81da3f309ab933d12c85f427d300534f700408")
 
 
 class _FakeVerdictReview:

greenlight/tests/test_pr_hash.py

@@ -1,12 +1,14 @@
 import pytest
 
 from greenlight.pr_hash import (
+    BOT_COMMAND_MENTIONS,
     BOT_LOGINS,
     HASH_SCHEME_VERSION,
     HumanEvent,
     PRFingerprint,
     compute_pr_hash,
     is_bot,
+    is_bot_command,
 )
 
 
@@ -154,8 +156,47 @@ def test_compute_pr_hash_golden_pins_canonical_sort_key_scheme_v5():
     assert compute_pr_hash(fp) == "1301c1364eff060bfb8b94f3252e8256637279b8c48be6638f502b06db24922d"
 
 
+def test_compute_pr_hash_golden_scheme_v6():
+    """Golden digest pinning the current scheme (v6, SHA-256).
+
+    Any change to the payload, its canonicalization, or the hash algorithm breaks
+    this on purpose; regenerate the literal only alongside a HASH_SCHEME_VERSION bump.
+    """
+    fp = PRFingerprint(
+        head_sha="def456",
+        human_events=(
+            HumanEvent(id=2, body="lgtm"),
+            HumanEvent(id=1, body="hi"),
+        ),
+        scheme_version=6,
+    )
+
+    assert compute_pr_hash(fp) == "cb4e0c1926ec5d63d4ba09290d1ccdd46ba7cf2e0c2e2ece2a9fbfefc5fc3226"
+
+
+def test_compute_pr_hash_golden_pins_canonical_sort_key_scheme_v6():
+    """Golden that pins the payload sort KEY (``key=_canonical``) at the current scheme (v6).
+
+    The fixture is crafted so canonical (sorted-dict-key) ordering diverges from a
+    naive ``key=str`` / field-order / insertion ordering: canonical weighs ``body``
+    first (``body`` sorts before ``id``), while ``str`` weighs ``id`` first. Swapping
+    ``key=_canonical`` for ``key=str`` (or dropping the sort) reorders the list and
+    breaks this digest.
+    """
+    fp = PRFingerprint(
+        head_sha="def456",
+        human_events=(
+            HumanEvent(id=1, body="z"),
+            HumanEvent(id=2, body="a"),
+        ),
+        scheme_version=6,
+    )
+
+    assert compute_pr_hash(fp) == "a6178791d05cc91fe764b8cdfc5321ea871652aaaff608a02c5a20a71e8f20da"
+
+
 def test_hash_scheme_version_is_pinned():
-    assert HASH_SCHEME_VERSION == 5
+    assert HASH_SCHEME_VERSION == 6
 
 
 def test_pr_fingerprint_scheme_version_defaults_to_current_scheme():
@@ -197,3 +238,46 @@ def test_bot_logins_are_lowercase_and_bracket_free():
     for login in BOT_LOGINS:
         assert login == login.lower()
         assert not login.endswith("[bot]")
+
+
+@pytest.mark.parametrize(
+    ("body", "expected"),
+    [
+        ("@pytorchbot merge", True),
+        ("@pytorchmergebot rebase", True),
+        ("@claude please review", True),
+        ("@greenlight status", True),
+        ("@PyTorchBot merge", True),
+        ("@PYTORCHMERGEBOT REBASE", True),
+        ("@Claude", True),
+        ("@GreenLight recheck", True),
+        ("please run @pytorchbot merge -f now", True),
+        ("lgtm, cc @claude", True),
+        ("plain human comment", False),
+        ("no handle at all", False),
+        ("reach me at foo@bar.com", False),
+        ("pytorchbot without the at-sign", False),
+        ("", False),
+        (None, False),
+    ],
+)
+def test_is_bot_command_truth_table(body: str | None, expected: bool) -> None:
+    assert is_bot_command(body) == expected
+
+
+def test_is_bot_command_matches_both_pytorch_handles_independently():
+    # '@pytorchbot' is not a substring of '@pytorchmergebot', so each must be listed to be caught.
+    assert "@pytorchbot" not in "@pytorchmergebot"
+    assert is_bot_command("@pytorchbot merge") is True
+    assert is_bot_command("@pytorchmergebot rebase") is True
+
+
+def test_is_bot_command_handles_lone_surrogate_without_raising():
+    assert is_bot_command("\ud800 @pytorchbot merge") is True
+    assert is_bot_command("\ud800 no handle here") is False
+
+
+def test_bot_command_mentions_are_lowercase_and_at_prefixed():
+    for mention in BOT_COMMAND_MENTIONS:
+        assert mention == mention.lower()
+        assert mention.startswith("@")