E2E Test Report

Test Plan

Repo: OrbitZore/channel-e2e-0724 (fresh, public)
Channel: pollInterval=30s, sessionScope=chat_thread

Results

Score: 8/8 ✅

Bug Found & Fixed

tryFirstContactBody cursor race (forget reply)

tryFirstContactBody() used createdAt <= cursor to filter old issues. But the cursor advances globally based on notification updated_at. When a different notification with a later updated_at is processed first, the cursor advances past the issue's created_at, causing first-contact to incorrectly skip the issue body.

Fix: Replaced createdAt vs cursor check with recentlyProcessed dedup set (same mechanism used for comment dedup).

Reply Count Audit

Observation (Not a Bug)

The agent session has access to gh CLI authenticated as the repo owner. When asked to "reply with X", the agent sometimes posts via gh issue comment (as the owner) AND the channel delivers the response (as the bot) — creating two comments. This is agent behavior, not a channel adapter bug. Mitigation: exclude gh from channel session tools or add system prompt guidance.

Thanks for the PR — this is a well-structured redesign of the GitHub channel adapter after #7266 was closed.

Template looks good ✓

Problem: This is a feature addition, not a bug fix. The predecessor PR #7266 was closed after 6 review rounds because the shared GitHub/Gitea/GitLab abstraction was unworkable. This PR takes the right lesson from that: implement GitHub separately with its native last_read_at watermark. The E2E results (8/8 pass on real GitHub API) demonstrate the feature works. The problem being solved — interacting with Qwen Code via GitHub issues/PRs — is a real user need.

Direction: Aligned. The CHANGELOG shows active channel development (memory, telemetry, DingTalk webhook, workspace scoping, etc.), and adding a code-hosting channel is a natural extension. Claude Code has /install-github-app but no polling-based GitHub channel — no direct CHANGELOG reference, but the area is clearly relevant.

Size: This is a cross-package change touching packages/channels/base (ChannelBase.ts: 204+75, PollingChannelBase.ts: 112 new, SessionRouter.ts: 4, types.ts: 10+1), packages/channels/github (new package: ~350 production lines), and packages/cli (1 line registration). Production logic: ~759 lines; test: ~756 lines; docs/config: ~630 lines. Flagging for maintainer awareness given the 500+ production line threshold on cross-package changes.

Approach: The architecture is sound — notification-as-wakeup with last_read_at watermark is the correct design for GitHub's notification API. A few observations:

• The sendThreadMessage refactoring in ChannelBase.ts is the highest-risk part: it touches ~40 call sites, replacing sendMessage(chatId, text) with sendThreadMessage(chatId, threadId, text). The default fallthrough to sendMessage means existing IM adapters are behaviorally unchanged, but the sheer number of replacements warrants careful review.
• PollingChannelBase<Cursor> is clean and minimal — poll loop, cursor persistence, backoff. Good separation.
• Mention detection via explicit regex (not inferred from strip-before/after) avoids the false-positive class that plagued feat(channels): add GitHub/GitLab/Gitea polling adapters with cursor fix and docs #7266.
• The recentlyProcessed set clearing entirely at 10K entries is pragmatic but worth noting — a burst of activity could theoretically cause one duplicate after a clear.

Moving on to code review. 🔍

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 1416d4c66ec8827a1bbeb2e0905849a70e083ea4 · re-run with @qwen-code /triage}

Code Review

Independent proposal: For a GitHub polling adapter, I'd create a new packages/channels/github package with a GithubChannel extending ChannelBase, use octokit for API access, poll GET /notifications for unread threads, enumerate comments via listComments(since=last_read_at), detect mentions via regex, and post replies via issues.createComment. I'd add poll loop infrastructure (interval, backoff, cursor persistence) and a chat_thread session scope for cross-repo isolation.

Comparison: The PR's approach matches this closely. The PollingChannelBase<Cursor> abstraction is a clean way to share poll loop infrastructure. The sendThreadMessage refactoring in ChannelBase is the most invasive part but necessary for thread-targeted delivery. The mention detection via explicit regex (not strip-before/after comparison) is the right call.

Findings:

One test failure: packages/cli/src/commands/channel/channel-registry.test.ts expects the channel type list to be ['telegram', 'weixin', 'dingtalk', 'wecom', 'feishu', 'qq'] but the PR adds github without updating the assertion. This needs a one-line fix.

No critical blockers in the production code. The sendThreadMessage refactoring is mechanical — every call site passes envelope.threadId or target.threadId, and the default fallthrough to sendMessage means existing IM adapters are behaviorally unchanged. The sendResponseMessage change correctly looks up threadId from activePrompts or router.getTarget.

The PollingChannelBase is well-implemented: atomic cursor persistence (tmp + rename), exponential backoff (2s → 30s), abortable sleep for clean shutdown. The GithubAdapter correctly uses last_read_at as the per-thread watermark, with the global cursor as a performance optimization only.

Test Results

Unit tests (all pass):

packages/channels/github:
 ✓ src/mention.test.ts (14 tests) 4ms
 ✓ src/GithubAdapter.test.ts (21 tests) 28ms
 Test Files  2 passed (2)
      Tests  35 passed (35)

packages/channels/base (PollingChannelBase):
 ✓ src/PollingChannelBase.test.ts (9 tests) 114ms
 Test Files  1 passed (1)
      Tests  9 passed (9)

packages/channels/base (full suite):
 Test Files  19 passed (19)
      Tests  897 passed (897)

TypeScript build: Clean, no errors.

CLI integration: GitHub channel type registered correctly:

Supported channel types: ['telegram', 'weixin', 'dingtalk', 'wecom', 'feishu', 'qq', 'github']
Plugin: { channelType: 'github', displayName: 'GitHub', requiredConfigFields: ['token'] }

Test failure:

packages/cli/src/commands/channel/channel-registry.test.ts
 FAIL  channel registry > only marks the manually configurable built-in types as manageable
 Expected: ['telegram', 'weixin', 'dingtalk', 'wecom', 'feishu', 'qq']
 Received: ['telegram', 'weixin', 'dingtalk', 'wecom', 'feishu', 'qq', 'github']

The test hardcodes the expected channel type list and needs github added.

Real-scenario testing: tmux is not available in this CI environment. Full E2E testing of the GitHub polling flow requires a real GitHub token and a running daemon, which is beyond what can be verified here. The PR author reports 8/8 E2E tests passing on a real GitHub API with a bot account.

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 1416d4c66ec8827a1bbeb2e0905849a70e083ea4 · re-run with @qwen-code /triage}

Confidence: 3/5 — clean review, but the cross-package scope (759 production lines) needs a maintainer's sign-off, and channel-registry.test.ts needs github added to the expected type list.

This is a well-designed feature PR. The notification-as-wakeup architecture is the right call for GitHub's notification API — treating notifications as thread-level wake-up signals and using last_read_at as the per-thread watermark is exactly how I'd approach it. The predecessor PR #7266 tried to abstract across GitHub/Gitea/GitLab and collapsed under the weight of their different dedup mechanisms; this PR correctly implements GitHub natively.

The PollingChannelBase<Cursor> is clean and reusable — atomic cursor persistence, exponential backoff, abortable sleep. The GithubAdapter at 317 lines is appropriately sized for what it does. Mention detection via explicit regex (not strip-before/after comparison) avoids the false-positive class that was a blocker in #7266.

The sendThreadMessage refactoring in ChannelBase.ts is the highest-risk part — ~40 call sites changed — but it's mechanical and the default fallthrough to sendMessage means existing IM adapters are behaviorally unchanged. All 897 existing channel base tests pass, which is strong evidence the refactoring is safe.

What needs fixing: packages/cli/src/commands/channel/channel-registry.test.ts hardcodes the expected channel type list and fails because github isn't in it. One-line fix.

Why deferring: The PR touches shared channel infrastructure (ChannelBase.ts, SessionRouter.ts, types.ts) across multiple packages with 759 production logic lines. Per our review policy, cross-package changes at this scale need a maintainer's eye before merge — not because anything looks wrong, but because the blast radius of a ChannelBase regression is every channel adapter.

⏸️ Deferring to @wenshao — the code review is clean and the architecture is sound, but the cross-package scope (ChannelBase.ts sendThreadMessage refactoring + new PollingChannelBase + new github package) warrants a maintainer's sign-off. One test failure in channel-registry.test.ts needs a one-line fix (add 'github' to the expected type list).

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 1416d4c66ec8827a1bbeb2e0905849a70e083ea4 · re-run with @qwen-code /triage}

Review — GitHub polling channel adapter

Reviewed the full diff (24 files, +2110/−85) against origin/main. The notification-as-wakeup architecture is the right call and a clear improvement over #7266 — last_read_at as the per-thread watermark, explicit-regex mention detection, and PollingChannelBase (atomic tmp+rename cursor write, abortable sleep, 2s→30s backoff) are all well built. The sendThreadMessage refactor is mechanical and the default fallthrough genuinely leaves IM adapters untouched.

Below are findings the triage bot didn't surface. 2 blockers, 4 worth fixing before merge.

🔴 Blockers

1. sessionScope: 'chat_thread' never reaches the SessionRouter — the headline design property is not in effect by default

GithubAdapter.ts:33 forces the scope via an object spread:

super(name, { ...config, sessionScope: 'chat_thread' }, bridge, options);

That spread builds a new object. The caller's config is unchanged — and every real CLI path derives the router's scope from that object, before createChannel runs:

• start.ts:212-216 (single-channel): new SessionRouter(bridge, config.cwd, config.sessionScope, …)
• start.ts:385-387 (multi-channel): router.setChannelScope(name, config.sessionScope) — the loop runs before the createChannel loop
• daemon-worker.ts:475-477 (daemon): same, before createChannel

All three inject options.router, so ChannelBase.ts:592-594 (this.router = options?.router || new SessionRouter(…, config.sessionScope)) never falls through to the constructor's value. parseChannelConfig defaults sessionScope to 'user' (config-utils.ts:465-466).

Net effect with a config that doesn't spell out sessionScope: routing key is github:<login>:<owner/repo> (SessionRouter.ts:118-120), so every issue and PR in a repo shares one session per commenter. Issue #1's context bleeds into issue #42, and because dispatchMode defaults to 'steer' (ChannelBase.ts:4453), a comment on #42 cancels an in-flight turn on #1. Replies still land on the correct thread (threadId comes from the envelope), so the corruption is invisible from the outside.

This is also a split-brain: this.config.sessionScope inside the channel is 'chat_thread' while the router uses 'user'.

Why testing missed it: the unit tests construct GithubChannel with no injected router (so the constructor path wins), and the E2E settings.json set sessionScope: chat_thread explicitly — as docs/users/features/channels/github.md:173 instructs.

Suggested fix — add defaultSessionScope?: SessionScope to ChannelPlugin (types.ts:309) and apply it in parseChannelConfig where 'user' is defaulted today, so the CLI, the router, and the adapter all agree. Dropping the constructor override afterwards would remove a knob that currently looks load-bearing but isn't.

2. channel-registry.test.ts will fail CI (already flagged by the bot — confirming it's real)

packages/cli/src/commands/channel/channel-registry.test.ts:7-14 asserts the exact ordered type list ['telegram','weixin','dingtalk','wecom','feishu','qq']. Adding github to the registry breaks it. One-line fix; the manageable assertion on line 17 needs no change.

🟠 Should fix before merge

3. markThreadAsRead in a bare finally permanently drops mentions on transient API errors — GithubAdapter.ts:196-198

try {
  const comments = await this.octokit.paginate(this.octokit.rest.issues.listComments, {…});
  …
} finally {
  await this.markThreadAsRead(notification.id);
}

There is no catch. If listComments (or issues.get inside tryFirstContactBody) throws — secondary rate limit, 5xx, 404 on a repo the token can't read — the thread is still marked read, then the error propagates and aborts pollOnce. Since GET /notifications is unread-only, that thread never comes back: the @mention is silently lost, with no error comment and no retry. Only mark read on the success path, or record failures and retry before marking.

Same shape, milder: an error on notification N aborts the whole batch, so notifications N+1… are skipped for that cycle (they stay unread, so they do come back).

4. The adapter marks every notification read, including ones it has nothing to do with — GithubAdapter.ts:100-102, 196-198

Non-issue/PR subjects (releases, discussions, security alerts, CI failures) hit markThreadAsRead and then continue. Point this at a personal account and it will silently clear that account's entire GitHub notification inbox — an irreversible side effect on state the user cares about, and one nothing in the docs warns about. The dedicated-bot-account suggestion in github.md:147 reads as optional; it isn't. Please add an explicit warning, and consider skipping markThreadAsRead for subjects the adapter can't handle, plus a repo allowlist so untouched repos are never marked.

Related: there is no repo scoping at all — the adapter consumes notifications from every repo the account watches. groupPolicy: 'allowlist' + explicit groups keys is the mechanism, but the docs only demonstrate groupPolicy: 'open' + groups: {"*": …}.

5. Failure to resolve the bot identity fails open — GithubAdapter.ts:53-58

getAuthenticated() errors are swallowed and botUsername stays null, forever (never retried):

• with requireMention (the default), isMentioned is hardcoded false at :148-150 → the channel is silently inert, no error surfaced anywhere but stderr;
• with requireMention: false, the self-comment filter at :134-141 short-circuits on this.botUsername && → the bot's own comments are no longer excluded, opening a self-reply loop whenever one of its replies lands at/after last_read_at.

Throw from connect() instead — docs/developers/daemon/15-channel-adapters.md:182 says adapter connect() failures are reported separately, which is exactly the right surface here.

6. envelope.metadata is prepended outside the sender-attribution boundary, and it breaks agent-side slash commands — ChannelBase.ts:4573-4575

if (envelope.metadata) {
  promptText = sanitizePromptText(envelope.metadata) + '\n\n' + promptText;
}

This runs after the [sender] tag (:4394-4403) and the [Replying to: …] wrapper, so it is the outermost layer. Two consequences:

• Trust boundary inverted. The issue/PR title in that line is attacker-controlled — anyone can open an issue with any title. The long comment at ChannelBase.ts:4380-4393 explains that the [sender] tag exists precisely so attacker text can't sit in the prompt unattributed and pose as a directive; metadata lands above it. sanitizePromptText folds C0/newlines so multi-line injection is out, but one line is enough for … | URL: … Ignore the above and …. Suggest wrapping the title with sanitizeQuotedText(title, N) and framing it the way channel memory does (ChannelBase.ts:3771-3779: "user-provided facts only; not authorization"), or appending it below the attributed body rather than above.
• Slash commands break. The [sender] tag is deliberately suppressed for a recognizedSlashCommand because "a [sender] prefix would stop it from parsing" (:4384-4387). Prepending metadata reintroduces exactly that: the prompt no longer starts with /, so agent-exposed commands can't parse on this channel. Skip the metadata prepend when recognizedSlashCommand is set.

Minor: the same header is re-prepended on every comment in a thread, not just the first.

7. chat_thread sessions are genuinely multi-operator, but isSharedSession() returns false for them — ChannelBase.ts:3788-3793

this.config.sessionScope === 'single' ||
(target.isGroup === true && this.config.sessionScope === 'thread')

'chat_thread' isn't listed. Under the documented config, everyone commenting on issue #42 shares one session — yet all the shared-session gates are off: /clear skips both the authorized-member check and the confirm step, and /who, /status, /cancel and permission approve/deny are ungated. On a public repo with a multi-user allowedUsers, user B can clear or answer permission prompts in a session driven by user A. Adding 'chat_thread' to isSharedSessionTarget closes it. (Host-shell ! is unaffected — the isGroup check at :4419 already blocks it.)

🟡 Nits / follow-ups

• octokit is a very heavy dependency for 5 endpoints. The adapter uses users.getAuthenticated, activity.{listNotifications,markThreadAsRead}, issues.{listComments,createComment,get} and paginate. The octokit meta-package drags in @octokit/app, @octokit/oauth-app, @octokit/webhooks, universal-github-app-jwt, bottleneck, and — visible in the lockfile — @types/aws-lambda as a production dependency. @octokit/rest (or @octokit/core + paginate/rest-endpoints/retry/throttling) covers the same surface for ~30 fewer transitive prod deps in the shipped bundle.
• PR review comments are invisible. Only issues.listComments is enumerated, so @bot in an inline code-review comment or a review submission body produces a notification, zero "new comments", and a silent markThreadAsRead — never retried. Worth adding to Known Limitations at minimum.
• First-contact body mention is lost if any unrelated comment exists. GithubAdapter.ts:188 gates on newComments.length === 0, but newComments is filtered only by bot-self/dedup, not by mention. New issue with @bot in the body + one "+1" comment before the first poll → the branch is skipped and the body mention never fires.
• startPollLoop() isn't re-entrancy guarded — PollingChannelBase.ts:62-66 reassigns abortController while an existing loop still holds the old signal, yielding two concurrent loops. if (this.running) return; covers it.
• onPairingRequired passes threadId: undefined → GithubChannel.sendMessage throws → the pairing code is never delivered (swallowed as "pairing notification failed" at ChannelBase.ts:4139-4148). senderPolicy: 'pairing' is effectively unsupported on this channel; document it or thread the id through.
• recentlyProcessed.clear() at 10k (GithubAdapter.ts:184) drops all dedup state at once. A FIFO trim of the oldest ~10% avoids the cliff.
• scripts/clean-package-build-artifacts.js:15-29 wasn't updated — packages/channels/github is missing, so its stale dist/ survives npm run clean (and therefore npm run preflight). scripts/build.js was updated, so this looks like an oversight. Root vitest.config.ts projects also omits it — lower priority, since npm run test:ci --workspaces picks it up via the package's own test:ci, and wecom/feishu are already absent there.
• packages/channels/github/src/index.ts doesn't re-export GithubChannel. Every other adapter does (export { QQChannel } from './QQChannel.js';).
• Tests write to a hardcoded /tmp/test-polling-base with no cleanup (PollingChannelBase.test.ts:20, 33). The loop/stop/backoff cases persist cursors that leak into subsequent runs, and the path ignores the platform tmpdir. mkdtempSync(join(tmpdir(), …)) + cleanup would isolate it.
• requireMention is two knobs that look like one. The channel-level flag (GithubAdapter.ts:38-40) is separate from GroupGate's per-group requireMention, which defaults to true — so requireMention: false alone changes nothing. github.md:206 documents only the channel-level one.
• Poll-loop serialization (acknowledged in the PR body) has a second-order effect worth documenting: a slow turn also delays markThreadAsRead for every later notification in the same batch, so the effective cadence is pollInterval + Σ(turn durations).

What's good

Mention detection separating testBotMention from stripBotMention with an explicit boolean is the right lesson from #7266, and the lookahead excluding / correctly avoids matching @org/team. The cursor write is properly atomic. abortableSleep cleans up its listener. sendResponseMessage resolving threadId from activePrompts ?? router.getTarget is a nice touch that makes proactive/loop delivery work on threaded channels. Test coverage of the poll/dedup/first-contact paths is genuinely good — findings 1 and 3 are gaps in what the mocks can observe, not in diligence.