From 9ff19fd369e060d94a882574a010c85422581b8a Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Mon, 3 Aug 2026 15:46:11 +0800 Subject: [PATCH 01/10] feat(channels): support group pairing --- docs/developers/qwen-serve-protocol.md | 12 +- docs/users/features/channels/overview.md | 25 +- packages/channels/base/README.md | 19 +- .../channels/base/src/ChannelBase.test.ts | 213 +++++++++++++++++- packages/channels/base/src/ChannelBase.ts | 74 +++++- packages/channels/base/src/GroupGate.test.ts | 21 ++ packages/channels/base/src/GroupGate.ts | 40 +++- .../channels/base/src/PairingStore.test.ts | 62 +++++ packages/channels/base/src/PairingStore.ts | 112 ++++++++- packages/channels/base/src/index.ts | 2 +- packages/channels/base/src/types.ts | 2 +- .../cli/src/commands/channel/pairing.test.ts | 33 +++ packages/cli/src/commands/channel/pairing.ts | 14 +- .../serve/channel-management-service.test.ts | 82 ++++++- .../src/serve/channel-management-service.ts | 37 ++- .../src/serve/channel-settings-store.test.ts | 2 + .../cli/src/serve/channel-settings-store.ts | 2 +- .../workspace-channel-management.test.ts | 51 ++++- .../routes/workspace-channel-management.ts | 40 +++- packages/sdk-typescript/src/daemon/index.ts | 1 + packages/sdk-typescript/src/daemon/types.ts | 14 +- packages/sdk-typescript/src/index.ts | 1 + .../test/unit/DaemonClient.test.ts | 13 ++ .../test/unit/daemon-public-surface.test.ts | 2 + 24 files changed, 797 insertions(+), 77 deletions(-) diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 0d7456f3e29..473ad744fff 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -794,19 +794,21 @@ Runtime actions are strict-gated `POST` requests to worker owned by the resolved workspace. Pairing management is available only for instances configured with the -`pairing` sender policy: +`pairing` sender policy or group policy: - `GET .../channels/:name/pairing-requests` - `POST .../channels/:name/pairing-requests/approve` with `{ "code": "..." }` - `GET .../channels/:name/pairing-approvals` - `DELETE .../channels/:name/pairing-approvals` with - `{ "senderId": "..." }` + either `{ "senderId": "..." }` or `{ "groupId": "..." }` All pairing routes require a bearer token and use `Cache-Control: no-store`. Requests, approvals, and revocations are scoped to the selected Channel -instance and workspace. The approvals snapshot contains sender IDs because the -allowlist does not persist sender display names. Revoking an unknown sender -returns `404 channel_pairing_approval_not_found`. +instance and workspace. Pending requests include a typed user or group subject; +group requests also retain the sender who initiated the request. Approval +snapshots contain `senderIds` and `groupIds` because allowlists do not persist +display names. Revoking an unknown user or group returns +`404 channel_pairing_approval_not_found`. ### Channel delivery and Notify diff --git a/docs/users/features/channels/overview.md b/docs/users/features/channels/overview.md index 88133ac5703..ef64f640415 100644 --- a/docs/users/features/channels/overview.md +++ b/docs/users/features/channels/overview.md @@ -66,7 +66,7 @@ Channels are configured under the `channels` key in `settings.json`. Each channe | `approvalMode` | No | Tool approval mode for channel sessions. Unattended webhook tasks require `yolo`; the setting applies to every session on the channel | | `instructions` | No | Custom instructions prepended to the first message of each session | | `webhooks` | No | Webhook sources and delivery targets for daemon-managed channels. See [Webhook-triggered tasks](#webhook-triggered-tasks) | -| `groupPolicy` | No | Group chat access: `disabled` (default), `allowlist`, or `open`. See [Group Chats](#group-chats) | +| `groupPolicy` | No | Group chat access: `disabled` (default), `allowlist`, `pairing`, or `open`. See [Group Chats](#group-chats) | | `dmPolicy` | No | Private/DM access: `open` (default) or `disabled` (silently drop all DMs). Useful for group-only bots | | `groupHistoryLimit` | No | Opt-in group history backfill. `0` or omitted disables it. A positive number persists that many authorized, unmentioned group messages for the next bot mention/reply. | | `groups` | No | Per-group settings. Keys are group chat IDs or `"*"` for defaults. See [Group Chats](#group-chats) | @@ -206,7 +206,7 @@ Run these from the channel's workspace directory (or pass `--cwd `) — pai ## Group Chats -By default, the bot only works in direct messages. To enable group chat support, set `groupPolicy` to `"allowlist"` or `"open"`. +By default, the bot only works in direct messages. To enable group chat support, set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"`. ### Group Policy @@ -214,8 +214,21 @@ Controls whether the bot participates in group chats at all: - **`disabled`** (default) — The bot ignores all group messages. Safest option. - **`allowlist`** — The bot only responds in groups explicitly listed in `groups` by chat ID. The `"*"` key provides default settings but does **not** act as a wildcard allow. +- **`pairing`** — A deliberate mention or reply from an unknown group creates one pairing request for the group. Once approved, every member can use the bot in that group; `senderPolicy` continues to control direct messages. - **`open`** — The bot responds in all groups it's added to. Use with caution. +Approve a group with the same CLI command used for user pairing. The pending +request identifies the group and the member who initiated it: + +```bash +qwen channel pairing approve my-channel +``` + +Group approvals are stored by stable chat ID in the channel's workspace scope. +An unmentioned message never creates a group pairing request, even when a group +sets `requireMention` to `false`; after approval, the configured mention policy +applies normally. + ### Mention Gating In groups, the bot requires an `@mention` or a reply to one of its messages by default. This prevents the bot from responding to every message in a group chat. @@ -262,7 +275,7 @@ By default, Qwen ignores unmentioned group messages and does not store them as s - Omitted or `0` disables backfill. - Group-level `groupHistoryLimit` overrides the channel-level value. -- Only messages from authorized senders are persisted. +- Only messages from authorized senders, or members of an approved paired group, are persisted. - Messages rejected by `groupPolicy` or group allowlist are not persisted. - Pending group history is stored as local JSONL under `~/.qwen/channels/-group-history.jsonl` or `$QWEN_HOME/channels/-group-history.jsonl`. - Cached messages are injected as untrusted context on the next real trigger and are not written as standalone session turns. @@ -270,10 +283,10 @@ By default, Qwen ignores unmentioned group messages and does not store them as s ### How group messages are evaluated ``` -1. groupPolicy — is this group allowed? (no → ignore) -2. dmPolicy — is this DM allowed? (disabled → ignore) +1. groupPolicy — is this group disabled, listed, paired, or open? (no → ignore/pairing flow) +2. dmPolicy — is this DM allowed? (disabled → ignore) 3. requireMention — was the bot mentioned/replied to? (no → ignore) -4. senderPolicy — is this sender approved? (no → pairing flow) +4. senderPolicy — is this sender approved? (skipped for a paired group; otherwise no → user pairing flow) 5. Route to session ``` diff --git a/packages/channels/base/README.md b/packages/channels/base/README.md index 3721c48520e..e19d92107b3 100644 --- a/packages/channels/base/README.md +++ b/packages/channels/base/README.md @@ -120,7 +120,7 @@ Everything between `handleInbound()` and `sendMessage()` is handled by the base | `ChannelPlugin` | Plugin factory interface (what you export) | | `Envelope` | Normalized inbound message format | | `SenderPolicy` | `'allowlist' \| 'pairing' \| 'open'` | -| `GroupPolicy` | `'disabled' \| 'allowlist' \| 'open'` | +| `GroupPolicy` | `'disabled' \| 'allowlist' \| 'pairing' \| 'open'` | | `SessionScope` | `'user' \| 'thread' \| 'single'` | | `GroupConfig` | Per-group settings (e.g. `requireMention`) | | `SessionTarget` | Maps a session back to its channel/sender/chat | @@ -278,12 +278,12 @@ constructor(policy: SenderPolicy, allowedUsers?: string[], pairingStore?: Pairin ### GroupGate ```typescript -constructor(policy?: GroupPolicy, groups?: Record) +constructor(policy?: GroupPolicy, groups?: Record, pairingStore?: PairingStore) ``` -| Method | Description | -| ----------------- | ---------------------------------------------------------------------------------------------- | -| `check(envelope)` | Returns `{ allowed: boolean, reason?: 'disabled' \| 'not_allowlisted' \| 'mention_required' }` | +| Method | Description | +| ----------------- | -------------------------------------------------------------- | +| `check(envelope)` | Returns the group policy decision and an optional pairing code | **Policy behavior:** @@ -291,6 +291,7 @@ constructor(policy?: GroupPolicy, groups?: Record) | ----------- | ---------------------------------------- | | `disabled` | All group messages rejected | | `allowlist` | Only groups listed in config are allowed | +| `pairing` | A group must be approved once by chat ID | | `open` | All groups allowed | When `requireMention` is `true` (default), group messages are only processed if the bot is @mentioned or the message is a reply to the bot. @@ -301,16 +302,20 @@ When `requireMention` is `true` (default), group messages are only processed if constructor(channelName: string, workspaceCwd?: string) ``` -Persists pairing state to `{channelName}-pairing.json` and `{channelName}-allowlist.json`. With `workspaceCwd` (what `ChannelBase` passes — the channel's `cwd`), the files live under the workspace-scoped directory `~/.qwen/channels//` so two workspaces reusing the same channel name never share pairing requests or allowlist entries. Without it, the legacy global `~/.qwen/channels/` layout is used. The first time a given (workspace, channel) pair is constructed, existing legacy global files are copied in once (grandfathering) so already-approved senders stay approved; a per-channel `.migrated` sentinel in the scope directory marks that decision, after which legacy files are never consulted again for that channel. Channel names are URI-encoded in file names, so a name containing path separators cannot escape the scope directory. `revoke(senderId)` removes the sender only from this store's allowlist and never mutates the legacy global baseline. +Persists pending pairing state to `{channelName}-pairing.json`, user approvals to `{channelName}-allowlist.json`, and group approvals to `{channelName}-group-allowlist.json`. With `workspaceCwd` (what `ChannelBase` passes — the channel's `cwd`), the files live under the workspace-scoped directory `~/.qwen/channels//` so two workspaces reusing the same channel name never share pairing requests or allowlist entries. Without it, the legacy global `~/.qwen/channels/` layout is used. The first time a given (workspace, channel) pair is constructed, existing legacy global files are copied in once (grandfathering) so already-approved senders stay approved; a per-channel `.migrated` sentinel in the scope directory marks that decision, after which legacy files are never consulted again for that channel. Channel names are URI-encoded in file names, so a name containing path separators cannot escape the scope directory. `revoke(senderId)` removes the sender only from this store's allowlist and never mutates the legacy global baseline. | Method | Description | | ------------------------------------- | --------------------------------------------------------------------------------------------------------- | | `createRequest(senderId, senderName)` | Generate an 8-char pairing code (or return existing). Returns `null` if 3 pending requests already exist. | -| `approve(code)` | Approve a pairing request, adds sender to allowlist. Returns the request or `null`. | +| `createGroupRequest(...)` | Generate a request keyed by stable group ID and record its initiating sender. | +| `approve(code)` | Approve a user or group request and update its corresponding allowlist. | | `isApproved(senderId)` | Check if sender is in the approved allowlist | +| `isGroupApproved(groupId)` | Check if a group is approved | | `listPending()` | Get active (non-expired) pending requests | | `getAllowlist()` | Get approved sender IDs | +| `getGroupAllowlist()` | Get approved group IDs | | `revoke(senderId)` | Remove an approved sender. Returns whether the sender was present. | +| `revokeGroup(groupId)` | Remove an approved group. Returns whether the group was present. | ## Envelope diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index 1799d109968..32282af2ca7 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { EventEmitter } from 'node:events'; -import { mkdtempSync } from 'node:fs'; +import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { @@ -34,6 +34,7 @@ import { ChannelProactiveDeliveryError, isChannelProactiveDeliveryError, } from './ChannelProactiveDeliveryError.js'; +import { PairingStore } from './PairingStore.js'; // Concrete test implementation class TestChannel extends ChannelBase { @@ -2662,6 +2663,59 @@ describe('ChannelBase', () => { ); }); + it('backfills messages from members of an approved paired group', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const store = new PairingStore('test-chan', '/tmp'); + const code = store.createGroupRequest( + 'chat1', + 'Release Team', + 'alice', + 'Alice', + ); + store.approve(code!); + const ch = createChannel( + { + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + groupHistoryLimit: 10, + groups: { '*': { requireMention: true } }, + }, + { groupHistoryPath: groupHistoryPath() }, + ); + + await ch.handleInbound( + envelope({ + isGroup: true, + senderId: 'bob', + senderName: 'Bob', + text: 'background', + }), + ); + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + senderId: 'carol', + senderName: 'Carol', + text: '@bot summarize', + }), + ); + + const prompt = (bridge.prompt as ReturnType).mock + .calls[0][1] as string; + expect(prompt).toContain('- [Bob] background'); + expect(prompt).toContain('[Carol] @bot summarize'); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + it('persists group history across channel instances', async () => { const historyPath = groupHistoryPath(); const config = { @@ -13348,6 +13402,73 @@ describe('ChannelBase', () => { expect(threadMessages[0]!.threadId).toBe('issue:42'); expect(threadMessages[0]!.text).toContain('pairing code'); }); + + it('pairs a mentioned group once and lets other members use it', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }); + const first = envelope({ + isGroup: true, + isMentioned: true, + chatId: 'group-1', + chatName: 'Release Team', + senderId: 'alice', + senderName: 'Alice', + }); + + await ch.handleInbound(first); + + expect(ch.sent).toHaveLength(1); + expect(bridge.prompt).not.toHaveBeenCalled(); + const store = new PairingStore('test-chan', '/tmp'); + const request = store.listPending()[0]; + expect(request?.subject).toEqual({ + type: 'group', + id: 'group-1', + name: 'Release Team', + }); + store.approve(request!.code); + + await ch.handleInbound({ + ...first, + senderId: 'bob', + senderName: 'Bob', + }); + + expect(bridge.prompt).toHaveBeenCalledOnce(); + expect(store.isApproved('alice')).toBe(false); + expect(store.isApproved('bob')).toBe(false); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + + it('does not create group pairing requests from ambient messages', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ groupPolicy: 'pairing' }); + + await ch.handleInbound(envelope({ isGroup: true, chatId: 'group-1' })); + + expect(ch.sent).toEqual([]); + expect(bridge.prompt).not.toHaveBeenCalled(); + expect(new PairingStore('test-chan', '/tmp').listPending()).toEqual([]); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); }); describe('setBridge', () => { @@ -18318,6 +18439,96 @@ describe('ChannelBase', () => { expect(bridge.prompt).toHaveBeenCalled(); }); + it('allows a stored group job after the group is paired', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const store = new PairingStore('test-chan', '/tmp'); + const code = store.createGroupRequest( + 'group-1', + 'Release Team', + 'alice', + 'Alice', + ); + store.approve(code!); + const disable = vi.fn().mockResolvedValue(true); + const ch = createChannel( + { + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }, + { + loopController: { + create: vi.fn(), + listForTarget: vi.fn(), + disable, + validateCron: vi.fn(), + }, + }, + ); + ch.proactiveSupported = true; + + await ch.runLoopPrompt({ + id: 'job-1', + channelName: 'test-chan', + target: { + channelName: 'test-chan', + senderId: 'bob', + chatId: 'group-1', + isGroup: true, + }, + cwd: '/tmp', + cron: '0 9 * * *', + prompt: 'post summary', + recurring: true, + enabled: true, + createdBy: 'Bob', + createdAt: '2026-06-30T01:00:00.000Z', + consecutiveFailures: 0, + runCount: 0, + }); + + expect(disable).not.toHaveBeenCalled(); + expect(bridge.prompt).toHaveBeenCalled(); + + store.revokeGroup('group-1'); + disable.mockClear(); + (bridge.prompt as ReturnType).mockClear(); + + await expect( + ch.runLoopPrompt({ + id: 'job-2', + channelName: 'test-chan', + target: { + channelName: 'test-chan', + senderId: 'bob', + chatId: 'group-1', + isGroup: true, + }, + cwd: '/tmp', + cron: '0 9 * * *', + prompt: 'post summary', + recurring: true, + enabled: true, + createdBy: 'Bob', + createdAt: '2026-06-30T01:00:00.000Z', + consecutiveFailures: 0, + runCount: 1, + }), + ).rejects.toThrow('no longer authorized'); + + expect(disable).toHaveBeenCalledWith('job-2'); + expect(bridge.prompt).not.toHaveBeenCalled(); + expect(store.listPending()).toEqual([]); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + it('rejects stored threaded jobs unless the adapter supports the target', async () => { const ch = createChannel(); ch.proactiveSupported = true; diff --git a/packages/channels/base/src/ChannelBase.ts b/packages/channels/base/src/ChannelBase.ts index 10450bcf359..d0ef49e0246 100644 --- a/packages/channels/base/src/ChannelBase.ts +++ b/packages/channels/base/src/ChannelBase.ts @@ -818,15 +818,18 @@ export abstract class ChannelBase { this.observedContacts = options?.observedContacts; this.bridgeRecovery = options?.bridgeRecovery; - this.groupGate = new GroupGate(config.groupPolicy, config.groups); - this.dmGate = new DmGate(config.dmPolicy); - // Scoped by the channel's workspace cwd: two workspaces reusing the same // channel name must not share pairing/allowlist state (#7017). const pairingStore = - config.senderPolicy === 'pairing' + config.senderPolicy === 'pairing' || config.groupPolicy === 'pairing' ? new PairingStore(name, config.cwd) : undefined; + this.groupGate = new GroupGate( + config.groupPolicy, + config.groups, + pairingStore, + ); + this.dmGate = new DmGate(config.dmPolicy); this.gate = new SenderGate( config.senderPolicy, config.allowedUsers, @@ -3604,9 +3607,11 @@ export abstract class ChannelBase { isReplyToBot: true, }; return ( - this.groupGate.check(envelope).allowed && + this.groupGate.check(envelope, { createPairingRequest: false }).allowed && this.dmGate.check(envelope).allowed && - this.gate.isAllowed(normalizedTarget.senderId) && + (normalizedTarget.isGroup && this.config.groupPolicy === 'pairing' + ? true + : this.gate.isAllowed(normalizedTarget.senderId)) && this.isAuthorizedForSharedSession(envelope) ); } @@ -4714,7 +4719,10 @@ export abstract class ChannelBase { return; } const senderId = truncateGroupHistoryField(envelope.senderId); - if (!this.gate.isAllowed(senderId)) { + if ( + this.config.groupPolicy !== 'pairing' && + !this.gate.isAllowed(senderId) + ) { return; } @@ -4777,9 +4785,10 @@ export abstract class ChannelBase { return promptText; } - const lines = entries.filter((entry) => - this.gate.isAllowed(entry.senderId), - ); + const lines = + this.config.groupPolicy === 'pairing' + ? entries + : entries.filter((entry) => this.gate.isAllowed(entry.senderId)); if (lines.length === 0) { return promptText; } @@ -4799,9 +4808,29 @@ export abstract class ChannelBase { protected preflightInbound(envelope: Envelope): boolean | Promise { const groupResult = this.groupGate.check(envelope); if (!groupResult.allowed) { + if (groupResult.pairingCode !== undefined) { + this.logPreflightRejected('group_pairing_required'); + return this.onGroupPairingRequired( + envelope.chatId, + groupResult.pairingCode, + envelope.threadId, + ) + .then(() => false) + .catch((err: unknown) => { + process.stderr.write( + `[Channel:${this.name}] group pairing notification failed: ${sanitizeLogText( + err instanceof Error ? err.message : String(err), + 200, + )}\n`, + ); + return false; + }); + } if (groupResult.reason === 'mention_required') { // This is the expected high-frequency drop path for group bots. this.recordPendingGroupHistory(envelope); + } else if (groupResult.reason === 'pairing_trigger_required') { + return false; } else { this.logPreflightRejected(`group_${groupResult.reason ?? 'denied'}`); } @@ -4814,6 +4843,11 @@ export abstract class ChannelBase { return false; } + if (envelope.isGroup && this.config.groupPolicy === 'pairing') { + this.markPreflighted(envelope); + return true; + } + const result = this.gate.check(envelope.senderId, envelope.senderName); if (!result.allowed) { if (result.pairingCode !== undefined) { @@ -5738,6 +5772,26 @@ export abstract class ChannelBase { ); } } + + protected async onGroupPairingRequired( + chatId: string, + code: string | null, + threadId?: string, + ): Promise { + if (code) { + await this.sendThreadMessage( + chatId, + threadId, + `This group requires approval. Its pairing code is: ${code}\n\nAsk the bot operator to approve the group with:\n qwen channel pairing approve ${this.name} ${code}`, + ); + } else { + await this.sendThreadMessage( + chatId, + threadId, + 'Too many pending pairing requests. Please try again later.', + ); + } + } } function truncateGroupHistoryField(value: string): string { diff --git a/packages/channels/base/src/GroupGate.test.ts b/packages/channels/base/src/GroupGate.test.ts index 4f541952306..fcaf60eb8fd 100644 --- a/packages/channels/base/src/GroupGate.test.ts +++ b/packages/channels/base/src/GroupGate.test.ts @@ -105,6 +105,27 @@ describe('GroupGate', () => { }); }); + describe('pairing policy', () => { + it('does not create a pairing request for ambient group messages', () => { + const gate = new GroupGate('pairing'); + const result = gate.check(envelope({ isGroup: true })); + expect(result).toEqual({ + allowed: false, + reason: 'pairing_trigger_required', + }); + }); + + it('requests pairing after an explicit mention', () => { + const gate = new GroupGate('pairing'); + const result = gate.check(envelope({ isGroup: true, isMentioned: true })); + expect(result).toEqual({ + allowed: false, + reason: 'pairing_required', + pairingCode: null, + }); + }); + }); + describe('defaults', () => { it('defaults to disabled policy', () => { const gate = new GroupGate(); diff --git a/packages/channels/base/src/GroupGate.ts b/packages/channels/base/src/GroupGate.ts index 82b9754e6da..5e8e14a40e3 100644 --- a/packages/channels/base/src/GroupGate.ts +++ b/packages/channels/base/src/GroupGate.ts @@ -1,20 +1,30 @@ import type { GroupPolicy, GroupConfig, Envelope } from './types.js'; +import type { PairingStore } from './PairingStore.js'; export interface GroupCheckResult { allowed: boolean; - reason?: 'disabled' | 'not_allowlisted' | 'mention_required'; + reason?: + | 'disabled' + | 'not_allowlisted' + | 'mention_required' + | 'pairing_trigger_required' + | 'pairing_required'; + pairingCode?: string | null; } export class GroupGate { private policy: GroupPolicy; private groups: Record; + private pairingStore: PairingStore | null; constructor( policy: GroupPolicy = 'disabled', groups: Record = {}, + pairingStore?: PairingStore, ) { this.policy = policy; this.groups = groups; + this.pairingStore = pairingStore ?? null; } /** @@ -27,7 +37,10 @@ export class GroupGate { * Mention gating runs before sender gate so that unmentioned messages * in groups don't trigger pairing flows. */ - check(envelope: Envelope): GroupCheckResult { + check( + envelope: Envelope, + options: { createPairingRequest?: boolean } = {}, + ): GroupCheckResult { if (!envelope.isGroup) { return { allowed: true }; } @@ -44,6 +57,29 @@ export class GroupGate { } } + if ( + this.policy === 'pairing' && + !this.pairingStore?.isGroupApproved(envelope.chatId) + ) { + if ( + options.createPairingRequest === false || + (!envelope.isMentioned && !envelope.isReplyToBot) + ) { + return { allowed: false, reason: 'pairing_trigger_required' }; + } + const code = this.pairingStore?.createGroupRequest( + envelope.chatId, + envelope.chatName || envelope.chatId, + envelope.senderId, + envelope.senderName, + ); + return { + allowed: false, + reason: 'pairing_required', + pairingCode: code ?? null, + }; + } + // Per-group config, falling back to "*" defaults, then built-in defaults const groupConfig = this.groups[envelope.chatId] || this.groups['*'] || {}; const requireMention = groupConfig.requireMention ?? true; diff --git a/packages/channels/base/src/PairingStore.test.ts b/packages/channels/base/src/PairingStore.test.ts index 21aa3fe9202..d69bf2024d4 100644 --- a/packages/channels/base/src/PairingStore.test.ts +++ b/packages/channels/base/src/PairingStore.test.ts @@ -55,6 +55,53 @@ describe('PairingStore workspace scoping (#7017)', () => { expect(storeB.isApproved('sender-1')).toBe(false); }); + it('approves a group without approving the member who requested it', () => { + const store = new PairingStore('support-bot', workspaceA); + const code = store.createGroupRequest( + 'group-1', + 'Release Team', + 'sender-1', + 'Alice', + ); + + expect(code).toBeTruthy(); + expect(store.listPending()).toEqual([ + expect.objectContaining({ + senderId: 'sender-1', + senderName: 'Alice', + subject: { + type: 'group', + id: 'group-1', + name: 'Release Team', + }, + }), + ]); + + store.approve(code!); + + expect(store.isGroupApproved('group-1')).toBe(true); + expect(store.isApproved('sender-1')).toBe(false); + }); + + it('isolates group approvals and revocation by workspace', () => { + const storeA = new PairingStore('support-bot', workspaceA); + const storeB = new PairingStore('support-bot', workspaceB); + + for (const store of [storeA, storeB]) { + const code = store.createGroupRequest( + 'group-1', + 'Release Team', + 'sender-1', + 'Alice', + ); + store.approve(code!); + } + + expect(storeA.revokeGroup('group-1')).toBe(true); + expect(storeA.isGroupApproved('group-1')).toBe(false); + expect(storeB.isGroupApproved('group-1')).toBe(true); + }); + it('revokes an approved sender only from the selected workspace', () => { const storeA = new PairingStore('support-bot', workspaceA); const storeB = new PairingStore('support-bot', workspaceB); @@ -155,6 +202,21 @@ describe('PairingStore workspace scoping (#7017)', () => { ]); }); + it('approves a legacy pending request as a user request', () => { + seedLegacy(); + const store = new PairingStore('support-bot', workspaceA); + + const approved = store.approve('ABCDEFGH'); + + expect(approved?.subject).toEqual({ + type: 'user', + id: 'pending-sender', + name: 'Pending', + }); + expect(store.isApproved('pending-sender')).toBe(true); + expect(store.getGroupAllowlist()).toEqual([]); + }); + it('lets every workspace grandfather the same legacy baseline (copy, not move)', () => { seedLegacy(); const storeA = new PairingStore('support-bot', workspaceA); diff --git a/packages/channels/base/src/PairingStore.ts b/packages/channels/base/src/PairingStore.ts index 8c409741692..db97deeb366 100644 --- a/packages/channels/base/src/PairingStore.ts +++ b/packages/channels/base/src/PairingStore.ts @@ -12,14 +12,26 @@ const MAX_PENDING = 3; export interface PairingRequest { senderId: string; senderName: string; + subject: PairingSubject; code: string; createdAt: number; // epoch ms } +export interface PairingSubject { + type: 'user' | 'group'; + id: string; + name: string; +} + +type StoredPairingRequest = Omit & { + subject?: PairingSubject; +}; + export class PairingStore { private dir: string; private pendingPath: string; private allowlistPath: string; + private groupAllowlistPath: string; private migratedSentinelPath: string; /** @@ -50,6 +62,10 @@ export class PairingStore { this.dir, `${safeChannelName}-allowlist.json`, ); + this.groupAllowlistPath = path.join( + this.dir, + `${safeChannelName}-group-allowlist.json`, + ); this.migratedSentinelPath = path.join( this.dir, `${safeChannelName}.migrated`, @@ -151,20 +167,53 @@ export class PairingStore { return list.includes(senderId); } + isGroupApproved(groupId: string): boolean { + return this.readGroupAllowlist().includes(groupId); + } + /** * Create a pairing request for an unknown sender. * Returns the code if created, or null if the pending cap is reached. * If the sender already has a non-expired pending request, returns that code. */ createRequest(senderId: string, senderName: string): string | null { + return this.createSubjectRequest( + { type: 'user', id: senderId, name: senderName }, + senderId, + senderName, + ); + } + + createGroupRequest( + groupId: string, + groupName: string, + senderId: string, + senderName: string, + ): string | null { + return this.createSubjectRequest( + { type: 'group', id: groupId, name: groupName }, + senderId, + senderName, + ); + } + + private createSubjectRequest( + subject: PairingSubject, + senderId: string, + senderName: string, + ): string | null { const pending = this.readPending(); // Purge expired const now = Date.now(); const active = pending.filter((r) => now - r.createdAt < EXPIRY_MS); - // Check if sender already has a pending request - const existing = active.find((r) => r.senderId === senderId); + // Check if the same user or group already has a pending request + const existing = active.find( + (request) => + request.subject.type === subject.type && + request.subject.id === subject.id, + ); if (existing) { return existing.code; } @@ -175,14 +224,14 @@ export class PairingStore { } const code = generateCode(); - active.push({ senderId, senderName, code, createdAt: now }); + active.push({ senderId, senderName, subject, code, createdAt: now }); this.writePending(active); return code; } /** * Approve a pairing request by code. - * Returns the sender ID if found, or null if not found / expired. + * Returns the request if found, or null if not found / expired. */ approve(code: string): PairingRequest | null { const pending = this.readPending(); @@ -196,11 +245,18 @@ export class PairingStore { pending.splice(idx, 1); this.writePending(pending); - // Add to allowlist - const list = this.readAllowlist(); - if (!list.includes(request.senderId)) { - list.push(request.senderId); - this.writeAllowlist(list); + if (request.subject.type === 'group') { + const groups = this.readGroupAllowlist(); + if (!groups.includes(request.subject.id)) { + groups.push(request.subject.id); + this.writeGroupAllowlist(groups); + } + } else { + const users = this.readAllowlist(); + if (!users.includes(request.subject.id)) { + users.push(request.subject.id); + this.writeAllowlist(users); + } } return request; @@ -216,6 +272,10 @@ export class PairingStore { return this.readAllowlist(); } + getGroupAllowlist(): string[] { + return this.readGroupAllowlist(); + } + revoke(senderId: string): boolean { const list = this.readAllowlist(); const next = list.filter((id) => id !== senderId); @@ -226,6 +286,16 @@ export class PairingStore { return true; } + revokeGroup(groupId: string): boolean { + const list = this.readGroupAllowlist(); + const next = list.filter((id) => id !== groupId); + if (next.length === list.length) { + return false; + } + this.writeGroupAllowlist(next); + return true; + } + private ensureDir(): void { if (!fs.existsSync(this.dir)) { fs.mkdirSync(this.dir, { recursive: true }); @@ -235,7 +305,15 @@ export class PairingStore { private readPending(): PairingRequest[] { try { const data = fs.readFileSync(this.pendingPath, 'utf-8'); - return JSON.parse(data) as PairingRequest[]; + const requests = JSON.parse(data) as StoredPairingRequest[]; + return requests.map((request) => ({ + ...request, + subject: request.subject ?? { + type: 'user', + id: request.senderId, + name: request.senderName, + }, + })); } catch { return []; } @@ -259,6 +337,20 @@ export class PairingStore { this.ensureDir(); fs.writeFileSync(this.allowlistPath, JSON.stringify(list, null, 2)); } + + private readGroupAllowlist(): string[] { + try { + const data = fs.readFileSync(this.groupAllowlistPath, 'utf-8'); + return JSON.parse(data) as string[]; + } catch { + return []; + } + } + + private writeGroupAllowlist(list: string[]): void { + this.ensureDir(); + fs.writeFileSync(this.groupAllowlistPath, JSON.stringify(list, null, 2)); + } } function generateCode(): string { diff --git a/packages/channels/base/src/index.ts b/packages/channels/base/src/index.ts index c454c9393d5..9062d5652e8 100644 --- a/packages/channels/base/src/index.ts +++ b/packages/channels/base/src/index.ts @@ -72,7 +72,7 @@ export type { ChannelLoopStoreOptions, } from './ChannelLoopStore.js'; export { PairingStore } from './PairingStore.js'; -export type { PairingRequest } from './PairingStore.js'; +export type { PairingRequest, PairingSubject } from './PairingStore.js'; export { GroupGate } from './GroupGate.js'; export type { GroupCheckResult } from './GroupGate.js'; export { DmGate } from './DmGate.js'; diff --git a/packages/channels/base/src/types.ts b/packages/channels/base/src/types.ts index f5338cc5646..77a1996009a 100644 --- a/packages/channels/base/src/types.ts +++ b/packages/channels/base/src/types.ts @@ -6,7 +6,7 @@ import type { ChannelWebhookConfig } from './ChannelWebhookTask.js'; export type SenderPolicy = 'allowlist' | 'pairing' | 'open'; export type SessionScope = 'user' | 'thread' | 'chat_thread' | 'single'; export type ChannelType = string; -export type GroupPolicy = 'disabled' | 'allowlist' | 'open'; +export type GroupPolicy = 'disabled' | 'allowlist' | 'pairing' | 'open'; export type DmPolicy = 'disabled' | 'open'; export type DispatchMode = 'collect' | 'steer' | 'followup'; diff --git a/packages/cli/src/commands/channel/pairing.test.ts b/packages/cli/src/commands/channel/pairing.test.ts index 351cd7615f4..072c106781c 100644 --- a/packages/cli/src/commands/channel/pairing.test.ts +++ b/packages/cli/src/commands/channel/pairing.test.ts @@ -114,6 +114,39 @@ describe('channel pairing CLI (--cwd scoping)', () => { ); }); + it('lists and approves a group request as a group', () => { + const store = new PairingStore('support-bot', wsA); + const code = store.createGroupRequest( + 'group-1', + 'Release Team', + 'user-alice', + 'Alice', + )!; + + pairingListCommand.handler!({ + name: 'support-bot', + cwd: wsA, + _: [], + $0: '', + } as unknown as ListArgs); + expect(stdoutText()).toContain('Group: Release Team (group-1)'); + expect(stdoutText()).toContain('Requested by: Alice (user-alice)'); + + pairingApproveCommand.handler!({ + name: 'support-bot', + code, + cwd: wsA, + _: [], + $0: '', + } as unknown as Parameters< + NonNullable + >[0]); + + expect(store.isGroupApproved('group-1')).toBe(true); + expect(store.isApproved('user-alice')).toBe(false); + expect(stdoutText()).toContain('Approved: group Release Team (group-1)'); + }); + it('approve with a code from another workspace fails with the scoped error', () => { const codeB = new PairingStore('support-bot', wsB).createRequest( 'user-bob', diff --git a/packages/cli/src/commands/channel/pairing.ts b/packages/cli/src/commands/channel/pairing.ts index 3ab0f31e6c9..6dcfca8298b 100644 --- a/packages/cli/src/commands/channel/pairing.ts +++ b/packages/cli/src/commands/channel/pairing.ts @@ -40,9 +40,11 @@ export const pairingListCommand: CommandModule< writeStdoutLine(`Pending pairing requests for "${argv.name}":\n`); for (const req of pending) { const ago = Math.round((Date.now() - req.createdAt) / 60000); - writeStdoutLine( - ` Code: ${req.code} Sender: ${req.senderName} (${req.senderId}) ${ago}m ago`, - ); + const subject = + req.subject.type === 'group' + ? `Group: ${req.subject.name} (${req.subject.id}) Requested by: ${req.senderName} (${req.senderId})` + : `Sender: ${req.subject.name} (${req.subject.id})`; + writeStdoutLine(` Code: ${req.code} ${subject} ${ago}m ago`); } }, }; @@ -78,8 +80,12 @@ export const pairingApproveCommand: CommandModule< return; // process.exit is mocked in tests; never fall through } + const approved = + request.subject.type === 'group' + ? `group ${request.subject.name} (${request.subject.id})` + : `${request.subject.name} (${request.subject.id})`; writeStdoutLine( - `Approved: ${request.senderName} (${request.senderId}) can now use channel "${argv.name}".`, + `Approved: ${approved} can now use channel "${argv.name}".`, ); }, }; diff --git a/packages/cli/src/serve/channel-management-service.test.ts b/packages/cli/src/serve/channel-management-service.test.ts index 1343f31f3c5..92a79b92470 100644 --- a/packages/cli/src/serve/channel-management-service.test.ts +++ b/packages/cli/src/serve/channel-management-service.test.ts @@ -306,7 +306,10 @@ describe('createChannelManagementService', () => { code: 'channel_workspace_mismatch', }); await expect( - service.revokePairingApproval('bot', 'sender-1'), + service.revokePairingApproval('bot', { + type: 'user', + id: 'sender-1', + }), ).rejects.toMatchObject({ code: 'channel_workspace_mismatch', }); @@ -354,16 +357,24 @@ describe('createChannelManagementService', () => { expect(pairing.isApproved('sender-1')).toBe(true); await expect(service.pairingApprovals('bot')).resolves.toEqual({ senderIds: ['sender-1'], + groupIds: [], }); await expect( - service.revokePairingApproval('bot', 'sender-1'), + service.revokePairingApproval('bot', { + type: 'user', + id: 'sender-1', + }), ).resolves.toEqual({ revoked: 'sender-1', senderIds: [], + groupIds: [], }); expect(pairing.isApproved('sender-1')).toBe(false); await expect( - service.revokePairingApproval('bot', 'sender-1'), + service.revokePairingApproval('bot', { + type: 'user', + id: 'sender-1', + }), ).rejects.toMatchObject({ code: 'channel_pairing_approval_not_found', }); @@ -374,6 +385,66 @@ describe('createChannelManagementService', () => { } }); + it('manages group pairing when groupPolicy uses pairing mode', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = await fs.mkdtemp( + path.join(os.tmpdir(), 'channel-management-group-pairing-'), + ); + process.env['QWEN_HOME'] = qwenHome; + try { + const { service } = setup({ + snapshot: settingsSnapshot({ + channels: { + bot: { + type: 'dingtalk', + senderPolicy: 'open', + groupPolicy: 'pairing', + }, + }, + }), + }); + const pairing = new PairingStore('bot', WORKSPACE); + const code = pairing.createGroupRequest( + 'group-1', + 'Release Team', + 'sender-1', + 'Alice', + ); + + await expect(service.pairingRequests('bot')).resolves.toEqual({ + requests: [ + expect.objectContaining({ + senderId: 'sender-1', + subject: { + type: 'group', + id: 'group-1', + name: 'Release Team', + }, + }), + ], + }); + await service.approvePairing('bot', code!); + await expect(service.pairingApprovals('bot')).resolves.toEqual({ + senderIds: [], + groupIds: ['group-1'], + }); + await expect( + service.revokePairingApproval('bot', { + type: 'group', + id: 'group-1', + }), + ).resolves.toEqual({ + revoked: 'group-1', + senderIds: [], + groupIds: [], + }); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + await fs.rm(qwenHome, { recursive: true, force: true }); + } + }); + it('retains the reload diagnostic when stopping the failed replacement also fails', async () => { const { service, manager } = setup({ committedNames: ['bot'] }); manager.reloadWorkspace.mockRejectedValueOnce( @@ -847,7 +918,10 @@ describe('createChannelManagementService', () => { code: 'channel_pairing_not_enabled', }); await expect( - service.revokePairingApproval('bot', 'sender-1'), + service.revokePairingApproval('bot', { + type: 'user', + id: 'sender-1', + }), ).rejects.toMatchObject({ code: 'channel_pairing_not_enabled' }); }); }); diff --git a/packages/cli/src/serve/channel-management-service.ts b/packages/cli/src/serve/channel-management-service.ts index 002ca22078d..75e647ed4d4 100644 --- a/packages/cli/src/serve/channel-management-service.ts +++ b/packages/cli/src/serve/channel-management-service.ts @@ -80,6 +80,12 @@ export interface ChannelPairingApprovalResult export interface ChannelPairingApprovalsSnapshot { senderIds: string[]; + groupIds: string[]; +} + +export interface ChannelPairingApprovalSubject { + type: 'user' | 'group'; + id: string; } export interface ChannelPairingRevocationResult @@ -112,7 +118,7 @@ export interface ChannelManagementService { pairingApprovals(name: string): Promise; revokePairingApproval( name: string, - senderId: string, + subject: ChannelPairingApprovalSubject, ): Promise; } @@ -398,7 +404,10 @@ export function createChannelManagementService( } const config = channels[name]!; assertWorkspaceConfig(config); - if (config['senderPolicy'] !== 'pairing') { + if ( + config['senderPolicy'] !== 'pairing' && + config['groupPolicy'] !== 'pairing' + ) { throw new ChannelManagementError( 'channel_pairing_not_enabled', `Channel "${name}" does not use pairing mode.`, @@ -555,17 +564,29 @@ export function createChannelManagementService( return { approved, requests: store.listPending() }; }, async pairingApprovals(name) { - return { senderIds: pairingStoreFor(name).getAllowlist() }; + const store = pairingStoreFor(name); + return { + senderIds: store.getAllowlist(), + groupIds: store.getGroupAllowlist(), + }; }, - async revokePairingApproval(name, senderId) { + async revokePairingApproval(name, subject) { const store = pairingStoreFor(name); - if (!store.revoke(senderId)) { + const revoked = + subject.type === 'group' + ? store.revokeGroup(subject.id) + : store.revoke(subject.id); + if (!revoked) { throw new ChannelManagementError( 'channel_pairing_approval_not_found', 'Pairing approval was not found.', ); } - return { revoked: senderId, senderIds: store.getAllowlist() }; + return { + revoked: subject.id, + senderIds: store.getAllowlist(), + groupIds: store.getGroupAllowlist(), + }; }, }; return { @@ -583,7 +604,7 @@ export function createChannelManagementService( approvePairing: (name, code) => inMutationLane(() => service.approvePairing(name, code)), pairingApprovals: (name) => service.pairingApprovals(name), - revokePairingApproval: (name, senderId) => - inMutationLane(() => service.revokePairingApproval(name, senderId)), + revokePairingApproval: (name, subject) => + inMutationLane(() => service.revokePairingApproval(name, subject)), }; } diff --git a/packages/cli/src/serve/channel-settings-store.test.ts b/packages/cli/src/serve/channel-settings-store.test.ts index 07ada91ee78..38b6716cce5 100644 --- a/packages/cli/src/serve/channel-settings-store.test.ts +++ b/packages/cli/src/serve/channel-settings-store.test.ts @@ -426,6 +426,7 @@ describe('WorkspaceChannelSettingsStore', () => { retries: 3, mode: 'safe', senderPolicy: 'open', + groupPolicy: 'pairing', allowedUsers: ['user-1'], groupHistoryLimit: 25, blockStreaming: 'on', @@ -446,6 +447,7 @@ describe('WorkspaceChannelSettingsStore', () => { retries: 3, mode: 'safe', senderPolicy: 'open', + groupPolicy: 'pairing', allowedUsers: ['user-1'], groupHistoryLimit: 25, blockStreaming: 'on', diff --git a/packages/cli/src/serve/channel-settings-store.ts b/packages/cli/src/serve/channel-settings-store.ts index d4458fc302b..02ddd836ed8 100644 --- a/packages/cli/src/serve/channel-settings-store.ts +++ b/packages/cli/src/serve/channel-settings-store.ts @@ -142,7 +142,7 @@ function assertSharedField(key: string, value: unknown): boolean { const enumValues: Record> = { senderPolicy: new Set(['allowlist', 'pairing', 'open']), dmPolicy: new Set(['open', 'disabled']), - groupPolicy: new Set(['disabled', 'allowlist', 'open']), + groupPolicy: new Set(['disabled', 'allowlist', 'pairing', 'open']), sessionScope: new Set(['user', 'thread', 'single']), dispatchMode: new Set(['steer', 'followup', 'collect']), blockStreaming: new Set(['on', 'off']), diff --git a/packages/cli/src/serve/routes/workspace-channel-management.test.ts b/packages/cli/src/serve/routes/workspace-channel-management.test.ts index 5af1cca8a9a..90ba8a999ec 100644 --- a/packages/cli/src/serve/routes/workspace-channel-management.test.ts +++ b/packages/cli/src/serve/routes/workspace-channel-management.test.ts @@ -55,15 +55,24 @@ function service(): ChannelManagementService { approved: { senderId: 'sender-1', senderName: 'Alice', + subject: { + type: 'user' as const, + id: 'sender-1', + name: 'Alice', + }, code, createdAt: 1, }, requests: [], })), - pairingApprovals: vi.fn(async () => ({ senderIds: ['sender-1'] })), - revokePairingApproval: vi.fn(async (_name, senderId) => ({ - revoked: senderId, + pairingApprovals: vi.fn(async () => ({ + senderIds: ['sender-1'], + groupIds: ['group-1'], + })), + revokePairingApproval: vi.fn(async (_name, subject) => ({ + revoked: subject.id, senderIds: [], + groupIds: [], })), }; } @@ -301,10 +310,10 @@ describe('workspace Channel management routes', () => { 'ABCDEFGH', ); expect(secondaryService.pairingApprovals).toHaveBeenCalledWith('bot'); - expect(secondaryService.revokePairingApproval).toHaveBeenCalledWith( - 'bot', - 'sender-1', - ); + expect(secondaryService.revokePairingApproval).toHaveBeenCalledWith('bot', { + type: 'user', + id: 'sender-1', + }); expect(primaryService.pairingRequests).not.toHaveBeenCalled(); expect(primaryService.pairingApprovals).not.toHaveBeenCalled(); @@ -315,6 +324,22 @@ describe('workspace Channel management routes', () => { expect(primaryService.pairingRequests).toHaveBeenCalledWith('bot'); }); + it('revokes a group pairing approval by stable group ID', async () => { + const { app, secondaryService } = mount(); + + const response = await auth( + request(app) + .delete('/workspaces/secondary/channels/bot/pairing-approvals') + .send({ groupId: 'group-1' }), + ); + + expect(response.status).toBe(200); + expect(secondaryService.revokePairingApproval).toHaveBeenCalledWith('bot', { + type: 'group', + id: 'group-1', + }); + }); + it('returns 404 when a pairing approval no longer exists', async () => { const { app, primaryService } = mount(); vi.mocked(primaryService.revokePairingApproval).mockRejectedValueOnce( @@ -450,6 +475,16 @@ describe('workspace Channel management routes', () => { .delete('/workspace/channels/bot/pairing-approvals') .send({ senderId: '' }), ); + const invalidGroupId = await auth( + request(app) + .delete('/workspace/channels/bot/pairing-approvals') + .send({ groupId: '' }), + ); + const ambiguousSubject = await auth( + request(app) + .delete('/workspace/channels/bot/pairing-approvals') + .send({ senderId: 'sender-1', groupId: 'group-1' }), + ); expect(invalidName.body.code).toBe('invalid_channel_instance_name'); expect(unsafeName.status).toBe(400); @@ -460,6 +495,8 @@ describe('workspace Channel management routes', () => { expect(invalidSecret.body.code).toBe('channel_settings_invalid_secret'); expect(invalidPairing.body.code).toBe('invalid_channel_pairing_code'); expect(invalidSenderId.body.code).toBe('invalid_channel_pairing_sender_id'); + expect(invalidGroupId.body.code).toBe('invalid_channel_pairing_group_id'); + expect(ambiguousSubject.body.code).toBe('invalid_channel_pairing_subject'); expect(primaryService.upsert).toHaveBeenCalledOnce(); expect(primaryService.start).not.toHaveBeenCalled(); expect(primaryService.approvePairing).not.toHaveBeenCalled(); diff --git a/packages/cli/src/serve/routes/workspace-channel-management.ts b/packages/cli/src/serve/routes/workspace-channel-management.ts index 13e23e61da1..e37a7ecaf06 100644 --- a/packages/cli/src/serve/routes/workspace-channel-management.ts +++ b/packages/cli/src/serve/routes/workspace-channel-management.ts @@ -9,6 +9,7 @@ import { redactLogCredentials } from '@qwen-code/acp-bridge/logRedaction'; import { sanitizeLogText } from '@qwen-code/channel-base'; import { supportedChannelCatalog } from '../../commands/channel/channel-registry.js'; import type { + ChannelPairingApprovalSubject, ChannelManagementService, ChannelStartupRequest, ChannelUpsertRequest, @@ -182,19 +183,44 @@ function parsePairingCode( return code.trim().toUpperCase(); } -function parsePairingSenderId( +function parsePairingApprovalSubject( body: Record, res: Response, -): string | undefined { +): ChannelPairingApprovalSubject | undefined { const senderId = body['senderId']; - if (typeof senderId !== 'string' || senderId.length === 0) { + const groupId = body['groupId']; + if (senderId !== undefined && groupId !== undefined) { + res.status(400).json({ + error: 'Provide exactly one of `senderId` or `groupId`.', + code: 'invalid_channel_pairing_subject', + }); + return undefined; + } + if (senderId !== undefined) { + if (typeof senderId === 'string' && senderId.length > 0) { + return { type: 'user', id: senderId }; + } res.status(400).json({ error: '`senderId` must be a non-empty string.', code: 'invalid_channel_pairing_sender_id', }); return undefined; } - return senderId; + if (groupId !== undefined) { + if (typeof groupId === 'string' && groupId.length > 0) { + return { type: 'group', id: groupId }; + } + res.status(400).json({ + error: '`groupId` must be a non-empty string.', + code: 'invalid_channel_pairing_group_id', + }); + return undefined; + } + res.status(400).json({ + error: 'Provide exactly one of `senderId` or `groupId`.', + code: 'invalid_channel_pairing_subject', + }); + return undefined; } function errorCode(error: unknown): string | undefined { @@ -390,13 +416,13 @@ export function registerWorkspaceChannelManagementRoutes( if (!resolved || !validateClient(req, res, resolved.runtime)) return; const name = parseInstanceName(req, res); if (!name) return; - const senderId = parsePairingSenderId(deps.safeBody(req), res); - if (!senderId) return; + const subject = parsePairingApprovalSubject(deps.safeBody(req), res); + if (!subject) return; try { noStore(res); res .status(200) - .json(await resolved.service.revokePairingApproval(name, senderId)); + .json(await resolved.service.revokePairingApproval(name, subject)); } catch (error) { sendManagementError(res, error); } diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index ce2c2a79d62..51ce82877df 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -367,6 +367,7 @@ export type { DaemonChannelStartupRequest, DaemonChannelMutationResult, DaemonChannelPairingRequest, + DaemonChannelPairingSubject, DaemonChannelPairingRequestsSnapshot, DaemonChannelPairingApprovalRequest, DaemonChannelPairingApprovalResult, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 48c985adaf5..9805f99208f 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -3059,10 +3059,17 @@ export interface DaemonChannelMutationResult { export interface DaemonChannelPairingRequest { senderId: string; senderName: string; + subject?: DaemonChannelPairingSubject; code: string; createdAt: number; } +export interface DaemonChannelPairingSubject { + type: 'user' | 'group'; + id: string; + name: string; +} + export interface DaemonChannelPairingRequestsSnapshot { requests: DaemonChannelPairingRequest[]; } @@ -3078,11 +3085,12 @@ export interface DaemonChannelPairingApprovalResult export interface DaemonChannelPairingApprovalsSnapshot { senderIds: string[]; + groupIds?: string[]; } -export interface DaemonChannelPairingRevocationRequest { - senderId: string; -} +export type DaemonChannelPairingRevocationRequest = + | { senderId: string; groupId?: never } + | { senderId?: never; groupId: string }; export interface DaemonChannelPairingRevocationResult extends DaemonChannelPairingApprovalsSnapshot { diff --git a/packages/sdk-typescript/src/index.ts b/packages/sdk-typescript/src/index.ts index 521eeb0da29..e5aa473a7c3 100644 --- a/packages/sdk-typescript/src/index.ts +++ b/packages/sdk-typescript/src/index.ts @@ -71,6 +71,7 @@ export { type DaemonChannelStartupRequest, type DaemonChannelMutationResult, type DaemonChannelPairingRequest, + type DaemonChannelPairingSubject, type DaemonChannelPairingRequestsSnapshot, type DaemonChannelPairingApprovalRequest, type DaemonChannelPairingApprovalResult, diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index ad21cc2245b..d60ec7eb035 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -7427,5 +7427,18 @@ describe('DaemonClient', () => { senderId: 'sender-1', }); }); + + it('sends a group ID when revoking a group pairing approval', async () => { + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, { revoked: 'group-1', senderIds: [], groupIds: [] }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + + await client.revokeWorkspaceChannelPairingApproval('bot', { + groupId: 'group-1', + }); + + expect(JSON.parse(calls[0]!.body!)).toEqual({ groupId: 'group-1' }); + }); }); }); diff --git a/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts b/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts index 2e110db031f..4fc4e7f56d6 100644 --- a/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts +++ b/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts @@ -27,6 +27,7 @@ import type { DaemonChannelNotifyRequest, DaemonChannelNotifyResult, DaemonChannelPairingApprovalsSnapshot, + DaemonChannelPairingSubject, DaemonChannelPairingRevocationRequest, DaemonChannelPairingRevocationResult, DaemonChannelDeliveryErrorCode, @@ -224,6 +225,7 @@ describe('public SDK entry — typed daemon event surface (#4217)', () => { expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); + expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); From e3da5f0070cdba6f1fb0aa51f2824c6fba174a01 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:46:54 +0800 Subject: [PATCH 02/10] fix(channels): address group pairing review --- packages/channels/base/README.md | 2 +- .../channels/base/src/PairingStore.test.ts | 18 +++ packages/channels/base/src/PairingStore.ts | 2 +- .../channels/ChannelEditorDialog.test.tsx | 38 +++++ .../channels/ChannelEditorDialog.tsx | 13 +- .../channels/ChannelPairingRequests.test.tsx | 101 +++++++++++- .../channels/ChannelPairingRequests.tsx | 147 +++++++++++++----- packages/web-shell/client/i18n.tsx | 30 ++-- .../src/daemon/workspace/actions.test.ts | 8 +- .../webui/src/daemon/workspace/actions.ts | 6 +- packages/webui/src/daemon/workspace/types.ts | 3 +- 11 files changed, 303 insertions(+), 65 deletions(-) diff --git a/packages/channels/base/README.md b/packages/channels/base/README.md index e19d92107b3..151fd36c09d 100644 --- a/packages/channels/base/README.md +++ b/packages/channels/base/README.md @@ -302,7 +302,7 @@ When `requireMention` is `true` (default), group messages are only processed if constructor(channelName: string, workspaceCwd?: string) ``` -Persists pending pairing state to `{channelName}-pairing.json`, user approvals to `{channelName}-allowlist.json`, and group approvals to `{channelName}-group-allowlist.json`. With `workspaceCwd` (what `ChannelBase` passes — the channel's `cwd`), the files live under the workspace-scoped directory `~/.qwen/channels//` so two workspaces reusing the same channel name never share pairing requests or allowlist entries. Without it, the legacy global `~/.qwen/channels/` layout is used. The first time a given (workspace, channel) pair is constructed, existing legacy global files are copied in once (grandfathering) so already-approved senders stay approved; a per-channel `.migrated` sentinel in the scope directory marks that decision, after which legacy files are never consulted again for that channel. Channel names are URI-encoded in file names, so a name containing path separators cannot escape the scope directory. `revoke(senderId)` removes the sender only from this store's allowlist and never mutates the legacy global baseline. +Persists pending pairing state to `{channelName}-pairing.json`, user approvals to `{channelName}-allowlist.json`, and group approvals to `{channelName}-groups.json`. With `workspaceCwd` (what `ChannelBase` passes — the channel's `cwd`), the files live under the workspace-scoped directory `~/.qwen/channels//` so two workspaces reusing the same channel name never share pairing requests or allowlist entries. Without it, the legacy global `~/.qwen/channels/` layout is used. The first time a given (workspace, channel) pair is constructed, existing legacy global files are copied in once (grandfathering) so already-approved senders stay approved; a per-channel `.migrated` sentinel in the scope directory marks that decision, after which legacy files are never consulted again for that channel. Channel names are URI-encoded in file names, so a name containing path separators cannot escape the scope directory. `revoke(senderId)` removes the sender only from this store's allowlist and never mutates the legacy global baseline. | Method | Description | | ------------------------------------- | --------------------------------------------------------------------------------------------------------- | diff --git a/packages/channels/base/src/PairingStore.test.ts b/packages/channels/base/src/PairingStore.test.ts index d69bf2024d4..ab452c6be47 100644 --- a/packages/channels/base/src/PairingStore.test.ts +++ b/packages/channels/base/src/PairingStore.test.ts @@ -102,6 +102,24 @@ describe('PairingStore workspace scoping (#7017)', () => { expect(storeB.isGroupApproved('group-1')).toBe(true); }); + it('keeps group approvals separate from similarly named channel user approvals', () => { + const groupStore = new PairingStore('support', workspaceA); + const userStore = new PairingStore('support-group', workspaceA); + const code = groupStore.createGroupRequest( + 'shared-id', + 'Release Team', + 'sender-1', + 'Alice', + ); + + groupStore.approve(code!); + + expect(groupStore.isGroupApproved('shared-id')).toBe(true); + expect(userStore.isApproved('shared-id')).toBe(false); + expect(userStore.revoke('shared-id')).toBe(false); + expect(groupStore.isGroupApproved('shared-id')).toBe(true); + }); + it('revokes an approved sender only from the selected workspace', () => { const storeA = new PairingStore('support-bot', workspaceA); const storeB = new PairingStore('support-bot', workspaceB); diff --git a/packages/channels/base/src/PairingStore.ts b/packages/channels/base/src/PairingStore.ts index db97deeb366..9bbf718ea21 100644 --- a/packages/channels/base/src/PairingStore.ts +++ b/packages/channels/base/src/PairingStore.ts @@ -64,7 +64,7 @@ export class PairingStore { ); this.groupAllowlistPath = path.join( this.dir, - `${safeChannelName}-group-allowlist.json`, + `${safeChannelName}-groups.json`, ); this.migratedSentinelPath = path.join( this.dir, diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx index 07054c667c2..41205a6aaf0 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx @@ -43,6 +43,23 @@ const OPTIONAL_SECRET: DaemonChannelTypeDescriptor = { ), }; +const DINGTALK_WITH_GROUP_POLICY: DaemonChannelTypeDescriptor = { + ...DINGTALK, + fields: [ + ...DINGTALK.fields, + { + key: 'groupPolicy', + label: 'Group Policy', + kind: 'enum', + required: true, + options: [ + { value: 'pairing', label: 'Pairing' }, + { value: 'open', label: 'Open' }, + ], + }, + ], +}; + const INSTANCE: DaemonChannelInstanceSnapshot = { name: 'release-bot', config: { @@ -266,4 +283,25 @@ describe('ChannelEditorDialog', () => { expect(document.body.textContent).toContain('Pairing approvals'); expect(document.body.textContent).not.toContain('Configured allowlist'); }); + + it('shows pairing management when only group pairing is enabled', async () => { + const groupPairingInstance: DaemonChannelInstanceSnapshot = { + ...INSTANCE, + config: { + ...INSTANCE.config, + senderPolicy: 'open', + groupPolicy: 'pairing', + }, + }; + const listPairingRequests = vi.fn().mockResolvedValue({ requests: [] }); + + await renderDialog({ + descriptor: DINGTALK_WITH_GROUP_POLICY, + instance: groupPairingInstance, + listPairingRequests, + }); + + expect(document.body.textContent).toContain('Pairing approvals'); + expect(listPairingRequests).toHaveBeenCalledWith('release-bot'); + }); }); diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx index 75326c5dfdf..99d7f7ef549 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx @@ -18,6 +18,7 @@ import type { DaemonChannelPairingApprovalResult, DaemonChannelPairingApprovalsSnapshot, DaemonChannelPairingRequestsSnapshot, + DaemonChannelPairingRevocationRequest, DaemonChannelPairingRevocationResult, DaemonChannelTypeDescriptor, DaemonChannelUpsertRequest, @@ -115,7 +116,7 @@ export interface ChannelEditorDialogProps { ) => Promise; revokePairingApproval: ( name: string, - senderId: string, + request: DaemonChannelPairingRevocationRequest, ) => Promise; } @@ -609,7 +610,12 @@ export function ChannelEditorDialog({ ? String(draft.values['senderPolicy'] ?? '') : draft.senderPolicy; const showRadioGroup = !descriptorPolicy; - const showPairing = effectivePolicy === 'pairing'; + const effectiveGroupPolicy = String( + draft.values['groupPolicy'] ?? '', + ); + const showPairing = + effectivePolicy === 'pairing' || + effectiveGroupPolicy === 'pairing'; if (!showRadioGroup && !showPairing) return null; return (
@@ -660,7 +666,8 @@ export function ChannelEditorDialog({ ) : null} {showPairing ? ( - instance?.config.senderPolicy === 'pairing' ? ( + instance?.config.senderPolicy === 'pairing' || + instance?.config.groupPolicy === 'pairing' ? ( Promise; revokeApproval?: ( name: string, - senderId: string, + request: DaemonChannelPairingRevocationRequest, ) => Promise; staticAllowedUsers?: readonly string[]; language?: 'en' | 'zh-CN'; @@ -113,6 +130,42 @@ describe('ChannelPairingRequests', () => { ); }); + it('identifies the group and requesting member for a group pairing request', async () => { + const approval: DaemonChannelPairingApprovalResult = { + approved: GROUP_PENDING.requests[0], + requests: [], + }; + const approve = vi.fn().mockResolvedValue(approval); + const listApprovals = vi + .fn() + .mockResolvedValueOnce({ senderIds: [], groupIds: [] }) + .mockResolvedValueOnce({ senderIds: [], groupIds: ['group-7'] }); + await renderRequests({ + list: vi.fn().mockResolvedValue(GROUP_PENDING), + approve, + listApprovals, + }); + + expect(container.textContent).toContain('Group: Release Team'); + expect(container.textContent).toContain('group-7'); + expect(container.textContent).toContain('Requested by Ada'); + const button = container.querySelector( + 'button[aria-label="Approve Group: Release Team, code GROUP123"]', + ); + + await act(async () => { + button?.click(); + }); + + expect(approve).toHaveBeenCalledWith('release-bot', 'GROUP123'); + expect(container.textContent).toContain( + 'Group: Release Team can now use this Channel.', + ); + expect( + container.querySelector('button[aria-label="Revoke Group: group-7"]'), + ).not.toBeNull(); + }); + it('shows pairing approvals and distinguishes configured allowlist access', async () => { await renderRequests({ listApprovals: vi.fn().mockResolvedValue({ @@ -186,7 +239,9 @@ describe('ChannelPairingRequests', () => { confirm?.click(); }); - expect(revokeApproval).toHaveBeenCalledWith('release-bot', 'paired-user'); + expect(revokeApproval).toHaveBeenCalledWith('release-bot', { + senderId: 'paired-user', + }); expect( container.querySelector('button[aria-label="Revoke paired-user"]'), ).toBeNull(); @@ -196,6 +251,48 @@ describe('ChannelPairingRequests', () => { ); }); + it('lists and revokes a group pairing approval by group ID', async () => { + const revokeApproval = vi.fn().mockResolvedValue({ + revoked: 'group-7', + senderIds: ['paired-user'], + groupIds: [], + }); + await renderRequests({ + listApprovals: vi.fn().mockResolvedValue({ + senderIds: ['paired-user'], + groupIds: ['group-7'], + }), + revokeApproval, + }); + + const revoke = container.querySelector( + 'button[aria-label="Revoke Group: group-7"]', + ); + await act(async () => { + revoke?.click(); + }); + + expect(document.body.textContent).toContain( + 'Revoke pairing approval for Group: group-7?', + ); + const confirm = Array.from(document.body.querySelectorAll('button')).find( + (item) => item.textContent?.trim() === 'Revoke approval', + ); + await act(async () => { + confirm?.click(); + }); + + expect(revokeApproval).toHaveBeenCalledWith('release-bot', { + groupId: 'group-7', + }); + expect( + container.querySelector('button[aria-label="Revoke Group: group-7"]'), + ).toBeNull(); + expect(container.textContent).toContain( + 'Pairing approval for Group: group-7 was revoked.', + ); + }); + it('does not approve a request while a revoke is in flight', async () => { const revokeApproval = vi .fn() diff --git a/packages/web-shell/client/components/channels/ChannelPairingRequests.tsx b/packages/web-shell/client/components/channels/ChannelPairingRequests.tsx index ef37d2ec98c..251effa6dd0 100644 --- a/packages/web-shell/client/components/channels/ChannelPairingRequests.tsx +++ b/packages/web-shell/client/components/channels/ChannelPairingRequests.tsx @@ -19,6 +19,7 @@ import type { DaemonChannelPairingApprovalsSnapshot, DaemonChannelPairingRequest, DaemonChannelPairingRequestsSnapshot, + DaemonChannelPairingRevocationRequest, DaemonChannelPairingRevocationResult, } from '@qwen-code/sdk/daemon'; import { useI18n } from '../../i18n'; @@ -51,15 +52,33 @@ export interface ChannelPairingRequestsProps { ) => Promise; revokeApproval: ( name: string, - senderId: string, + request: DaemonChannelPairingRevocationRequest, ) => Promise; staticAllowedUsers?: readonly string[]; } +type PairingApprovalTarget = + | { type: 'user'; id: string } + | { type: 'group'; id: string }; + function senderLabel(request: DaemonChannelPairingRequest): string { return request.senderName.trim() || request.senderId; } +function requestSubject(request: DaemonChannelPairingRequest) { + return ( + request.subject ?? { + type: 'user' as const, + id: request.senderId, + name: request.senderName, + } + ); +} + +function targetKey(target: PairingApprovalTarget): string { + return `${target.type}:${target.id}`; +} + function errorCode(error: unknown): string | undefined { if (!error || typeof error !== 'object') return undefined; const body = (error as { body?: unknown }).body; @@ -101,12 +120,23 @@ export function ChannelPairingRequests({ const [error, setError] = useState(); const [success, setSuccess] = useState(); const [approvedSenderIds, setApprovedSenderIds] = useState([]); + const [approvedGroupIds, setApprovedGroupIds] = useState([]); const [approvalsLoading, setApprovalsLoading] = useState(true); const [approvalsReloadToken, setApprovalsReloadToken] = useState(0); const [approvalsError, setApprovalsError] = useState(); const [revokeSuccess, setRevokeSuccess] = useState(); - const [revokeTarget, setRevokeTarget] = useState(); - const [revokingSenderId, setRevokingSenderId] = useState(); + const [revokeTarget, setRevokeTarget] = useState(); + const [revokingTarget, setRevokingTarget] = useState(); + + const approvalLabel = (target: PairingApprovalTarget) => + target.type === 'group' + ? t('channels.editor.pairing.subject.group', { name: target.id }) + : target.id; + + const approvedTargets: PairingApprovalTarget[] = [ + ...approvedSenderIds.map((id) => ({ type: 'user' as const, id })), + ...approvedGroupIds.map((id) => ({ type: 'group' as const, id })), + ]; useEffect(() => { mounted.current = true; @@ -148,14 +178,16 @@ export function ChannelPairingRequests({ let active = true; setApprovalsLoading(true); setApprovedSenderIds([]); + setApprovedGroupIds([]); setApprovalsError(undefined); setRevokeSuccess(undefined); setRevokeTarget(undefined); - setRevokingSenderId(undefined); + setRevokingTarget(undefined); void listApprovals(channelName).then( (snapshot) => { if (!active) return; setApprovedSenderIds(snapshot.senderIds); + setApprovedGroupIds(snapshot.groupIds ?? []); setApprovalsLoading(false); }, (loadError: unknown) => { @@ -187,9 +219,15 @@ export function ChannelPairingRequests({ return; } setRequests(result.requests); + const subject = requestSubject(result.approved); setSuccess( t('channels.editor.pairing.approved', { - sender: senderLabel(result.approved), + sender: + subject.type === 'group' + ? t('channels.editor.pairing.subject.group', { + name: subject.name.trim() || subject.id, + }) + : subject.name.trim() || subject.id, }), ); setApprovalsReloadToken((current) => current + 1); @@ -237,22 +275,30 @@ export function ChannelPairingRequests({ } }; - const revoke = async (senderId: string) => { - if (revokingSenderId) return; + const revoke = async (target: PairingApprovalTarget) => { + if (revokingTarget) return; const revokeChannel = channelName; + const key = targetKey(target); + const label = approvalLabel(target); setRevokeTarget(undefined); - setRevokingSenderId(senderId); + setRevokingTarget(key); setApprovalsError(undefined); setRevokeSuccess(undefined); setSuccess(undefined); try { - const result = await revokeApproval(channelName, senderId); + const result = await revokeApproval( + channelName, + target.type === 'group' + ? { groupId: target.id } + : { senderId: target.id }, + ); if (!mounted.current || currentChannelName.current !== revokeChannel) { return; } setApprovedSenderIds(result.senderIds); + setApprovedGroupIds(result.groupIds ?? []); setRevokeSuccess( - t('channels.editor.pairing.approvals.revoked', { senderId }), + t('channels.editor.pairing.approvals.revoked', { senderId: label }), ); } catch (revokeError) { if (!mounted.current || currentChannelName.current !== revokeChannel) { @@ -263,6 +309,7 @@ export function ChannelPairingRequests({ const snapshot = await listApprovals(revokeChannel); if (mounted.current && currentChannelName.current === revokeChannel) { setApprovedSenderIds(snapshot.senderIds); + setApprovedGroupIds(snapshot.groupIds ?? []); } return; } catch (refreshError) { @@ -288,7 +335,7 @@ export function ChannelPairingRequests({ ); } finally { if (mounted.current && currentChannelName.current === revokeChannel) { - setRevokingSenderId(undefined); + setRevokingTarget(undefined); } } }; @@ -364,13 +411,26 @@ export function ChannelPairingRequests({ {requests.length > 0 ? (
    {requests.map((request) => { - const label = senderLabel(request); + const subject = requestSubject(request); + const label = + subject.type === 'group' + ? t('channels.editor.pairing.subject.group', { + name: subject.name.trim() || subject.id, + }) + : subject.name.trim() || subject.id; return (
  • {label} - {label !== request.senderId ? ( - {request.senderId} + {label !== subject.id ? ( + {subject.id} + ) : null} + {subject.type === 'group' ? ( + + {t('channels.editor.pairing.requestedBy', { + sender: senderLabel(request), + })} + ) : null} {formatRelativeTime( @@ -383,7 +443,7 @@ export function ChannelPairingRequests({
    - {approvedSenderIds.length} + {approvedTargets.length} -
    • - ))} + {approvedTargets.map((target) => { + const label = approvalLabel(target); + const key = targetKey(target); + return ( +
  • +
    +
    + +
    • + ); + })}
) : null} @@ -524,7 +589,7 @@ export function ChannelPairingRequests({ {t('channels.editor.pairing.approvals.confirm.title', { - senderId: revokeTarget ?? '', + senderId: revokeTarget ? approvalLabel(revokeTarget) : '', })} diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 825dfaf179c..cfa644a839d 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -2516,7 +2516,11 @@ const EN: Messages = { 'People receive a pairing code and can chat after you approve them.', 'channels.editor.pairing.title': 'Pending requests', 'channels.editor.pairing.description': - 'Match the code shared by the person before approving access. Approvals take effect immediately; Save and Cancel do not undo them.', + 'Match the shared code before approving access. Approvals take effect immediately; Save and Cancel do not undo them.', + 'channels.editor.pairing.subject.group': (v) => + `Group: ${v?.name ?? 'Unknown'}`, + 'channels.editor.pairing.requestedBy': (v) => + `Requested by ${v?.sender ?? 'Unknown'}`, 'channels.editor.pairing.refresh': 'Refresh pairing requests', 'channels.editor.pairing.approve': 'Approve', 'channels.editor.pairing.approveFor': (v) => @@ -2535,7 +2539,7 @@ const EN: Messages = { 'Pending requests will appear here after this Channel is saved in pairing mode.', 'channels.editor.pairing.approvals.title': 'Pairing approvals', 'channels.editor.pairing.approvals.description': - 'Sender IDs approved through pairing for this Channel.', + 'User and group IDs approved through pairing for this Channel.', 'channels.editor.pairing.approvals.refresh': 'Refresh pairing approvals', 'channels.editor.pairing.approvals.revoke': 'Revoke', 'channels.editor.pairing.approvals.revokeFor': (v) => @@ -2548,7 +2552,7 @@ const EN: Messages = { 'Pairing approvals are temporarily unavailable. Try again.', 'channels.editor.pairing.approvals.empty.title': 'No pairing approvals', 'channels.editor.pairing.approvals.empty.description': - 'Approved sender IDs will appear here.', + 'Approved user and group IDs will appear here.', 'channels.editor.pairing.approvals.confirm.title': (v) => `Revoke pairing approval for ${v?.senderId || 'this sender'}?`, 'channels.editor.pairing.approvals.confirm.description': @@ -5034,7 +5038,11 @@ const ZH: Messages = { '用户会收到配对码，经您批准后才能开始对话。', 'channels.editor.pairing.title': '待处理的配对请求', 'channels.editor.pairing.description': - '批准前，请核对用户提供的配对码是否一致。批准会立即生效，保存或取消都不会撤销已批准的访问。', + '批准前，请核对收到的配对码是否一致。批准会立即生效，保存或取消都不会撤销已批准的访问。', + 'channels.editor.pairing.subject.group': (v) => + `群聊：${v?.name ?? '未知群聊'}`, + 'channels.editor.pairing.requestedBy': (v) => + `由 ${v?.sender ?? '未知用户'} 发起`, 'channels.editor.pairing.refresh': '刷新配对请求', 'channels.editor.pairing.approve': '批准', 'channels.editor.pairing.approveFor': (v) => @@ -5050,21 +5058,21 @@ const ZH: Messages = { 'channels.editor.pairing.saveFirst.title': '请先保存配对模式', 'channels.editor.pairing.saveFirst.description': '频道以配对模式保存后，待处理请求会显示在这里。', - 'channels.editor.pairing.approvals.title': '已配对用户', + 'channels.editor.pairing.approvals.title': '已配对对象', 'channels.editor.pairing.approvals.description': - '通过配对获得此频道访问权限的用户 ID。', - 'channels.editor.pairing.approvals.refresh': '刷新已配对用户', + '通过配对获得此频道访问权限的用户和群聊 ID。', + 'channels.editor.pairing.approvals.refresh': '刷新已配对对象', 'channels.editor.pairing.approvals.revoke': '撤销', 'channels.editor.pairing.approvals.revokeFor': (v) => `撤销 ${v?.senderId ?? '该用户'} 的配对授权`, 'channels.editor.pairing.approvals.revoked': (v) => `已撤销 ${v?.senderId ?? '该用户'} 的配对授权。`, - 'channels.editor.pairing.approvals.error': '未能更新已配对用户', + 'channels.editor.pairing.approvals.error': '未能更新已配对对象', 'channels.editor.pairing.approvals.unavailable': - '暂时无法获取已配对用户，请重试。', - 'channels.editor.pairing.approvals.empty.title': '暂无已配对用户', + '暂时无法获取已配对对象，请重试。', + 'channels.editor.pairing.approvals.empty.title': '暂无已配对对象', 'channels.editor.pairing.approvals.empty.description': - '批准配对请求后，用户 ID 会显示在这里。', + '批准配对请求后，用户或群聊 ID 会显示在这里。', 'channels.editor.pairing.approvals.confirm.title': (v) => `撤销 ${v?.senderId || '该用户'} 的配对授权？`, 'channels.editor.pairing.approvals.confirm.description': diff --git a/packages/webui/src/daemon/workspace/actions.test.ts b/packages/webui/src/daemon/workspace/actions.test.ts index 0cd6498be80..3691eb0a81b 100644 --- a/packages/webui/src/daemon/workspace/actions.test.ts +++ b/packages/webui/src/daemon/workspace/actions.test.ts @@ -373,7 +373,10 @@ describe('workspace actions', () => { pairingApprovals, ); await expect( - actions.channelPairing.revoke('bot', 'sender-1'), + actions.channelPairing.revoke('bot', { senderId: 'sender-1' }), + ).resolves.toBe(pairingRevocation); + await expect( + actions.channelPairing.revoke('bot', { groupId: 'group-1' }), ).resolves.toBe(pairingRevocation); expect(workspaceByCwd).toHaveBeenNthCalledWith(1, '/workspace-a'); @@ -402,6 +405,9 @@ describe('workspace actions', () => { expect( workspace.revokeWorkspaceChannelPairingApproval, ).toHaveBeenCalledWith('bot', { senderId: 'sender-1' }); + expect( + workspace.revokeWorkspaceChannelPairingApproval, + ).toHaveBeenCalledWith('bot', { groupId: 'group-1' }); }); it('rejects Channel management without a selected workspace', async () => { diff --git a/packages/webui/src/daemon/workspace/actions.ts b/packages/webui/src/daemon/workspace/actions.ts index 0d6d9e84edc..48b6943c598 100644 --- a/packages/webui/src/daemon/workspace/actions.ts +++ b/packages/webui/src/daemon/workspace/actions.ts @@ -258,15 +258,13 @@ export function createDaemonWorkspaceActions({ ); }, - async revoke(name, senderId) { + async revoke(name, request) { const workspace = requireWorkspaceClient( getClient, getWorkspaceCwd, 'Revoke channel pairing approval failed', ); - return workspace.revokeWorkspaceChannelPairingApproval(name, { - senderId, - }); + return workspace.revokeWorkspaceChannelPairingApproval(name, request); }, }, diff --git a/packages/webui/src/daemon/workspace/types.ts b/packages/webui/src/daemon/workspace/types.ts index 2825c20adcb..a33182a0147 100644 --- a/packages/webui/src/daemon/workspace/types.ts +++ b/packages/webui/src/daemon/workspace/types.ts @@ -17,6 +17,7 @@ import type { DaemonChannelPairingApprovalResult, DaemonChannelPairingApprovalsSnapshot, DaemonChannelPairingRequestsSnapshot, + DaemonChannelPairingRevocationRequest, DaemonChannelPairingRevocationResult, DaemonChannelsSnapshot, DaemonChannelStartupRequest, @@ -206,7 +207,7 @@ export interface DaemonChannelPairingActions { approvals(name: string): Promise; revoke( name: string, - senderId: string, + request: DaemonChannelPairingRevocationRequest, ): Promise; } From 842031c52e634b0df3536e6059bc599ac553d67b Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:04:34 +0800 Subject: [PATCH 03/10] fix(web-shell): show group pairing management --- .../channels/ChannelEditorDialog.test.tsx | 18 ------------------ .../channels/ChannelEditorDialog.tsx | 7 +++++-- 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx index 41205a6aaf0..9cee4897b13 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx @@ -43,23 +43,6 @@ const OPTIONAL_SECRET: DaemonChannelTypeDescriptor = { ), }; -const DINGTALK_WITH_GROUP_POLICY: DaemonChannelTypeDescriptor = { - ...DINGTALK, - fields: [ - ...DINGTALK.fields, - { - key: 'groupPolicy', - label: 'Group Policy', - kind: 'enum', - required: true, - options: [ - { value: 'pairing', label: 'Pairing' }, - { value: 'open', label: 'Open' }, - ], - }, - ], -}; - const INSTANCE: DaemonChannelInstanceSnapshot = { name: 'release-bot', config: { @@ -296,7 +279,6 @@ describe('ChannelEditorDialog', () => { const listPairingRequests = vi.fn().mockResolvedValue({ requests: [] }); await renderDialog({ - descriptor: DINGTALK_WITH_GROUP_POLICY, instance: groupPairingInstance, listPairingRequests, }); diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx index 99d7f7ef549..7939eaad2d3 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.tsx @@ -610,9 +610,12 @@ export function ChannelEditorDialog({ ? String(draft.values['senderPolicy'] ?? '') : draft.senderPolicy; const showRadioGroup = !descriptorPolicy; - const effectiveGroupPolicy = String( - draft.values['groupPolicy'] ?? '', + const descriptorGroupPolicy = descriptor.fields.some( + (field) => field.key === 'groupPolicy', ); + const effectiveGroupPolicy = descriptorGroupPolicy + ? String(draft.values['groupPolicy'] ?? '') + : String(instance?.config.groupPolicy ?? ''); const showPairing = effectivePolicy === 'pairing' || effectiveGroupPolicy === 'pairing'; From 8af2dccef5ed9645d81f73d02fb236e9d90ba792 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:11:42 +0800 Subject: [PATCH 04/10] fix(channels): recheck group pairing before history backfill --- .../channels/base/src/ChannelBase.test.ts | 65 +++++++++++++++++++ packages/channels/base/src/ChannelBase.ts | 12 +++- packages/channels/base/src/GroupGate.ts | 4 ++ 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index 32282af2ca7..f7391e2ce3f 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -2716,6 +2716,71 @@ describe('ChannelBase', () => { } }); + it('does not backfill paired-group history after the group is revoked', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const store = new PairingStore('test-chan', '/tmp'); + const code = store.createGroupRequest( + 'chat1', + 'Release Team', + 'alice', + 'Alice', + ); + store.approve(code!); + const recoveryState: { current?: Promise } = {}; + const ch = createChannel( + { + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + groupHistoryLimit: 10, + groups: { '*': { requireMention: true } }, + }, + { + bridgeRecovery: () => recoveryState.current, + groupHistoryPath: groupHistoryPath(), + }, + ); + + await ch.handleInbound( + envelope({ + isGroup: true, + senderId: 'bob', + senderName: 'Bob', + text: 'background', + }), + ); + + let releaseRecovery!: () => void; + recoveryState.current = new Promise((resolve) => { + releaseRecovery = resolve; + }); + const current = ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + senderId: 'carol', + senderName: 'Carol', + text: '@bot summarize', + }), + ); + store.revokeGroup('chat1'); + releaseRecovery(); + await current; + + const prompt = (bridge.prompt as ReturnType).mock + .calls[0][1] as string; + expect(prompt).not.toContain('- [Bob] background'); + expect(prompt).toContain('[Carol] @bot summarize'); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + it('persists group history across channel instances', async () => { const historyPath = groupHistoryPath(); const config = { diff --git a/packages/channels/base/src/ChannelBase.ts b/packages/channels/base/src/ChannelBase.ts index d0ef49e0246..18ac653c7c9 100644 --- a/packages/channels/base/src/ChannelBase.ts +++ b/packages/channels/base/src/ChannelBase.ts @@ -4751,7 +4751,17 @@ export abstract class ChannelBase { return []; } try { - return this.groupHistory.drain(this.groupHistoryKey(envelope), limit); + const entries = this.groupHistory.drain( + this.groupHistoryKey(envelope), + limit, + ); + if ( + this.config.groupPolicy === 'pairing' && + !this.groupGate.isGroupApproved(envelope.chatId) + ) { + return []; + } + return entries; } catch (err) { process.stderr.write( `[${this.name}] failed to drain group history for chat ${sanitizeLogText(envelope.chatId, 64)}: ${err instanceof Error ? err.message : err}\n`, diff --git a/packages/channels/base/src/GroupGate.ts b/packages/channels/base/src/GroupGate.ts index 5e8e14a40e3..2dde1bd8d0d 100644 --- a/packages/channels/base/src/GroupGate.ts +++ b/packages/channels/base/src/GroupGate.ts @@ -90,4 +90,8 @@ export class GroupGate { return { allowed: true }; } + + isGroupApproved(groupId: string): boolean { + return this.pairingStore?.isGroupApproved(groupId) ?? false; + } } From 7ae9f150f44a93456a2e728981d0275274ad312f Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Wed, 5 Aug 2026 02:07:07 +0800 Subject: [PATCH 05/10] test(channels): verify group approval isolation --- packages/channels/base/src/PairingStore.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/channels/base/src/PairingStore.test.ts b/packages/channels/base/src/PairingStore.test.ts index ab452c6be47..143d377784e 100644 --- a/packages/channels/base/src/PairingStore.test.ts +++ b/packages/channels/base/src/PairingStore.test.ts @@ -80,6 +80,8 @@ describe('PairingStore workspace scoping (#7017)', () => { store.approve(code!); expect(store.isGroupApproved('group-1')).toBe(true); + expect(store.isApproved('group-1')).toBe(false); + expect(store.revoke('group-1')).toBe(false); expect(store.isApproved('sender-1')).toBe(false); }); From 5c49687e29a42a85ad459a7545f09a1470a23e1b Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 07:35:52 +0000 Subject: [PATCH 06/10] fix(channels): address group pairing review findings and pin behaviors (#8440) - Grandfather the group allowlist file in PairingStore legacy migration - Offer the pairing groupPolicy option in github/gitlab descriptors - Re-export DaemonChannelPairingSubject from the webui barrels - Refresh the GroupGate doc comment and channel docs rows - Pin the unpinned group pairing behaviors called out in review: subject dedup, trigger matrix, notification content/cap/failure/ thread routing, DM negative space under groupPolicy pairing, stored DM loop authz, pairing-enabled guard negative space, approval/revocation HTTP bodies, descriptor-driven gate branch, and the web-shell group approval mirrors - Add a compile-time assertion for the revocation request union --- docs/users/features/channels/overview.md | 48 ++-- packages/channels/base/README.md | 6 +- .../channels/base/src/ChannelBase.test.ts | 261 +++++++++++++++++- packages/channels/base/src/GroupGate.test.ts | 65 ++++- packages/channels/base/src/GroupGate.ts | 13 +- .../channels/base/src/PairingStore.test.ts | 39 +++ packages/channels/base/src/PairingStore.ts | 4 + packages/channels/github/src/index.ts | 1 + packages/channels/gitlab/src/index.ts | 1 + .../serve/channel-management-service.test.ts | 88 ++++-- .../workspace-channel-management.test.ts | 30 ++ .../test/unit/DaemonClient.test.ts | 16 +- .../channels/ChannelEditorDialog.test.tsx | 53 ++++ .../channels/ChannelPairingRequests.test.tsx | 71 ++++- packages/webui/src/daemon-react-sdk.ts | 1 + packages/webui/src/daemon/index.ts | 1 + 16 files changed, 636 insertions(+), 62 deletions(-) diff --git a/docs/users/features/channels/overview.md b/docs/users/features/channels/overview.md index ef64f640415..780acd4d306 100644 --- a/docs/users/features/channels/overview.md +++ b/docs/users/features/channels/overview.md @@ -50,30 +50,30 @@ Channels are configured under the `channels` key in `settings.json`. Each channe ### Options -| Option | Required | Description | -| ------------------------ | ---------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `type` | Yes | Channel type: `telegram`, `weixin`, `qq`, `dingtalk`, `wecom`, `feishu`, `github`, or a custom type from an extension (see [Plugins](./plugins)) | -| `token` | Telegram | Bot token. Supports `$ENV_VAR` syntax to read from environment variables. Not needed for WeChat, DingTalk, WeCom, or Feishu | -| `clientId` | DingTalk, Feishu | DingTalk AppKey or Feishu App ID. Supports `$ENV_VAR` syntax | -| `clientSecret` | DingTalk, Feishu | DingTalk AppSecret or Feishu App Secret. Supports `$ENV_VAR` syntax | -| `botId` | WeCom | WeCom intelligent robot Bot ID. Supports `$ENV_VAR` syntax. See [WeCom](./wecom) | -| `secret` | WeCom | WeCom intelligent robot Secret. Supports `$ENV_VAR` syntax. See [WeCom](./wecom) | -| `model` | No | Model to use for this channel (e.g., `qwen3.5-plus`). Overrides the default model. Useful for multimodal models that support image input | -| `senderPolicy` | No | Who can talk to the bot: `allowlist` (default), `open`, or `pairing` | -| `allowedUsers` | No | List of user IDs allowed to use the bot (used by `allowlist` and `pairing` policies) | -| `sessionScope` | No | How sessions are scoped: `user` (default), `thread`, or `single` | -| `cwd` | No | Working directory for the agent. Defaults to the current directory | -| `approvalMode` | No | Tool approval mode for channel sessions. Unattended webhook tasks require `yolo`; the setting applies to every session on the channel | -| `instructions` | No | Custom instructions prepended to the first message of each session | -| `webhooks` | No | Webhook sources and delivery targets for daemon-managed channels. See [Webhook-triggered tasks](#webhook-triggered-tasks) | -| `groupPolicy` | No | Group chat access: `disabled` (default), `allowlist`, `pairing`, or `open`. See [Group Chats](#group-chats) | -| `dmPolicy` | No | Private/DM access: `open` (default) or `disabled` (silently drop all DMs). Useful for group-only bots | -| `groupHistoryLimit` | No | Opt-in group history backfill. `0` or omitted disables it. A positive number persists that many authorized, unmentioned group messages for the next bot mention/reply. | -| `groups` | No | Per-group settings. Keys are group chat IDs or `"*"` for defaults. See [Group Chats](#group-chats) | -| `dispatchMode` | No | What happens when you send a message while the bot is busy: `steer` (default), `collect`, or `followup`. See [Dispatch Modes](#dispatch-modes) | -| `blockStreaming` | No | Progressive response delivery: `on` or `off` (default). See [Block Streaming](#block-streaming) | -| `blockStreamingChunk` | No | Chunk size bounds: `{ "minChars": 400, "maxChars": 1000 }`. See [Block Streaming](#block-streaming) | -| `blockStreamingCoalesce` | No | Idle flush: `{ "idleMs": 1500 }`. See [Block Streaming](#block-streaming) | +| Option | Required | Description | +| ------------------------ | ---------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `type` | Yes | Channel type: `telegram`, `weixin`, `qq`, `dingtalk`, `wecom`, `feishu`, `github`, or a custom type from an extension (see [Plugins](./plugins)) | +| `token` | Telegram | Bot token. Supports `$ENV_VAR` syntax to read from environment variables. Not needed for WeChat, DingTalk, WeCom, or Feishu | +| `clientId` | DingTalk, Feishu | DingTalk AppKey or Feishu App ID. Supports `$ENV_VAR` syntax | +| `clientSecret` | DingTalk, Feishu | DingTalk AppSecret or Feishu App Secret. Supports `$ENV_VAR` syntax | +| `botId` | WeCom | WeCom intelligent robot Bot ID. Supports `$ENV_VAR` syntax. See [WeCom](./wecom) | +| `secret` | WeCom | WeCom intelligent robot Secret. Supports `$ENV_VAR` syntax. See [WeCom](./wecom) | +| `model` | No | Model to use for this channel (e.g., `qwen3.5-plus`). Overrides the default model. Useful for multimodal models that support image input | +| `senderPolicy` | No | Who can talk to the bot: `allowlist` (default), `open`, or `pairing` | +| `allowedUsers` | No | List of user IDs allowed to use the bot (used by `allowlist` and `pairing` policies) | +| `sessionScope` | No | How sessions are scoped: `user` (default), `thread`, or `single` | +| `cwd` | No | Working directory for the agent. Defaults to the current directory | +| `approvalMode` | No | Tool approval mode for channel sessions. Unattended webhook tasks require `yolo`; the setting applies to every session on the channel | +| `instructions` | No | Custom instructions prepended to the first message of each session | +| `webhooks` | No | Webhook sources and delivery targets for daemon-managed channels. See [Webhook-triggered tasks](#webhook-triggered-tasks) | +| `groupPolicy` | No | Group chat access: `disabled` (default), `allowlist`, `pairing`, or `open`. See [Group Chats](#group-chats) | +| `dmPolicy` | No | Private/DM access: `open` (default) or `disabled` (silently drop all DMs). Useful for group-only bots | +| `groupHistoryLimit` | No | Opt-in group history backfill. `0` or omitted disables it. A positive number persists that many unmentioned group messages from authorized senders or members of approved paired groups for the next bot mention/reply. | +| `groups` | No | Per-group settings. Keys are group chat IDs or `"*"` for defaults. See [Group Chats](#group-chats) | +| `dispatchMode` | No | What happens when you send a message while the bot is busy: `steer` (default), `collect`, or `followup`. See [Dispatch Modes](#dispatch-modes) | +| `blockStreaming` | No | Progressive response delivery: `on` or `off` (default). See [Block Streaming](#block-streaming) | +| `blockStreamingChunk` | No | Chunk size bounds: `{ "minChars": 400, "maxChars": 1000 }`. See [Block Streaming](#block-streaming) | +| `blockStreamingCoalesce` | No | Idle flush: `{ "idleMs": 1500 }`. See [Block Streaming](#block-streaming) | ### Sender Policy diff --git a/packages/channels/base/README.md b/packages/channels/base/README.md index 151fd36c09d..dfd33e89f17 100644 --- a/packages/channels/base/README.md +++ b/packages/channels/base/README.md @@ -281,9 +281,9 @@ constructor(policy: SenderPolicy, allowedUsers?: string[], pairingStore?: Pairin constructor(policy?: GroupPolicy, groups?: Record, pairingStore?: PairingStore) ``` -| Method | Description | -| ----------------- | -------------------------------------------------------------- | -| `check(envelope)` | Returns the group policy decision and an optional pairing code | +| Method | Description | +| --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `check(envelope, options?)` | Returns the group policy decision and an optional pairing code. Under `pairing`, a mention/reply from an unapproved group creates (or reuses) a pending pairing request; pass `{ createPairingRequest: false }` for read-only probes | **Policy behavior:** diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index f7391e2ce3f..c436a518166 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { EventEmitter } from 'node:events'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { existsSync, mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { @@ -2884,6 +2884,66 @@ describe('ChannelBase', () => { expect(prompt).toBe('[User 1] @bot current'); }); + it('does not record ambient messages from unapproved pairing groups', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + const historyPath = groupHistoryPath(); + try { + const ch = createChannel( + { + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + groupHistoryLimit: 10, + groups: { '*': { requireMention: true } }, + }, + { groupHistoryPath: historyPath }, + ); + + await ch.handleInbound( + envelope({ + isGroup: true, + chatId: 'group-1', + senderId: 'bob', + senderName: 'Bob', + text: 'pre-approval chatter', + }), + ); + + expect(existsSync(historyPath)).toBe(false); + + const store = new PairingStore('test-chan', '/tmp'); + const code = store.createGroupRequest( + 'group-1', + 'Release Team', + 'alice', + 'Alice', + ); + store.approve(code!); + + await ch.handleInbound( + envelope({ + isGroup: true, + chatId: 'group-1', + isMentioned: true, + senderId: 'carol', + senderName: 'Carol', + text: '@bot summarize', + }), + ); + + const prompt = (bridge.prompt as ReturnType).mock + .calls[0][1] as string; + expect(prompt).not.toContain('pre-approval chatter'); + expect(prompt).toContain('[Carol] @bot summarize'); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + it('uses group-level groupHistoryLimit over channel-level limit', async () => { const ch = createChannel( { @@ -13498,6 +13558,10 @@ describe('ChannelBase', () => { id: 'group-1', name: 'Release Team', }); + expect(request?.senderId).toBe('alice'); + expect(request?.senderName).toBe('Alice'); + expect(ch.sent[0]!.text).toContain(request!.code); + expect(ch.sent[0]!.text).toContain('pairing approve'); store.approve(request!.code); await ch.handleInbound({ @@ -13534,6 +13598,155 @@ describe('ChannelBase', () => { rmSync(qwenHome, { recursive: true, force: true }); } }); + + it('tells a group when the pending pairing cap is reached', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const store = new PairingStore('test-chan', '/tmp'); + for (let index = 1; index <= 3; index++) { + store.createGroupRequest( + `group-${index}`, + `Group ${index}`, + 'alice', + 'Alice', + ); + } + const ch = createChannel({ groupPolicy: 'pairing' }); + + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + chatId: 'group-4', + senderId: 'bob', + senderName: 'Bob', + }), + ); + + expect(ch.sent).toHaveLength(1); + expect(ch.sent[0]!.text).toContain('Too many pending pairing requests'); + expect(bridge.prompt).not.toHaveBeenCalled(); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + + it('treats group pairing notification failures as preflight rejection', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + class GroupPairingFailureChannel extends TestChannel { + override async sendMessage(): Promise { + throw new Error('send failed'); + } + } + const ch = new GroupPairingFailureChannel( + 'test-chan', + defaultConfig({ groupPolicy: 'pairing' }), + bridge, + ); + + await expect( + ch.handleInbound( + envelope({ isGroup: true, isMentioned: true, chatId: 'group-1' }), + ), + ).resolves.toBeUndefined(); + + expect(bridge.prompt).not.toHaveBeenCalled(); + expect(stderr).toHaveBeenCalledWith( + expect.stringContaining('group pairing notification failed'), + ); + } finally { + stderr.mockRestore(); + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + + it('passes threadId through to group pairing notifications', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ groupPolicy: 'pairing' }); + const threadMessages: Array<{ + chatId: string; + threadId?: string; + text: string; + }> = []; + vi.spyOn(ch as never, 'sendThreadMessage').mockImplementation( + async ( + chatId: string, + threadId: string | undefined, + text: string, + ) => { + threadMessages.push({ chatId, threadId, text }); + }, + ); + + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + chatId: 'group-1', + threadId: 'issue:42', + }), + ); + + expect(threadMessages).toHaveLength(1); + expect(threadMessages[0]!.threadId).toBe('issue:42'); + expect(threadMessages[0]!.text).toContain('requires approval'); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + + it('still gates DMs by senderPolicy when groupPolicy uses pairing', async () => { + const ch = createChannel({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }); + + await ch.handleInbound(envelope({ senderId: 'stranger' })); + + expect(bridge.prompt).not.toHaveBeenCalled(); + expect(ch.sent).toEqual([]); + }); + + it('keeps DM pairing on the sender flow when groupPolicy uses pairing', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ + groupPolicy: 'pairing', + senderPolicy: 'pairing', + allowedUsers: [], + }); + + await ch.handleInbound(envelope({ senderId: 'stranger' })); + + expect(ch.sent).toHaveLength(1); + expect(ch.sent[0]!.text).toContain('pairing code'); + expect(bridge.prompt).not.toHaveBeenCalled(); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); }); describe('setBridge', () => { @@ -18594,6 +18807,52 @@ describe('ChannelBase', () => { } }); + it('rejects a stored DM job for an unlisted sender when groupPolicy uses pairing', async () => { + const disable = vi.fn().mockResolvedValue(true); + const ch = createChannel( + { + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }, + { + loopController: { + create: vi.fn(), + listForTarget: vi.fn(), + disable, + validateCron: vi.fn(), + }, + }, + ); + ch.proactiveSupported = true; + + await expect( + ch.runLoopPrompt({ + id: 'job-1', + channelName: 'test-chan', + target: { + channelName: 'test-chan', + senderId: 'bob', + chatId: 'chat1', + isGroup: false, + }, + cwd: '/tmp', + cron: '0 9 * * *', + prompt: 'post summary', + label: 'daily summary', + recurring: true, + enabled: true, + createdBy: 'Bob', + createdAt: '2026-06-30T01:00:00.000Z', + consecutiveFailures: 0, + runCount: 0, + }), + ).rejects.toThrow('no longer authorized'); + + expect(disable).toHaveBeenCalledWith('job-1'); + expect(bridge.prompt).not.toHaveBeenCalled(); + }); + it('rejects stored threaded jobs unless the adapter supports the target', async () => { const ch = createChannel(); ch.proactiveSupported = true; diff --git a/packages/channels/base/src/GroupGate.test.ts b/packages/channels/base/src/GroupGate.test.ts index fcaf60eb8fd..51af88da80e 100644 --- a/packages/channels/base/src/GroupGate.test.ts +++ b/packages/channels/base/src/GroupGate.test.ts @@ -1,7 +1,24 @@ import { describe, it, expect } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { GroupGate } from './GroupGate.js'; +import { PairingStore } from './PairingStore.js'; import type { Envelope } from './types.js'; +function withQwenHome(fn: () => T): T { + const previous = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-gate-')); + process.env['QWEN_HOME'] = qwenHome; + try { + return fn(); + } finally { + if (previous === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previous; + rmSync(qwenHome, { recursive: true, force: true }); + } +} + function envelope(overrides: Partial = {}): Envelope { return { channelName: 'test', @@ -19,7 +36,12 @@ function envelope(overrides: Partial = {}): Envelope { describe('GroupGate', () => { describe('non-group messages', () => { it('always allows DM messages regardless of policy', () => { - for (const policy of ['disabled', 'allowlist', 'open'] as const) { + for (const policy of [ + 'disabled', + 'allowlist', + 'open', + 'pairing', + ] as const) { const gate = new GroupGate(policy); expect(gate.check(envelope()).allowed).toBe(true); } @@ -124,6 +146,47 @@ describe('GroupGate', () => { pairingCode: null, }); }); + + it('returns a null pairing code when constructed without a store', () => { + const gate = new GroupGate('pairing'); + const result = gate.check(envelope({ isGroup: true, isMentioned: true })); + expect(result.pairingCode).toBeNull(); + }); + + it('requests pairing after a reply to the bot', () => { + withQwenHome(() => { + const store = new PairingStore('test-chan', '/tmp'); + const gate = new GroupGate('pairing', {}, store); + + const result = gate.check( + envelope({ isGroup: true, isReplyToBot: true }), + ); + + expect(result.allowed).toBe(false); + expect(result.reason).toBe('pairing_required'); + expect(result.pairingCode).toBeTypeOf('string'); + expect(store.listPending()).toHaveLength(1); + }); + }); + + it('does not create requests for ambient messages even with requireMention=false', () => { + withQwenHome(() => { + const store = new PairingStore('test-chan', '/tmp'); + const gate = new GroupGate( + 'pairing', + { '*': { requireMention: false } }, + store, + ); + + const result = gate.check(envelope({ isGroup: true })); + + expect(result).toEqual({ + allowed: false, + reason: 'pairing_trigger_required', + }); + expect(store.listPending()).toEqual([]); + }); + }); }); describe('defaults', () => { diff --git a/packages/channels/base/src/GroupGate.ts b/packages/channels/base/src/GroupGate.ts index 2dde1bd8d0d..ce343ed372e 100644 --- a/packages/channels/base/src/GroupGate.ts +++ b/packages/channels/base/src/GroupGate.ts @@ -28,14 +28,19 @@ export class GroupGate { } /** - * Full group check: policy + allowlist + mention gating. + * Full group check: policy + allowlist + pairing + mention gating. * Evaluation order: * 1. groupPolicy (disabled → drop) * 2. group allowlist (allowlist mode, no match → drop) - * 3. mention gating (requireMention + not mentioned → drop silently) + * 3. group pairing (pairing mode, group not approved → drop; an explicit + * mention or reply creates or returns a pending pairing request unless + * `options.createPairingRequest` is false) + * 4. mention gating (requireMention + not mentioned → drop silently) * - * Mention gating runs before sender gate so that unmentioned messages - * in groups don't trigger pairing flows. + * Under the pairing policy the pairing step itself drops ambient + * (unmentioned, non-reply) messages before any request is created. Mention + * gating then runs before the sender gate so unmentioned messages in + * approved groups don't trigger sender pairing flows. */ check( envelope: Envelope, diff --git a/packages/channels/base/src/PairingStore.test.ts b/packages/channels/base/src/PairingStore.test.ts index 143d377784e..5fa46c9c119 100644 --- a/packages/channels/base/src/PairingStore.test.ts +++ b/packages/channels/base/src/PairingStore.test.ts @@ -85,6 +85,34 @@ describe('PairingStore workspace scoping (#7017)', () => { expect(store.isApproved('sender-1')).toBe(false); }); + it('reuses one pending request per group regardless of the initiating sender', () => { + const store = new PairingStore('support-bot', workspaceA); + + const first = store.createGroupRequest( + 'group-1', + 'Release Team', + 'alice', + 'Alice', + ); + const second = store.createGroupRequest( + 'group-1', + 'Release Team', + 'bob', + 'Bob', + ); + + expect(first).toBeTruthy(); + expect(second).toBe(first); + expect(store.listPending()).toEqual([ + expect.objectContaining({ + senderId: 'alice', + senderName: 'Alice', + subject: { type: 'group', id: 'group-1', name: 'Release Team' }, + code: first, + }), + ]); + }); + it('isolates group approvals and revocation by workspace', () => { const storeA = new PairingStore('support-bot', workspaceA); const storeB = new PairingStore('support-bot', workspaceB); @@ -212,6 +240,17 @@ describe('PairingStore workspace scoping (#7017)', () => { ); }; + it('copies the legacy group allowlist into the scoped store', () => { + fs.mkdirSync(channelsRoot(), { recursive: true }); + fs.writeFileSync( + path.join(channelsRoot(), 'support-bot-groups.json'), + JSON.stringify(['legacy-group']), + ); + const store = new PairingStore('support-bot', workspaceA); + + expect(store.isGroupApproved('legacy-group')).toBe(true); + }); + it('copies legacy global state into the scoped store once', () => { seedLegacy(); const store = new PairingStore('support-bot', workspaceA); diff --git a/packages/channels/base/src/PairingStore.ts b/packages/channels/base/src/PairingStore.ts index 9bbf718ea21..775385e931b 100644 --- a/packages/channels/base/src/PairingStore.ts +++ b/packages/channels/base/src/PairingStore.ts @@ -118,6 +118,10 @@ export class PairingStore { path.join(channelsRoot, `${channelName}-allowlist.json`), this.allowlistPath, ], + [ + path.join(channelsRoot, `${channelName}-groups.json`), + this.groupAllowlistPath, + ], ]; this.ensureDir(); let allSucceeded = true; diff --git a/packages/channels/github/src/index.ts b/packages/channels/github/src/index.ts index 9e613000dd2..d7137ff03e2 100644 --- a/packages/channels/github/src/index.ts +++ b/packages/channels/github/src/index.ts @@ -37,6 +37,7 @@ export const plugin: ChannelPlugin = { options: [ { value: 'open', label: 'Open' }, { value: 'allowlist', label: 'Allowlist' }, + { value: 'pairing', label: 'Pairing' }, { value: 'disabled', label: 'Disabled' }, ], }, diff --git a/packages/channels/gitlab/src/index.ts b/packages/channels/gitlab/src/index.ts index fe40f1f4820..c82065309a2 100644 --- a/packages/channels/gitlab/src/index.ts +++ b/packages/channels/gitlab/src/index.ts @@ -37,6 +37,7 @@ export const plugin: ChannelPlugin = { options: [ { value: 'open', label: 'Open' }, { value: 'allowlist', label: 'Allowlist' }, + { value: 'pairing', label: 'Pairing' }, { value: 'disabled', label: 'Disabled' }, ], }, diff --git a/packages/cli/src/serve/channel-management-service.test.ts b/packages/cli/src/serve/channel-management-service.test.ts index 92a79b92470..c72476d36da 100644 --- a/packages/cli/src/serve/channel-management-service.test.ts +++ b/packages/cli/src/serve/channel-management-service.test.ts @@ -410,6 +410,12 @@ describe('createChannelManagementService', () => { 'sender-1', 'Alice', ); + const secondCode = pairing.createGroupRequest( + 'group-2', + 'Platform Team', + 'sender-2', + 'Bob', + ); await expect(service.pairingRequests('bot')).resolves.toEqual({ requests: [ @@ -421,12 +427,30 @@ describe('createChannelManagementService', () => { name: 'Release Team', }, }), + expect.objectContaining({ + senderId: 'sender-2', + subject: { + type: 'group', + id: 'group-2', + name: 'Platform Team', + }, + }), + ], + }); + await expect(service.approvePairing('bot', code!)).resolves.toEqual({ + approved: expect.objectContaining({ + subject: { type: 'group', id: 'group-1', name: 'Release Team' }, + }), + requests: [ + expect.objectContaining({ + subject: { type: 'group', id: 'group-2', name: 'Platform Team' }, + }), ], }); - await service.approvePairing('bot', code!); + await service.approvePairing('bot', secondCode!); await expect(service.pairingApprovals('bot')).resolves.toEqual({ senderIds: [], - groupIds: ['group-1'], + groupIds: ['group-1', 'group-2'], }); await expect( service.revokePairingApproval('bot', { @@ -436,7 +460,15 @@ describe('createChannelManagementService', () => { ).resolves.toEqual({ revoked: 'group-1', senderIds: [], - groupIds: [], + groupIds: ['group-2'], + }); + await expect( + service.revokePairingApproval('bot', { + type: 'group', + id: 'group-1', + }), + ).rejects.toMatchObject({ + code: 'channel_pairing_approval_not_found', }); } finally { if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; @@ -900,28 +932,34 @@ describe('createChannelManagementService', () => { }); it('rejects pairing operations on a channel without pairing mode', async () => { - const { service } = setup({ - snapshot: settingsSnapshot({ - channels: { - bot: { type: 'dingtalk', senderPolicy: 'open' }, - }, - }), - }); + for (const config of [ + { type: 'dingtalk', senderPolicy: 'open' }, + { type: 'dingtalk', senderPolicy: 'open', groupPolicy: 'allowlist' }, + { type: 'dingtalk', senderPolicy: 'open', groupPolicy: 'disabled' }, + ]) { + const { service } = setup({ + snapshot: settingsSnapshot({ + channels: { + bot: config, + }, + }), + }); - await expect(service.pairingRequests('bot')).rejects.toMatchObject({ - code: 'channel_pairing_not_enabled', - }); - await expect( - service.approvePairing('bot', 'ABCDEFGH'), - ).rejects.toMatchObject({ code: 'channel_pairing_not_enabled' }); - await expect(service.pairingApprovals('bot')).rejects.toMatchObject({ - code: 'channel_pairing_not_enabled', - }); - await expect( - service.revokePairingApproval('bot', { - type: 'user', - id: 'sender-1', - }), - ).rejects.toMatchObject({ code: 'channel_pairing_not_enabled' }); + await expect(service.pairingRequests('bot')).rejects.toMatchObject({ + code: 'channel_pairing_not_enabled', + }); + await expect( + service.approvePairing('bot', 'ABCDEFGH'), + ).rejects.toMatchObject({ code: 'channel_pairing_not_enabled' }); + await expect(service.pairingApprovals('bot')).rejects.toMatchObject({ + code: 'channel_pairing_not_enabled', + }); + await expect( + service.revokePairingApproval('bot', { + type: 'user', + id: 'sender-1', + }), + ).rejects.toMatchObject({ code: 'channel_pairing_not_enabled' }); + } }); }); diff --git a/packages/cli/src/serve/routes/workspace-channel-management.test.ts b/packages/cli/src/serve/routes/workspace-channel-management.test.ts index 90ba8a999ec..beb8141168e 100644 --- a/packages/cli/src/serve/routes/workspace-channel-management.test.ts +++ b/packages/cli/src/serve/routes/workspace-channel-management.test.ts @@ -298,12 +298,32 @@ describe('workspace Channel management routes', () => { expect(pairingRequests.status).toBe(200); expect(pairingRequests.headers['cache-control']).toBe('no-store'); + expect(pairingRequests.body).toEqual({ requests: [] }); expect(approval.status).toBe(200); expect(approval.headers['cache-control']).toBe('no-store'); + expect(approval.body).toEqual({ + approved: { + senderId: 'sender-1', + senderName: 'Alice', + subject: { type: 'user', id: 'sender-1', name: 'Alice' }, + code: 'ABCDEFGH', + createdAt: 1, + }, + requests: [], + }); expect(approvals.status).toBe(200); expect(approvals.headers['cache-control']).toBe('no-store'); + expect(approvals.body).toEqual({ + senderIds: ['sender-1'], + groupIds: ['group-1'], + }); expect(revocation.status).toBe(200); expect(revocation.headers['cache-control']).toBe('no-store'); + expect(revocation.body).toEqual({ + revoked: 'sender-1', + senderIds: [], + groupIds: [], + }); expect(secondaryService.pairingRequests).toHaveBeenCalledWith('bot'); expect(secondaryService.approvePairing).toHaveBeenCalledWith( 'bot', @@ -334,6 +354,11 @@ describe('workspace Channel management routes', () => { ); expect(response.status).toBe(200); + expect(response.body).toEqual({ + revoked: 'group-1', + senderIds: [], + groupIds: [], + }); expect(secondaryService.revokePairingApproval).toHaveBeenCalledWith('bot', { type: 'group', id: 'group-1', @@ -485,6 +510,9 @@ describe('workspace Channel management routes', () => { .delete('/workspace/channels/bot/pairing-approvals') .send({ senderId: 'sender-1', groupId: 'group-1' }), ); + const missingSubject = await auth( + request(app).delete('/workspace/channels/bot/pairing-approvals').send({}), + ); expect(invalidName.body.code).toBe('invalid_channel_instance_name'); expect(unsafeName.status).toBe(400); @@ -497,6 +525,8 @@ describe('workspace Channel management routes', () => { expect(invalidSenderId.body.code).toBe('invalid_channel_pairing_sender_id'); expect(invalidGroupId.body.code).toBe('invalid_channel_pairing_group_id'); expect(ambiguousSubject.body.code).toBe('invalid_channel_pairing_subject'); + expect(missingSubject.status).toBe(400); + expect(missingSubject.body.code).toBe('invalid_channel_pairing_subject'); expect(primaryService.upsert).toHaveBeenCalledOnce(); expect(primaryService.start).not.toHaveBeenCalled(); expect(primaryService.approvePairing).not.toHaveBeenCalled(); diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index d60ec7eb035..916f2543ec0 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, afterEach } from 'vitest'; +import { describe, it, expect, expectTypeOf, vi, afterEach } from 'vitest'; import { DaemonClient, DaemonHttpError, @@ -22,6 +22,7 @@ import { } from '../../src/daemon/types.js'; import type { DaemonCapabilities, + DaemonChannelPairingRevocationRequest, DaemonSessionContextStatus, DaemonSessionLspStatus, DaemonSessionOrganizationResult, @@ -7440,5 +7441,18 @@ describe('DaemonClient', () => { expect(JSON.parse(calls[0]!.body!)).toEqual({ groupId: 'group-1' }); }); + + it('pins the revocation request union at the type level', () => { + expectTypeOf<{ + senderId: string; + }>().toMatchTypeOf(); + expectTypeOf<{ + groupId: string; + }>().toMatchTypeOf(); + expectTypeOf<{ + senderId: string; + groupId: string; + }>().not.toMatchTypeOf(); + }); }); }); diff --git a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx index 9cee4897b13..fa5c5a75e3e 100644 --- a/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelEditorDialog.test.tsx @@ -43,6 +43,27 @@ const OPTIONAL_SECRET: DaemonChannelTypeDescriptor = { ), }; +const GROUP_POLICY_DESCRIPTOR: DaemonChannelTypeDescriptor = { + type: 'github', + displayName: 'GitHub', + manageable: true, + fields: [ + ...DINGTALK.fields, + { + key: 'groupPolicy', + label: 'Group Policy', + kind: 'enum', + required: false, + options: [ + { value: 'open', label: 'Open' }, + { value: 'allowlist', label: 'Allowlist' }, + { value: 'pairing', label: 'Pairing' }, + { value: 'disabled', label: 'Disabled' }, + ], + }, + ], +}; + const INSTANCE: DaemonChannelInstanceSnapshot = { name: 'release-bot', config: { @@ -267,6 +288,38 @@ describe('ChannelEditorDialog', () => { expect(document.body.textContent).not.toContain('Configured allowlist'); }); + it('shows pairing affordance from a descriptor-driven groupPolicy draft', async () => { + const groupPolicyInstance: DaemonChannelInstanceSnapshot = { + ...INSTANCE, + config: { + ...INSTANCE.config, + senderPolicy: 'open', + groupPolicy: 'open', + }, + }; + await renderDialog({ + descriptor: GROUP_POLICY_DESCRIPTOR, + instance: groupPolicyInstance, + }); + + expect(document.body.textContent).not.toContain('Save pairing mode first'); + + const trigger = inputByLabel('Group Policy'); + expect(trigger).not.toBeNull(); + await act(async () => { + trigger!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); + const option = Array.from( + document.querySelectorAll('[role="option"]'), + ).find((item) => item.textContent?.trim() === 'Pairing'); + expect(option).toBeDefined(); + await act(async () => { + option!.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); + + expect(document.body.textContent).toContain('Save pairing mode first'); + }); + it('shows pairing management when only group pairing is enabled', async () => { const groupPairingInstance: DaemonChannelInstanceSnapshot = { ...INSTANCE, diff --git a/packages/web-shell/client/components/channels/ChannelPairingRequests.test.tsx b/packages/web-shell/client/components/channels/ChannelPairingRequests.test.tsx index ed34f854c7a..ed9c7b88ad4 100644 --- a/packages/web-shell/client/components/channels/ChannelPairingRequests.test.tsx +++ b/packages/web-shell/client/components/channels/ChannelPairingRequests.test.tsx @@ -161,6 +161,9 @@ describe('ChannelPairingRequests', () => { expect(container.textContent).toContain( 'Group: Release Team can now use this Channel.', ); + expect(container.textContent).toContain('No pending requests'); + expect(container.textContent).not.toContain('GROUP123'); + expect(container.textContent).not.toContain('No pairing approvals'); expect( container.querySelector('button[aria-label="Revoke Group: group-7"]'), ).not.toBeNull(); @@ -255,12 +258,12 @@ describe('ChannelPairingRequests', () => { const revokeApproval = vi.fn().mockResolvedValue({ revoked: 'group-7', senderIds: ['paired-user'], - groupIds: [], + groupIds: ['group-8'], }); await renderRequests({ listApprovals: vi.fn().mockResolvedValue({ senderIds: ['paired-user'], - groupIds: ['group-7'], + groupIds: ['group-7', 'group-8'], }), revokeApproval, }); @@ -288,6 +291,12 @@ describe('ChannelPairingRequests', () => { expect( container.querySelector('button[aria-label="Revoke Group: group-7"]'), ).toBeNull(); + expect( + container.querySelector('button[aria-label="Revoke Group: group-8"]'), + ).not.toBeNull(); + expect( + container.querySelector('button[aria-label="Revoke paired-user"]'), + ).not.toBeNull(); expect(container.textContent).toContain( 'Pairing approval for Group: group-7 was revoked.', ); @@ -471,6 +480,55 @@ describe('ChannelPairingRequests', () => { ).toBeNull(); }); + it('refreshes group approvals when a group revoke target is already gone', async () => { + const listApprovals = vi + .fn() + .mockResolvedValueOnce({ + senderIds: ['paired-user'], + groupIds: ['group-7', 'group-8'], + }) + .mockResolvedValueOnce({ + senderIds: ['paired-user'], + groupIds: ['group-8'], + }); + const revokeError = Object.assign(new Error('Approval is gone.'), { + status: 404, + body: { + error: 'Pairing approval was not found.', + code: 'channel_pairing_approval_not_found', + }, + }); + const revokeApproval = vi.fn().mockRejectedValue(revokeError); + await renderRequests({ listApprovals, revokeApproval }); + + const revoke = container.querySelector( + 'button[aria-label="Revoke Group: group-7"]', + ); + await act(async () => { + revoke?.click(); + }); + const confirm = Array.from(document.body.querySelectorAll('button')).find( + (item) => item.textContent?.trim() === 'Revoke approval', + ); + await act(async () => { + confirm?.click(); + }); + + expect(listApprovals).toHaveBeenCalledTimes(2); + expect(container.textContent).not.toContain( + 'Pairing approval was not found.', + ); + expect( + container.querySelector('button[aria-label="Revoke Group: group-7"]'), + ).toBeNull(); + expect( + container.querySelector('button[aria-label="Revoke Group: group-8"]'), + ).not.toBeNull(); + expect( + container.querySelector('button[aria-label="Revoke paired-user"]'), + ).not.toBeNull(); + }); + it('shows an error when refreshing after a missing approval fails', async () => { const refreshError = Object.assign(new Error('Refresh failed.'), { status: 503, @@ -742,15 +800,22 @@ describe('ChannelPairingRequests', () => { it('does not show approvals from the previous Channel while loading', async () => { const listApprovals = vi .fn() - .mockResolvedValueOnce({ senderIds: ['paired-user'] }) + .mockResolvedValueOnce({ + senderIds: ['paired-user'], + groupIds: ['group-7'], + }) .mockReturnValueOnce( new Promise(() => undefined), ); await renderRequests({ listApprovals }); expect(container.textContent).toContain('paired-user'); + expect( + container.querySelector('button[aria-label="Revoke Group: group-7"]'), + ).not.toBeNull(); await renderRequests({ channelName: 'other-bot', listApprovals }); expect(container.textContent).not.toContain('paired-user'); + expect(container.textContent).not.toContain('group-7'); }); }); diff --git a/packages/webui/src/daemon-react-sdk.ts b/packages/webui/src/daemon-react-sdk.ts index d06f85ebbc2..d047d56497f 100644 --- a/packages/webui/src/daemon-react-sdk.ts +++ b/packages/webui/src/daemon-react-sdk.ts @@ -384,6 +384,7 @@ export type { DaemonChannelStartupRequest, DaemonChannelMutationResult, DaemonChannelPairingRequest, + DaemonChannelPairingSubject, DaemonChannelPairingRequestsSnapshot, DaemonChannelPairingApprovalRequest, DaemonChannelPairingApprovalResult, diff --git a/packages/webui/src/daemon/index.ts b/packages/webui/src/daemon/index.ts index a2be807d2a5..2e6041ced4a 100644 --- a/packages/webui/src/daemon/index.ts +++ b/packages/webui/src/daemon/index.ts @@ -194,6 +194,7 @@ export type { DaemonChannelStartupRequest, DaemonChannelMutationResult, DaemonChannelPairingRequest, + DaemonChannelPairingSubject, DaemonChannelPairingRequestsSnapshot, DaemonChannelPairingApprovalRequest, DaemonChannelPairingApprovalResult, From 9f7eb5cca6d01fd08fe70b73ecd7fdd73b9c9ad0 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 09:02:26 +0000 Subject: [PATCH 07/10] fix(channels): address group pairing review findings (#8440) - Accept 'pairing' in the GitLab connect warning, descriptor help text, and gitlab.md: todos dispatch after one-time group approval. - Model group approvals in the web-shell e2e mock daemon (approve by subject type, GET returns senderIds+groupIds, DELETE accepts groupId) and exercise the group pairing flow in the channels spec. - Add 'pairing' to the groupPolicy enumerations in the plugins and per-channel docs (telegram, feishu, dingtalk, qqbot, wecom). - Update the channel pairing CLI help to cover group requests. - Cap pending pairing requests at one per sender so a single member cannot occupy every shared pending slot. --- docs/users/features/channels/dingtalk.md | 6 +- docs/users/features/channels/feishu.md | 6 +- docs/users/features/channels/gitlab.md | 22 +++---- docs/users/features/channels/overview.md | 2 +- docs/users/features/channels/plugins.md | 2 +- docs/users/features/channels/qqbot.md | 6 +- docs/users/features/channels/telegram.md | 6 +- docs/users/features/channels/wecom.md | 2 +- packages/channels/base/README.md | 24 +++---- .../channels/base/src/ChannelBase.test.ts | 4 +- .../channels/base/src/PairingStore.test.ts | 46 ++++++++++++++ packages/channels/base/src/PairingStore.ts | 13 +++- .../channels/gitlab/src/GitlabAdapter.test.ts | 38 +++++++++++ packages/channels/gitlab/src/GitlabAdapter.ts | 8 ++- packages/channels/gitlab/src/index.ts | 3 +- packages/cli/src/commands/channel.ts | 2 +- .../web-shell/client/e2e/utils/mockDaemon.ts | 63 ++++++++++++++----- .../client/e2e/web-shell.channels.spec.ts | 29 +++++++++ 18 files changed, 226 insertions(+), 56 deletions(-) diff --git a/docs/users/features/channels/dingtalk.md b/docs/users/features/channels/dingtalk.md index 33afdd2b5e5..2933aad2607 100644 --- a/docs/users/features/channels/dingtalk.md +++ b/docs/users/features/channels/dingtalk.md @@ -115,9 +115,10 @@ Every target must set `isGroup` explicitly. For a direct message, `chatId` is th DingTalk bots work in both DM and group conversations. To enable group support: -1. Set `groupPolicy` to `"allowlist"` or `"open"` in your channel config +1. Set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"` in your channel config 2. Add the bot to a DingTalk group 3. @mention the bot in the group to trigger a response +4. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start By default, the bot requires an @mention in group chats (`requireMention: true`). Set `"requireMention": false` for a specific group to make it respond to all messages. See [Group Chats](./overview#group-chats) for full details. @@ -161,7 +162,8 @@ You can send photos and documents to the bot, not just text. ### Bot doesn't respond in groups -- Check that `groupPolicy` is set to `"allowlist"` or `"open"` (default is `"disabled"`) +- Check that `groupPolicy` is set to `"allowlist"`, `"pairing"`, or `"open"` (default is `"disabled"`) +- If using `"pairing"`, verify the group's pairing request has been approved - Make sure you @mention the bot in the group message - Verify the bot has been added to the group diff --git a/docs/users/features/channels/feishu.md b/docs/users/features/channels/feishu.md index 660d3b0e312..0ed38160e18 100644 --- a/docs/users/features/channels/feishu.md +++ b/docs/users/features/channels/feishu.md @@ -121,9 +121,10 @@ Then set the request URL in Feishu Open Platform to `http://:9321`. Feishu bots work in both DM and group conversations. To enable group support: -1. Set `groupPolicy` to `"allowlist"` or `"open"` in your channel config +1. Set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"` in your channel config 2. Add the bot to a Feishu group 3. @mention the bot in the group to trigger a response +4. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start By default, the bot requires an @mention in group chats (`requireMention: true`). Set `"requireMention": false` for a specific group to make it respond to all messages. @@ -170,7 +171,8 @@ Multiple users can send messages simultaneously in the same group chat. Each mes ### Bot doesn't respond in groups -- Check that `groupPolicy` is set to `"allowlist"` or `"open"` (default is `"disabled"`) +- Check that `groupPolicy` is set to `"allowlist"`, `"pairing"`, or `"open"` (default is `"disabled"`) +- If using `"pairing"`, verify the group's pairing request has been approved - Make sure you @mention the bot in the group message - Verify the bot has been added to the group diff --git a/docs/users/features/channels/gitlab.md b/docs/users/features/channels/gitlab.md index 6e4d9d57716..7b4729421eb 100644 --- a/docs/users/features/channels/gitlab.md +++ b/docs/users/features/channels/gitlab.md @@ -56,14 +56,14 @@ For self-hosted instances, set `baseUrl`: ## Configuration Options -| Option | Default | Description | -| ------------------------ | ------------------------- | ---------------------------------------------------------- | -| `token` | (required) | PAT with `read_api` + `api` scopes | -| `pollInterval` | `60000` | Poll interval in ms | -| `baseUrl` | `https://gitlab.com` | GitLab instance URL | -| `action_prompt_template` | (required for processing) | Maps GitLab action names to metadata templates | -| `groupPolicy` | `"disabled"` | Must be `"open"`, or `"allowlist"` with the project listed | -| `senderPolicy` | `"allowlist"` | Who can trigger the bot | +| Option | Default | Description | +| ------------------------ | ------------------------- | ------------------------------------------------------------------------------------------------- | +| `token` | (required) | PAT with `read_api` + `api` scopes | +| `pollInterval` | `60000` | Poll interval in ms | +| `baseUrl` | `https://gitlab.com` | GitLab instance URL | +| `action_prompt_template` | (required for processing) | Maps GitLab action names to metadata templates | +| `groupPolicy` | `"disabled"` | Must be `"open"`, `"allowlist"` with the project listed, or `"pairing"` with the project approved | +| `senderPolicy` | `"allowlist"` | Who can trigger the bot | ## action_prompt_template @@ -136,9 +136,9 @@ Always use `senderPolicy: "allowlist"` with explicit `allowedUsers` on public pr The adapter always sets `isMentioned = true` on dispatched envelopes, because GitLab has already determined the mention when creating the todo. The `action_prompt_template` config is the real event filter — only actions with a configured template are processed. The `@bot` mention is stripped from the message text before dispatch via `stripBotMention`. -### ⚠️ groupPolicy Must Be "open" or "allowlist" +### ⚠️ groupPolicy Must Be "open", "allowlist", or "pairing" -`groupPolicy` must be set to `"open"`, or `"allowlist"` with the project explicitly listed, for todos to be processed. The default value `"disabled"` drops all mentions: todos are marked done and the cursor advances, but no dispatch occurs. A rejection is logged (`preflight rejected reason=group_disabled`) but the todo is still consumed. If your bot is not responding to mentions, check that `groupPolicy` is not `"disabled"`. +`groupPolicy` must be set to `"open"`, `"allowlist"` with the project explicitly listed, or `"pairing"` for todos to be processed. Under `"pairing"`, the first mention from an unapproved project creates a group pairing request; approve it once with `qwen channel pairing approve`, and todos from that project are dispatched from then on. The default value `"disabled"` drops all mentions: todos are marked done and the cursor advances, but no dispatch occurs. A rejection is logged (`preflight rejected reason=group_disabled`) but the todo is still consumed. If your bot is not responding to mentions, check that `groupPolicy` is not `"disabled"`. ## How It Works @@ -151,7 +151,7 @@ The adapter uses GitLab's Todos API as the message source: 5. **Detect mention type** via `target_url` anchor: - `#note_123` present → comment mention → text is `todo.body` (the comment) - No anchor → description mention → text is the issue/MR description -6. **Dispatch** the envelope through `handleInbound` (requires `groupPolicy: "open"` or `"allowlist"` with the project listed) +6. **Dispatch** the envelope through `handleInbound` (requires `groupPolicy: "open"`, `"allowlist"` with the project listed, or `"pairing"` with the project approved) 7. **Advance cursor** and **mark todo done** (best-effort) The cursor (`lastProcessedId`) advances regardless of dispatch success or failure. Failed dispatches post a ⚠️ error comment on the issue/MR and are not retried — the user can re-mention the bot to trigger a new todo. diff --git a/docs/users/features/channels/overview.md b/docs/users/features/channels/overview.md index 780acd4d306..1eaa2e09614 100644 --- a/docs/users/features/channels/overview.md +++ b/docs/users/features/channels/overview.md @@ -200,7 +200,7 @@ Run these from the channel's workspace directory (or pass `--cwd `) — pai - Codes are 8 characters, uppercase, using an unambiguous alphabet (no `0`/`O`/`1`/`I`) - Codes expire after 1 hour -- Maximum 3 pending requests per channel at a time — additional requests are ignored until one expires or is approved +- Maximum 3 pending requests per channel at a time, and at most one per sender — additional requests are ignored until one expires or is approved - Users listed in `allowedUsers` in `settings.json` always skip pairing - Approved users are stored per workspace in `~/.qwen/channels//-allowlist.json` — treat this file as sensitive diff --git a/docs/users/features/channels/plugins.md b/docs/users/features/channels/plugins.md index 7f96db9a61d..d6ef02437ea 100644 --- a/docs/users/features/channels/plugins.md +++ b/docs/users/features/channels/plugins.md @@ -54,7 +54,7 @@ All standard channel options work with custom channels: | `cwd` | Working directory for the agent | | `instructions` | Prepended to the first message of each session | | `model` | Model override for the channel | -| `groupPolicy` | `disabled`, `allowlist`, or `open` | +| `groupPolicy` | `disabled`, `allowlist`, `pairing`, or `open` | | `dmPolicy` | `open` or `disabled` | | `groups` | Per-group settings | diff --git a/docs/users/features/channels/qqbot.md b/docs/users/features/channels/qqbot.md index 74d8d7c0807..1456d4aaf4d 100644 --- a/docs/users/features/channels/qqbot.md +++ b/docs/users/features/channels/qqbot.md @@ -100,9 +100,10 @@ Open QQ and send a message to your bot. You should see the response arrive in yo To use the bot in QQ groups: -1. Set `groupPolicy` to `"allowlist"` or `"open"` in your channel config +1. Set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"` in your channel config 2. Add the bot to a QQ group via the QQ Bot Open Platform dashboard or by having a group admin invite it 3. Group members must **@mention** the bot to trigger a response +4. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start QQ Bot API V2 only delivers group messages that @mention the bot — the bot does not see all group messages. By default, `requireMention` is `true` and should be left that way for QQ. @@ -159,7 +160,8 @@ Token refresh continues across WebSocket reconnects — the channel never goes o ### Bot doesn't respond in groups -- Check that `groupPolicy` is set to `"allowlist"` or `"open"` (default is `"disabled"`) +- Check that `groupPolicy` is set to `"allowlist"`, `"pairing"`, or `"open"` (default is `"disabled"`) +- If using `"pairing"`, verify the group's pairing request has been approved - **You must @mention the bot** — QQ only delivers messages that tag the bot - Verify the bot has been added to the group diff --git a/docs/users/features/channels/telegram.md b/docs/users/features/channels/telegram.md index 3e62ebbce85..563fbc7aa3a 100644 --- a/docs/users/features/channels/telegram.md +++ b/docs/users/features/channels/telegram.md @@ -70,10 +70,11 @@ Then open your bot in Telegram and send a message. You should see "Working..." a To use the bot in Telegram groups: -1. Set `groupPolicy` to `"allowlist"` or `"open"` in your channel config +1. Set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"` in your channel config 2. **Disable privacy mode** in BotFather: `/mybots` → select your bot → Bot Settings → Group Privacy → Turn Off 3. Add the bot to a group. If it was already in the group, **remove and re-add it** (Telegram caches privacy settings from when the bot joined) 4. If using `groupPolicy: "allowlist"`, add the group's chat ID to `groups` in your config +5. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start By default, the bot requires an @mention or a reply to respond in groups. Set `"requireMention": false` for a specific group to make it respond to all messages (useful for dedicated task groups). See [Group Chats](./overview#group-chats) for full details. @@ -105,8 +106,9 @@ The agent's markdown responses are automatically converted to Telegram-compatibl ### Bot doesn't respond in groups -- Check that `groupPolicy` is set to `"allowlist"` or `"open"` (default is `"disabled"`) +- Check that `groupPolicy` is set to `"allowlist"`, `"pairing"`, or `"open"` (default is `"disabled"`) - If using `"allowlist"`, verify the group's chat ID is in the `groups` config +- If using `"pairing"`, verify the group's pairing request has been approved - Make sure **Group Privacy is turned off** in BotFather — without this, the bot can't see non-command messages in groups - If you changed privacy mode after adding the bot to a group, **remove and re-add the bot** to the group - By default, the bot requires an @mention or a reply. Send `@yourbotname hello` to test diff --git a/docs/users/features/channels/wecom.md b/docs/users/features/channels/wecom.md index dc64f300b37..09ab5726f08 100644 --- a/docs/users/features/channels/wecom.md +++ b/docs/users/features/channels/wecom.md @@ -79,7 +79,7 @@ Open WeCom and send a message to the intelligent robot. - `pairing`: users must pair before using the bot. - `open`: anyone who can message the robot can use it. -For groups, set `groupPolicy` to `"allowlist"` or `"open"`. WeCom only delivers group messages that mention the intelligent robot, so every delivered group callback is treated as mentioned. The `requireMention` setting cannot enable responses to unmentioned group messages because those messages are not delivered to the bot. +For groups, set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"`. Under `"pairing"`, the group's first mention creates a pairing request that must be approved once before responses start. WeCom only delivers group messages that mention the intelligent robot, so every delivered group callback is treated as mentioned. The `requireMention` setting cannot enable responses to unmentioned group messages because those messages are not delivered to the bot. ### Group Mention Compatibility diff --git a/packages/channels/base/README.md b/packages/channels/base/README.md index dfd33e89f17..46e016d0805 100644 --- a/packages/channels/base/README.md +++ b/packages/channels/base/README.md @@ -304,18 +304,18 @@ constructor(channelName: string, workspaceCwd?: string) Persists pending pairing state to `{channelName}-pairing.json`, user approvals to `{channelName}-allowlist.json`, and group approvals to `{channelName}-groups.json`. With `workspaceCwd` (what `ChannelBase` passes — the channel's `cwd`), the files live under the workspace-scoped directory `~/.qwen/channels//` so two workspaces reusing the same channel name never share pairing requests or allowlist entries. Without it, the legacy global `~/.qwen/channels/` layout is used. The first time a given (workspace, channel) pair is constructed, existing legacy global files are copied in once (grandfathering) so already-approved senders stay approved; a per-channel `.migrated` sentinel in the scope directory marks that decision, after which legacy files are never consulted again for that channel. Channel names are URI-encoded in file names, so a name containing path separators cannot escape the scope directory. `revoke(senderId)` removes the sender only from this store's allowlist and never mutates the legacy global baseline. -| Method | Description | -| ------------------------------------- | --------------------------------------------------------------------------------------------------------- | -| `createRequest(senderId, senderName)` | Generate an 8-char pairing code (or return existing). Returns `null` if 3 pending requests already exist. | -| `createGroupRequest(...)` | Generate a request keyed by stable group ID and record its initiating sender. | -| `approve(code)` | Approve a user or group request and update its corresponding allowlist. | -| `isApproved(senderId)` | Check if sender is in the approved allowlist | -| `isGroupApproved(groupId)` | Check if a group is approved | -| `listPending()` | Get active (non-expired) pending requests | -| `getAllowlist()` | Get approved sender IDs | -| `getGroupAllowlist()` | Get approved group IDs | -| `revoke(senderId)` | Remove an approved sender. Returns whether the sender was present. | -| `revokeGroup(groupId)` | Remove an approved group. Returns whether the group was present. | +| Method | Description | +| ------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | +| `createRequest(senderId, senderName)` | Generate an 8-char pairing code (or return existing). Returns `null` if 3 pending requests already exist or the sender already holds one. | +| `createGroupRequest(...)` | Generate a request keyed by stable group ID and record its initiating sender. | +| `approve(code)` | Approve a user or group request and update its corresponding allowlist. | +| `isApproved(senderId)` | Check if sender is in the approved allowlist | +| `isGroupApproved(groupId)` | Check if a group is approved | +| `listPending()` | Get active (non-expired) pending requests | +| `getAllowlist()` | Get approved sender IDs | +| `getGroupAllowlist()` | Get approved group IDs | +| `revoke(senderId)` | Remove an approved sender. Returns whether the sender was present. | +| `revokeGroup(groupId)` | Remove an approved group. Returns whether the group was present. | ## Envelope diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index c436a518166..d34aaa62916 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -13609,8 +13609,8 @@ describe('ChannelBase', () => { store.createGroupRequest( `group-${index}`, `Group ${index}`, - 'alice', - 'Alice', + `sender-${index}`, + `Sender ${index}`, ); } const ch = createChannel({ groupPolicy: 'pairing' }); diff --git a/packages/channels/base/src/PairingStore.test.ts b/packages/channels/base/src/PairingStore.test.ts index 5fa46c9c119..a418df769e2 100644 --- a/packages/channels/base/src/PairingStore.test.ts +++ b/packages/channels/base/src/PairingStore.test.ts @@ -113,6 +113,52 @@ describe('PairingStore workspace scoping (#7017)', () => { ]); }); + it('limits each sender to one pending request across subjects', () => { + const store = new PairingStore('support-bot', workspaceA); + + const first = store.createGroupRequest( + 'group-1', + 'Release Team', + 'alice', + 'Alice', + ); + expect(first).toBeTruthy(); + + // Same sender, different subjects: no additional slots. + expect( + store.createGroupRequest('group-2', 'Platform Team', 'alice', 'Alice'), + ).toBeNull(); + expect(store.createRequest('alice', 'Alice')).toBeNull(); + + // Re-mentioning the same subject still returns its existing code. + expect( + store.createGroupRequest('group-1', 'Release Team', 'alice', 'Alice'), + ).toBe(first); + + // Other senders are unaffected until the shared cap (3) is reached. + expect( + store.createGroupRequest('group-2', 'Platform Team', 'bob', 'Bob'), + ).toBeTruthy(); + expect(store.createRequest('carol', 'Carol')).toBeTruthy(); + expect(store.createRequest('dave', 'Dave')).toBeNull(); + }); + + it('frees the sender slot once their pending request is approved', () => { + const store = new PairingStore('support-bot', workspaceA); + const code = store.createGroupRequest( + 'group-1', + 'Release Team', + 'alice', + 'Alice', + )!; + + expect(store.createRequest('alice', 'Alice')).toBeNull(); + + store.approve(code); + + expect(store.createRequest('alice', 'Alice')).toBeTruthy(); + }); + it('isolates group approvals and revocation by workspace', () => { const storeA = new PairingStore('support-bot', workspaceA); const storeB = new PairingStore('support-bot', workspaceB); diff --git a/packages/channels/base/src/PairingStore.ts b/packages/channels/base/src/PairingStore.ts index 775385e931b..519a8089a3f 100644 --- a/packages/channels/base/src/PairingStore.ts +++ b/packages/channels/base/src/PairingStore.ts @@ -177,8 +177,9 @@ export class PairingStore { /** * Create a pairing request for an unknown sender. - * Returns the code if created, or null if the pending cap is reached. - * If the sender already has a non-expired pending request, returns that code. + * Returns the code if created, or null if the pending cap is reached or the + * sender already holds a pending request for another subject. If the subject + * already has a non-expired pending request, returns that code. */ createRequest(senderId: string, senderName: string): string | null { return this.createSubjectRequest( @@ -222,6 +223,14 @@ export class PairingStore { return existing.code; } + // One sender may hold only one pending request at a time: subject-keyed + // dedup lets a single member vouch for distinct groups, and without this + // limit one sender could occupy every shared slot and block all other + // pairing requests until expiry. + if (active.some((request) => request.senderId === senderId)) { + return null; + } + // Cap check if (active.length >= MAX_PENDING) { return null; diff --git a/packages/channels/gitlab/src/GitlabAdapter.test.ts b/packages/channels/gitlab/src/GitlabAdapter.test.ts index 1fb089197c1..958f07369d6 100644 --- a/packages/channels/gitlab/src/GitlabAdapter.test.ts +++ b/packages/channels/gitlab/src/GitlabAdapter.test.ts @@ -222,6 +222,44 @@ describe('GitlabChannel', () => { expect(ch.config.allowedUsers).toEqual(['alice']); ch.disconnect(); }); + + it('does not warn about groupPolicy when pairing is configured', async () => { + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const config = makeConfig({ groupPolicy: 'pairing' }); + const ch = new TestableGitlabChannel('test-gl', config, makeBridge()); + await ch.connect(); + ch.disconnect(); + expect( + stderr.mock.calls.some((call) => + String(call[0]).includes('groupPolicy is'), + ), + ).toBe(false); + } finally { + stderr.mockRestore(); + } + }); + + it('warns on connect when groupPolicy cannot dispatch todos', async () => { + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const config = makeConfig({ groupPolicy: 'disabled' }); + const ch = new TestableGitlabChannel('test-gl', config, makeBridge()); + await ch.connect(); + ch.disconnect(); + const warnings = stderr.mock.calls + .map((call) => String(call[0])) + .filter((text) => text.includes('groupPolicy is "disabled"')); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('"pairing"'); + } finally { + stderr.mockRestore(); + } + }); }); describe('pollOnce', () => { diff --git a/packages/channels/gitlab/src/GitlabAdapter.ts b/packages/channels/gitlab/src/GitlabAdapter.ts index 1cbd3d0d007..37403c5ad56 100644 --- a/packages/channels/gitlab/src/GitlabAdapter.ts +++ b/packages/channels/gitlab/src/GitlabAdapter.ts @@ -76,9 +76,13 @@ export class GitlabChannel extends PollingChannelBase { ); } - if (cfg.groupPolicy !== 'open' && cfg.groupPolicy !== 'allowlist') { + if ( + cfg.groupPolicy !== 'open' && + cfg.groupPolicy !== 'allowlist' && + cfg.groupPolicy !== 'pairing' + ) { process.stderr.write( - `[Channel:${this.name}] warning: groupPolicy is "${cfg.groupPolicy ?? 'disabled'}"; must be "open" (or "allowlist" with the project listed) for todos to be dispatched\n`, + `[Channel:${this.name}] warning: groupPolicy is "${cfg.groupPolicy ?? 'disabled'}"; must be "open", "allowlist" (with the project listed), or "pairing" (after one-time group approval) for todos to be dispatched\n`, ); } diff --git a/packages/channels/gitlab/src/index.ts b/packages/channels/gitlab/src/index.ts index c82065309a2..14d0bc3c15b 100644 --- a/packages/channels/gitlab/src/index.ts +++ b/packages/channels/gitlab/src/index.ts @@ -32,7 +32,8 @@ export const plugin: ChannelPlugin = { label: 'Group Policy', kind: 'enum', required: true, - description: 'Must be "Open" or "Allowlist" for todos to be processed', + description: + 'Must be "Open", "Allowlist", or "Pairing" for todos to be processed', default: 'open', options: [ { value: 'open', label: 'Open' }, diff --git a/packages/cli/src/commands/channel.ts b/packages/cli/src/commands/channel.ts index d2c0c9c193f..63e2da5624b 100644 --- a/packages/cli/src/commands/channel.ts +++ b/packages/cli/src/commands/channel.ts @@ -13,7 +13,7 @@ import { configureWeixinCommand } from './channel/configure.js'; const pairingCommand: CommandModule = { command: 'pairing', - describe: 'Manage DM pairing requests', + describe: 'Manage DM and group pairing requests', builder: (yargs: Argv) => yargs .command(pairingListCommand) diff --git a/packages/web-shell/client/e2e/utils/mockDaemon.ts b/packages/web-shell/client/e2e/utils/mockDaemon.ts index ccb62da095b..3b1b132cd5b 100644 --- a/packages/web-shell/client/e2e/utils/mockDaemon.ts +++ b/packages/web-shell/client/e2e/utils/mockDaemon.ts @@ -60,6 +60,7 @@ export interface WebShellDaemonScenario { channels: DaemonChannelsSnapshot; pairingRequests: Record; pairingApprovals: Record; + pairingGroupApprovals: Record; sessions: DaemonSessionSummary[]; sessionGroups: DaemonSessionGroup[]; events: DaemonEvent[]; @@ -114,6 +115,7 @@ type ScenarioOverrides = Partial< | 'channels' | 'pairingRequests' | 'pairingApprovals' + | 'pairingGroupApprovals' | 'sessions' | 'sessionGroups' | 'state' @@ -131,6 +133,7 @@ type ScenarioOverrides = Partial< channels?: DaemonChannelsSnapshot; pairingRequests?: Record; pairingApprovals?: Record; + pairingGroupApprovals?: Record; sessions?: DaemonSessionSummary[]; sessionGroups?: DaemonSessionGroup[]; state?: Partial; @@ -346,6 +349,7 @@ export function createWebShellDaemonScenario( channels: overrides.channels ?? { revision: '1', instances: {} }, pairingRequests: overrides.pairingRequests ?? {}, pairingApprovals: overrides.pairingApprovals ?? {}, + pairingGroupApprovals: overrides.pairingGroupApprovals ?? {}, sessions, sessionGroups: overrides.sessionGroups ?? [], events: overrides.events ?? [], @@ -896,15 +900,27 @@ async function handleDaemonRoute( ...scenario.pairingRequests, [name]: remaining, }; - scenario.pairingApprovals = { - ...scenario.pairingApprovals, - [name]: Array.from( - new Set([ - ...(scenario.pairingApprovals[name] ?? []), - approved.senderId, - ]), - ), - }; + if (approved.subject?.type === 'group') { + scenario.pairingGroupApprovals = { + ...scenario.pairingGroupApprovals, + [name]: Array.from( + new Set([ + ...(scenario.pairingGroupApprovals[name] ?? []), + approved.subject.id, + ]), + ), + }; + } else { + scenario.pairingApprovals = { + ...scenario.pairingApprovals, + [name]: Array.from( + new Set([ + ...(scenario.pairingApprovals[name] ?? []), + approved.senderId, + ]), + ), + }; + } await json(route, { approved, requests: remaining }); return; } @@ -915,13 +931,18 @@ async function handleDaemonRoute( if (pairingApprovalsMatch) { const name = decodeURIComponent(pairingApprovalsMatch[1]); const senderIds = scenario.pairingApprovals[name] ?? []; + const groupIds = scenario.pairingGroupApprovals[name] ?? []; if (method === 'GET') { - await json(route, { senderIds }); + await json(route, { senderIds, groupIds }); return; } if (method === 'DELETE') { const senderId = String(getRecordValue(body, 'senderId') ?? ''); - if (!senderIds.includes(senderId)) { + const groupId = String(getRecordValue(body, 'groupId') ?? ''); + const known = senderId + ? senderIds.includes(senderId) + : groupIds.includes(groupId); + if (!known) { await json( route, { @@ -932,12 +953,26 @@ async function handleDaemonRoute( ); return; } - const remaining = senderIds.filter((item) => item !== senderId); + const remainingSenders = senderId + ? senderIds.filter((item) => item !== senderId) + : senderIds; + const remainingGroups = + groupId && !senderId + ? groupIds.filter((item) => item !== groupId) + : groupIds; scenario.pairingApprovals = { ...scenario.pairingApprovals, - [name]: remaining, + [name]: remainingSenders, + }; + scenario.pairingGroupApprovals = { + ...scenario.pairingGroupApprovals, + [name]: remainingGroups, }; - await json(route, { revoked: senderId, senderIds: remaining }); + await json(route, { + revoked: senderId || groupId, + senderIds: remainingSenders, + groupIds: remainingGroups, + }); return; } } diff --git a/packages/web-shell/client/e2e/web-shell.channels.spec.ts b/packages/web-shell/client/e2e/web-shell.channels.spec.ts index 07df572ee23..b37d300fea7 100644 --- a/packages/web-shell/client/e2e/web-shell.channels.spec.ts +++ b/packages/web-shell/client/e2e/web-shell.channels.spec.ts @@ -70,6 +70,13 @@ test('creates and deletes a typed Channel configuration', async ({ code: 'ABCD1234', createdAt: Date.parse('2026-07-28T00:00:00.000Z'), }, + { + senderId: 'user-77', + senderName: 'Grace', + subject: { type: 'group', id: 'group-9', name: 'Release Team' }, + code: 'QW3N5678', + createdAt: Date.parse('2026-07-28T00:02:00.000Z'), + }, ], }, }); @@ -138,6 +145,11 @@ test('creates and deletes a typed Channel configuration', async ({ await page .getByRole('button', { name: 'Approve Ada, code ABCD1234' }) .click(); + await page + .getByRole('button', { + name: 'Approve Group: Release Team, code QW3N5678', + }) + .click(); await expect(page.getByText('No pending requests')).toBeVisible(); await expect .poll(() => @@ -153,7 +165,21 @@ test('creates and deletes a typed Channel configuration', async ({ expect.objectContaining({ body: { code: 'ABCD1234' }, }), + expect.objectContaining({ + body: { code: 'QW3N5678' }, + }), ]); + await expect( + page.getByRole('button', { name: 'Revoke Group: group-9' }), + ).toBeVisible(); + await page.getByRole('button', { name: 'Revoke Group: group-9' }).click(); + const groupRevokeConfirmation = page.getByRole('alertdialog'); + await expect(groupRevokeConfirmation).toContainText( + 'Only the approval created through pairing will be removed.', + ); + await groupRevokeConfirmation + .getByRole('button', { name: 'Revoke approval' }) + .click(); await expect( page.getByRole('button', { name: 'Revoke user-42' }), ).toBeVisible(); @@ -175,6 +201,9 @@ test('creates and deletes a typed Channel configuration', async ({ ), ) .toEqual([ + expect.objectContaining({ + body: { groupId: 'group-9' }, + }), expect.objectContaining({ body: { senderId: 'user-42' }, }), From cc56445eaf294f2bc38a198a3d4109013b20ddcc Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 14:57:11 +0000 Subject: [PATCH 08/10] fix(channels): address group pairing review findings round 7 (#8440) --- docs/users/features/channels/github.md | 2 +- docs/users/features/channels/gitlab.md | 2 + docs/users/features/channels/overview.md | 7 +- packages/channels/base/README.md | 36 +-- .../channels/base/src/ChannelBase.test.ts | 216 +++++++++++++----- packages/channels/base/src/ChannelBase.ts | 33 ++- packages/channels/base/src/GroupGate.test.ts | 8 +- packages/channels/base/src/GroupGate.ts | 12 +- .../channels/base/src/PairingStore.test.ts | 109 +++++---- packages/channels/base/src/PairingStore.ts | 28 ++- packages/channels/base/src/SenderGate.test.ts | 26 ++- packages/channels/base/src/SenderGate.ts | 15 +- packages/channels/base/src/index.ts | 6 +- packages/channels/github/src/index.ts | 3 +- .../cli/src/commands/channel/pairing.test.ts | 31 ++- .../serve/channel-management-service.test.ts | 43 ++-- packages/web-shell/client/i18n.tsx | 8 +- 17 files changed, 390 insertions(+), 195 deletions(-) diff --git a/docs/users/features/channels/github.md b/docs/users/features/channels/github.md index 73523f7156d..50d03aac07c 100644 --- a/docs/users/features/channels/github.md +++ b/docs/users/features/channels/github.md @@ -72,7 +72,7 @@ For GitHub Enterprise Server, set `baseUrl`: | `token` | (required) | Classic PAT with `notifications` scope | | `pollInterval` | `60000` | Poll interval in ms | | `baseUrl` | `https://api.github.com` | API base URL (for GHE) | -| `groupPolicy` | `"disabled"` | Must be `"open"` for notifications to flow | +| `groupPolicy` | `"disabled"` | Must be `"open"`, `"allowlist"`, or `"pairing"` for notifications to flow | | `senderPolicy` | `"allowlist"` | Who can trigger the bot | | `groups.*.requireMention` | `true` | Require @mentions for ordinary comments; directed notification reasons still run | | `blockStreaming` | `"off"` | Always forced to `"off"`; intermediate model chunks aren't published; `"on"` is not supported | diff --git a/docs/users/features/channels/gitlab.md b/docs/users/features/channels/gitlab.md index 7b4729421eb..f73176939e6 100644 --- a/docs/users/features/channels/gitlab.md +++ b/docs/users/features/channels/gitlab.md @@ -132,6 +132,8 @@ On a **public project**, setting `senderPolicy: "open"` allows **any GitLab user Always use `senderPolicy: "allowlist"` with explicit `allowedUsers` on public projects. +Note that under `groupPolicy: "pairing"`, access is granted per project: once a project is approved, **any GitLab user** can drive the bot through that project's issues and merge requests. All GitLab traffic is group traffic, so `senderPolicy` and `allowedUsers` do not gate members of an approved project. + ## Mention Detection The adapter always sets `isMentioned = true` on dispatched envelopes, because GitLab has already determined the mention when creating the todo. The `action_prompt_template` config is the real event filter — only actions with a configured template are processed. The `@bot` mention is stripped from the message text before dispatch via `stripBotMention`. diff --git a/docs/users/features/channels/overview.md b/docs/users/features/channels/overview.md index 1eaa2e09614..86f6252dcef 100644 --- a/docs/users/features/channels/overview.md +++ b/docs/users/features/channels/overview.md @@ -200,7 +200,7 @@ Run these from the channel's workspace directory (or pass `--cwd `) — pai - Codes are 8 characters, uppercase, using an unambiguous alphabet (no `0`/`O`/`1`/`I`) - Codes expire after 1 hour -- Maximum 3 pending requests per channel at a time, and at most one per sender — additional requests are ignored until one expires or is approved +- Maximum 3 pending requests per channel at a time, and at most one per sender — additional requests are declined until one expires or is approved - Users listed in `allowedUsers` in `settings.json` always skip pairing - Approved users are stored per workspace in `~/.qwen/channels//-allowlist.json` — treat this file as sensitive @@ -229,6 +229,11 @@ An unmentioned message never creates a group pairing request, even when a group sets `requireMention` to `false`; after approval, the configured mention policy applies normally. +Group pairing requests share the same pending queue as DM pairing requests: +a channel holds at most 3 pending requests overall, and a sender holds at most +one pending request across user and group requests (see +[Pairing Rules](#pairing-rules)). + ### Mention Gating In groups, the bot requires an `@mention` or a reply to one of its messages by default. This prevents the bot from responding to every message in a group chat. diff --git a/packages/channels/base/README.md b/packages/channels/base/README.md index 46e016d0805..f44c1fda6b5 100644 --- a/packages/channels/base/README.md +++ b/packages/channels/base/README.md @@ -263,9 +263,9 @@ constructor(bridge: ChannelAgentBridge, defaultCwd: string, scope?: SessionScope constructor(policy: SenderPolicy, allowedUsers?: string[], pairingStore?: PairingStore) ``` -| Method | Description | -| ------------------------------ | ------------------------------------------------------------ | -| `check(senderId, senderName?)` | Returns `{ allowed: boolean, pairingCode?: string \| null }` | +| Method | Description | +| ------------------------------ | -------------------------------------------------------------------- | +| `check(senderId, senderName?)` | Returns `{ allowed: boolean, pairing?: CreatePairingRequestResult }` | **Policy behavior:** @@ -281,9 +281,9 @@ constructor(policy: SenderPolicy, allowedUsers?: string[], pairingStore?: Pairin constructor(policy?: GroupPolicy, groups?: Record, pairingStore?: PairingStore) ``` -| Method | Description | -| --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `check(envelope, options?)` | Returns the group policy decision and an optional pairing code. Under `pairing`, a mention/reply from an unapproved group creates (or reuses) a pending pairing request; pass `{ createPairingRequest: false }` for read-only probes | +| Method | Description | +| --------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `check(envelope, options?)` | Returns the group policy decision and an optional pairing result. Under `pairing`, a mention/reply from an unapproved group creates (or reuses) a pending pairing request; pass `{ createPairingRequest: false }` for read-only probes | **Policy behavior:** @@ -304,18 +304,18 @@ constructor(channelName: string, workspaceCwd?: string) Persists pending pairing state to `{channelName}-pairing.json`, user approvals to `{channelName}-allowlist.json`, and group approvals to `{channelName}-groups.json`. With `workspaceCwd` (what `ChannelBase` passes — the channel's `cwd`), the files live under the workspace-scoped directory `~/.qwen/channels//` so two workspaces reusing the same channel name never share pairing requests or allowlist entries. Without it, the legacy global `~/.qwen/channels/` layout is used. The first time a given (workspace, channel) pair is constructed, existing legacy global files are copied in once (grandfathering) so already-approved senders stay approved; a per-channel `.migrated` sentinel in the scope directory marks that decision, after which legacy files are never consulted again for that channel. Channel names are URI-encoded in file names, so a name containing path separators cannot escape the scope directory. `revoke(senderId)` removes the sender only from this store's allowlist and never mutates the legacy global baseline. -| Method | Description | -| ------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | -| `createRequest(senderId, senderName)` | Generate an 8-char pairing code (or return existing). Returns `null` if 3 pending requests already exist or the sender already holds one. | -| `createGroupRequest(...)` | Generate a request keyed by stable group ID and record its initiating sender. | -| `approve(code)` | Approve a user or group request and update its corresponding allowlist. | -| `isApproved(senderId)` | Check if sender is in the approved allowlist | -| `isGroupApproved(groupId)` | Check if a group is approved | -| `listPending()` | Get active (non-expired) pending requests | -| `getAllowlist()` | Get approved sender IDs | -| `getGroupAllowlist()` | Get approved group IDs | -| `revoke(senderId)` | Remove an approved sender. Returns whether the sender was present. | -| `revokeGroup(groupId)` | Remove an approved group. Returns whether the group was present. | +| Method | Description | +| ------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `createRequest(senderId, senderName)` | Generate an 8-char pairing code (or return the existing one). Returns `{ rejected: 'sender_pending' }` if the sender already holds a request, or `{ rejected: 'cap_reached' }` if 3 requests are already pending. | +| `createGroupRequest(...)` | Generate a request keyed by stable group ID and record its initiating sender. | +| `approve(code)` | Approve a user or group request and update its corresponding allowlist. | +| `isApproved(senderId)` | Check if sender is in the approved allowlist | +| `isGroupApproved(groupId)` | Check if a group is approved | +| `listPending()` | Get active (non-expired) pending requests | +| `getAllowlist()` | Get approved sender IDs | +| `getGroupAllowlist()` | Get approved group IDs | +| `revoke(senderId)` | Remove an approved sender. Returns whether the sender was present. | +| `revokeGroup(groupId)` | Remove an approved group. Returns whether the group was present. | ## Envelope diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index d34aaa62916..768b64d3091 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -35,6 +35,7 @@ import { isChannelProactiveDeliveryError, } from './ChannelProactiveDeliveryError.js'; import { PairingStore } from './PairingStore.js'; +import type { CreatePairingRequestResult } from './PairingStore.js'; // Concrete test implementation class TestChannel extends ChannelBase { @@ -343,6 +344,13 @@ function envelope(overrides: Partial = {}): Envelope { }; } +function pairingCodeOf(result: CreatePairingRequestResult): string { + if ('code' in result) return result.code; + throw new Error( + `expected a pairing code, got rejection "${result.rejected}"`, + ); +} + function groupHistoryPath(): string { return join( mkdtempSync(join(tmpdir(), 'qwen-channel-history-')), @@ -2669,13 +2677,13 @@ describe('ChannelBase', () => { process.env['QWEN_HOME'] = qwenHome; try { const store = new PairingStore('test-chan', '/tmp'); - const code = store.createGroupRequest( + const created = store.createGroupRequest( 'chat1', 'Release Team', 'alice', 'Alice', ); - store.approve(code!); + store.approve(pairingCodeOf(created)); const ch = createChannel( { groupPolicy: 'pairing', @@ -2722,13 +2730,13 @@ describe('ChannelBase', () => { process.env['QWEN_HOME'] = qwenHome; try { const store = new PairingStore('test-chan', '/tmp'); - const code = store.createGroupRequest( + const created = store.createGroupRequest( 'chat1', 'Release Team', 'alice', 'Alice', ); - store.approve(code!); + store.approve(pairingCodeOf(created)); const recoveryState: { current?: Promise } = {}; const ch = createChannel( { @@ -2914,13 +2922,13 @@ describe('ChannelBase', () => { expect(existsSync(historyPath)).toBe(false); const store = new PairingStore('test-chan', '/tmp'); - const code = store.createGroupRequest( + const created = store.createGroupRequest( 'group-1', 'Release Team', 'alice', 'Alice', ); - store.approve(code!); + store.approve(pairingCodeOf(created)); await ch.handleInbound( envelope({ @@ -13550,6 +13558,7 @@ describe('ChannelBase', () => { await ch.handleInbound(first); expect(ch.sent).toHaveLength(1); + expect(ch.sent[0]!.chatId).toBe('group-1'); expect(bridge.prompt).not.toHaveBeenCalled(); const store = new PairingStore('test-chan', '/tmp'); const request = store.listPending()[0]; @@ -13703,6 +13712,7 @@ describe('ChannelBase', () => { ); expect(threadMessages).toHaveLength(1); + expect(threadMessages[0]!.chatId).toBe('group-1'); expect(threadMessages[0]!.threadId).toBe('issue:42'); expect(threadMessages[0]!.text).toContain('requires approval'); } finally { @@ -13713,16 +13723,25 @@ describe('ChannelBase', () => { }); it('still gates DMs by senderPolicy when groupPolicy uses pairing', async () => { - const ch = createChannel({ - groupPolicy: 'pairing', - senderPolicy: 'allowlist', - allowedUsers: [], - }); + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }); - await ch.handleInbound(envelope({ senderId: 'stranger' })); + await ch.handleInbound(envelope({ senderId: 'stranger' })); - expect(bridge.prompt).not.toHaveBeenCalled(); - expect(ch.sent).toEqual([]); + expect(bridge.prompt).not.toHaveBeenCalled(); + expect(ch.sent).toEqual([]); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } }); it('keeps DM pairing on the sender flow when groupPolicy uses pairing', async () => { @@ -13739,7 +13758,83 @@ describe('ChannelBase', () => { await ch.handleInbound(envelope({ senderId: 'stranger' })); expect(ch.sent).toHaveLength(1); - expect(ch.sent[0]!.text).toContain('pairing code'); + expect(ch.sent[0]!.chatId).toBe('chat1'); + expect(ch.sent[0]!.text).toContain('Your pairing code'); + expect(bridge.prompt).not.toHaveBeenCalled(); + expect( + new PairingStore('test-chan', '/tmp').listPending()[0]?.subject, + ).toEqual({ type: 'user', id: 'stranger', name: 'User 1' }); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + + it('tells a sender with a pending group request their DM cannot pair yet', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ + groupPolicy: 'pairing', + senderPolicy: 'pairing', + allowedUsers: [], + }); + + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + chatId: 'group-1', + chatName: 'Release Team', + senderId: 'stranger', + senderName: 'User 1', + }), + ); + await ch.handleInbound(envelope({ senderId: 'stranger' })); + + expect(ch.sent).toHaveLength(2); + expect(ch.sent[1]!.chatId).toBe('chat1'); + expect(ch.sent[1]!.text).toContain( + 'You already have a pending pairing request', + ); + expect(bridge.prompt).not.toHaveBeenCalled(); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + + it('tells a group when the mentioning sender already holds a pending request', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ + groupPolicy: 'pairing', + senderPolicy: 'pairing', + allowedUsers: [], + }); + + await ch.handleInbound(envelope({ senderId: 'stranger' })); + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + chatId: 'group-1', + chatName: 'Release Team', + senderId: 'stranger', + senderName: 'User 1', + }), + ); + + expect(ch.sent).toHaveLength(2); + expect(ch.sent[1]!.chatId).toBe('group-1'); + expect(ch.sent[1]!.text).toContain( + 'You already have a pending pairing request', + ); expect(bridge.prompt).not.toHaveBeenCalled(); } finally { if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; @@ -18723,13 +18818,13 @@ describe('ChannelBase', () => { process.env['QWEN_HOME'] = qwenHome; try { const store = new PairingStore('test-chan', '/tmp'); - const code = store.createGroupRequest( + const created = store.createGroupRequest( 'group-1', 'Release Team', 'alice', 'Alice', ); - store.approve(code!); + store.approve(pairingCodeOf(created)); const disable = vi.fn().mockResolvedValue(true); const ch = createChannel( { @@ -18808,49 +18903,58 @@ describe('ChannelBase', () => { }); it('rejects a stored DM job for an unlisted sender when groupPolicy uses pairing', async () => { - const disable = vi.fn().mockResolvedValue(true); - const ch = createChannel( - { - groupPolicy: 'pairing', - senderPolicy: 'allowlist', - allowedUsers: [], - }, - { - loopController: { - create: vi.fn(), - listForTarget: vi.fn(), - disable, - validateCron: vi.fn(), + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const disable = vi.fn().mockResolvedValue(true); + const ch = createChannel( + { + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], }, - }, - ); - ch.proactiveSupported = true; + { + loopController: { + create: vi.fn(), + listForTarget: vi.fn(), + disable, + validateCron: vi.fn(), + }, + }, + ); + ch.proactiveSupported = true; - await expect( - ch.runLoopPrompt({ - id: 'job-1', - channelName: 'test-chan', - target: { + await expect( + ch.runLoopPrompt({ + id: 'job-1', channelName: 'test-chan', - senderId: 'bob', - chatId: 'chat1', - isGroup: false, - }, - cwd: '/tmp', - cron: '0 9 * * *', - prompt: 'post summary', - label: 'daily summary', - recurring: true, - enabled: true, - createdBy: 'Bob', - createdAt: '2026-06-30T01:00:00.000Z', - consecutiveFailures: 0, - runCount: 0, - }), - ).rejects.toThrow('no longer authorized'); + target: { + channelName: 'test-chan', + senderId: 'bob', + chatId: 'chat1', + isGroup: false, + }, + cwd: '/tmp', + cron: '0 9 * * *', + prompt: 'post summary', + label: 'daily summary', + recurring: true, + enabled: true, + createdBy: 'Bob', + createdAt: '2026-06-30T01:00:00.000Z', + consecutiveFailures: 0, + runCount: 0, + }), + ).rejects.toThrow('no longer authorized'); - expect(disable).toHaveBeenCalledWith('job-1'); - expect(bridge.prompt).not.toHaveBeenCalled(); + expect(disable).toHaveBeenCalledWith('job-1'); + expect(bridge.prompt).not.toHaveBeenCalled(); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } }); it('rejects stored threaded jobs unless the adapter supports the target', async () => { diff --git a/packages/channels/base/src/ChannelBase.ts b/packages/channels/base/src/ChannelBase.ts index 18ac653c7c9..18cc0ac8077 100644 --- a/packages/channels/base/src/ChannelBase.ts +++ b/packages/channels/base/src/ChannelBase.ts @@ -37,6 +37,7 @@ import { GroupHistoryStore } from './group-history-store.js'; import type { GroupHistoryEntry } from './group-history-store.js'; import { SenderGate } from './SenderGate.js'; import { PairingStore } from './PairingStore.js'; +import type { CreatePairingRequestResult } from './PairingStore.js'; import { SessionRouter } from './SessionRouter.js'; import { getGlobalQwenDir } from './paths.js'; import { @@ -4818,11 +4819,11 @@ export abstract class ChannelBase { protected preflightInbound(envelope: Envelope): boolean | Promise { const groupResult = this.groupGate.check(envelope); if (!groupResult.allowed) { - if (groupResult.pairingCode !== undefined) { + if (groupResult.pairing !== undefined) { this.logPreflightRejected('group_pairing_required'); return this.onGroupPairingRequired( envelope.chatId, - groupResult.pairingCode, + groupResult.pairing, envelope.threadId, ) .then(() => false) @@ -4860,11 +4861,11 @@ export abstract class ChannelBase { const result = this.gate.check(envelope.senderId, envelope.senderName); if (!result.allowed) { - if (result.pairingCode !== undefined) { + if (result.pairing !== undefined) { this.logPreflightRejected('sender_pairing_required'); return this.onPairingRequired( envelope.chatId, - result.pairingCode, + result.pairing, envelope.threadId, ) .then(() => false) @@ -5763,42 +5764,50 @@ export abstract class ChannelBase { await current; } + private pairingRejectionMessage( + rejected: 'sender_pending' | 'cap_reached', + ): string { + return rejected === 'sender_pending' + ? 'You already have a pending pairing request. It must be approved or expire before another can be created.' + : 'Too many pending pairing requests. Please try again later.'; + } + protected async onPairingRequired( chatId: string, - code: string | null, + result: CreatePairingRequestResult, threadId?: string, ): Promise { - if (code) { + if ('code' in result) { await this.sendThreadMessage( chatId, threadId, - `Your pairing code is: ${code}\n\nAsk the bot operator to approve you with:\n qwen channel pairing approve ${this.name} ${code}`, + `Your pairing code is: ${result.code}\n\nAsk the bot operator to approve you with:\n qwen channel pairing approve ${this.name} ${result.code}`, ); } else { await this.sendThreadMessage( chatId, threadId, - 'Too many pending pairing requests. Please try again later.', + this.pairingRejectionMessage(result.rejected), ); } } protected async onGroupPairingRequired( chatId: string, - code: string | null, + result: CreatePairingRequestResult, threadId?: string, ): Promise { - if (code) { + if ('code' in result) { await this.sendThreadMessage( chatId, threadId, - `This group requires approval. Its pairing code is: ${code}\n\nAsk the bot operator to approve the group with:\n qwen channel pairing approve ${this.name} ${code}`, + `This group requires approval. Its pairing code is: ${result.code}\n\nAsk the bot operator to approve the group with:\n qwen channel pairing approve ${this.name} ${result.code}`, ); } else { await this.sendThreadMessage( chatId, threadId, - 'Too many pending pairing requests. Please try again later.', + this.pairingRejectionMessage(result.rejected), ); } } diff --git a/packages/channels/base/src/GroupGate.test.ts b/packages/channels/base/src/GroupGate.test.ts index 51af88da80e..662bcdc5250 100644 --- a/packages/channels/base/src/GroupGate.test.ts +++ b/packages/channels/base/src/GroupGate.test.ts @@ -143,14 +143,14 @@ describe('GroupGate', () => { expect(result).toEqual({ allowed: false, reason: 'pairing_required', - pairingCode: null, + pairing: { rejected: 'cap_reached' }, }); }); - it('returns a null pairing code when constructed without a store', () => { + it('returns a cap_reached rejection when constructed without a store', () => { const gate = new GroupGate('pairing'); const result = gate.check(envelope({ isGroup: true, isMentioned: true })); - expect(result.pairingCode).toBeNull(); + expect(result.pairing).toEqual({ rejected: 'cap_reached' }); }); it('requests pairing after a reply to the bot', () => { @@ -164,7 +164,7 @@ describe('GroupGate', () => { expect(result.allowed).toBe(false); expect(result.reason).toBe('pairing_required'); - expect(result.pairingCode).toBeTypeOf('string'); + expect(result.pairing).toEqual({ code: expect.any(String) }); expect(store.listPending()).toHaveLength(1); }); }); diff --git a/packages/channels/base/src/GroupGate.ts b/packages/channels/base/src/GroupGate.ts index ce343ed372e..d30d640e8e0 100644 --- a/packages/channels/base/src/GroupGate.ts +++ b/packages/channels/base/src/GroupGate.ts @@ -1,5 +1,8 @@ import type { GroupPolicy, GroupConfig, Envelope } from './types.js'; -import type { PairingStore } from './PairingStore.js'; +import type { + CreatePairingRequestResult, + PairingStore, +} from './PairingStore.js'; export interface GroupCheckResult { allowed: boolean; @@ -9,7 +12,8 @@ export interface GroupCheckResult { | 'mention_required' | 'pairing_trigger_required' | 'pairing_required'; - pairingCode?: string | null; + /** Set when the pairing policy denies the group. */ + pairing?: CreatePairingRequestResult; } export class GroupGate { @@ -72,7 +76,7 @@ export class GroupGate { ) { return { allowed: false, reason: 'pairing_trigger_required' }; } - const code = this.pairingStore?.createGroupRequest( + const result = this.pairingStore?.createGroupRequest( envelope.chatId, envelope.chatName || envelope.chatId, envelope.senderId, @@ -81,7 +85,7 @@ export class GroupGate { return { allowed: false, reason: 'pairing_required', - pairingCode: code ?? null, + pairing: result ?? { rejected: 'cap_reached' }, }; } diff --git a/packages/channels/base/src/PairingStore.test.ts b/packages/channels/base/src/PairingStore.test.ts index a418df769e2..290b1f5f7af 100644 --- a/packages/channels/base/src/PairingStore.test.ts +++ b/packages/channels/base/src/PairingStore.test.ts @@ -3,8 +3,16 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { PairingStore } from './PairingStore.js'; +import type { CreatePairingRequestResult } from './PairingStore.js'; import { getWorkspaceScopeDirName } from './paths.js'; +function codeOf(result: CreatePairingRequestResult): string { + if ('code' in result) return result.code; + throw new Error( + `expected a pairing code, got rejection "${result.rejected}"`, + ); +} + describe('PairingStore workspace scoping (#7017)', () => { let qwenHome: string; let workspaceA: string; @@ -36,8 +44,8 @@ describe('PairingStore workspace scoping (#7017)', () => { const storeA = new PairingStore('support-bot', workspaceA); const storeB = new PairingStore('support-bot', workspaceB); - const code = storeA.createRequest('sender-1', 'Sender One'); - expect(code).toBeTruthy(); + const result = storeA.createRequest('sender-1', 'Sender One'); + expect(result).toEqual({ code: expect.any(String) }); expect(storeA.listPending()).toHaveLength(1); expect(storeB.listPending()).toHaveLength(0); @@ -47,7 +55,7 @@ describe('PairingStore workspace scoping (#7017)', () => { const storeA = new PairingStore('support-bot', workspaceA); const storeB = new PairingStore('support-bot', workspaceB); - const code = storeA.createRequest('sender-1', 'Sender One')!; + const code = codeOf(storeA.createRequest('sender-1', 'Sender One')); const approved = storeA.approve(code); expect(approved?.senderId).toBe('sender-1'); @@ -57,14 +65,14 @@ describe('PairingStore workspace scoping (#7017)', () => { it('approves a group without approving the member who requested it', () => { const store = new PairingStore('support-bot', workspaceA); - const code = store.createGroupRequest( + const result = store.createGroupRequest( 'group-1', 'Release Team', 'sender-1', 'Alice', ); - expect(code).toBeTruthy(); + expect(result).toEqual({ code: expect.any(String) }); expect(store.listPending()).toEqual([ expect.objectContaining({ senderId: 'sender-1', @@ -77,7 +85,7 @@ describe('PairingStore workspace scoping (#7017)', () => { }), ]); - store.approve(code!); + store.approve(codeOf(result)); expect(store.isGroupApproved('group-1')).toBe(true); expect(store.isApproved('group-1')).toBe(false); @@ -101,14 +109,14 @@ describe('PairingStore workspace scoping (#7017)', () => { 'Bob', ); - expect(first).toBeTruthy(); - expect(second).toBe(first); + expect(first).toEqual({ code: expect.any(String) }); + expect(second).toEqual(first); expect(store.listPending()).toEqual([ expect.objectContaining({ senderId: 'alice', senderName: 'Alice', subject: { type: 'group', id: 'group-1', name: 'Release Team' }, - code: first, + code: codeOf(first), }), ]); }); @@ -122,41 +130,48 @@ describe('PairingStore workspace scoping (#7017)', () => { 'alice', 'Alice', ); - expect(first).toBeTruthy(); + expect(first).toEqual({ code: expect.any(String) }); // Same sender, different subjects: no additional slots. expect( store.createGroupRequest('group-2', 'Platform Team', 'alice', 'Alice'), - ).toBeNull(); - expect(store.createRequest('alice', 'Alice')).toBeNull(); + ).toEqual({ rejected: 'sender_pending' }); + expect(store.createRequest('alice', 'Alice')).toEqual({ + rejected: 'sender_pending', + }); // Re-mentioning the same subject still returns its existing code. expect( store.createGroupRequest('group-1', 'Release Team', 'alice', 'Alice'), - ).toBe(first); + ).toEqual(first); // Other senders are unaffected until the shared cap (3) is reached. expect( store.createGroupRequest('group-2', 'Platform Team', 'bob', 'Bob'), - ).toBeTruthy(); - expect(store.createRequest('carol', 'Carol')).toBeTruthy(); - expect(store.createRequest('dave', 'Dave')).toBeNull(); + ).toEqual({ code: expect.any(String) }); + expect(store.createRequest('carol', 'Carol')).toEqual({ + code: expect.any(String), + }); + expect(store.createRequest('dave', 'Dave')).toEqual({ + rejected: 'cap_reached', + }); }); it('frees the sender slot once their pending request is approved', () => { const store = new PairingStore('support-bot', workspaceA); - const code = store.createGroupRequest( - 'group-1', - 'Release Team', - 'alice', - 'Alice', - )!; + const code = codeOf( + store.createGroupRequest('group-1', 'Release Team', 'alice', 'Alice'), + ); - expect(store.createRequest('alice', 'Alice')).toBeNull(); + expect(store.createRequest('alice', 'Alice')).toEqual({ + rejected: 'sender_pending', + }); store.approve(code); - expect(store.createRequest('alice', 'Alice')).toBeTruthy(); + expect(store.createRequest('alice', 'Alice')).toEqual({ + code: expect.any(String), + }); }); it('isolates group approvals and revocation by workspace', () => { @@ -164,13 +179,15 @@ describe('PairingStore workspace scoping (#7017)', () => { const storeB = new PairingStore('support-bot', workspaceB); for (const store of [storeA, storeB]) { - const code = store.createGroupRequest( - 'group-1', - 'Release Team', - 'sender-1', - 'Alice', + const code = codeOf( + store.createGroupRequest( + 'group-1', + 'Release Team', + 'sender-1', + 'Alice', + ), ); - store.approve(code!); + store.approve(code); } expect(storeA.revokeGroup('group-1')).toBe(true); @@ -181,14 +198,16 @@ describe('PairingStore workspace scoping (#7017)', () => { it('keeps group approvals separate from similarly named channel user approvals', () => { const groupStore = new PairingStore('support', workspaceA); const userStore = new PairingStore('support-group', workspaceA); - const code = groupStore.createGroupRequest( - 'shared-id', - 'Release Team', - 'sender-1', - 'Alice', + const code = codeOf( + groupStore.createGroupRequest( + 'shared-id', + 'Release Team', + 'sender-1', + 'Alice', + ), ); - groupStore.approve(code!); + groupStore.approve(code); expect(groupStore.isGroupApproved('shared-id')).toBe(true); expect(userStore.isApproved('shared-id')).toBe(false); @@ -201,7 +220,7 @@ describe('PairingStore workspace scoping (#7017)', () => { const storeB = new PairingStore('support-bot', workspaceB); for (const store of [storeA, storeB]) { - const code = store.createRequest('sender-1', 'Sender One')!; + const code = codeOf(store.createRequest('sender-1', 'Sender One')); store.approve(code); } @@ -215,7 +234,7 @@ describe('PairingStore workspace scoping (#7017)', () => { // `../support` climbs out of the scope directory and both workspaces // share one file at the channels root — silently undoing the isolation. const storeA = new PairingStore('../support', workspaceA); - const code = storeA.createRequest('mallory', 'Mallory')!; + const code = codeOf(storeA.createRequest('mallory', 'Mallory')); storeA.approve(code); const storeB = new PairingStore('../support', workspaceB); @@ -235,8 +254,8 @@ describe('PairingStore workspace scoping (#7017)', () => { path.join(workspaceA, 'sub', '..'), ); - const code = store.createRequest('sender-1', 'Sender One'); - expect(code).toBeTruthy(); + const result = store.createRequest('sender-1', 'Sender One'); + expect(result).toEqual({ code: expect.any(String) }); expect(sameViaRelativeHop.listPending()).toHaveLength(1); }); @@ -258,7 +277,7 @@ describe('PairingStore workspace scoping (#7017)', () => { it('keeps the legacy global layout when no workspace is given', () => { const store = new PairingStore('support-bot'); - const code = store.createRequest('sender-1', 'Sender One')!; + const code = codeOf(store.createRequest('sender-1', 'Sender One')); store.approve(code); expect( @@ -339,7 +358,7 @@ describe('PairingStore workspace scoping (#7017)', () => { const storeA = new PairingStore('support-bot', workspaceA); const storeB = new PairingStore('support-bot', workspaceB); - const code = storeA.createRequest('new-sender', 'New Sender')!; + const code = codeOf(storeA.createRequest('new-sender', 'New Sender')); storeA.approve(code); expect(storeA.isApproved('new-sender')).toBe(true); @@ -441,8 +460,8 @@ describe('PairingStore workspace scoping (#7017)', () => { }); const store = new PairingStore('support-bot', workspaceA); expect(store.isApproved('anyone')).toBe(false); - const code = store.createRequest('new-sender', 'New')!; - expect(typeof code).toBe('string'); + const result = store.createRequest('new-sender', 'New'); + expect(result).toEqual({ code: expect.any(String) }); }); it('migrates every channel of a workspace, not only the first one constructed', () => { @@ -546,7 +565,7 @@ describe('PairingStore workspace scoping (#7017)', () => { it('never overwrites existing scoped state with legacy content', () => { const store = new PairingStore('support-bot', workspaceA); - const code = store.createRequest('scoped-sender', 'Scoped')!; + const code = codeOf(store.createRequest('scoped-sender', 'Scoped')); store.approve(code); seedLegacy(); diff --git a/packages/channels/base/src/PairingStore.ts b/packages/channels/base/src/PairingStore.ts index 519a8089a3f..692f9cd32a4 100644 --- a/packages/channels/base/src/PairingStore.ts +++ b/packages/channels/base/src/PairingStore.ts @@ -23,6 +23,10 @@ export interface PairingSubject { name: string; } +export type CreatePairingRequestResult = + | { code: string } + | { rejected: 'sender_pending' | 'cap_reached' }; + type StoredPairingRequest = Omit & { subject?: PairingSubject; }; @@ -177,11 +181,15 @@ export class PairingStore { /** * Create a pairing request for an unknown sender. - * Returns the code if created, or null if the pending cap is reached or the - * sender already holds a pending request for another subject. If the subject - * already has a non-expired pending request, returns that code. + * Returns the code if created; if the subject already has a non-expired + * pending request, returns that code. Rejects with `sender_pending` when + * the sender already holds a request for another subject, or + * `cap_reached` when the pending cap is reached. */ - createRequest(senderId: string, senderName: string): string | null { + createRequest( + senderId: string, + senderName: string, + ): CreatePairingRequestResult { return this.createSubjectRequest( { type: 'user', id: senderId, name: senderName }, senderId, @@ -194,7 +202,7 @@ export class PairingStore { groupName: string, senderId: string, senderName: string, - ): string | null { + ): CreatePairingRequestResult { return this.createSubjectRequest( { type: 'group', id: groupId, name: groupName }, senderId, @@ -206,7 +214,7 @@ export class PairingStore { subject: PairingSubject, senderId: string, senderName: string, - ): string | null { + ): CreatePairingRequestResult { const pending = this.readPending(); // Purge expired @@ -220,7 +228,7 @@ export class PairingStore { request.subject.id === subject.id, ); if (existing) { - return existing.code; + return { code: existing.code }; } // One sender may hold only one pending request at a time: subject-keyed @@ -228,18 +236,18 @@ export class PairingStore { // limit one sender could occupy every shared slot and block all other // pairing requests until expiry. if (active.some((request) => request.senderId === senderId)) { - return null; + return { rejected: 'sender_pending' }; } // Cap check if (active.length >= MAX_PENDING) { - return null; + return { rejected: 'cap_reached' }; } const code = generateCode(); active.push({ senderId, senderName, subject, code, createdAt: now }); this.writePending(active); - return code; + return { code }; } /** diff --git a/packages/channels/base/src/SenderGate.test.ts b/packages/channels/base/src/SenderGate.test.ts index 3666a8f11ed..fe962726199 100644 --- a/packages/channels/base/src/SenderGate.test.ts +++ b/packages/channels/base/src/SenderGate.test.ts @@ -5,7 +5,7 @@ import type { PairingStore } from './PairingStore.js'; function mockPairingStore(overrides: Partial = {}): PairingStore { return { isApproved: vi.fn().mockReturnValue(false), - createRequest: vi.fn().mockReturnValue('ABCD1234'), + createRequest: vi.fn().mockReturnValue({ code: 'ABCD1234' }), approve: vi.fn(), listPending: vi.fn().mockReturnValue([]), getAllowlist: vi.fn().mockReturnValue([]), @@ -36,7 +36,7 @@ describe('SenderGate', () => { const gate = new SenderGate('allowlist', ['alice']); const result = gate.check('eve'); expect(result.allowed).toBe(false); - expect(result.pairingCode).toBeUndefined(); + expect(result.pairing).toBeUndefined(); }); it('works with empty allowlist', () => { @@ -77,26 +77,36 @@ describe('SenderGate', () => { it('generates pairing code for unknown sender', () => { const store = mockPairingStore({ - createRequest: vi.fn().mockReturnValue('XYZW5678'), + createRequest: vi.fn().mockReturnValue({ code: 'XYZW5678' }), }); const gate = new SenderGate('pairing', [], store); const result = gate.check('stranger', 'Stranger Name'); expect(result.allowed).toBe(false); - expect(result.pairingCode).toBe('XYZW5678'); + expect(result.pairing).toEqual({ code: 'XYZW5678' }); expect(store.createRequest).toHaveBeenCalledWith( 'stranger', 'Stranger Name', ); }); - it('returns null pairingCode when cap reached', () => { + it('passes through the store rejection when the cap is reached', () => { const store = mockPairingStore({ - createRequest: vi.fn().mockReturnValue(null), + createRequest: vi.fn().mockReturnValue({ rejected: 'cap_reached' }), }); const gate = new SenderGate('pairing', [], store); const result = gate.check('stranger'); expect(result.allowed).toBe(false); - expect(result.pairingCode).toBeNull(); + expect(result.pairing).toEqual({ rejected: 'cap_reached' }); + }); + + it('passes through the rejection when the sender already holds a request', () => { + const store = mockPairingStore({ + createRequest: vi.fn().mockReturnValue({ rejected: 'sender_pending' }), + }); + const gate = new SenderGate('pairing', [], store); + const result = gate.check('stranger'); + expect(result.allowed).toBe(false); + expect(result.pairing).toEqual({ rejected: 'sender_pending' }); }); it('uses senderId as senderName fallback', () => { @@ -110,7 +120,7 @@ describe('SenderGate', () => { const gate = new SenderGate('pairing'); const result = gate.check('anyone'); expect(result.allowed).toBe(false); - expect(result.pairingCode).toBeNull(); + expect(result.pairing).toEqual({ rejected: 'cap_reached' }); }); it('passively checks pairing authorization without creating requests', () => { diff --git a/packages/channels/base/src/SenderGate.ts b/packages/channels/base/src/SenderGate.ts index a7df973eebc..7287de53560 100644 --- a/packages/channels/base/src/SenderGate.ts +++ b/packages/channels/base/src/SenderGate.ts @@ -1,9 +1,13 @@ import type { SenderPolicy } from './types.js'; -import type { PairingStore } from './PairingStore.js'; +import type { + CreatePairingRequestResult, + PairingStore, +} from './PairingStore.js'; export interface SenderCheckResult { allowed: boolean; - pairingCode?: string | null; // set when pairing policy returns a code (null = cap reached) + /** Set when the pairing policy denies the sender. */ + pairing?: CreatePairingRequestResult; } export class SenderGate { @@ -57,11 +61,14 @@ export class SenderGate { return { allowed: true }; } // Generate pairing code - const code = this.pairingStore?.createRequest( + const result = this.pairingStore?.createRequest( senderId, senderName || senderId, ); - return { allowed: false, pairingCode: code ?? null }; + return { + allowed: false, + pairing: result ?? { rejected: 'cap_reached' }, + }; } default: throw new Error(`Unknown sender policy: ${this.policy}`); diff --git a/packages/channels/base/src/index.ts b/packages/channels/base/src/index.ts index 9062d5652e8..ec739653b82 100644 --- a/packages/channels/base/src/index.ts +++ b/packages/channels/base/src/index.ts @@ -72,7 +72,11 @@ export type { ChannelLoopStoreOptions, } from './ChannelLoopStore.js'; export { PairingStore } from './PairingStore.js'; -export type { PairingRequest, PairingSubject } from './PairingStore.js'; +export type { + CreatePairingRequestResult, + PairingRequest, + PairingSubject, +} from './PairingStore.js'; export { GroupGate } from './GroupGate.js'; export type { GroupCheckResult } from './GroupGate.js'; export { DmGate } from './DmGate.js'; diff --git a/packages/channels/github/src/index.ts b/packages/channels/github/src/index.ts index d7137ff03e2..5d3f62e9ce0 100644 --- a/packages/channels/github/src/index.ts +++ b/packages/channels/github/src/index.ts @@ -32,7 +32,8 @@ export const plugin: ChannelPlugin = { label: 'Group Policy', kind: 'enum', required: true, - description: 'Must be "Open" for notifications to flow', + description: + 'Must be "Open", "Allowlist", or "Pairing" for notifications to flow', default: 'open', options: [ { value: 'open', label: 'Open' }, diff --git a/packages/cli/src/commands/channel/pairing.test.ts b/packages/cli/src/commands/channel/pairing.test.ts index 072c106781c..0a241c7921e 100644 --- a/packages/cli/src/commands/channel/pairing.test.ts +++ b/packages/cli/src/commands/channel/pairing.test.ts @@ -9,6 +9,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { PairingStore } from '@qwen-code/channel-base'; +import type { CreatePairingRequestResult } from '@qwen-code/channel-base'; import { pairingListCommand, pairingApproveCommand } from './pairing.js'; vi.mock('../../utils/stdioHelpers.js', () => ({ @@ -53,6 +54,13 @@ describe('channel pairing CLI (--cwd scoping)', () => { } }); + function codeOf(result: CreatePairingRequestResult): string { + if ('code' in result) return result.code; + throw new Error( + `expected a pairing code, got rejection "${result.rejected}"`, + ); + } + const stdoutText = () => vi .mocked(writeStdoutLine) @@ -93,7 +101,7 @@ describe('channel pairing CLI (--cwd scoping)', () => { it('approve acts on the --cwd workspace and leaves the other untouched', () => { const storeA = new PairingStore('support-bot', wsA); - const code = storeA.createRequest('user-alice', 'Alice')!; + const code = codeOf(storeA.createRequest('user-alice', 'Alice')); new PairingStore('support-bot', wsB).createRequest('user-bob', 'Bob'); pairingApproveCommand.handler!({ @@ -116,12 +124,14 @@ describe('channel pairing CLI (--cwd scoping)', () => { it('lists and approves a group request as a group', () => { const store = new PairingStore('support-bot', wsA); - const code = store.createGroupRequest( - 'group-1', - 'Release Team', - 'user-alice', - 'Alice', - )!; + const code = codeOf( + store.createGroupRequest( + 'group-1', + 'Release Team', + 'user-alice', + 'Alice', + ), + ); pairingListCommand.handler!({ name: 'support-bot', @@ -148,10 +158,9 @@ describe('channel pairing CLI (--cwd scoping)', () => { }); it('approve with a code from another workspace fails with the scoped error', () => { - const codeB = new PairingStore('support-bot', wsB).createRequest( - 'user-bob', - 'Bob', - )!; + const codeB = codeOf( + new PairingStore('support-bot', wsB).createRequest('user-bob', 'Bob'), + ); const exitSpy = vi .spyOn(process, 'exit') .mockImplementation((() => undefined) as unknown as typeof process.exit); diff --git a/packages/cli/src/serve/channel-management-service.test.ts b/packages/cli/src/serve/channel-management-service.test.ts index c72476d36da..5986aa1c2dd 100644 --- a/packages/cli/src/serve/channel-management-service.test.ts +++ b/packages/cli/src/serve/channel-management-service.test.ts @@ -8,6 +8,7 @@ import { promises as fs } from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { PairingStore } from '@qwen-code/channel-base'; +import type { CreatePairingRequestResult } from '@qwen-code/channel-base'; import { describe, expect, it, vi } from 'vitest'; import type { ChannelSettingsSnapshot } from './channel-settings-store.js'; import { @@ -138,6 +139,13 @@ function setup(options: { return { service, store, manager, persisted: () => persisted }; } +function codeOf(result: CreatePairingRequestResult): string { + if ('code' in result) return result.code; + throw new Error( + `expected a pairing code, got rejection "${result.rejected}"`, + ); +} + describe('createChannelManagementService', () => { it('lists sanitized config, secret presence, startup selection, and runtime', async () => { const { service } = setup({ committedNames: ['bot'] }); @@ -338,8 +346,9 @@ describe('createChannelManagementService', () => { }), }); const pairing = new PairingStore('bot', WORKSPACE); - const code = pairing.createRequest('sender-1', 'Alice'); - expect(code).toBeTypeOf('string'); + const created = pairing.createRequest('sender-1', 'Alice'); + expect(created).toEqual({ code: expect.any(String) }); + const code = codeOf(created); await expect(service.pairingRequests('bot')).resolves.toEqual({ requests: [ @@ -350,7 +359,7 @@ describe('createChannelManagementService', () => { }), ], }); - await expect(service.approvePairing('bot', code!)).resolves.toEqual({ + await expect(service.approvePairing('bot', code)).resolves.toEqual({ approved: expect.objectContaining({ senderId: 'sender-1', code }), requests: [], }); @@ -404,17 +413,21 @@ describe('createChannelManagementService', () => { }), }); const pairing = new PairingStore('bot', WORKSPACE); - const code = pairing.createGroupRequest( - 'group-1', - 'Release Team', - 'sender-1', - 'Alice', + const code = codeOf( + pairing.createGroupRequest( + 'group-1', + 'Release Team', + 'sender-1', + 'Alice', + ), ); - const secondCode = pairing.createGroupRequest( - 'group-2', - 'Platform Team', - 'sender-2', - 'Bob', + const secondCode = codeOf( + pairing.createGroupRequest( + 'group-2', + 'Platform Team', + 'sender-2', + 'Bob', + ), ); await expect(service.pairingRequests('bot')).resolves.toEqual({ @@ -437,7 +450,7 @@ describe('createChannelManagementService', () => { }), ], }); - await expect(service.approvePairing('bot', code!)).resolves.toEqual({ + await expect(service.approvePairing('bot', code)).resolves.toEqual({ approved: expect.objectContaining({ subject: { type: 'group', id: 'group-1', name: 'Release Team' }, }), @@ -447,7 +460,7 @@ describe('createChannelManagementService', () => { }), ], }); - await service.approvePairing('bot', secondCode!); + await service.approvePairing('bot', secondCode); await expect(service.pairingApprovals('bot')).resolves.toEqual({ senderIds: [], groupIds: ['group-1', 'group-2'], diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 0a34fce424b..5c41d40200b 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -2490,7 +2490,7 @@ const EN: Messages = { 'GitHub Enterprise API root (e.g. https://ghe.example.com/api/v3). Leave empty for github.com', 'channels.editor.field.github.groupPolicy': 'Group Policy', 'channels.editor.field.github.groupPolicy.description': - 'Must be "Open" for notifications to flow', + 'Must be "Open", "Allowlist", or "Pairing" for notifications to flow', 'channels.editor.field.github.senderPolicy': 'Sender Policy', 'channels.editor.field.github.senderPolicy.description': 'Use "Allowlist" with allowed users on public repos', @@ -2509,7 +2509,7 @@ const EN: Messages = { 'Self-hosted instance URL (e.g. https://gitlab.example.com). Leave empty for gitlab.com', 'channels.editor.field.gitlab.groupPolicy': 'Group Policy', 'channels.editor.field.gitlab.groupPolicy.description': - 'Must be "Open" or "Allowlist" for todos to be processed', + 'Must be "Open", "Allowlist", or "Pairing" for todos to be processed', 'channels.editor.field.gitlab.senderPolicy': 'Sender Policy', 'channels.editor.field.gitlab.senderPolicy.description': 'Use "Allowlist" with allowed users on public projects', @@ -5048,7 +5048,7 @@ const ZH: Messages = { 'GitHub Enterprise API 根地址（如 https://ghe.example.com/api/v3），github.com 留空', 'channels.editor.field.github.groupPolicy': '群组策略', 'channels.editor.field.github.groupPolicy.description': - '必须设为 "Open" 才能接收通知', + '必须设为 "Open"、"Allowlist" 或 "Pairing" 才能接收通知', 'channels.editor.field.github.senderPolicy': '发送者策略', 'channels.editor.field.github.senderPolicy.description': '公开仓库建议使用 "Allowlist" 并指定允许的用户', @@ -5066,7 +5066,7 @@ const ZH: Messages = { '自托管实例地址（如 https://gitlab.example.com），gitlab.com 留空', 'channels.editor.field.gitlab.groupPolicy': '群组策略', 'channels.editor.field.gitlab.groupPolicy.description': - '必须设为 "Open" 或 "Allowlist" 才能处理 Todo', + '必须设为 "Open"、"Allowlist" 或 "Pairing" 才能处理 Todo', 'channels.editor.field.gitlab.senderPolicy': '发送者策略', 'channels.editor.field.gitlab.senderPolicy.description': '公开项目建议使用 "Allowlist" 并指定允许的用户', From ae4fbb2f5431c624f59024d41a1353ef9db79b8c Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 21:29:11 +0000 Subject: [PATCH 09/10] fix(channels): address group pairing review findings round 8 (#8440) Co-authored-by: Qwen-Coder --- docs/users/features/channels/github.md | 24 ++-- docs/users/features/channels/gitlab.md | 2 +- docs/users/features/channels/overview.md | 2 +- docs/users/features/channels/plugins.md | 2 +- docs/users/features/channels/wecom.md | 2 +- .../channels/base/src/ChannelBase.test.ts | 41 ++++++ packages/channels/base/src/GroupGate.ts | 2 +- .../channels/github/src/GithubAdapter.test.ts | 123 ++++++++++++++++++ packages/channels/github/src/GithubAdapter.ts | 21 ++- .../commands/channel/channel-registry.test.ts | 5 + 10 files changed, 205 insertions(+), 19 deletions(-) diff --git a/docs/users/features/channels/github.md b/docs/users/features/channels/github.md index f5503ca3160..f1405f1e347 100644 --- a/docs/users/features/channels/github.md +++ b/docs/users/features/channels/github.md @@ -78,17 +78,17 @@ Local `gh` authentication requires an HTTPS `baseUrl` so the daemon host credent ## Configuration Options -| Option | Default | Description | -| ------------------------- | ------------------------ | --------------------------------------------------------------------------------------------- | -| `token` | unset | Optional classic PAT with `notifications` scope; overrides local `gh` authentication | -| `useLocalGh` | `false` | Explicitly reuse the daemon host's account-wide GitHub CLI authentication | -| `pollInterval` | `60000` | Poll interval in ms | -| `baseUrl` | `https://api.github.com` | API base URL (for GHE) | -| `groupPolicy` | `"disabled"` | Must be `"open"`, `"allowlist"`, or `"pairing"` for notifications to flow | -| `senderPolicy` | `"allowlist"` | Who can trigger the bot | -| `groups.*.requireMention` | `true` | Require @mentions for ordinary comments; directed notification reasons still run | -| `blockStreaming` | `"off"` | Always forced to `"off"`; intermediate model chunks aren't published; `"on"` is not supported | -| `reasonFilter` | unset | Optional allowlist of GitHub notification reasons to process | +| Option | Default | Description | +| ------------------------- | ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------ | +| `token` | unset | Optional classic PAT with `notifications` scope; overrides local `gh` authentication | +| `useLocalGh` | `false` | Explicitly reuse the daemon host's account-wide GitHub CLI authentication | +| `pollInterval` | `60000` | Poll interval in ms | +| `baseUrl` | `https://api.github.com` | API base URL (for GHE) | +| `groupPolicy` | `"disabled"` | Must be `"open"`, `"allowlist"` with the repo (`owner/repo`) listed in `groups`, or `"pairing"` with the repo approved for notifications to flow | +| `senderPolicy` | `"allowlist"` | Who can trigger the bot | +| `groups.*.requireMention` | `true` | Require @mentions for ordinary comments; directed notification reasons still run | +| `blockStreaming` | `"off"` | Always forced to `"off"`; intermediate model chunks aren't published; `"on"` is not supported | +| `reasonFilter` | unset | Optional allowlist of GitHub notification reasons to process | Use `reasonFilter` to drop noisy notification classes such as `ci_activity` or `state_change`. Do not use `reasonFilter: ["mention"]` as a replacement for `groups.*.requireMention`: GitHub's `mention` reason is sticky at the thread level, so real new @mentions can arrive later under `comment`, `subscribed`, `author`, or other reasons and would be skipped. @@ -104,6 +104,8 @@ Always use `senderPolicy: "allowlist"` with explicit `allowedUsers` on public re Allowlist and pairing entries follow the **username**, not the immutable account ID. If an allowlisted user renames their GitHub account, remove the stale entry — GitHub releases the old username for anyone else to claim, and the new holder would inherit the allowlist/pairing authorization. +Note that under `groupPolicy: "pairing"`, access is granted per repository: once a repository is approved, **any GitHub user** can drive the bot through that repository's issues and pull requests. All GitHub traffic is group traffic, so `senderPolicy` and `allowedUsers` do not gate members of an approved repository. + ## Mention Detection The adapter detects mentions by scanning comment text and first-contact issue or PR bodies for `@bot-username` using a case-insensitive regex. It does not trust `reason: "mention"` alone because that value is sticky at the thread level. Other reasons select review, triage, followed-thread, or fallback prompts. diff --git a/docs/users/features/channels/gitlab.md b/docs/users/features/channels/gitlab.md index f73176939e6..4149290d9cd 100644 --- a/docs/users/features/channels/gitlab.md +++ b/docs/users/features/channels/gitlab.md @@ -132,7 +132,7 @@ On a **public project**, setting `senderPolicy: "open"` allows **any GitLab user Always use `senderPolicy: "allowlist"` with explicit `allowedUsers` on public projects. -Note that under `groupPolicy: "pairing"`, access is granted per project: once a project is approved, **any GitLab user** can drive the bot through that project's issues and merge requests. All GitLab traffic is group traffic, so `senderPolicy` and `allowedUsers` do not gate members of an approved project. +Note that under `groupPolicy: "pairing"`, access is granted per project: once a project is approved, **any GitLab user** can drive the bot through that project's issues and merge requests. All GitLab traffic is group traffic, so `senderPolicy` and `allowedUsers` do not gate members of an approved project. Approvals are keyed by the project path (`owner/repo`), which changes on rename or transfer — revoke stale group approvals after any project rename, transfer, or deletion. ## Mention Detection diff --git a/docs/users/features/channels/overview.md b/docs/users/features/channels/overview.md index 86f6252dcef..b7cd986a709 100644 --- a/docs/users/features/channels/overview.md +++ b/docs/users/features/channels/overview.md @@ -224,7 +224,7 @@ request identifies the group and the member who initiated it: qwen channel pairing approve my-channel ``` -Group approvals are stored by stable chat ID in the channel's workspace scope. +Group approvals are stored by the group's chat ID in the channel's workspace scope. On GitHub and GitLab the chat ID is the repository/project path, so a rename or transfer detaches the stored approval — re-approve the group after renaming. An unmentioned message never creates a group pairing request, even when a group sets `requireMention` to `false`; after approval, the configured mention policy applies normally. diff --git a/docs/users/features/channels/plugins.md b/docs/users/features/channels/plugins.md index d6ef02437ea..ae108cd5eb9 100644 --- a/docs/users/features/channels/plugins.md +++ b/docs/users/features/channels/plugins.md @@ -77,7 +77,7 @@ Custom channels automatically support everything built-in channels do: - **Sender policies** — `allowlist`, `pairing`, and `open` access control - **Group policies** — Per-group settings with optional @mention gating - **Session routing** — Per-user, per-thread, or single shared sessions -- **DM pairing** — Full pairing code flow for unknown users +- **DM and group pairing** — Full pairing code flow for unknown users and groups - **Slash commands** — `/help`, `/clear`, `/status` work out of the box - **Custom instructions** — Prepended to the first message in each session - **Crash recovery** — Automatic restart with session preservation diff --git a/docs/users/features/channels/wecom.md b/docs/users/features/channels/wecom.md index 09ab5726f08..e075b7a0673 100644 --- a/docs/users/features/channels/wecom.md +++ b/docs/users/features/channels/wecom.md @@ -79,7 +79,7 @@ Open WeCom and send a message to the intelligent robot. - `pairing`: users must pair before using the bot. - `open`: anyone who can message the robot can use it. -For groups, set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"`. Under `"pairing"`, the group's first mention creates a pairing request that must be approved once before responses start. WeCom only delivers group messages that mention the intelligent robot, so every delivered group callback is treated as mentioned. The `requireMention` setting cannot enable responses to unmentioned group messages because those messages are not delivered to the bot. +For groups, set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"`. Under `"pairing"`, the group's first mention creates a pairing request that must be approved once before responses start. Note that under `groupPolicy: "pairing"`, access is granted per group: once a group is approved, **any member of that group** can use the bot; `senderPolicy` and `allowedUsers` do not gate members of an approved group. WeCom only delivers group messages that mention the intelligent robot, so every delivered group callback is treated as mentioned. The `requireMention` setting cannot enable responses to unmentioned group messages because those messages are not delivered to the bot. ### Group Mention Compatibility diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index 768b64d3091..04452b75078 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -13608,6 +13608,47 @@ describe('ChannelBase', () => { } }); + it('lets an approved paired group talk without mentions when requireMention is false', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const store = new PairingStore('test-chan', '/tmp'); + const created = store.createGroupRequest( + 'chat1', + 'Release Team', + 'alice', + 'Alice', + ); + store.approve(pairingCodeOf(created)); + const ch = createChannel({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + groups: { '*': { requireMention: false } }, + }); + + await ch.handleInbound( + envelope({ + isGroup: true, + chatId: 'chat1', + senderId: 'bob', + senderName: 'Bob', + text: 'ambient message', + }), + ); + + expect(bridge.prompt).toHaveBeenCalledOnce(); + expect( + ch.sent.some((message) => message.text.includes('pairing code')), + ).toBe(false); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + it('tells a group when the pending pairing cap is reached', async () => { const previousQwenHome = process.env['QWEN_HOME']; const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); diff --git a/packages/channels/base/src/GroupGate.ts b/packages/channels/base/src/GroupGate.ts index d30d640e8e0..a8662190c1f 100644 --- a/packages/channels/base/src/GroupGate.ts +++ b/packages/channels/base/src/GroupGate.ts @@ -12,7 +12,7 @@ export interface GroupCheckResult { | 'mention_required' | 'pairing_trigger_required' | 'pairing_required'; - /** Set when the pairing policy denies the group. */ + /** Set when the check denies with `pairing_required` (a pairing request was created or reused). */ pairing?: CreatePairingRequestResult; } diff --git a/packages/channels/github/src/GithubAdapter.test.ts b/packages/channels/github/src/GithubAdapter.test.ts index e6ac4e281a5..1214ce30101 100644 --- a/packages/channels/github/src/GithubAdapter.test.ts +++ b/packages/channels/github/src/GithubAdapter.test.ts @@ -21,6 +21,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { getWorkspaceScopeDirName, + PairingStore, type ChannelAgentBridge, type ChannelConfig, type Envelope, @@ -1799,6 +1800,128 @@ describe('GithubChannel', () => { expect(mockOctokit.rest.issues.createComment).not.toHaveBeenCalled(); }); + it('dispatches directed follow-ups from an approved paired repo on the aggregate lane', async () => { + await initWithoutLoop({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }); + channel.usePreflight = true; + const store = new PairingStore('test-github', '/tmp/test'); + const created = store.createGroupRequest( + 'owner/repo', + 'owner/repo', + 'alice', + 'Alice', + ); + if (!('code' in created)) { + throw new Error(`expected a pairing code, got ${created.rejected}`); + } + store.approve(created.code); + mockOctokit.paginate + .mockResolvedValueOnce([ + makeNotification({ + reason: 'comment', + last_read_at: '2026-07-01T12:00:00.000Z', + }), + ]) + .mockResolvedValueOnce([ + makeComment({ body: 'please take a look' }), + makeComment({ + id: 1002, + body: 'second opinion', + user: { login: 'bob' }, + }), + ]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(2); + expect(channel.inboundEnvelopes[0]).toMatchObject({ + senderId: 'alice', + text: 'please take a look', + isMentioned: true, + }); + expect(channel.inboundEnvelopes[1]).toMatchObject({ + senderId: 'bob', + text: 'second opinion', + isMentioned: true, + }); + expect(mockOctokit.rest.issues.createComment).not.toHaveBeenCalled(); + }); + + it('does not feed the issue body after a mentioning comment from an approved paired repo', async () => { + await initWithoutLoop({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }); + channel.usePreflight = true; + const store = new PairingStore('test-github', '/tmp/test'); + const created = store.createGroupRequest( + 'owner/repo', + 'owner/repo', + 'alice', + 'Alice', + ); + if (!('code' in created)) { + throw new Error(`expected a pairing code, got ${created.rejected}`); + } + store.approve(created.code); + mockOctokit.paginate + .mockResolvedValueOnce([ + makeNotification({ reason: 'mention', last_read_at: null }), + ]) + .mockResolvedValueOnce([makeComment()]); + mockOctokit.rest.issues.get.mockResolvedValue({ + data: { + title: 'Test Issue', + body: '@test-bot the issue body mentions the bot too', + user: { login: 'alice' }, + }, + }); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(1); + expect(channel.inboundEnvelopes[0]).toMatchObject({ + messageId: '1001', + senderId: 'alice', + }); + expect(mockOctokit.rest.issues.get).not.toHaveBeenCalled(); + }); + + it('posts one pairing comment when a mentioning comment and body arrive together', async () => { + await initWithoutLoop({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }); + channel.usePreflight = true; + mockOctokit.paginate + .mockResolvedValueOnce([ + makeNotification({ reason: 'mention', last_read_at: null }), + ]) + .mockResolvedValueOnce([makeComment()]); + mockOctokit.rest.issues.get.mockResolvedValue({ + data: { + title: 'Test Issue', + body: '@test-bot the issue body mentions the bot too', + user: { login: 'alice' }, + }, + }); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(0); + expect(mockOctokit.rest.issues.createComment).toHaveBeenCalledTimes(1); + expect(mockOctokit.rest.issues.createComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining('pairing code'), + }), + ); + }); + it('bounds each aggregated comment without hiding later comments', async () => { await initWithoutLoop(); mockOctokit.paginate diff --git a/packages/channels/github/src/GithubAdapter.ts b/packages/channels/github/src/GithubAdapter.ts index 938b64059ce..87a7566007b 100644 --- a/packages/channels/github/src/GithubAdapter.ts +++ b/packages/channels/github/src/GithubAdapter.ts @@ -1314,7 +1314,13 @@ export class GithubChannel extends PollingChannelBase { if (onlyMentioned && !hasMention) continue; const senderId = (comment.user?.login || 'unknown').toLowerCase(); - const allowed = this.gate.isAllowed(senderId); + // Approved paired groups bypass the sender gate in preflight, so the + // directed lane must mirror that or follow-ups fail mention gating. + const allowed = + this.gate.isAllowed(senderId) || + (directed && + this.config.groupPolicy === 'pairing' && + this.groupGate.isGroupApproved(ctx.chatId)); const envelope: Envelope = { channelName: this.name, senderId, @@ -1339,7 +1345,13 @@ export class GithubChannel extends PollingChannelBase { } if (allowed) { this.recordDispatchedComment(key); - if (hasMention) dispatched = true; + } + // A mention under group pairing has a visible effect (dispatch or a + // pairing code comment) even when the sender gate rejects the sender; + // suppress the first-contact body feed so the same intent cannot be + // dispatched twice. + if (hasMention && (allowed || this.config.groupPolicy === 'pairing')) { + dispatched = true; } } @@ -1389,7 +1401,10 @@ export class GithubChannel extends PollingChannelBase { } private async processAggregateLane(ctx: NotificationContext): Promise { - if (this.config.senderPolicy === 'pairing') { + if ( + this.config.senderPolicy === 'pairing' || + this.config.groupPolicy === 'pairing' + ) { await this.processCommentLane(ctx, false, true); return; } diff --git a/packages/cli/src/commands/channel/channel-registry.test.ts b/packages/cli/src/commands/channel/channel-registry.test.ts index cf61b763d1e..439f8d7f458 100644 --- a/packages/cli/src/commands/channel/channel-registry.test.ts +++ b/packages/cli/src/commands/channel/channel-registry.test.ts @@ -35,6 +35,11 @@ describe('channel registry', () => { required: true, }), ); + expect( + fields + ?.find((field) => field.key === 'groupPolicy') + ?.options?.map((option) => option.value), + ).toContain('pairing'); expect(fields).toContainEqual( expect.objectContaining({ key: 'senderPolicy', From 286150b7eded5b2cac662ab754f803505a6e7fba Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 6 Aug 2026 04:09:57 +0000 Subject: [PATCH 10/10] fix(channels): address group pairing review findings round 9 (#8440) --- docs/users/features/channels/github.md | 2 +- docs/users/features/channels/overview.md | 4 +- docs/users/features/channels/qqbot.md | 2 +- docs/users/features/channels/telegram.md | 2 +- .../channels/base/src/ChannelBase.test.ts | 111 ++++++++++++++++-- packages/channels/base/src/ChannelBase.ts | 21 +++- .../channels/base/src/PairingStore.test.ts | 91 +++++++++++++- packages/channels/base/src/PairingStore.ts | 47 ++++++-- .../channels/github/src/GithubAdapter.test.ts | 107 +++++++++++++++++ packages/channels/github/src/GithubAdapter.ts | 15 ++- .../test/unit/DaemonClient.test.ts | 16 +-- 11 files changed, 378 insertions(+), 40 deletions(-) diff --git a/docs/users/features/channels/github.md b/docs/users/features/channels/github.md index f1405f1e347..c3e1bc7d464 100644 --- a/docs/users/features/channels/github.md +++ b/docs/users/features/channels/github.md @@ -104,7 +104,7 @@ Always use `senderPolicy: "allowlist"` with explicit `allowedUsers` on public re Allowlist and pairing entries follow the **username**, not the immutable account ID. If an allowlisted user renames their GitHub account, remove the stale entry — GitHub releases the old username for anyone else to claim, and the new holder would inherit the allowlist/pairing authorization. -Note that under `groupPolicy: "pairing"`, access is granted per repository: once a repository is approved, **any GitHub user** can drive the bot through that repository's issues and pull requests. All GitHub traffic is group traffic, so `senderPolicy` and `allowedUsers` do not gate members of an approved repository. +Note that under `groupPolicy: "pairing"`, access is granted per repository: once a repository is approved, **any GitHub user** can drive the bot through that repository's issues and pull requests. All GitHub traffic is group traffic, so `senderPolicy` and `allowedUsers` do not gate members of an approved repository. Approvals are keyed by the repository full name (`owner/repo`), which changes on rename or transfer — revoke stale group approvals after any repository rename, transfer, or deletion. ## Mention Detection diff --git a/docs/users/features/channels/overview.md b/docs/users/features/channels/overview.md index b7cd986a709..56a03a50f98 100644 --- a/docs/users/features/channels/overview.md +++ b/docs/users/features/channels/overview.md @@ -201,7 +201,7 @@ Run these from the channel's workspace directory (or pass `--cwd `) — pai - Codes are 8 characters, uppercase, using an unambiguous alphabet (no `0`/`O`/`1`/`I`) - Codes expire after 1 hour - Maximum 3 pending requests per channel at a time, and at most one per sender — additional requests are declined until one expires or is approved -- Users listed in `allowedUsers` in `settings.json` always skip pairing +- Users listed in `allowedUsers` in `settings.json` skip user pairing; under `groupPolicy: "pairing"`, the group itself must still be approved - Approved users are stored per workspace in `~/.qwen/channels//-allowlist.json` — treat this file as sensitive ## Group Chats @@ -224,7 +224,7 @@ request identifies the group and the member who initiated it: qwen channel pairing approve my-channel ``` -Group approvals are stored by the group's chat ID in the channel's workspace scope. On GitHub and GitLab the chat ID is the repository/project path, so a rename or transfer detaches the stored approval — re-approve the group after renaming. +Group approvals are stored by the group's chat ID in the channel's workspace scope. On GitHub and GitLab the chat ID is the repository/project path, so a rename or transfer detaches the stored approval — re-approve the group after renaming. A repo or project re-created under the same path inherits any stale approval — revoke group approvals after any rename, transfer, or deletion. An unmentioned message never creates a group pairing request, even when a group sets `requireMention` to `false`; after approval, the configured mention policy applies normally. diff --git a/docs/users/features/channels/qqbot.md b/docs/users/features/channels/qqbot.md index 1456d4aaf4d..904b4b7c19f 100644 --- a/docs/users/features/channels/qqbot.md +++ b/docs/users/features/channels/qqbot.md @@ -103,7 +103,7 @@ To use the bot in QQ groups: 1. Set `groupPolicy` to `"allowlist"`, `"pairing"`, or `"open"` in your channel config 2. Add the bot to a QQ group via the QQ Bot Open Platform dashboard or by having a group admin invite it 3. Group members must **@mention** the bot to trigger a response -4. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start +4. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start. Note that once a group is approved, **any member of that group** can use the bot; `senderPolicy` and `allowedUsers` do not gate members of an approved group. QQ Bot API V2 only delivers group messages that @mention the bot — the bot does not see all group messages. By default, `requireMention` is `true` and should be left that way for QQ. diff --git a/docs/users/features/channels/telegram.md b/docs/users/features/channels/telegram.md index 563fbc7aa3a..e35f9fa982b 100644 --- a/docs/users/features/channels/telegram.md +++ b/docs/users/features/channels/telegram.md @@ -74,7 +74,7 @@ To use the bot in Telegram groups: 2. **Disable privacy mode** in BotFather: `/mybots` → select your bot → Bot Settings → Group Privacy → Turn Off 3. Add the bot to a group. If it was already in the group, **remove and re-add it** (Telegram caches privacy settings from when the bot joined) 4. If using `groupPolicy: "allowlist"`, add the group's chat ID to `groups` in your config -5. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start +5. If using `groupPolicy: "pairing"`, approve the group's pairing request once before responses start. Note that once a group is approved, **any member of that group** can use the bot; `senderPolicy` and `allowedUsers` do not gate members of an approved group. By default, the bot requires an @mention or a reply to respond in groups. Set `"requireMention": false` for a specific group to make it respond to all messages (useful for dedicated task groups). See [Group Chats](./overview#group-chats) for full details. diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index 04452b75078..2ac638d101a 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -2782,6 +2782,33 @@ describe('ChannelBase', () => { .calls[0][1] as string; expect(prompt).not.toContain('- [Bob] background'); expect(prompt).toContain('[Carol] @bot summarize'); + + // Re-approve and mention again: the history recorded before the + // revocation must stay discarded. This pins that the revocation-time + // drain actually removed the entries from disk — a check-before-drain + // ordering would leave them behind and surface them here. + const recreated = store.createGroupRequest( + 'chat1', + 'Release Team', + 'dave', + 'Dave', + ); + store.approve(pairingCodeOf(recreated)); + + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + senderId: 'carol', + senderName: 'Carol', + text: '@bot follow-up', + }), + ); + + const secondPrompt = (bridge.prompt as ReturnType).mock + .calls[1][1] as string; + expect(secondPrompt).not.toContain('- [Bob] background'); + expect(secondPrompt).toContain('[Carol] @bot follow-up'); } finally { if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; else process.env['QWEN_HOME'] = previousQwenHome; @@ -2921,14 +2948,28 @@ describe('ChannelBase', () => { expect(existsSync(historyPath)).toBe(false); - const store = new PairingStore('test-chan', '/tmp'); - const created = store.createGroupRequest( - 'group-1', - 'Release Team', - 'alice', - 'Alice', + // The pairing-trigger half is dropped without recording too: content + // that fails authorization at preflight must not reach the model + // prompt later through the group-history backfill path. + await ch.handleInbound( + envelope({ + isGroup: true, + chatId: 'group-1', + isMentioned: true, + senderId: 'dave', + senderName: 'Dave', + text: '@bot pair this group', + }), ); - store.approve(pairingCodeOf(created)); + + expect(existsSync(historyPath)).toBe(false); + expect(ch.sent).toHaveLength(1); + expect(ch.sent[0]!.text).toContain('pairing code'); + + const store = new PairingStore('test-chan', '/tmp'); + const pending = store.listPending(); + expect(pending).toHaveLength(1); + store.approve(pending[0]!.code); await ch.handleInbound( envelope({ @@ -2944,6 +2985,7 @@ describe('ChannelBase', () => { const prompt = (bridge.prompt as ReturnType).mock .calls[0][1] as string; expect(prompt).not.toContain('pre-approval chatter'); + expect(prompt).not.toContain('pair this group'); expect(prompt).toContain('[Carol] @bot summarize'); } finally { if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; @@ -13608,6 +13650,50 @@ describe('ChannelBase', () => { } }); + it('posts one pairing notification when multiple mentions trigger the same group request', async () => { + const previousQwenHome = process.env['QWEN_HOME']; + const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); + process.env['QWEN_HOME'] = qwenHome; + try { + const ch = createChannel({ groupPolicy: 'pairing' }); + + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + chatId: 'group-1', + chatName: 'Release Team', + senderId: 'alice', + senderName: 'Alice', + }), + ); + await ch.handleInbound( + envelope({ + isGroup: true, + isMentioned: true, + chatId: 'group-1', + chatName: 'Release Team', + senderId: 'bob', + senderName: 'Bob', + }), + ); + + // The request is deduped by subject; the public notification must be + // deduped the same way instead of posting once per mention. + expect(ch.sent).toHaveLength(1); + expect(ch.sent[0]!.chatId).toBe('group-1'); + expect(ch.sent[0]!.text).toContain('pairing code'); + expect(bridge.prompt).not.toHaveBeenCalled(); + expect( + new PairingStore('test-chan', '/tmp').listPending(), + ).toHaveLength(1); + } finally { + if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; + else process.env['QWEN_HOME'] = previousQwenHome; + rmSync(qwenHome, { recursive: true, force: true }); + } + }); + it('lets an approved paired group talk without mentions when requireMention is false', async () => { const previousQwenHome = process.env['QWEN_HOME']; const qwenHome = mkdtempSync(join(tmpdir(), 'qwen-group-pairing-')); @@ -13678,6 +13764,9 @@ describe('ChannelBase', () => { expect(ch.sent).toHaveLength(1); expect(ch.sent[0]!.text).toContain('Too many pending pairing requests'); expect(bridge.prompt).not.toHaveBeenCalled(); + expect( + new PairingStore('test-chan', '/tmp').listPending(), + ).toHaveLength(3); } finally { if (previousQwenHome === undefined) delete process.env['QWEN_HOME']; else process.env['QWEN_HOME'] = previousQwenHome; @@ -13873,7 +13962,15 @@ describe('ChannelBase', () => { expect(ch.sent).toHaveLength(2); expect(ch.sent[1]!.chatId).toBe('group-1'); + // Group wording must not publicly attribute the sender's unrelated + // pending (DM) request to the whole group. + expect(ch.sent[1]!.text).toContain( + 'A pairing request cannot be created right now', + ); expect(ch.sent[1]!.text).toContain( + 'Another member can mention the bot', + ); + expect(ch.sent[1]!.text).not.toContain( 'You already have a pending pairing request', ); expect(bridge.prompt).not.toHaveBeenCalled(); diff --git a/packages/channels/base/src/ChannelBase.ts b/packages/channels/base/src/ChannelBase.ts index 18cc0ac8077..3566941eba1 100644 --- a/packages/channels/base/src/ChannelBase.ts +++ b/packages/channels/base/src/ChannelBase.ts @@ -376,6 +376,11 @@ export abstract class ChannelBase { observation: ChannelMemoryRecallObservation, ) => void; private groupHistory: GroupHistoryStore; + // Tracks the pairing code already announced per group so repeated triggers + // of the same pending request (extra mentions, parallel notification lanes) + // post the public notification once. In-memory by design: a restart can + // re-post once per still-pending request, but never per trigger. + private readonly groupPairingNotified = new Map(); private readonly loopController?: ChannelLoopController; private readonly observedContacts?: ChannelBaseOptions['observedContacts']; private readonly observedContactEnvelopes = new WeakSet(); @@ -5772,6 +5777,16 @@ export abstract class ChannelBase { : 'Too many pending pairing requests. Please try again later.'; } + private groupPairingRejectionMessage( + rejected: 'sender_pending' | 'cap_reached', + ): string { + // Group variant: the DM wording would publicly attribute the mentioning + // member's unrelated pending request to the whole group. + return rejected === 'sender_pending' + ? 'A pairing request cannot be created right now. Another member can mention the bot to start group approval, or try again later.' + : 'Too many pending pairing requests. Please try again later.'; + } + protected async onPairingRequired( chatId: string, result: CreatePairingRequestResult, @@ -5798,16 +5813,20 @@ export abstract class ChannelBase { threadId?: string, ): Promise { if ('code' in result) { + if (this.groupPairingNotified.get(chatId) === result.code) { + return; + } await this.sendThreadMessage( chatId, threadId, `This group requires approval. Its pairing code is: ${result.code}\n\nAsk the bot operator to approve the group with:\n qwen channel pairing approve ${this.name} ${result.code}`, ); + this.groupPairingNotified.set(chatId, result.code); } else { await this.sendThreadMessage( chatId, threadId, - this.pairingRejectionMessage(result.rejected), + this.groupPairingRejectionMessage(result.rejected), ); } } diff --git a/packages/channels/base/src/PairingStore.test.ts b/packages/channels/base/src/PairingStore.test.ts index 290b1f5f7af..b21fcbb1e44 100644 --- a/packages/channels/base/src/PairingStore.test.ts +++ b/packages/channels/base/src/PairingStore.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -576,6 +576,95 @@ describe('PairingStore workspace scoping (#7017)', () => { }); }); +describe('group allowlist durability', () => { + let qwenHome: string; + let workspace: string; + let prevQwenHome: string | undefined; + + beforeEach(() => { + qwenHome = fs.mkdtempSync(path.join(os.tmpdir(), 'pairing-home-')); + workspace = fs.mkdtempSync(path.join(os.tmpdir(), 'pairing-ws-')); + prevQwenHome = process.env['QWEN_HOME']; + process.env['QWEN_HOME'] = qwenHome; + }); + + afterEach(() => { + if (prevQwenHome === undefined) { + delete process.env['QWEN_HOME']; + } else { + process.env['QWEN_HOME'] = prevQwenHome; + } + for (const dir of [qwenHome, workspace]) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + const groupsPath = () => + path.join( + qwenHome, + 'channels', + getWorkspaceScopeDirName(workspace), + 'support-bot-groups.json', + ); + + it('refuses to rebuild an unreadable group allowlist on approve', () => { + const store = new PairingStore('support-bot', workspace); + const code = codeOf( + store.createGroupRequest('group-a', 'Group A', 'sender-1', 'Alice'), + ); + fs.mkdirSync(path.dirname(groupsPath()), { recursive: true }); + fs.writeFileSync(groupsPath(), '["group-b", "group-c"'); // torn JSON + + expect(() => store.approve(code)).toThrow(/unreadable group allowlist/); + + // The pending request must survive the failed approve so the code stays + // usable, and the corrupt file must be left untouched for the operator. + expect(store.listPending()).toHaveLength(1); + expect(fs.readFileSync(groupsPath(), 'utf-8')).toBe( + '["group-b", "group-c"', + ); + }); + + it('fails closed and logs when the group allowlist is unreadable', () => { + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const store = new PairingStore('support-bot', workspace); + fs.mkdirSync(path.dirname(groupsPath()), { recursive: true }); + fs.writeFileSync(groupsPath(), 'not json'); + + expect(store.isGroupApproved('group-b')).toBe(false); + expect(store.getGroupAllowlist()).toEqual([]); + expect(stderr).toHaveBeenCalledWith( + expect.stringContaining('group allowlist'), + ); + } finally { + stderr.mockRestore(); + } + }); + + it('keeps stored approvals when approving into an existing allowlist', () => { + const store = new PairingStore('support-bot', workspace); + fs.mkdirSync(path.dirname(groupsPath()), { recursive: true }); + fs.writeFileSync(groupsPath(), JSON.stringify(['group-b'], null, 2)); + const code = codeOf( + store.createGroupRequest('group-a', 'Group A', 'sender-1', 'Alice'), + ); + + const approved = store.approve(code); + + expect(approved?.subject.id).toBe('group-a'); + expect(store.getGroupAllowlist()).toEqual(['group-b', 'group-a']); + // The atomic write must not leave a temp file behind. + expect( + fs + .readdirSync(path.dirname(groupsPath())) + .filter((name) => name.includes('.tmp')), + ).toEqual([]); + }); +}); + describe('getWorkspaceScopeDirName', () => { it('is stable for a given path and unique across paths', () => { const a = getWorkspaceScopeDirName('/projects/app'); diff --git a/packages/channels/base/src/PairingStore.ts b/packages/channels/base/src/PairingStore.ts index 692f9cd32a4..30b81f0fe23 100644 --- a/packages/channels/base/src/PairingStore.ts +++ b/packages/channels/base/src/PairingStore.ts @@ -231,10 +231,11 @@ export class PairingStore { return { code: existing.code }; } - // One sender may hold only one pending request at a time: subject-keyed - // dedup lets a single member vouch for distinct groups, and without this + // One sender may hold only one pending request at a time: without this // limit one sender could occupy every shared slot and block all other - // pairing requests until expiry. + // pairing requests until expiry. Subject-keyed dedup (above) keeps + // re-mentions of the same group returning its existing code instead of + // tripping this check. if (active.some((request) => request.senderId === senderId)) { return { rejected: 'sender_pending' }; } @@ -263,11 +264,13 @@ export class PairingStore { if (idx === -1) return null; const request = pending[idx]!; - pending.splice(idx, 1); - this.writePending(pending); + // Persist the approval BEFORE consuming the request: if the allowlist + // write fails (or an existing allowlist file is unreadable and we refuse + // to rebuild it from []), the request stays pending and the code remains + // usable instead of being silently burned. if (request.subject.type === 'group') { - const groups = this.readGroupAllowlist(); + const groups = this.readGroupAllowlist(true); if (!groups.includes(request.subject.id)) { groups.push(request.subject.id); this.writeGroupAllowlist(groups); @@ -280,6 +283,9 @@ export class PairingStore { } } + pending.splice(idx, 1); + this.writePending(pending); + return request; } @@ -359,18 +365,41 @@ export class PairingStore { fs.writeFileSync(this.allowlistPath, JSON.stringify(list, null, 2)); } - private readGroupAllowlist(): string[] { + private readGroupAllowlist(strict = false): string[] { try { const data = fs.readFileSync(this.groupAllowlistPath, 'utf-8'); return JSON.parse(data) as string[]; - } catch { + } catch (err) { + const exists = fs.existsSync(this.groupAllowlistPath); + if (strict && exists) { + // An existing-but-unreadable allowlist (torn write, permissions) + // must not be silently rebuilt from []: that would permanently + // discard every stored group approval on the next write. + throw new Error( + `refusing to rewrite unreadable group allowlist ` + + `"${path.basename(this.groupAllowlistPath)}": ` + + `${(err as Error)?.message}`, + ); + } + if (exists) { + process.stderr.write( + `[PairingStore] group allowlist ` + + `"${path.basename(this.groupAllowlistPath)}" is unreadable ` + + `(${(err as Error)?.message}); treating as empty — stored group ` + + `approvals are not in effect and will be lost on the next approve\n`, + ); + } return []; } } private writeGroupAllowlist(list: string[]): void { this.ensureDir(); - fs.writeFileSync(this.groupAllowlistPath, JSON.stringify(list, null, 2)); + // Atomic temp+rename: this file is an authorization artifact, and a torn + // write left behind by a crash mid-write would read as "no approvals". + const tmpPath = `${this.groupAllowlistPath}.${process.pid}.tmp`; + fs.writeFileSync(tmpPath, JSON.stringify(list, null, 2)); + fs.renameSync(tmpPath, this.groupAllowlistPath); } } diff --git a/packages/channels/github/src/GithubAdapter.test.ts b/packages/channels/github/src/GithubAdapter.test.ts index 1214ce30101..9e90eca41ac 100644 --- a/packages/channels/github/src/GithubAdapter.test.ts +++ b/packages/channels/github/src/GithubAdapter.test.ts @@ -1922,6 +1922,113 @@ describe('GithubChannel', () => { ); }); + it('does not turn ambient comments into pairing requests under senderPolicy open', async () => { + await initWithoutLoop({ groupPolicy: 'pairing' }); + channel.usePreflight = true; + mockOctokit.paginate + .mockResolvedValueOnce([ + makeNotification({ + reason: 'comment', + last_read_at: '2026-07-01T12:00:00.000Z', + }), + ]) + .mockResolvedValueOnce([ + makeComment({ body: 'ambient chatter without a mention' }), + ]); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(0); + expect(mockOctokit.rest.issues.createComment).not.toHaveBeenCalled(); + expect( + new PairingStore('test-github', '/tmp/test').listPending(), + ).toEqual([]); + }); + + it('posts one pairing comment when assign and body mention both trigger pairing', async () => { + await initWithoutLoop({ groupPolicy: 'pairing' }); + channel.usePreflight = true; + mockOctokit.paginate + .mockResolvedValueOnce([ + makeNotification({ reason: 'assign', last_read_at: null }), + ]) + .mockResolvedValueOnce([ + makeIssueEvent({ + event: 'assigned', + assigner: { login: 'maintainer' }, + assignee: { login: 'test-bot' }, + }), + ]) + .mockResolvedValueOnce([]); + mockOctokit.rest.issues.get.mockResolvedValue({ + data: { + title: 'broken build', + state: 'open', + user: { login: 'alice' }, + body: '@test-bot please look at this issue', + }, + }); + + await pollOnce(); + + expect(channel.inboundEnvelopes).toHaveLength(0); + expect(mockOctokit.rest.issues.createComment).toHaveBeenCalledTimes(1); + expect(mockOctokit.rest.issues.createComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining('pairing code'), + }), + ); + expect( + new PairingStore('test-github', '/tmp/test').listPending(), + ).toHaveLength(1); + }); + + it('does not re-feed the body when a re-listed thread already had a pairing effect', async () => { + await initWithoutLoop({ + groupPolicy: 'pairing', + senderPolicy: 'allowlist', + allowedUsers: [], + }); + channel.usePreflight = true; + mockOctokit.paginate + .mockResolvedValueOnce([ + makeNotification({ reason: 'mention', last_read_at: null }), + ]) + .mockResolvedValueOnce([makeComment()]); + mockOctokit.rest.issues.get.mockResolvedValue({ + data: { + title: 'Test Issue', + body: '@test-bot the issue body mentions the bot too', + user: { login: 'alice' }, + }, + }); + + await pollOnce(); + + expect(mockOctokit.rest.issues.createComment).toHaveBeenCalledTimes(1); + expect(channel.cursor.dispatchedBodies).toContain('owner/repo|issue:42'); + + // Poll 2: marking the thread read failed, so it is listed as unread + // again. The mentioning comment is now outside the comment window; the + // body feed must stay suppressed or it would post a second identical + // pairing-code comment. + mockOctokit.paginate + .mockResolvedValueOnce([ + makeNotification({ + reason: 'mention', + last_read_at: null, + updated_at: '2026-07-02T11:00:00.000Z', + }), + ]) + .mockResolvedValueOnce([makeComment()]); + + await pollOnce(); + + expect(mockOctokit.rest.issues.createComment).toHaveBeenCalledTimes(1); + expect(mockOctokit.rest.issues.get).not.toHaveBeenCalled(); + expect(channel.inboundEnvelopes).toHaveLength(0); + }); + it('bounds each aggregated comment without hiding later comments', async () => { await initWithoutLoop(); mockOctokit.paginate diff --git a/packages/channels/github/src/GithubAdapter.ts b/packages/channels/github/src/GithubAdapter.ts index 87a7566007b..dca5cbc7015 100644 --- a/packages/channels/github/src/GithubAdapter.ts +++ b/packages/channels/github/src/GithubAdapter.ts @@ -1330,7 +1330,15 @@ export class GithubChannel extends PollingChannelBase { messageId: String(comment.id), text: this.botUsername ? stripBotMention(body, this.botUsername) : body, isGroup: true, - isMentioned: hasMention || (directed && allowed), + // Never synthesize a mention into an unapproved pairing group: the + // pairing step must keep dropping ambient comments, and a synthesized + // mention would turn every ambient comment into a pairing request. + isMentioned: + hasMention || + (directed && + allowed && + (this.config.groupPolicy !== 'pairing' || + this.groupGate.isGroupApproved(ctx.chatId))), isReplyToBot: false, metadata: this.buildRouteMetadata(ctx), }; @@ -1349,9 +1357,12 @@ export class GithubChannel extends PollingChannelBase { // A mention under group pairing has a visible effect (dispatch or a // pairing code comment) even when the sender gate rejects the sender; // suppress the first-contact body feed so the same intent cannot be - // dispatched twice. + // dispatched twice. Record the body as consumed too: if the thread is + // later re-listed as unread (mark-read can fail), the body feed must + // not re-trigger the same pairing intent on a later poll. if (hasMention && (allowed || this.config.groupPolicy === 'pairing')) { dispatched = true; + this.recordDispatchedBody(`${ctx.chatId}|${ctx.threadId}`); } } diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index bba2eef058f..37616d49450 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, expectTypeOf, vi, afterEach } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import { DaemonClient, DaemonHttpError, @@ -22,7 +22,6 @@ import { } from '../../src/daemon/types.js'; import type { DaemonCapabilities, - DaemonChannelPairingRevocationRequest, DaemonSessionContextStatus, DaemonSessionLspStatus, DaemonSessionOrganizationResult, @@ -7442,18 +7441,5 @@ describe('DaemonClient', () => { expect(JSON.parse(calls[0]!.body!)).toEqual({ groupId: 'group-1' }); }); - - it('pins the revocation request union at the type level', () => { - expectTypeOf<{ - senderId: string; - }>().toMatchTypeOf(); - expectTypeOf<{ - groupId: string; - }>().toMatchTypeOf(); - expectTypeOf<{ - senderId: string; - groupId: string; - }>().not.toMatchTypeOf(); - }); }); });