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
15 changes: 12 additions & 3 deletions docs/sources.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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: <login>`. 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 |
Expand Down
92 changes: 90 additions & 2 deletions nerve/sources/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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:
Expand Down Expand Up @@ -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}",
Expand All @@ -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}",
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Comment on lines +396 to +421

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberate, and I've kept the behaviour — but you're right that the code didn't explain itself, so I've documented it and pinned it with a test (test_enrich_assignment_does_not_fall_back_to_an_older_assigner).

Continuing the scan would mean that when the triggering assignment can't be resolved (deleted account, "actor": null), the notification gets credited to whoever assigned it last time. That login is exactly the kind likely to be on the allowlist — so the failure mode isn't "we miss an assigner", it's "an assignment by an unidentifiable account is admitted on the authority of someone who did not act." For a rule whose whole job is gating on who acted, that's worse than not resolving at all.

Leaving assigner unset is the safe outcome: the record simply behaves as it did before this method existed, and a fail-closed allowlist drops it. No assigner, no pass.

I verified the new test actually guards this rather than just asserting current behaviour — implementing the suggested continue makes it fail with assigner: 'earlier-maintainer', which is the false attribution above.


async def _enrich_pr_reviews(
self, pr_url: str, result: dict[str, Any],
) -> None:
Expand Down
Loading
Loading