diff --git a/docs/sources.md b/docs/sources.md index 1e45593d..9d8b90ce 100644 --- a/docs/sources.md +++ b/docs/sources.md @@ -294,9 +294,10 @@ With `allow_repos` empty (the default), all repos pass — behavior is unchanged ### GitHub actor guardrail The GitHub notification source also matches on `actors` — the list of every GitHub login -involved in a notification (issue/PR author, assignees, and comment/review authors). This -restricts **who** can put a notification in front of the worker, so a drive-by `@mention` -from an untrusted account is dropped before the agent ever sees it: +involved in a notification (issue/PR author, assignees, comment/review authors, and on a +`reason=assign` notification the account that performed the assignment). This restricts +**who** can put a notification in front of the worker, so a drive-by `@mention` from an +untrusted account is dropped before the agent ever sees it: ```yaml sync: @@ -310,6 +311,14 @@ allowlist. A non-empty `allow_actors` is **fail-closed**: a notification with no identifiable actor (e.g. enrichment failed) is dropped. With `allow_actors` empty (the default), all actors pass — behavior is unchanged. The repo and actor rules AND together. +Assignments are worth calling out. An `assign` notification names the assignee but not the +assigner, and carries no comment text, so the assigner is fetched from the thread's event +history and rendered as `Assigned by: `. Without it, an assignment on a subject the +inbox account itself authored has no third-party login anywhere — every candidate is that +one account — and a non-empty `allow_actors` would drop it fail-closed. If you rely on +"a maintainer assigns an issue to the bot" as a work signal, that login needs to be in +`allow_actors`. + | Field | Type | Default | Description | |-------|------|---------|-------------| | `github.allow_actors` | list | `[]` | Allowlist of GitHub login globs. Empty = all actors pass | diff --git a/nerve/sources/github.py b/nerve/sources/github.py index b87bfe2d..a830dcf3 100644 --- a/nerve/sources/github.py +++ b/nerve/sources/github.py @@ -37,6 +37,7 @@ def _collect_actors( latest_review: dict[str, Any] | None, inline_comments: list[dict[str, Any]], recent_comments: list[dict[str, Any]], + assigner: str = "", ) -> list[str]: """Every GitHub login involved in a notification, de-duplicated. @@ -46,8 +47,18 @@ def _collect_actors( involved (see :mod:`nerve.sources.filters`). The raw ``/notifications`` payload carries no actor, but enrichment has already fetched these logins for the rendered content. + + ``assigner`` is the login that performed the assignment on a + ``reason=assign`` notification (see + :meth:`GitHubSource._enrich_assignment`). It is the one actor such a + notification is *about*, and it appears nowhere else in the payload: an + assignment carries no comment, so when the subject was authored by — and + assigned to — the same account (e.g. a bot files an issue and a maintainer + hands it back), every other candidate below is that same account. Without + this, the notification has no third-party login at all and a non-empty + ``allow_actors`` allowlist can never pass it. """ - candidates: list[str] = [subject_user, *assignees] + candidates: list[str] = [subject_user, *assignees, assigner] if comment: candidates.append(comment.get("user", "")) if latest_review: @@ -143,6 +154,7 @@ async def fetch(self, cursor: str | None, limit: int = 100) -> FetchResult: assignees = extra.get("assignees", []) labels = extra.get("labels", []) comment = extra.get("latest_comment") + assigner = extra.get("assigner", "") content_parts = [ f"Repository: {repo_name}", @@ -152,6 +164,9 @@ async def fetch(self, cursor: str | None, limit: int = 100) -> FetchResult: f"State: {subject_state}" if subject_state else None, f"Author: {subject_user}" if subject_user else None, f"Assignees: {', '.join(assignees)}" if assignees else None, + # An assignment carries no comment text, so without this the + # record never says who asked for the work. + f"Assigned by: {assigner}" if assigner else None, f"Labels: {', '.join(labels)}" if labels else None, f"Updated: {updated_at}", f"URL: {html_url}", @@ -222,7 +237,7 @@ async def fetch(self, cursor: str | None, limit: int = 100) -> FetchResult: # allow/deny by actor (the raw notification carries no actor). actors = _collect_actors( subject_user, assignees, comment, latest_review, - inline_comments, recent_comments, + inline_comments, recent_comments, assigner, ) records.append(SourceRecord( @@ -330,8 +345,81 @@ async def _enrich_notification( if reason in ("mention", "assign", "review_requested", "team_mention") and s_type in ("PullRequest", "Issue"): await self._enrich_recent_comments(subject_url, s_type, result) + # For assignments: fetch who assigned it. The notification payload + # names the assignee but never the assigner, and an assignment has + # no comment body to fall back on. + if reason == "assign" and s_type in ("PullRequest", "Issue"): + await self._enrich_assignment(subject_url, s_type, result) + return result + async def _enrich_assignment( + self, subject_url: str, subject_type: str, result: dict[str, Any], + ) -> None: + """Resolve who performed the assignment on a ``reason=assign`` thread. + + Mutates *result* in place, adding ``assigner`` when found. + + The assigner is the whole point of an assignment notification — it is + the account requesting the work — yet neither ``/notifications`` nor the + issue payload carries it, and an assignment has no comment text. It is + only available from the issue's event history, so this costs one extra + API call, taken only for ``reason=assign`` (a rare reason). + + Reads the *last* ``assigned`` event, which is the one that triggered the + notification, and skips it when that assignee is no longer assigned — so + a stale assigner from a since-reverted assignment is not surfaced as an + actor. When the assignee list is empty or unknown the event is accepted + anyway: assigning requires write or triage access, so the assigner is + privileged by construction, and refusing would just reinstate the + fail-closed drop this method exists to prevent. + """ + # A PR's event history lives under /issues/{n}/, never /pulls/{n}/. + if subject_type == "PullRequest": + base = subject_url.replace("/pulls/", "/issues/") + else: + base = subject_url + + # /events, not /timeline: both carry `assigned` entries, but /events + # excludes comments, so a busy thread is far less likely to push the + # assignment out of the single page read here (measured on real threads: + # 11 events vs 30 timeline entries, 7 vs 26). Neither endpoint offers a + # reverse sort, and paginating an unbounded history on every assignment + # is not worth it — past 100 entries `assigner` is simply absent, exactly + # as before this method existed. + events = await self._gh_api_get(f"{base}/events?per_page=100") + if not isinstance(events, list) or not events: + return + + current = {a.lower() for a in result.get("assignees", [])} + + for ev in reversed(events): + if ev.get("event") != "assigned": + continue + assignee = (ev.get("assignee") or {}).get("login", "") + if current and assignee.lower() not in current: + continue + # The two endpoints carrying `assigned` disagree about where the + # acting login sits: /events puts it in `assigner` and the *assignee* + # in `actor` (verified on two separate real assignments), while + # /timeline puts it in `actor` and omits `assigner`. Read both, so + # reading `.actor` alone can't silently yield the assignee, and so + # this survives switching endpoint or GitHub aligning the payloads. + login = ( + (ev.get("assigner") or {}).get("login", "") + or (ev.get("actor") or {}).get("login", "") + ) + if login: + result["assigner"] = login + # Stop at the triggering event whether or not it resolved. Falling + # back to an older `assigned` event would credit this notification to + # whoever assigned it *last* time — and that login is precisely the + # kind likely to be on the allowlist, so an unresolvable assignment + # would be admitted on the authority of someone who did not act. An + # actor we cannot identify (deleted account, null `actor`) must stay + # unidentified: no assigner, no pass. + return + async def _enrich_pr_reviews( self, pr_url: str, result: dict[str, Any], ) -> None: diff --git a/tests/test_github_source_actors.py b/tests/test_github_source_actors.py index bb640eea..6ea62df5 100644 --- a/tests/test_github_source_actors.py +++ b/tests/test_github_source_actors.py @@ -54,13 +54,44 @@ def test_collect_actors_spans_all_enrichment_sources(): latest_review={"user": "reviewer"}, inline_comments=[{"user": "inline1"}, {"user": "inline2"}], recent_comments=[{"user": "recent"}], + assigner="assigner", ) assert actors == [ - "author", "assignee", "commenter", "reviewer", + "author", "assignee", "assigner", "commenter", "reviewer", "inline1", "inline2", "recent", ] +def test_collect_actors_includes_assigner_when_it_is_the_only_third_party(): + # The self-authored, self-assigned, comment-free case: a bot files an issue + # and a maintainer assigns it back. Every login except the assigner is the + # bot itself, so without the assigner there is no third-party actor at all + # and a non-empty allow_actors allowlist can never pass the notification. + actors = _collect_actors( + subject_user="bot", + assignees=["bot"], + comment=None, + latest_review=None, + inline_comments=[], + recent_comments=[], + assigner="maintainer", + ) + assert actors == ["bot", "maintainer"] + + +def test_collect_actors_assigner_defaults_to_absent(): + # Every non-assign reason omits the argument entirely. + actors = _collect_actors( + subject_user="author", + assignees=[], + comment=None, + latest_review=None, + inline_comments=[], + recent_comments=[], + ) + assert actors == ["author"] + + # --------------------------------------------------------------------------- # Config — allow_actors / deny_actors parsing # --------------------------------------------------------------------------- @@ -202,6 +233,253 @@ async def test_build_source_runners_actor_deny_wins(db): assert gh.inbox_filter.passes(_gh_rec("no", ["alice", "spammer"])) is False +# --------------------------------------------------------------------------- +# _enrich_assignment — resolving who assigned a reason=assign notification +# --------------------------------------------------------------------------- + +def _timeline_assigned(actor: str, assignee: str) -> dict: + """An `assigned` entry in the /timeline shape (acting login in `actor`).""" + return { + "event": "assigned", + "actor": {"login": actor}, + "assignee": {"login": assignee}, + } + + +@pytest.mark.asyncio +async def test_enrich_assignment_reads_last_assigned_event(monkeypatch): + src = GitHubSource() + calls: list[str] = [] + + async def fake_get(url, timeout=30): + calls.append(url) + return [ + {"event": "labeled", "actor": {"login": "someone"}}, + _timeline_assigned("first-assigner", "bot"), + {"event": "commented", "actor": {"login": "noise"}}, + _timeline_assigned("maintainer", "bot"), + ] + + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + result: dict = {"assignees": ["bot"]} + await src._enrich_assignment( + "https://api.github.com/repos/owner/repo/issues/521", "Issue", result, + ) + + # The *last* assigned event is the one that triggered the notification. + assert result["assigner"] == "maintainer" + assert calls == [ + "https://api.github.com/repos/owner/repo/issues/521/events?per_page=100", + ] + + +@pytest.mark.asyncio +async def test_enrich_assignment_prefers_assigner_field_over_actor(monkeypatch): + # /events disagrees with /timeline: it puts the assignee in + # `actor` and the real actor in `assigner`. Preferring `assigner` keeps + # either payload shape correct. + src = GitHubSource() + + async def fake_get(url, timeout=30): + return [{ + "event": "assigned", + "actor": {"login": "bot"}, + "assigner": {"login": "maintainer"}, + "assignee": {"login": "bot"}, + }] + + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + result: dict = {"assignees": ["bot"]} + await src._enrich_assignment( + "https://api.github.com/repos/owner/repo/issues/1", "Issue", result, + ) + assert result["assigner"] == "maintainer" + + +@pytest.mark.asyncio +async def test_enrich_assignment_ignores_stale_assignment(monkeypatch): + # The newest `assigned` event targets someone who is no longer assigned + # (assignment since reverted) — that assigner must not become an actor. + src = GitHubSource() + + async def fake_get(url, timeout=30): + return [_timeline_assigned("stranger", "someone-else")] + + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + result: dict = {"assignees": ["bot"]} + await src._enrich_assignment( + "https://api.github.com/repos/owner/repo/issues/1", "Issue", result, + ) + assert "assigner" not in result + + +@pytest.mark.asyncio +async def test_enrich_assignment_does_not_fall_back_to_an_older_assigner( + monkeypatch, +): + # An unresolvable triggering event (deleted account → null actor) must leave + # `assigner` absent rather than reaching back to an earlier assignment. + # Crediting the previous assigner would admit the notification on the + # authority of someone who did not act — and that login is exactly the kind + # likely to be on the allowlist. Fail closed instead. + src = GitHubSource() + + async def fake_get(url, timeout=30): + return [ + _timeline_assigned("earlier-maintainer", "bot"), + {"event": "assigned", "actor": None, "assignee": {"login": "bot"}}, + ] + + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + result: dict = {"assignees": ["bot"]} + await src._enrich_assignment( + "https://api.github.com/repos/owner/repo/issues/1", "Issue", result, + ) + assert "assigner" not in result + + +@pytest.mark.asyncio +async def test_enrich_assignment_uses_issues_path_for_prs(monkeypatch): + # A PR's event history lives under /issues/{n}/, never /pulls/{n}/. + src = GitHubSource() + calls: list[str] = [] + + async def fake_get(url, timeout=30): + calls.append(url) + return [_timeline_assigned("maintainer", "bot")] + + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + result: dict = {"assignees": ["bot"]} + await src._enrich_assignment( + "https://api.github.com/repos/owner/repo/pulls/9", "PullRequest", result, + ) + assert result["assigner"] == "maintainer" + assert "/issues/9/events" in calls[0] + assert "/pulls/" not in calls[0] + + +@pytest.mark.asyncio +async def test_enrich_assignment_tolerates_missing_history(monkeypatch): + # A failed/empty event-history call must leave `assigner` absent, not raise. + src = GitHubSource() + + async def fake_get(url, timeout=30): + return None + + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + result: dict = {"assignees": ["bot"]} + await src._enrich_assignment( + "https://api.github.com/repos/owner/repo/issues/1", "Issue", result, + ) + assert "assigner" not in result + + +@pytest.mark.asyncio +async def test_enrich_notification_skips_event_fetch_for_non_assign_reasons(monkeypatch): + # The extra API call is taken only for reason=assign. + src = GitHubSource() + calls: list[str] = [] + + async def fake_get(url, timeout=30): + calls.append(url) + if url.endswith("/issues/1"): + return {"html_url": "h", "user": {"login": "alice"}, "assignees": []} + return [] + + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + import asyncio as _asyncio + + await src._enrich_notification( + { + "reason": "mention", + "subject": { + "type": "Issue", + "url": "https://api.github.com/repos/owner/repo/issues/1", + }, + }, + _asyncio.Semaphore(1), + ) + assert not any("/events" in c for c in calls) + + +# --------------------------------------------------------------------------- +# Regression — a maintainer assigning an issue the bot itself filed +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_maintainer_assignment_on_self_filed_issue_passes_guardrail( + monkeypatch, db, +): + """The hand-back route: bot files an issue, maintainer assigns it back. + + Author, assignee and (absent) commenters are all the bot, so before the + assigner was collected this notification's only actor was the bot itself — + a non-empty ``allow_actors`` dropped it fail-closed and the assignment was + silently lost. Exercises the real fetch + enrichment + production guardrail. + """ + subject_url = "https://api.github.com/repos/owner/repo/issues/521" + notifications = [{ + "id": "n-assign", + "reason": "assign", + "unread": True, + "updated_at": "2026-01-02T10:00:00Z", + "subject": {"title": "Bug", "type": "Issue", "url": subject_url}, + "repository": { + "full_name": "owner/repo", + "html_url": "https://github.com/owner/repo", + }, + }] + + async def fake_exec(*args, **kwargs): + return _FakeProc(json.dumps(notifications).encode()) + + monkeypatch.setattr( + "nerve.sources.github.asyncio.create_subprocess_exec", fake_exec, + ) + + async def fake_get(url, timeout=30): + if url == subject_url: + return { + "html_url": "https://github.com/owner/repo/issues/521", + "body": "the bug", + "state": "open", + "user": {"login": "bot"}, # the bot filed it + "assignees": [{"login": "bot"}], # ...and is the assignee + "labels": [{"name": "bug"}], + } + if "/events" in url: + return [_timeline_assigned("maintainer", "bot")] + return [] # no comments at all + + src = GitHubSource() + monkeypatch.setattr(src, "_gh_api_get", fake_get) + + result = await src.fetch(cursor="2026-01-02T09:00:00Z") + assert len(result.records) == 1 + record = result.records[0] + + assert record.metadata["actors"] == ["bot", "maintainer"] + # The agent can also see who asked, which an assignment otherwise never says. + assert "Assigned by: maintainer" in record.content + + # ...and the production guardrail now keeps it. + cfg = NerveConfig.from_dict({ + "sync": {"github": {"enabled": True, "allow_actors": ["maintainer"]}}, + }) + gh = next( + r for r in build_source_runners(cfg, db) + if r.source.source_name == "github" + ) + assert gh.inbox_filter.passes(record) is True + + @pytest.mark.asyncio async def test_fetch_actors_empty_when_enrichment_fails(monkeypatch): # When enrichment raises, the loop falls back to extra={} — the record is