github source: collect the assigner on reason=assign notifications - #285
github source: collect the assigner on reason=assign notifications#285polyglotAI-bot wants to merge 2 commits into
Conversation
An assignment notification is the one case where the account that triggered
it appears nowhere in the record. `/notifications` carries no actor, the
assignment has no comment body, and `_collect_actors` only ever saw the
subject author, the assignees and comment/review authors.
That is invisible until an `allow_actors` allowlist is set. When the inbox
account itself authored the subject and is also the assignee — a bot filing
an issue that a maintainer then assigns back to it — every candidate login
is that one account, so the record's only actor is the bot. A non-empty
allowlist drops it fail-closed, the source cursor advances past it, and the
assignment is gone with a single INFO line. No configuration can recover it:
the guardrail's rules are ANDed, so config can only tighten, and the login
that would satisfy the allowlist is simply not in the record.
Fetch the assigner from the thread's event history for reason=assign only,
add it to `actors`, and render it as `Assigned by: <login>` so the agent can
also see who asked for the work.
The two endpoints exposing this disagree, so both shapes are handled:
/issues/{n}/events puts the real actor in `assigner` (its `actor` is the
assignee), while /issues/{n}/timeline puts it in `actor` and omits
`assigner`.
Follow-up to ClickHouse#137, which added the actor guardrail.
|
Cross-reference for whoever reviews this: #283 ( Worth noting they're complementary rather than overlapping:
One heads-up on CI: no workflow has run on this PR (0 check-runs on |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds support for capturing the assigner for reason=assign GitHub notifications so actor-based guardrails can correctly allowlist/denylist these records and the rendered content shows who requested the assignment.
Changes:
- Extend
_collect_actors()to include an optionalassignerlogin. - Add
_enrich_assignment()and invoke it forreason=assignto resolve the assigner from the issue/PR timeline. - Update documentation and add regression/unit tests covering assigner resolution and guardrail behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_github_source_actors.py | Adds coverage for assigner inclusion, assignment enrichment behavior, and a regression test for actor-guardrail drops. |
| nerve/sources/github.py | Implements assignment enrichment via timeline lookup; surfaces assigner in content and actors. |
| docs/sources.md | Documents how actor guardrails behave for assign notifications and why assigner enrichment matters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 that expose this disagree: /issues/{n}/events | ||
| # carries the true actor in `assigner` (its `actor` is the assignee), | ||
| # while /issues/{n}/timeline carries it in `actor` and omits | ||
| # `assigner`. Prefer `assigner`, fall back to `actor`, so a payload | ||
| # of either shape resolves correctly. | ||
| login = ( | ||
| (ev.get("assigner") or {}).get("login", "") | ||
| or (ev.get("actor") or {}).get("login", "") | ||
| ) | ||
| if login: | ||
| result["assigner"] = login | ||
| return |
There was a problem hiding this comment.
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.
| # The two endpoints that expose this disagree: /issues/{n}/events | ||
| # carries the true actor in `assigner` (its `actor` is the assignee), | ||
| # while /issues/{n}/timeline carries it in `actor` and omits | ||
| # `assigner`. Prefer `assigner`, fall back to `actor`, so a payload | ||
| # of either shape resolves correctly. |
There was a problem hiding this comment.
Good catch, and it led somewhere better than a comment fix — I took a variant of (b) and switched the endpoint to /events in 052c38b.
The observation that made it worthwhile: /events excludes comments, so it's much denser than /timeline. Measured on real threads in another repo — 11 events vs 30 timeline entries, and 7 vs 26. Since this reads a single page of 100 with no reverse sort available, the dilution was the one real limitation of the method: a chatty thread could push the assignment out of the window. Switching shrinks that materially at identical cost, and /events carries the explicit assigner field.
Reading both fields is still load-bearing, and the comment now says why: the two endpoints disagree about where the acting login sits, and on /events the actor field is the assignee. Verified on two separate real assignments:
/issues/{n}/events → actor=<assignee> assigner=<real actor>
/issues/{n}/timeline → actor=<real actor> (no assigner field)
So on the endpoint now being called, reading .actor alone would silently record the wrong person — the assignee, who is typically the inbox account itself. That's precisely the bug this PR fixes, reintroduced one layer down. The assigner or actor order keeps it correct on either shape, and survives a future endpoint change or GitHub aligning the payloads.
Both carry `assigned` entries, but /events excludes comments, so a busy thread is much less likely to push the assignment out of the single page read here — measured on real threads, 11 events vs 30 timeline entries and 7 vs 26. That shrinks the one limitation this method had. Reading both `assigner` and `actor` still matters and is now explained: the two endpoints disagree about where the acting login sits, and /events puts the *assignee* in `actor`, so reading `.actor` alone would silently record the wrong person. Also document why the scan stops at the triggering event even when it does not resolve, and pin it with a test. Falling back to an older `assigned` event would credit the notification to whoever assigned it last time — a login likely to be on the allowlist — so an assignment by an unidentifiable account would be admitted on the authority of someone who did not act.
Summary
An
assignnotification is the one case where the account that triggered it appears nowhere in the resulting record./notificationscarries no actor, an assignment has no comment body, and_collect_actorsonly ever saw the subject author, the assignees, and comment/review authors.That gap is invisible until someone sets an
allow_actorsallowlist. When the inbox account itself authored the subject and is the assignee — a bot files an issue, a maintainer assigns it back to the bot — every candidate login is that one account, so the record's only actor is the bot. A non-empty allowlist then drops it fail-closed, the source cursor advances past it, and the assignment is gone, leaving one INFO line:No configuration can recover it. The guardrail's rules are ANDed and each is allow/deny on a single field, so config can only ever tighten the gate; the login that would satisfy the allowlist is simply not in the record. The only escapes are adding the inbox account to
allow_actors— which, withparticipating=true, makes it an actor on virtually every notification and so passes everything — or emptying the list. Both amount to deleting the guardrail.This is a follow-up to #137, which added the actor guardrail and shipped this blind spot with it.
What changed
_enrich_assignment()resolves the assigner from the thread's event history, forreason=assignonly (one extra API call on a rare reason).actors, so the allowlist can see the maintainer who asked.Assigned by: <login>, so the agent can tell who requested the work — an assignment otherwise never says.docs/sources.mddocuments the assignment case under the actor guardrail.Deliberate choices worth a reviewer's eye:
/issues/{n}/eventsputs the real actor inassignerand the assignee inactor;/issues/{n}/timelineputs the real actor inactorand omitsassigner. Verified against a live assignment:events→actor=bot, assigner=maintainer,timeline→actor=maintainer. The code prefersassigner, falls back toactor. Without that fallback a reader would "simplify" this into a silent regression on one of the two shapes.assigneris then simply absent — exactly the behaviour before this method existed — rather than paying unbounded pagination on every assignment. Non-regressive degradation.assignedevent targets someone no longer assigned, its assigner does not become 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 reinstate the very fail-closed drop this fixes.reason=assignpays the extra call.review_requestedhas the same structural gap (the requester is not captured either) and the same fix shape, but it is a separate concern and is left out of this PR.Test plan
tests/test_github_source_actors.py— 8 new tests: assigner in_collect_actors, last-assigned-event selection,assigner-over-actorpreference, stale-assignment skip, PR/pulls/→/issues/rewrite, missing-timeline tolerance, no timeline call for non-assign reasons.test_maintainer_assignment_on_self_filed_issue_passes_guardraildrives the realfetch+ enrichment + the production guardrail frombuild_source_runners. Reverting onlynerve/sources/github.pyfails it withassert ['bot'] == ['bot', 'maintainer']— the production symptom — and 7 of the other new tests fail too.3023 passed(3.13, matching CI).assignerdefaults to"", so every non-assign path and existing caller is unchanged.