diff --git a/docs/cron.md b/docs/cron.md index 97c8e020..9b9cf8b6 100644 --- a/docs/cron.md +++ b/docs/cron.md @@ -257,6 +257,41 @@ run_if: > `idle_consumer: ` fields still work — they are translated into an > equivalent `messages` gate at load time. Prefer `run_if` for new jobs. +#### `github_pr_activity` + +Satisfied when something actionable changed on an author's open PRs: a merge or +close (`state`), a new commit (`headRefOid`), a review verdict +(`reviewDecision`), a CI transition (`statusCheckRollup`), or a human +comment/review. Those fields are fingerprinted via `gh` and the job fires only +when the fingerprint moves, so an expensive PR-monitor job can stay idle for the +whole life of an open PR while nothing actually happens on it. + +| Field | Type | Default | Description | +|-------|------|---------|-------------| +| `author` | string | — (required) | GitHub login whose open PRs to watch. Also ignored as a comment/review actor, so the job's own replies don't wake it | +| `force_run_after_hours` | number | 8 | Fire regardless if this long has elapsed since the last run; `0` disables the safety net | +| `ignore_actors` | list | — | Logins whose comments/reviews don't count as activity, for chatty bots that would otherwise wake the job on every push. Matching is case-insensitive and a trailing `[bot]` is ignored, so `codecov` also covers `codecov[bot]`. A bare `ignore_actors:` means unset; any other non-list value, or an empty login, is refused rather than read as an empty list | + +```yaml +run_if: + - type: tasks # cheap DB check first... + status: in_progress + tag: pr-open + - type: github_pr_activity # ...then the network check + author: my-bot + force_run_after_hours: 8 + ignore_actors: [codecov, sonarqubecloud] +``` + +Two behaviours worth knowing. Any `gh` failure fails **open** — the job fires — +so a transient error can never strand a PR. And `ignore_actors` is a denylist +rather than bot autodetection, because bots are not reliably identifiable here: +`author.is_bot` is `null` inside `gh`'s comment payloads and GraphQL strips the +`[bot]` login suffix, while `authorAssociation` reports real maintainers as +`CONTRIBUTOR` — the same value as some review bots. A denylist fails in the safe +direction: an unlisted bot costs one extra wake, whereas a wrong autodetect +would silently drop human feedback. + ### Adding a built-in gate type Built-in gates live in `nerve/cron/gates.py`. To add one: subclass `CronGate`, diff --git a/nerve/cron/gates.py b/nerve/cron/gates.py index 70ec1de0..9b998486 100644 --- a/nerve/cron/gates.py +++ b/nerve/cron/gates.py @@ -263,15 +263,29 @@ class GitHubPrActivityGate(CronGate): The motivating case: wake an expensive (LLM) PR-monitor cron only when something it actually acts on has changed on one of the author's open PRs — merge/close (``state``), a new commit (``headRefOid``), a review - verdict (``reviewDecision``), or a CI transition (``statusCheckRollup``) - — instead of every N minutes for the entire lifetime of an open PR. - - Why a dedicated gate (not the ``messages``/``tasks`` gates): comments and - @-mentions arrive via the ``github`` sync source, but **CI status changes - and silent merges do not** (a check flipping red posts no comment), so a - poll-based signal is required. A PR's ``updatedAt`` is *not* enough — check - runs attach to the head commit, not the PR, so CI changes often don't bump - it; we therefore fingerprint the CI rollup directly. + verdict (``reviewDecision``), a CI transition (``statusCheckRollup``), or + a human comment/review (``comments``/``reviews``, see below) — instead of + every N minutes for the entire lifetime of an open PR. + + Why a dedicated gate (not the ``messages``/``tasks`` gates): **CI status + changes and silent merges post no comment**, so a poll-based signal is + required. A PR's ``updatedAt`` is *not* enough — check runs attach to the + head commit, not the PR, so CI changes often don't bump it; we therefore + fingerprint the CI rollup directly. + + Comment/review activity is fingerprinted here too, and must be. It is + tempting to assume a reviewer's comment arrives via the ``github`` sync + source instead — it does *not* reliably reach a consumer. A plain comment + on a PR **we** authored has notification ``reason=author`` (not + ``mention``), and an inbox cron that delegates own-PR activity to this + monitor will discard it — after its poll has already advanced the durable + consumer cursor, so it is never re-delivered. Nothing else observes it. + (Regression: 2026-08-04, a maintainer comment on clickhouse-cs#513 sat + unseen for ~2h because this gate hashed only the four fields above.) + + Note ``reviewDecision`` does not stand in for review activity either: + GitHub only populates it with ``APPROVED``/``CHANGES_REQUESTED``, so a + ``COMMENTED`` review leaves it unchanged. Mechanics (cheap, non-LLM): shell out to ``gh`` (same async-subprocess pattern as ``nerve.sources.github``), hash the fields above across all of @@ -291,24 +305,59 @@ class GitHubPrActivityGate(CronGate): - type: github_pr_activity # network: did any open PR change? author: my-bot force_run_after_hours: 8 + ignore_actors: [codecov, sonarqubecloud] Config: - * ``author`` (required) — GitHub login whose open PRs to watch. + * ``author`` (required) — GitHub login whose open PRs to watch. Also + ignored as a comment/review actor: our own replies must not wake us. * ``force_run_after_hours`` (default 8) — force a run if this long has elapsed since the last fire; ``0`` disables the safety net. + * ``ignore_actors`` (default none) — logins whose comments/reviews don't + count as activity, for chatty bots that would otherwise wake the cron on + every push (coverage/lint/AI-review bots). Matching is case-insensitive + and a trailing ``[bot]`` is ignored, so ``codecov`` also covers + ``codecov[bot]``. + + Why a denylist and not bot autodetection: ``author.is_bot`` is ``null`` + inside ``gh``'s ``comments``/``reviews`` payloads, and GraphQL strips the + ``[bot]`` login suffix, so bots are not reliably identifiable. + ``authorAssociation`` does not work either — real maintainers show up as + ``CONTRIBUTOR``, the same value as some review bots, so filtering on it + would silently drop human feedback. A denylist fails in the safe + direction: an unlisted bot costs an extra wake, never a missed human. """ type = "github_pr_activity" - spec_keys = frozenset({"author", "force_run_after_hours"}) + spec_keys = frozenset({"author", "force_run_after_hours", "ignore_actors"}) - def __init__(self, author: str, force_run_after_hours: float = 8.0) -> None: + def __init__( + self, + author: str, + force_run_after_hours: float = 8.0, + ignore_actors: list[str] | None = None, + ) -> None: if not author: raise GateConfigError( "'github_pr_activity' gate requires a non-empty 'author'" ) self.author = author self.force_run_after_hours = force_run_after_hours + # Our own comments/replies must never count as activity to react to. + # A login that normalises to empty is dropped rather than stored: + # ``_actor`` reports "" for a deleted/ghost user, so an empty entry here + # would quietly mute every ghost-authored comment. ``from_config`` + # rejects those loudly; this keeps the invariant for direct callers too. + self.ignore_actors = { + norm for a in (ignore_actors or []) if (norm := self._norm_actor(a)) + } + self.ignore_actors.add(self._norm_actor(author)) + + @staticmethod + def _norm_actor(login: str) -> str: + """Normalise a login for denylist comparison ('Codecov[bot]' -> 'codecov').""" + s = (login or "").strip().lower() + return s[:-5] if s.endswith("[bot]") else s async def is_satisfied(self, ctx: GateContext) -> bool: fp = await self._fingerprint() @@ -338,7 +387,8 @@ async def is_satisfied(self, ctx: GateContext) -> bool: def describe(self) -> str: return ( f"new activity on {self.author}'s open PRs " - f"(state/CI/review change; force every {self.force_run_after_hours}h)" + f"(state/CI/review/comment change; " + f"force every {self.force_run_after_hours}h)" ) @classmethod @@ -352,7 +402,27 @@ def from_config(cls, spec: dict) -> "GitHubPrActivityGate": "'github_pr_activity' gate 'force_run_after_hours' must be a " f"number, got {hours!r}" ) - return cls(author=author, force_run_after_hours=hours) + # Only a bare `ignore_actors:` (None) means "not set". Anything else that + # merely happens to be falsy — 0, false, "" — is a misconfiguration, and + # reading it as an empty denylist would hide the mistake behind a gate + # that still fires on every bot comment. + ignore = spec.get("ignore_actors") + if ignore is None: + ignore = [] + elif isinstance(ignore, str): + ignore = [ignore] + if not isinstance(ignore, list) or not all( + isinstance(a, str) and a.strip() for a in ignore + ): + raise GateConfigError( + "'github_pr_activity' gate 'ignore_actors' must be a list of " + f"non-empty login strings, got {ignore!r}" + ) + return cls( + author=author, + force_run_after_hours=hours, + ignore_actors=ignore, + ) # --- internals --------------------------------------------------------- @@ -383,7 +453,8 @@ async def _list_open_prs(self) -> list[tuple[str, int]] | None: async def _pr_detail(self, repo: str, number: int) -> dict | None: out = await self._gh( "pr", "view", str(number), "--repo", repo, - "--json", "state,reviewDecision,headRefOid,statusCheckRollup", + "--json", + "state,reviewDecision,headRefOid,statusCheckRollup,comments,reviews", ) if out is None: return None @@ -392,6 +463,23 @@ async def _pr_detail(self, repo: str, number: int) -> dict | None: (c.get("name", ""), c.get("status", ""), c.get("conclusion", "")) for c in (d.get("statusCheckRollup") or []) ) + # `includesCreatedEdit` flips False->True the first time a comment is + # edited, so an edited-in-place comment still moves the fingerprint. + comments = sorted( + (c.get("createdAt", ""), self._actor(c), bool(c.get("includesCreatedEdit"))) + for c in (d.get("comments") or []) + if not self._is_ignored(c) + ) + reviews = sorted( + ( + r.get("submittedAt", ""), + self._actor(r), + r.get("state", ""), + bool(r.get("includesCreatedEdit")), + ) + for r in (d.get("reviews") or []) + if not self._is_ignored(r) + ) return { "repo": repo, "number": number, @@ -399,8 +487,18 @@ async def _pr_detail(self, repo: str, number: int) -> dict | None: "review": d.get("reviewDecision"), "head": d.get("headRefOid"), "checks": checks, + "comments": comments, + "reviews": reviews, } + @staticmethod + def _actor(item: dict) -> str: + """Login of a comment/review author ('' for a deleted/ghost user).""" + return ((item.get("author") or {}).get("login") or "") + + def _is_ignored(self, item: dict) -> bool: + return self._norm_actor(self._actor(item)) in self.ignore_actors + @staticmethod async def _gh(*args: str, timeout: float = 30.0) -> str | None: """Run a ``gh`` command (inherits the daemon's gh auth). None on failure. diff --git a/tests/test_cron_gates.py b/tests/test_cron_gates.py index a67aa40f..b0199e44 100644 --- a/tests/test_cron_gates.py +++ b/tests/test_cron_gates.py @@ -577,3 +577,267 @@ def test_describe_mentions_author(self): def test_build_gate_via_registry(self): gate = build_gate({"type": "github_pr_activity", "author": "bot"}) assert isinstance(gate, GitHubPrActivityGate) + + +# --------------------------------------------------------------------------- +# GitHubPrActivityGate — comment/review activity in the fingerprint +# --------------------------------------------------------------------------- + +def _comment(login: str, created: str = "2026-08-04T12:19:24Z", edited: bool = False): + return { + "author": {"login": login}, + "createdAt": created, + "includesCreatedEdit": edited, + } + + +def _review( + login: str, + state: str = "COMMENTED", + submitted: str = "2026-08-04T12:19:24Z", + edited: bool = False, +): + return { + "author": {"login": login}, + "state": state, + "submittedAt": submitted, + "includesCreatedEdit": edited, + } + + +def _pr_payload(comments=None, reviews=None, **overrides): + """A `gh pr view --json ...` payload with stable non-comment state.""" + payload = { + "state": "OPEN", + "reviewDecision": "REVIEW_REQUIRED", + "headRefOid": "cd46fa400806f5634728a5a7c9963afefbc940bd", + "statusCheckRollup": [ + {"name": "build", "status": "COMPLETED", "conclusion": "SUCCESS"}, + ], + "comments": comments or [], + "reviews": reviews or [], + } + payload.update(overrides) + return payload + + +def _stub_gh(gate: GitHubPrActivityGate, payload: dict) -> None: + """Point the gate's `gh` shell-out at a single fake open PR.""" + async def fake_gh(*args, **kwargs): + if args[0] == "search": + return json.dumps( + [{"repository": {"nameWithOwner": "acme/widget"}, "number": 7}] + ) + return json.dumps(payload) + + gate._gh = fake_gh # type: ignore[method-assign] + + +async def _fp(payload: dict, **kwargs) -> str: + gate = GitHubPrActivityGate(author="my-bot", **kwargs) + _stub_gh(gate, payload) + return await gate._fingerprint() + + +class TestGitHubPrActivityCommentActivity: + """Regression cover for the 2026-08-04 comment-blindness bug. + + The gate originally hashed only state/reviewDecision/headRefOid/checks, so a + maintainer's comment on one of our PRs never woke the monitor — and the + `github` inbox path silently consumed it (reason=author on our own PR), so + nothing observed it until the 8h force-run. + """ + + @pytest.mark.asyncio + async def test_human_comment_changes_fingerprint(self): + """THE regression: a new human comment must move the fingerprint.""" + before = await _fp(_pr_payload()) + after = await _fp(_pr_payload(comments=[_comment("alex-clickhouse")])) + assert before != after + + @pytest.mark.asyncio + async def test_human_commented_review_changes_fingerprint(self): + """A COMMENTED review leaves reviewDecision untouched, so hash it too.""" + before = await _fp(_pr_payload()) + after = await _fp(_pr_payload(reviews=[_review("alex-clickhouse")])) + assert before != after + + @pytest.mark.asyncio + async def test_comment_edit_changes_fingerprint(self): + """Editing a comment in place keeps count/timestamp — catch it anyway.""" + before = await _fp(_pr_payload(comments=[_comment("alex-clickhouse")])) + after = await _fp( + _pr_payload(comments=[_comment("alex-clickhouse", edited=True)]) + ) + assert before != after + + @pytest.mark.asyncio + async def test_ignored_bot_comment_does_not_change_fingerprint(self): + """Chatty bots must not wake an expensive monitor (CI covers them).""" + quiet = await _fp(_pr_payload(), ignore_actors=["codecov"]) + noisy = await _fp( + _pr_payload(comments=[_comment("codecov")]), ignore_actors=["codecov"] + ) + assert quiet == noisy + + @pytest.mark.asyncio + async def test_ignored_bot_review_does_not_change_fingerprint(self): + quiet = await _fp(_pr_payload(), ignore_actors=["cursor"]) + noisy = await _fp( + _pr_payload(reviews=[_review("cursor")]), ignore_actors=["cursor"] + ) + assert quiet == noisy + + @pytest.mark.asyncio + async def test_bot_suffix_is_normalised(self): + """GraphQL strips `[bot]`; REST keeps it. One denylist entry covers both.""" + quiet = await _fp(_pr_payload(), ignore_actors=["codecov"]) + noisy = await _fp( + _pr_payload(comments=[_comment("Codecov[bot]")]), + ignore_actors=["codecov"], + ) + assert quiet == noisy + + @pytest.mark.asyncio + async def test_own_comment_does_not_change_fingerprint(self): + """Our own replies must never wake us.""" + quiet = await _fp(_pr_payload()) + noisy = await _fp(_pr_payload(comments=[_comment("my-bot")])) + assert quiet == noisy + + @pytest.mark.asyncio + async def test_unlisted_bot_still_counts(self): + """Denylist fails safe: an unknown actor costs a wake, never a miss.""" + before = await _fp(_pr_payload(), ignore_actors=["codecov"]) + after = await _fp( + _pr_payload(comments=[_comment("brand-new-bot")]), + ignore_actors=["codecov"], + ) + assert before != after + + @pytest.mark.asyncio + async def test_ghost_author_counts_as_activity(self): + """A deleted user's comment (author=null) must not be silently dropped.""" + before = await _fp(_pr_payload()) + after = await _fp( + _pr_payload( + comments=[{"author": None, "createdAt": "x", "includesCreatedEdit": False}] + ) + ) + assert before != after + + @pytest.mark.asyncio + async def test_non_comment_state_still_fingerprinted(self): + """Pre-existing signals must keep working.""" + base = _pr_payload() + assert await _fp(base) != await _fp(_pr_payload(state="MERGED")) + assert await _fp(base) != await _fp(_pr_payload(headRefOid="deadbeef")) + assert await _fp(base) != await _fp( + _pr_payload(reviewDecision="CHANGES_REQUESTED") + ) + assert await _fp(base) != await _fp( + _pr_payload( + statusCheckRollup=[ + {"name": "build", "status": "COMPLETED", "conclusion": "FAILURE"} + ] + ) + ) + + @pytest.mark.asyncio + async def test_comments_are_requested_from_gh(self): + """The payload is useless if the fields aren't asked for.""" + gate = GitHubPrActivityGate(author="my-bot") + seen: list[tuple] = [] + + async def fake_gh(*args, **kwargs): + seen.append(args) + if args[0] == "search": + return json.dumps( + [{"repository": {"nameWithOwner": "acme/widget"}, "number": 7}] + ) + return json.dumps(_pr_payload()) + + gate._gh = fake_gh # type: ignore[method-assign] + await gate._fingerprint() + view = next(a for a in seen if a[0] == "pr") + # Compare tokens, not substrings: `"reviews" in "...,latestReviews"` is + # true, so a substring check would still pass if the code asked for a + # differently-named field that `_pr_detail` never reads. + fields = set(view[view.index("--json") + 1].split(",")) + assert {"state", "reviewDecision", "headRefOid", + "statusCheckRollup", "comments", "reviews"} <= fields + + def test_author_is_always_ignored(self): + gate = GitHubPrActivityGate(author="My-Bot") + assert "my-bot" in gate.ignore_actors + + def test_from_config_ignore_actors(self): + gate = build_gate({ + "type": "github_pr_activity", + "author": "my-bot", + "ignore_actors": ["Codecov[bot]", "cursor"], + }) + assert {"codecov", "cursor", "my-bot"} <= gate.ignore_actors + + def test_from_config_ignore_actors_string_coerced(self): + gate = build_gate({ + "type": "github_pr_activity", + "author": "my-bot", + "ignore_actors": "codecov", + }) + assert "codecov" in gate.ignore_actors + + def test_from_config_ignore_actors_omitted(self): + gate = build_gate({"type": "github_pr_activity", "author": "my-bot"}) + assert gate.ignore_actors == {"my-bot"} + + def test_from_config_bare_ignore_actors_means_unset(self): + """A bare `ignore_actors:` is None — the one value that means "not set".""" + gate = build_gate({ + "type": "github_pr_activity", "author": "my-bot", + "ignore_actors": None, + }) + assert gate.ignore_actors == {"my-bot"} + + def test_from_config_bad_ignore_actors(self): + with pytest.raises(GateConfigError): + build_gate({ + "type": "github_pr_activity", + "author": "my-bot", + "ignore_actors": [{"login": "codecov"}], + }) + + @pytest.mark.parametrize("bad", [0, False, "", [""], [" "], ["ok", ""], {}]) + def test_from_config_falsy_ignore_actors_is_refused(self, bad): + """Refused, not read as an empty denylist. + + `spec.get(...) or []` would have taken every one of these as "not set", + leaving a job that still wakes on every bot comment while the config + says otherwise. + """ + with pytest.raises(GateConfigError, match="ignore_actors"): + build_gate({ + "type": "github_pr_activity", + "author": "my-bot", + "ignore_actors": bad, + }) + + @pytest.mark.asyncio + async def test_empty_login_cannot_mute_a_ghost_author(self): + """An empty entry must never reach the denylist. + + `_actor` reports "" for a deleted/ghost user, so a stored "" would mute + exactly the comments `test_ghost_author_counts_as_activity` says must + count. from_config refuses one; a direct caller gets it dropped. + """ + gate = GitHubPrActivityGate(author="my-bot", ignore_actors=["", " "]) + assert gate.ignore_actors == {"my-bot"} + + quiet = await _fp(_pr_payload(), ignore_actors=["", " "]) + loud = await _fp( + _pr_payload(comments=[_comment("")]), ignore_actors=["", " "] + ) + assert quiet != loud + + def test_describe_mentions_comments(self): + assert "comment" in GitHubPrActivityGate(author="my-bot").describe()