[theaspirational]

Fixes to contributor mode (#6) found while running it together with #16 on current main.

This PR builds on #6 and #16. It contains both of them merged onto main (4a8964b8), with the #6 conflicts against current main resolved (27e4331), plus 13 commits on top. If #6 and #16 land first, only the top commits remain. Please review only 27e4331..HEAD (16 files, +737/-70) and the merge resolution in 27e4331. If you would rather take these as fixes inside #6, say so and I will retarget.

What changes (contributor mode)

• Delivery is not lost when no host can receive it. A notification pass pauses, with no API or state changes, when no connected MCPL host has a granted event egress. An eligible host resumes polling; cursors survive the pause.
• Refused handoffs are retried without freezing the listing. A refused thread event keeps its own cursor; the global listing watermark and conditional header still move. After five failed passes a persisted terminal record stops the cycle and its drop notice from repeating, including across a restart. A refused checks.completed keeps its existing head/TTL schedule.
• Either granted egress counts as delivery. If the channel message is accepted and the push is declined (or the reverse), the poller no longer re-sends the accepted one; the decline is warned about and treated as final for that egress.
• Truncated notification windows stay available for retry instead of being skipped.
• Review pagination walks descending pages through empty pages, with an explicit read cap, and keeps first-page reviews after an empty tail page.
• Docs: README and changelog.d/contributor-delivery.fixed.md describe the delivery, retry and truncation contracts, including the shared-token rate-limit cost of a window that stays saturated.
• Tests: synthetic tests for each of the above; the host lifecycle e2e check now starts its fixtures through the host's Node path.

Tests

On this exact tree, Node 22.12.0 and 22.23.3: npm ci, typecheck, build, npm test: 771 tests, 751 pass, 0 fail, 20 skipped. Also 771/771 with @animalabs/agent-framework 0.30.0 installed. All tests are synthetic: no GitHub API calls and no credentials.

Co-authored with GPT-6-Astra; each commit carries a Co-Authored-By trailer.

[github-actions]

Automated review

Recommendation: needs a maintainer's decision. The change belongs, but part of it needs a maintainer's call.

Reviewed commit 2d39f8b with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.

Does this change belong in the project?

It fits, with concerns. The top commits are reliability fixes and docs for contributor mode, and they work through the server's single event pipeline, so they belong if #6 is taken. The concern is that this PR also carries all of #6 and #16, two separate and still-unmerged proposals, and those need their own decisions.

5 reasons with sources

• The PR bundles all of #6 (contributor mode) and #16 (mcpl-core 0.4, Node floor, CI matrix) along with its own fixes. The diff adds src/notifications-poller.ts in full, plus changelog.d/contributor-mode.added.md, mcpl-core-0.4.breaking.md and mcpl-core-0.4.fixed.md. It also changes package.json and package-lock.json, and drops the 22.0.0 install check from ci.yml. changelog.d/README.md asks for one fragment per change, and this PR adds four from three different changes. Maintainers should decide on #6 and #16 separately and merge this only as the top commits (27e4331..HEAD). (README.md (base); changelog.d/README.md)
• Whether this PR belongs depends on #6, because the base project has no contributor mode. The base README says one server serves one repository or several of one owner, with one process per owner, and that GITHUB_REPOS entries must share one owner (one App installation, one token). Contributor mode changes that with a user token and repositories across owners. That scope decision belongs to #6. These fixes only matter if it is accepted. (README.md (base), lines 3-7 and 242)
• The fixes route contributor events through the existing shared event pipeline and do not add a second path. The poller hands events to the same onEvent/handleEvent that the webhook and the App-mode poller use. The new canReceiveEvents only reuses the existing eventEgressAllowed check. Per the separate code review, it treats a grant of channels.incoming without channels.register as ready, so the readiness check does not fully work yet. (src/server.ts, event pipeline section)
• Not losing events when no host can receive them fits the project's 'wake, not poll-and-hope' purpose. The README sells the connector on reliable push wakes. Pausing when there is no granted egress, retrying refused handoffs, and counting either egress as delivery all follow from the base README's rule that github.events needs only one of pushEvents or the channel paths (lines 323-328). (README.md (base), lines 9-12)
• The docs and changelog fragment follow the repository's conventions. contributor-delivery.fixed.md is a flat .fixed.md file made of bullets, as the fragment rules require. The behavior is documented in the README alongside the existing mode sections. (changelog.d/README.md)

Findings

6 findings of medium or high severity, see the inline comments:

• Medium severity. canReceiveEvents reports ready for a host that granted channels.incoming without channels.register, though nothing can be delivered.
• Medium severity. Terminal (stopped) retry entries are never removed, so the persisted retry list grows without bound.
• Medium severity. A pipeline exception now becomes a refused receipt, so one event that always fails blocks its whole thread forever.
• Medium severity. Readiness is checked only at the start of a pass, so a host that leaves mid-pass uses up retry budget.
• Medium severity. After a refused row, later rows in the same thread are still delivered, so the refused one comes later and out of order.
• Medium severity. A walk over empty review pages throws ReadCapError, so the agent is told about a 2,000-review backlog that does not exist.

4 more findings, less severe

• Low severity, documentation, src/server.ts:1282-1295. The pipeline's JSDoc now sits on canReceiveEvents, leaving two stacked doc comments. Inserting canReceiveEvents between the existing 'Every ingress path hands its events here…' doc block and onEvent attaches that block to nothing, followed directly by the new block for canReceiveEvents. Tooling and readers will link the pipeline description to the wrong member, and onEvent, now returning a boolean receipt, has no doc saying what true and false mean. Move the old block back onto onEvent and document the receipt there.
• Low severity, maintainability, .github/workflows/ci.yml:14-16. The install-and-start check no longer runs on the engines floor. Before, the matrix ran the install/start check on the engines floor (22.0.0). It now runs only on '24' and 'latest', even though engines was raised to >=22.12.0. The full suite runs on 22.12.0, but the packaged install-and-start path is no longer exercised on the minimum supported Node. Adding '22.12.0' to the matrix keeps the floor covered. This CI change is also not mentioned in the PR's stated purpose.
• Low severity, test coverage, test/notifications-poller.test.ts:1382-1441. No test for a grant that passes canReceiveEvents but cannot deliver (incoming without register). The receipt tests grant ['channels.register', 'channels.incoming'] or ['pushEvents'] together, and test 'ungranted' as nothing at all. No case grants channels.incoming alone, so the gap between canReceiveEvents and the registry's canRegister is not covered. Add a case that asserts the poller pauses there, or at least does not drop threads.
• Low severity, test coverage, test/notifications-poller.test.ts:1061-1084. Tests lock in that terminal retry records stay after success, with no test that they are ever pruned. The test asserts a dropAnnounced entry is still there (notificationRetries()[0].dropAnnounced === true) after the thread succeeded. No test covers terminal entries piling up across many threads, or their eventual removal, so unbounded growth of notifications.retry would go unnoticed.

For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.

[github-actions]

Medium severity. canReceiveEvents reports ready for a host that granted channels.incoming without channels.register, though nothing can be delivered.

canReceiveEvents returns true when eventEgressAllowed(conn, 'incoming') holds, and that check only tests channels.incoming. Channel delivery also needs registration: ChannelRegistry.register returns undefined when canRegister() (grant.has('channels.register'), server.ts:668) is false, and deliver then answers channel-not-registered. Take a host that grants channels.incoming but neither channels.register nor push. The poller does not pause, every thread handoff returns false, and each thread uses up its five attempts. Each is then dropped with a notice the host also refuses. This is the case the PR says it handles: delivery should pause while no host can receive. The predicate should require registration for the incoming path, or ask the registry the same question it asks itself.

[github-actions]

Medium severity. Terminal (stopped) retry entries are never removed, so the persisted retry list grows without bound.

Terminal records are kept: stopped: true entries for lasting failures, exhausted retries, skipped backlogs, and threads that succeeded after a dropAnnounced notice (lines 602-607). Such a record is removed only when a strictly newer notification for the same thread is listed and its read then succeeds. A thread that is deleted, goes private or never moves again keeps its entry forever. notificationRetries() copies the whole list, and setNotificationRetries plus flush() serialize the full snapshot after every thread of every pass (lines 637-638). A long-running contributor process therefore keeps writing a growing state file, and nothing ever trims it. Old terminal records need an age or count bound, for example dropping them once the watermark has passed their updatedAt by some margin.

[github-actions]

Medium severity. A pipeline exception now becomes a refused receipt, so one event that always fails blocks its whole thread forever.

handleEvent used to return void when tagContext, tagsFor or renderEvent threw. It now returns false, and the notifications poller treats false as non-delivery. Take an event whose rendering or tagging fails every time (for example malformed content). Its thread's cursor never moves past it. The thread is retried for five passes, stopped, and then reopened by each newer notification from the same held cursor (attempts reset to 0). The same event fails again each time, so every later comment or review on that thread is held behind it indefinitely. Before this change the event was logged and skipped. Transient GitHub failures inside tagContext (including rate limits) are also counted as ordinary 'failed' attempts. Errors that will repeat should be told apart from refusals, or the event should be skipped once retries are exhausted.

[github-actions]

Medium severity. Readiness is checked only at the start of a pass, so a host that leaves mid-pass uses up retry budget.

pollOnce calls this.ready() once, before the listing. Suppose the host disconnects or revokes egress while the pass is reading threads. Each remaining thread's handoff then returns false (handleEvent sees no conn or no grant). The thread is counted as failed, its attempts go up, and with enough such passes (for example a host that reconnects often) it is dropped with a drop notice that nobody receives. The PR says passes pause without state changes while no host can receive. The loop over toRead (and pollOwnChecks) should check canPoll() again and stop the pass rather than count failures.

[github-actions]

Medium severity. After a refused row, later rows in the same thread are still delivered, so the refused one comes later and out of order.

In pollThread's deliver closure, a refused or thrown handoff only records refusal and does continue. Later candidates in the same sorted batch are still handed to the host and recorded in the dedup ring. The refused row is held by the cursor and delivered on a later pass, after newer comments or reviews on the same thread. Now that hosts can decline with an explicit false receipt, this happens on ordinary refusals and not only on exceptions. The agent then sees replies before the comments they answer. Stopping the batch at the first refusal would keep thread order.

[github-actions]

Medium severity. A walk over empty review pages throws ReadCapError, so the agent is told about a 2,000-review backlog that does not exist.

When the Link header names a last page more than THREAD_MAX_PAGES (20) above the walk's position and the tail pages are empty, listReviewsSince throws ReadCapError. The poller turns that into BacklogTooLarge: it moves the thread's cursor to its current update, skipping any unread rows, and posts a thread.read_failed notice. The notice reads 'it had more than 2,000 comments or reviews to read at once', yet those pages held no reviews at all. The read budget is reasonable, but this failure should be reported as its own kind (an empty or inconsistent listing), not as a backlog skip that misinforms the agent.