feat(channels): support group pairing - #8440
Conversation
|
Re-run at Template looks good ✓ Problem: unchanged — the next staged slice of the Channel management roadmap (#7209), and the granularity gap is real: one group chat currently means one pairing approval per member, which doesn't match a group-level trust decision. Direction: aligned, unchanged from the last pass — group pairing sits inside the roadmap issue's scope, and claude-code's CHANGELOG continues to show chat-channel investment in the same direction. Size: ~824 production-logic lines (including ~63 of web-shell e2e mock-daemon plumbing), ~2,068 test lines, ~211 docs lines, 54 files. Unchanged against main since the last pass. Still above the 500-production-line mark and cross-package, so it stays flagged for maintainer awareness per the core-module rules ( Approach: unchanged and still sound — group subjects reuse the existing pairing-code flow, group approvals live in a separate Risk: no elevated risk signals — none of the changed files match the high-risk revert-correlated paths. Moving on to code review. 🔍 中文说明在 模板完整 ✓ 问题：结论不变 —— 这是 Channel 管理路线图（#7209）的下一个分阶段切片，粒度缺口真实存在：今天一个群聊意味着每个成员各做一次配对审批，与群级别的信任决策不匹配。 方向：对齐，与上次结论一致 —— 群配对在路线图 issue 的范围内，claude-code 的 CHANGELOG 也持续出现同方向的 chat-channel 投入。 规模：约 824 行生产逻辑（含约 63 行 web-shell e2e mock-daemon 管线），约 2,068 行测试，约 211 行文档，54 个文件。相对 main 的 diff 与上次一致。仍超过 500 生产行门槛且跨包，按核心模块规则继续标记供 maintainer 知悉（ 方案：不变且依然合理 —— 群主体复用现有配对码流程，群审批单独存放在 风险：无升级风险信号 —— 变更文件均未命中与 revert 相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Code reviewThis re-run re-attests the diff at The properties that matter are all structurally enforced, not flag-checked:
No correctness, security, or convention blockers found. The three non-blocking notes from the last pass stand: (1) the PR body still says the Web Shell UI is deferred while the diff includes it; (2) the user allowlist and pending file still use plain The group pairing flow this PR introduces: sequenceDiagram
participant P1 as Chat member
participant P2 as Channel runtime preflight
participant P3 as GroupGate
participant P4 as PairingStore
participant P5 as Operator CLI or daemon
P1->>P2: mention or reply in unapproved group
P2->>P3: check envelope
P3->>P4: createGroupRequest (chat id, initiator)
P4-->>P3: pairing code
P3-->>P2: pairing_required
P2-->>P1: code posted once per group and code
P5->>P4: approve code
P4->>P4: write group allowlist, then consume request
P1->>P2: later message from any member
P2->>P3: check envelope
P3-->>P2: approved group passes, sender gate skipped in group context
Files changed (30 of 54 shown)
Testing evidence — the PR's own CI at this commit (never run by the triage)Unattended run, so no PR code was built or executed here; this is the check signal on
The central claim here is behavioural, and the strongest evidence for it is not mine to produce on the CI path: the maintainer's real-stack verification (real runtime, real daemon, real SDK, real browser, plus three mutation tests) is on record in this thread at the prior head 中文说明代码审查：本次重跑在 关键属性都是结构性保证：普通消息静默（先于 store 返回 测试证据：无人值守运行，未构建或执行任何 PR 代码；上表为 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review and green CI at this commit with the maintainer's real-stack sign-off already on record; the Stage 0 size escalation (~824 production lines, cross-package) caps the bot verdict at defer, and that cap is pure policy, not doubt. Stepping back: the only change since the last pass is the maintainer's own merge of What withholds the bot's approval is mechanical, not substantive: a cross-package 中文说明信心：3/5 —— 该 commit 上审查干净、CI 全绿，且 maintainer 的真实栈背书已记录在案；Stage 0 规模升级（约 824 行生产代码、跨包）将机器人结论上限锁定在 defer，该上限纯属策略，而非疑虑。 退一步看：距上次通过的唯一变化是 maintainer 亲自把 机器人不投批准票的原因是程序性的，而非实质性的：跨包且超过 500 生产行的 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| senderId: string; | ||
| senderName: string; | ||
| subject?: DaemonChannelPairingSubject; | ||
| code: string; | ||
| createdAt: number; |
There was a problem hiding this comment.
[Critical] R1-1: The Web Shell pairing panel is not subject-aware. This diff makes subject the approval target and demotes senderId/senderName to "the initiating sender", but the shipped panel (ChannelPairingRequests.tsx, untouched here) still renders senderName || senderId, lists only snapshot.senderIds, and revokes with { senderId } only — there are zero subject|groupIds matches in the web-shell channel components.
Failure scenario: on the supported dual config (senderPolicy:'pairing' + groupPolicy:'pairing') the panel IS shown (it gates on senderPolicy === 'pairing', ChannelEditorDialog.tsx:663). A mention in an unapproved group shows a row labeled "Alice" (the initiator); the operator clicks Approve believing they pair one user — the daemon approves the entire group, granting every member bot access; the success toast again says "Alice"; the grant never appears in the approvals list and cannot be revoked from the UI.
Suggested fix: render request.subject (e.g. Group: {name} ({id}) — requested by {senderName}), report the subject in the approve toast, show snapshot.groupIds in the approvals list, and extend the webui revoke action to pass { groupId } (the SDK union already supports it) — or hide group requests in the panel until the staged UI PR lands.
中文说明
Web Shell 配对面板不感知 subject。本 diff 将 subject 作为审批对象，senderId/senderName 降级为"发起成员"，但已上架的面板（ChannelPairingRequests.tsx，本 PR 未改动）仍渲染 senderName || senderId、审批列表只读 snapshot.senderIds、撤销只发 { senderId }——web-shell 频道组件中没有任何 subject|groupIds 引用。
失败场景：在官方支持的双配置（senderPolicy:'pairing' + groupPolicy:'pairing'）下面板会显示（显示条件为 senderPolicy === 'pairing'）。未审批群中的 @ 会生成一条显示为 "Alice"（发起者）的请求；运营者以为在为单个用户授权并点击批准——daemon 实际批准了整个群，所有群成员都获得机器人使用权；成功提示仍显示 "Alice"；该授权不出现在审批列表中，也无法在 UI 中撤销。
建议修复：渲染 request.subject（如 Group: {name} ({id}) — requested by {senderName}）、批准提示使用 subject、审批列表展示 snapshot.groupIds、将 webui 撤销扩展为可传 { groupId }（SDK 联合类型已支持）——或在分阶段 UI PR 落地前在面板中隐藏群请求。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| this.groupAllowlistPath = path.join( | ||
| this.dir, | ||
| `${safeChannelName}-group-allowlist.json`, | ||
| ); |
There was a problem hiding this comment.
[Critical] R1-10: Allowlist filename suffix collision across channels. The new group allowlist file is <name>-group-allowlist.json; because that suffix ends with the pre-existing user-allowlist suffix -allowlist.json, a channel named support and a channel named support-group in the same workspace resolve to the identical path — channel support's GROUP allowlist file IS channel support-group's USER allowlist file. Channel names are unrestricted config keys, and pre-PR the two suffixes were disjoint, so this collision class is introduced by this diff.
Failure scenario (probe-confirmed in this tree): approving a group on support writes the group ID into support-group's user allowlist (it surfaces in that channel's approvals; where user and group IDs share a namespace, a matching sender becomes approved on a channel nobody approved them for), and revoking on support-group removes channel support's group approval. The same hazard exists in the legacy unscoped layout.
Suggested fix: use a suffix that does not embed another suffix, e.g. -groups.json (only PairingStore and the README reference the spelling), plus a regression test mirroring the workspace-scoping one: stores for channels support and support-group in one workspace, approve a group on one, assert the other channel's approvals are unaffected.
中文说明
allowlist 文件名后缀跨频道冲突。新的群 allowlist 文件为 <name>-group-allowlist.json；由于该后缀以既有用户 allowlist 后缀 -allowlist.json 结尾，同一工作空间内名为 support 和 support-group 的两个频道会解析到同一路径——support 的群 allowlist 文件就是 support-group 的用户 allowlist 文件。频道名是不受限的配置键，且 PR 之前两个后缀互不包含，因此这一冲突类是本 diff 引入的。
失败场景（已在本树用探针证实）：在 support 上批准一个群会把群 ID 写入 support-group 的用户 allowlist（会出现在该频道的审批列表；在用户与群 ID 共用命名空间时，匹配的发送者会在无人批准的频道上获得授权）；在 support-group 上撤销会移除 support 的群授权。旧的无作用域布局同样存在该风险。
建议修复：改用不嵌套其他后缀的文件名，如 -groups.json（仅 PairingStore 与 README 引用该拼写），并仿照工作空间隔离测试新增回归测试：在同一工作空间构造 support 与 support-group 两个 store，在其中一个批准群，断言另一个频道的审批不受影响。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| const existing = active.find( | ||
| (request) => | ||
| request.subject.type === subject.type && | ||
| request.subject.id === subject.id, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-2: Subject-based pending-request dedup has no test. The dedup match changed from sender-keyed (r.senderId === senderId) to (subject.type, subject.id); no test calls createGroupRequest twice for an existing subject (whether from the same or a different initiating sender).
Failure scenario: if the match regresses to sender-keyed, two different members mentioning the bot in the same group create two pending entries with distinct codes instead of reusing one — pairing list shows duplicates for one group, each consumes one of the 3 cap slots, and a third group's mention hits the overflow path despite only two groups having asked. All tests stay green.
Suggested fix: add a PairingStore test — createGroupRequest('group-1', …, 'alice', …) then createGroupRequest('group-1', …, 'bob', …) returns the same code and listPending() has exactly one entry with subject.id === 'group-1' and senderId still 'alice'.
中文说明
按 subject 去重的待处理请求逻辑没有测试。去重匹配从按发送者（r.senderId === senderId）改为按 (subject.type, subject.id)；没有任何测试对同一 subject 二次调用 createGroupRequest（无论发起者是否相同）。
失败场景：若匹配退化为按发送者，同一群内两个不同成员 @ 机器人会创建两条不同配对码的待处理请求而非复用一条——pairing list 显示同一群的重复项，各占 3 个上限槽位之一，第三个群的 @ 会触发溢出提示，尽管实际只有两个群请求过。所有测试仍为绿色。
建议修复：新增 PairingStore 测试——先 createGroupRequest('group-1', …, 'alice', …) 再 createGroupRequest('group-1', …, 'bob', …)，断言返回相同配对码且 listPending() 仅一条 subject.id === 'group-1'、senderId 仍为 'alice' 的记录。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| if ( | ||
| options.createPairingRequest === false || | ||
| (!envelope.isMentioned && !envelope.isReplyToBot) | ||
| ) { | ||
| return { allowed: false, reason: 'pairing_trigger_required' }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-3: The pairing trigger matrix has two untested cells. (a) isReplyToBot: true under the pairing policy appears in no test, though the docs added by this PR promise "A deliberate mention or reply from an unknown group creates one pairing request". (b) No test pairs requireMention: false with the pairing policy, so the documented "unmentioned messages never create a request even with requireMention: false" guarantee is unpinned.
Failure scenario: (a) deleting && !envelope.isReplyToBot makes reply-only groups unpairable, all tests green; (b) deleting the in-block trigger check (harmless under default requireMention: true) lets ambient chatter in requireMention: false groups create requests and consume the pending cap, all tests green.
Suggested fix: add to the pairing policy describe — a test with envelope({ isGroup: true, isReplyToBot: true }) + a store asserting a request is created; and a test with new GroupGate('pairing', { '*': { requireMention: false } }, store) + an unmentioned envelope asserting pairing_trigger_required and an empty pending list.
中文说明
配对触发矩阵有两格未测试。(a) 配对策略下 isReplyToBot: true 没有任何测试，而本 PR 新增文档承诺"未审批群的明确 @ 或回复会创建一个配对请求"；(b) 没有测试将 requireMention: false 与配对策略组合，因此文档中"即使群设置 requireMention: false，未 @ 消息也绝不创建请求"的保证未被固化。
失败场景：(a) 删除 && !envelope.isReplyToBot 会使"仅回复"的群永远无法配对，所有测试仍绿；(b) 删除块内触发检查（在默认 requireMention: true 下无害）会让 requireMention: false 群的普通消息创建请求并占满待处理上限，所有测试仍绿。
建议修复：在 pairing policy describe 中新增两个测试——envelope({ isGroup: true, isReplyToBot: true }) + store，断言创建了请求；new GroupGate('pairing', { '*': { requireMention: false } }, store) + 未 @ 消息，断言返回 pairing_trigger_required 且待处理列表为空。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| protected async onGroupPairingRequired( | ||
| chatId: string, | ||
| code: string | null, | ||
| threadId?: string, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] R1-4: Group pairing notification content and the MAX_PENDING overflow branch are untested. The only test covering onGroupPairingRequired asserts expect(ch.sent).toHaveLength(1) — nothing about the text; the code === null branch ("Too many pending pairing requests…") appears in zero test files, and no test seeds the group pending queue to cap. The adjacent user-pairing tests DO assert content (toContain('pairing code')).
Failure scenario: dropping ${code} from the template, or inverting if (code) so every group gets the cap message, ships green — groups can never learn their code from the chat.
Suggested fix: in the group test assert expect(ch.sent[0]!.text).toContain(request!.code) (ideally also 'pairing approve'); add one cap test: seed 3 pending requests, mention from a fourth group, assert the "Too many pending pairing requests" message is sent.
中文说明
群配对通知内容与 MAX_PENDING 溢出分支未被测试。覆盖 onGroupPairingRequired 的唯一测试只断言 ch.sent 长度为 1，不断言文本；code === null 分支（"Too many pending pairing requests…"）在任何测试文件中都未出现，也没有测试把群待处理队列填满到上限。相邻的用户配对测试是有内容断言的（toContain('pairing code')）。
失败场景：从模板中删掉 ${code}，或反转 if (code) 使每个群都收到上限提示，都能在所有测试绿色的情况下上线——群永远无法从聊天中获知配对码。
建议修复：在群测试中断言 expect(ch.sent[0]!.text).toContain(request!.code)（最好也断言 'pairing approve'）；新增一个上限测试：预置 3 条待处理请求后让第四个群 @，断言发送了 "Too many pending pairing requests" 消息。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| if (envelope.isGroup && this.config.groupPolicy === 'pairing') { | ||
| this.markPreflighted(envelope); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-22 (pattern 3/3 — DM negative space under groupPolicy:'pairing'): this approved-group early return is scoped to group messages only by envelope.isGroup &&, and no test sends a DM on a groupPolicy: 'pairing' channel — so the live inbound path's DM guard is unpinned (distinct from R1-14's GroupGate-level and R1-20's loop-path mutants; this mutant over-admits).
Failure scenario (probe-confirmed): deleting envelope.isGroup && leaves all 605 channel-base tests green, and a probe DM from an unlisted stranger reaches the agent session (bridge.prompt called) under both senderPolicy:'allowlist' (empty) and 'pairing' — no rejection, no pairing request — contradicting the doc guarantee this PR adds: "senderPolicy continues to control direct messages".
Suggested fix: add a ChannelBase inbound test on groupPolicy: 'pairing' — (a) senderPolicy: 'allowlist', allowedUsers: [] asserting an unapproved sender's DM is rejected (no bridge.prompt); (b) senderPolicy: 'pairing' asserting the DM triggers the user pairing flow (code notification) rather than a session.
中文说明
（模式 3/3 —— groupPolicy:'pairing' 下的 DM 负空间）这个"已审批群提前返回"仅靠 envelope.isGroup && 限定于群消息，而没有任何测试在 groupPolicy: 'pairing' 频道上发送 DM——因此实时入站路径上的 DM 守卫未被固化（不同于 R1-14 的 GroupGate 层与 R1-20 的循环路径变异体；此变异体是过度放行）。
失败场景（已用探针证实）：删除 envelope.isGroup && 后全部 605 个 channel-base 测试仍绿，探针中未列入名单的陌生人的 DM 在 senderPolicy:'allowlist'（空名单）与 'pairing' 下都能到达 agent 会话（bridge.prompt 被调用）——无拒绝、无配对请求——与本 PR 新增的文档保证"senderPolicy 继续控制私聊"相悖。
建议修复：新增 groupPolicy: 'pairing' 的 ChannelBase 入站测试——(a) senderPolicy: 'allowlist', allowedUsers: [] 时断言未审批发送者的 DM 被拒绝（bridge.prompt 未调用）；(b) senderPolicy: 'pairing' 时断言 DM 触发用户配对流程（发送配对码）而非进入会话。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| private allowlistPath: string; | ||
| private groupAllowlistPath: string; |
There was a problem hiding this comment.
[Suggestion] R1-15: The new group-allowlist file is missing from migrateLegacyState. legacyPairs copies only <name>-pairing.json and <name>-allowlist.json from the legacy global layout before writing the one-shot .migrated sentinel; this PR adds a third state file but no third migration pair. The legacy layout remains reachable (no-cwd construction is supported and tested; hand-written channel configs may omit cwd).
Failure scenario: a channel running in the legacy layout with groupPolicy:'pairing' accumulates approved groups in the global <name>-group-allowlist.json; when the channel later resolves to a scoped layout, the migration copies only user state and seals the gate — approved groups are silently lost (group messages drop or re-trigger pairing) and the orphaned file is never absorbed. Fails safe (drops access, grants none), but loses approvals the migration exists to preserve.
Suggested fix: add a third pair to legacyPairs — [path.join(channelsRoot, ${channelName}-group-allowlist.json), this.groupAllowlistPath] — plus a grandfathering test that seeds a legacy group-allowlist file and asserts the scoped store reports the group approved.
中文说明
新的群 allowlist 文件未纳入 migrateLegacyState。legacyPairs 在写入一次性的 .migrated 哨兵前只从旧的全局布局复制 <name>-pairing.json 与 <name>-allowlist.json；本 PR 新增了第三个状态文件却没有新增第三个迁移对。旧布局仍然可达（不传 cwd 的构造受支持且有测试；手写频道配置可能省略 cwd）。
失败场景：以旧布局运行且 groupPolicy:'pairing' 的频道会在全局 <name>-group-allowlist.json 中积累已审批群；当该频道后续解析到带作用域的布局时，迁移只复制用户状态并永久封口——已审批群被静默丢失（群消息被丢弃或重新触发配对），孤儿文件再也不会被吸收。失败方向安全（只收不放），但丢失了迁移本应保留的授权。
建议修复：为 legacyPairs 增加第三对——[path.join(channelsRoot, ${channelName}-group-allowlist.json), this.groupAllowlistPath]——并新增祖父条款测试：预置旧布局的群 allowlist 文件，断言带作用域的 store 报告该群已审批。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| const revoked = | ||
| subject.type === 'group' | ||
| ? store.revokeGroup(subject.id) | ||
| : store.revoke(subject.id); |
There was a problem hiding this comment.
[Suggestion] R1-16: The group revocation not-found path is unpinned end-to-end. No test drives revokeGroup returning false through the service/route: the user-flow double-revoke 404 IS tested, the new group test revokes only the sole present entry, route-level 404 tests mock the service, and PairingStore.test.ts has no revokeGroup(...)).toBe(false) assertion.
Failure scenario: a mutant making the group path always report success (e.g. store.revokeGroup(subject.id) || true) passes the whole suite green — DELETE .../pairing-approvals with a never-approved (or already-revoked) { groupId } returns 200 success instead of 404 channel_pairing_approval_not_found, so an operator believes they revoked a group's bot access when nothing changed.
Suggested fix: in 'manages group pairing when groupPolicy uses pairing mode', after the successful revocation add await expect(service.revokePairingApproval('bot', { type: 'group', id: 'group-1' })).rejects.toMatchObject({ code: 'channel_pairing_approval_not_found' }); (mirroring the user-flow double-revoke).
中文说明
群撤销"未找到"路径端到端未被固化。没有测试让 revokeGroup 返回 false 并穿透 service/路由：用户流程的二次撤销 404 有测试，新群测试只撤销唯一存在的条目，路由层 404 测试 mock 了 service，PairingStore.test.ts 也没有 revokeGroup(...)).toBe(false) 断言。
失败场景：让群路径恒报成功的变异体（如 store.revokeGroup(subject.id) || true）可在整套测试绿色下通过——对从未批准（或已撤销）的 { groupId } 执行 DELETE .../pairing-approvals 会返回 200 成功而非 404 channel_pairing_approval_not_found，运营者以为撤销了某群的机器人权限，实际什么都没变。
建议修复：在 'manages group pairing when groupPolicy uses pairing mode' 的成功撤销之后追加 await expect(service.revokePairingApproval('bot', { type: 'group', id: 'group-1' })).rejects.toMatchObject({ code: 'channel_pairing_approval_not_found' });（仿照用户流程的二次撤销）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| if ( | ||
| config['senderPolicy'] !== 'pairing' && | ||
| config['groupPolicy'] !== 'pairing' | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R1-17: The rewritten pairing-enabled guard's negative space is unpinned. No test pairs a present but non-pairing groupPolicy ('open'/'allowlist'/'disabled') with a non-pairing senderPolicy; the tested configs are senderPolicy:'pairing' (no groupPolicy), senderPolicy:'open' + groupPolicy:'pairing', and senderPolicy:'open' (no groupPolicy).
Failure scenario: the mutant config['groupPolicy'] !== 'pairing' → config['groupPolicy'] === undefined passes every current test. Under it, on a channel configured senderPolicy:'open', groupPolicy:'open' (no pairing anywhere), all four pairing operations succeed instead of 409 channel_pairing_not_enabled — an operator can approve a stale pending code on a fully-open channel and write allowlist entries that nothing enforces.
Suggested fix: add a negative case to the service test — config { type: 'dingtalk', senderPolicy: 'open', groupPolicy: 'allowlist' } (and/or 'disabled'), expecting all four pairing operations to reject with code: 'channel_pairing_not_enabled'.
中文说明
重写后的"配对已启用"守卫的负空间未被固化。没有测试用存在但非配对的 groupPolicy（'open'/'allowlist'/'disabled'）搭配非配对 senderPolicy；现有测试配置为 senderPolicy:'pairing'（无 groupPolicy）、senderPolicy:'open' + groupPolicy:'pairing'、senderPolicy:'open'（无 groupPolicy）。
失败场景：变异体 config['groupPolicy'] !== 'pairing' → config['groupPolicy'] === undefined 可通过当前全部测试。在该变异体下，配置为 senderPolicy:'open', groupPolicy:'open'（完全无配对）的频道上，四个配对操作都会成功而非返回 409 channel_pairing_not_enabled——运营者可以在全开放频道上批准过期的待处理配对码，写入无人强制执行的 allowlist 条目。
建议修复：在 service 测试中新增反例——配置 { type: 'dingtalk', senderPolicy: 'open', groupPolicy: 'allowlist' }（和/或 'disabled'），断言四个配对操作均以 code: 'channel_pairing_not_enabled' 拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| DaemonChannelPairingRequest, | ||
| DaemonChannelPairingSubject, | ||
| DaemonChannelPairingRequestsSnapshot, |
There was a problem hiding this comment.
[Suggestion] R1-23: DaemonChannelPairingSubject is missing from the webui re-export barrels. This PR exports the new type from both SDK barrels but not from the curated @qwen-code/webui barrels (packages/webui/src/daemon/index.ts and daemon-react-sdk.ts), where every sibling pairing type IS re-exported under a comment stating the list exists "to allow web-shell and other UI packages to depend only on @qwen-code/webui". The re-exported parent DaemonChannelPairingRequest now has a member (subject) whose type cannot be named from the same barrel.
Failure scenario: the subject-aware rendering the Web Shell panel needs (see R1-1) cannot name request.subject's type from the barrel the contract tells UI packages to use, forcing the direct @qwen-code/sdk/daemon import the barrels exist to prevent; the barrel surface silently diverges from the SDK surface at the exact release the family grows. (Current consumers bypass the barrel for this family, so nothing breaks today — impact bounded at Suggestion.)
Suggested fix: add DaemonChannelPairingSubject next to DaemonChannelPairingRequest in the export type { ... } from '@qwen-code/sdk/daemon' block of packages/webui/src/daemon/index.ts and in the pairing-family block of packages/webui/src/daemon-react-sdk.ts.
中文说明
DaemonChannelPairingSubject 未加入 webui 的重导出桶。本 PR 在两个 SDK 桶中导出了新类型，却没有加入精选的 @qwen-code/webui 桶（packages/webui/src/daemon/index.ts 与 daemon-react-sdk.ts）——那里重导出了所有同族配对类型，且注释写明该列表的存在是"为了让 web-shell 和其他 UI 包只依赖 @qwen-code/webui"。被重导出的父类型 DaemonChannelPairingRequest 现在有一个成员（subject），其类型无法从同一个桶中命名。
失败场景：Web Shell 面板所需的 subject 感知渲染（见 R1-1）无法从契约指定的桶中命名 request.subject 的类型，被迫直接 import @qwen-code/sdk/daemon——正是这些桶要避免的；桶表面在该类型族扩展的版本悄悄与 SDK 表面脱节。（当前消费者本就绕过该桶使用此类型族，因此今天不会有东西坏掉——影响界定为 Suggestion。）
建议修复：在 packages/webui/src/daemon/index.ts 的 export type { ... } from '@qwen-code/sdk/daemon' 块中、以及 packages/webui/src/daemon-react-sdk.ts 的配对类型族块中，把 DaemonChannelPairingSubject 加在 DaemonChannelPairingRequest 旁边。
— qwen3.8-max via Qwen Code /review (v0.21.4)
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| const effectiveGroupPolicy = String( | ||
| draft.values['groupPolicy'] ?? '', | ||
| ); | ||
| const showPairing = | ||
| effectivePolicy === 'pairing' || | ||
| effectiveGroupPolicy === 'pairing'; |
There was a problem hiding this comment.
[Critical] R2-1: The group-pairing half of the showPairing gate reads draft.values['groupPolicy'], which is only populated when the channel descriptor declares a groupPolicy management field. Only the github and gitlab adapters declare one; the manageable IM adapters (dingtalk, wecom, feishu) do not — and createChannelEditorDraft has no instance-config fallback for groupPolicy, unlike senderPolicy above. For a dingtalk/wecom/feishu channel configured senderPolicy: 'open'|'allowlist' + groupPolicy: 'pairing' — a first-class config this PR's daemon service and its own service test (manages group pairing when groupPolicy uses pairing mode) explicitly support — showPairing is false and the pairing panel (pending group requests, approval, revocation — the Web Shell surface this PR adds) never renders. The inner render decision (~line 669) correctly consults instance?.config.groupPolicy === 'pairing' — asymmetry inside the same diff. Probe-confirmed: rendering this dialog with the real dingtalk descriptor and instance {senderPolicy: 'open', groupPolicy: 'pairing'} shows no pairing section and never calls listPairingRequests; the fix below flips the probe and keeps all 34 existing dialog/panel tests green. Note: the new test shows pairing management when only group pairing is enabled masks this because its synthetic descriptor fabricates a groupPolicy field no real IM plugin provides — it should use the real descriptor shape. — Failure scenario: operator configures a dingtalk/wecom/feishu channel with senderPolicy: 'open' + groupPolicy: 'pairing'; an unapproved group mentions the bot and a pending group pairing request is created; opening the channel editor in Web Shell shows no pairing section at all, and no other Web UI surface exists (ChannelsManagerPage wires it only into this dialog). Approve/revoke from Web Shell is impossible; the operator must fall back to the CLI with no indication anything is missing.
| const effectiveGroupPolicy = String( | |
| draft.values['groupPolicy'] ?? '', | |
| ); | |
| const showPairing = | |
| effectivePolicy === 'pairing' || | |
| effectiveGroupPolicy === 'pairing'; | |
| const hasDescriptorGroupPolicy = descriptor.fields.some( | |
| (f) => f.key === 'groupPolicy', | |
| ); | |
| const effectiveGroupPolicy = hasDescriptorGroupPolicy | |
| ? String(draft.values['groupPolicy'] ?? '') | |
| : String(instance?.config.groupPolicy ?? ''); | |
| const showPairing = | |
| effectivePolicy === 'pairing' || | |
| effectiveGroupPolicy === 'pairing'; |
中文说明
showPairing 门控中 group-pairing 的一半读取 draft.values['groupPolicy']，而该值只有在频道 descriptor 声明了 groupPolicy 管理字段时才会被填充。只有 github 和 gitlab adapter 声明了该字段；可管理的 IM adapter（dingtalk、wecom、feishu）都没有——而且 createChannelEditorDraft 对 groupPolicy 没有 instance-config 回退（上面的 senderPolicy 有）。对于配置了 senderPolicy: 'open'|'allowlist' + groupPolicy: 'pairing' 的 dingtalk/wecom/feishu 频道——这是本 PR 的 daemon service 及其自身 service 测试（manages group pairing when groupPolicy uses pairing mode）明确支持的一等配置——showPairing 为 false，配对面板（待审批群请求、批准、撤销，即本 PR 新增的 Web Shell 能力）永远不会渲染。而内层渲染判断（约第 669 行）却正确地读取 instance?.config.groupPolicy === 'pairing'——同一 diff 内存在不对称。已用探针证实：使用真实 dingtalk descriptor 和 instance {senderPolicy: 'open', groupPolicy: 'pairing'} 渲染该对话框时不显示配对区域，且从不调用 listPairingRequests；下方修复可翻转探针结果，且 34 个既有 dialog/panel 测试全部保持绿色。注意：新测试 shows pairing management when only group pairing is enabled 掩盖了该缺陷，因为其合成 descriptor 虚构了真实 IM 插件都不提供的 groupPolicy 字段——应改用真实 descriptor 形态。失败场景：运营者为 dingtalk/wecom/feishu 频道配置 senderPolicy: 'open' + groupPolicy: 'pairing'；未审批群 @ 机器人后产生待审批群配对请求；在 Web Shell 打开频道编辑器时看不到任何配对区域，也没有其他 Web UI 入口（ChannelsManagerPage 只把该面板接入此对话框）。无法在 Web Shell 中批准/撤销，运营者只能回退到 CLI，且没有任何提示。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| * 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 { |
There was a problem hiding this comment.
[Suggestion] R2-2: This diff inserts the pairing gate into check() between the allowlist step and mention gating, but the doc comment above it (lines 30-39) still lists only the pre-PR three stages — its summary (policy + allowlist + mention gating) and numbered evaluation order omit the only step with a side effect (writing pairing requests) and the only step whose trigger conditions (options.createPairingRequest, isMentioned/isReplyToBot) are not obvious from call sites. The comment is this file's self-declared ordering contract, and its closing sentence now misattributes ambient-message protection; the precise intra-check() ordering is specified nowhere else. Every sibling doc surface (overview.md, README, protocol doc) was updated by this diff — this comment was overlooked. Behavioral harms are largely test-mitigated (GroupGate.test.ts pins pairing_trigger_required; the ChannelBase ambient test pins no request creation), which is why this stays a Suggestion. — Failure scenario: a maintainer extending or reordering gate logic reads the 3-step contract, concludes mention gating is the last gate and nothing between steps 2 and 3 has side effects, and reorders or adds an early return; ambient messages could then reach the pairing-request creation path. — Suggested fix (spans comment lines outside this hunk, so no one-click block): update the summary to Full group check: policy + allowlist + pairing + mention gating., insert evaluation-order step 3 = group pairing (pairing mode, group not approved → drop; an explicit mention/reply creates or returns a pending pairing request unless options.createPairingRequest is false), and renumber mention gating to step 4.
中文说明
本 diff 在 check() 的 allowlist 步骤与 mention gating 之间插入了 pairing 门控，但上方文档注释（第 30-39 行）仍只列出 PR 前的三个阶段——其摘要（policy + allowlist + mention gating）和编号评估顺序遗漏了唯一带副作用的步骤（写入配对请求），也遗漏了唯一触发条件不直观的步骤（options.createPairingRequest、isMentioned/isReplyToBot）。该注释是本文件自声明的顺序契约，其结尾句现在将环境消息防护归因错误；check() 内部的精确顺序在别处无任何说明。本 diff 更新了所有同级文档面（overview.md、README、protocol 文档）——唯独漏掉了这条注释。行为层面的危害大部分已被测试缓解（GroupGate.test.ts 固定了 pairing_trigger_required；ChannelBase 环境消息测试固定了不创建请求），因此仍为 Suggestion。失败场景：维护者扩展或重排门控逻辑时读到这份三步契约，误以为 mention gating 是最后一道门、步骤 2 与 3 之间没有副作用，于是重排顺序或新增提前返回；环境消息便可能进入配对请求创建路径。建议修复（涉及本 hunk 之外的注释行，故不提供一键 suggestion）：将摘要改为 Full group check: policy + allowlist + pairing + mention gating.，在评估顺序中插入第 3 步 = group pairing（pairing 模式下群未审批 → 丢弃；显式 @/回复会创建或返回待处理配对请求，除非 options.createPairingRequest 为 false），并将 mention gating 重新编号为第 4 步。
— qwen3.8-max via Qwen Code /review (v0.21.4)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| const lines = | ||
| this.config.groupPolicy === 'pairing' | ||
| ? entries | ||
| : entries.filter((entry) => this.gate.isAllowed(entry.senderId)); |
There was a problem hiding this comment.
[Suggestion] Under groupPolicy: 'pairing', group-history injection skips the per-entry sender filter here, and the drain site in processInbound performs no group-approval re-check. Probe-verified at HEAD: a prompt accepted before a group revocation but executing after it (queued turn or collect-buffer re-entry) still gets the revoked group's recorded history injected (- [Bob] background observed post-revoke; adding a drain-time approval re-check flipped the probe). Blast radius is bounded — history is keyed per [channel, chatId, threadId], so only an envelope from the same group drains it, and every post-revocation trigger from that group is blocked at preflight (no cross-group/DM leak) — hence Suggestion rather than Critical. — Failure scenario: group approved → ambient member messages recorded to group history → operator revokes the group → an in-flight/queued prompt for that same group is constructed with the revoked group's history entries injected into model context.
Suggested fix: re-check group approval at the drain site before prepending (e.g. groupGate.check(envelope, { createPairingRequest: false })), or clear the group's pending history entries on revokeGroup.
中文说明
在 groupPolicy: 'pairing' 下，此处群历史注入跳过了逐条 sender 过滤，且 processInbound 中的消费点不做群授权复查。已在 HEAD 用探针证实：群撤销前已受理、撤销后才执行的 prompt（排队轮次或 collect-buffer 重入）仍会注入该群已记录的历史（撤销后观察到 - [Bob] background；在消费点加入授权复查后探针结果翻转）。影响面有界——历史按 [channel, chatId, threadId] 隔离，只有同群的 envelope 会消费它，且撤销后该群的所有触发都会在 preflight 被拦截（无跨群/私聊泄漏）——因此是 Suggestion 而非 Critical。失败场景：群被批准 → 普通成员消息被记入群历史 → 运营者撤销该群 → 该群一个在途/排队的 prompt 构造时注入了已撤销群的历史内容。
建议修复：在消费点拼接前复查群授权（如 groupGate.check(envelope, { createPairingRequest: false })），或在 revokeGroup 时清空该群的待注入历史。
— qwen3.8-max via Qwen Code /review (v0.21.3)
There was a problem hiding this comment.
已修复：在群历史 drain 后、注入 prompt 前重新检查当前群 pairing approval；群已撤销时丢弃已 drain 的旧历史。
验证证据：cd packages/channels/base && npx vitest run src/ChannelBase.test.ts src/GroupGate.test.ts（580 passed）；npm run build --workspace @qwen-code/channel-base 通过；npm run typecheck 通过；commit 8af2dccef5。
| const showPairing = | ||
| effectivePolicy === 'pairing' || | ||
| effectiveGroupPolicy === 'pairing'; |
There was a problem hiding this comment.
[Suggestion] The PR description still declares the Web Shell UI out of scope — Evidence says "The Channel management UI is intentionally deferred to the next staged PR", and Risk & Scope lists "Web Shell configuration and approval UI" as not validated — yet this diff ships a Web Shell pairing surface: this showPairing gate with group-policy awareness, the subject-aware ChannelPairingRequests panel, EN+ZH i18n strings, and 4 component tests. The Reviewer Test Plan's four steps cover only CLI/daemon flows. — Failure scenario: a maintainer verifying against the Test Plan, or writing release notes from Risk & Scope, exercises/documents a strictly smaller surface than ships; the group-pairing Web Shell flow (panel visibility for group-only configs, group approval/revocation UI) lands verified only by unit tests.
Suggested fix: update the PR description — remove the "UI deferred" claims from Evidence and Risk & Scope, and add a Web Shell verification step (e.g. configure a channel with groupPolicy: 'pairing' whose descriptor has no groupPolicy management field such as dingtalk, open the channel editor, and confirm the pending group request shows Group: {name} — Requested by {sender} and group approval/revocation work).
中文说明
PR 描述仍声明 Web Shell UI 不在本 PR 范围内——Evidence 写着 "The Channel management UI is intentionally deferred to the next staged PR"，Risk & Scope 将 "Web Shell configuration and approval UI" 列为未验证——但本 diff 实际上线了 Web Shell 配对能力：这个感知 group policy 的 showPairing 门控、感知 subject 的 ChannelPairingRequests 面板、中英文 i18n 文案以及 4 个组件测试。Reviewer Test Plan 的四步只覆盖 CLI/daemon 流程。失败场景：维护者按 Test Plan 验证、或依据 Risk & Scope 撰写发布说明时，实际验证/记录的范围小于真实上线范围；群配对 Web Shell 流程（仅群配置下面板可见性、群批准/撤销 UI）只有单元测试验证。
建议修复：更新 PR 描述——移除 Evidence 与 Risk & Scope 中 "UI 延后" 的表述，并补充 Web Shell 验证步骤（例如为 descriptor 没有 groupPolicy 管理字段的频道如 dingtalk 配置 groupPolicy: 'pairing'，打开频道编辑器，确认待审批群请求显示 Group: {name} — Requested by {sender}，且群批准/撤销可用）。
— qwen3.8-max via Qwen Code /review (v0.21.3)
There was a problem hiding this comment.
Deferred to maintainer. The substance of this finding stands — the PR description still declares the Web Shell UI out of scope while this PR does ship it: the group-policy-aware showPairing gate, the subject-aware ChannelPairingRequests panel, EN+ZH i18n strings, and component tests that now also cover the descriptor-driven gate branch, group approval/revocation, and the 404-refresh recovery. However, this autofix round can only commit code in the checkout; it cannot edit the PR description (no GitHub write access by design), so the fix cannot land here. Recommendation for the maintainer: remove the "UI deferred" claims from Evidence and Risk & Scope, and add a Web Shell verification step (e.g. configure a dingtalk channel with senderPolicy: 'open' + groupPolicy: 'pairing', open the channel editor, and confirm the pending group request renders Group: {name} — Requested by {sender} and group approval/revocation work). Open question: update the description now, or after the follow-up staged UI work lands?
中文说明
延迟至维护者处理。该发现的事实部分成立——PR 描述仍声明 Web Shell UI 不在范围内，但本 PR 实际上线了该能力：感知 group policy 的 showPairing 门控、感知 subject 的 ChannelPairingRequests 面板、中英文 i18n 文案，以及现已覆盖 descriptor 驱动门控分支、群批准/撤销、404 刷新恢复的组件测试。然而本轮 autofix 只能在检出中提交代码，无法编辑 PR 描述（按设计不持有 GitHub 写权限），因此该修复无法在本轮落地。给维护者的建议：移除 Evidence 与 Risk & Scope 中 "UI 延后" 的表述，并补充 Web Shell 验证步骤（例如为 dingtalk 频道配置 senderPolicy: 'open' + groupPolicy: 'pairing'，打开频道编辑器，确认待审批群请求显示 Group: {name} — Requested by {sender}，且群批准/撤销可用）。待决问题：现在就更新 PR 描述，还是等后续分阶段 UI 工作落地后再更新？
| | `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 | |
There was a problem hiding this comment.
[Suggestion] This rewritten row omits the new options parameter ({ createPairingRequest?: boolean }) and the store-mutating side effect: probe-verified, a default check(envelope) call on a mention/reply from an unapproved group creates (or reuses) a pending pairing request — exactly the flag ChannelBase's own stored-loop probe must pass to stay read-only. Same-group probes dedupe, but read-only probes across 3+ distinct unapproved groups exhaust MAX_PENDING (3), after which real pairing gets pairingCode: null. — Failure scenario: an adapter author following this README probes gate state read-only across 3+ distinct unapproved groups (e.g. filtering proactive delivery targets); real group pairing requests then fail with "Too many pending pairing requests. Please try again later." until the 1-hour expiry.
| | `check(envelope)` | Returns the group policy decision and an optional pairing code | | |
| | `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 | |
中文说明
该行重写后遗漏了新的 options 参数（{ createPairingRequest?: boolean }）及其对 store 的副作用：已用探针证实，对未审批群的 @/回复以默认参数调用 check(envelope) 会创建（或复用）一个待审批配对请求——这正是 ChannelBase 自身的定时任务探测为保持只读必须传的参数。同群探测会去重，但跨 3 个以上不同未审批群的只读探测会耗尽 MAX_PENDING（3），此后真实配对只能拿到 pairingCode: null。失败场景：adapter 作者按本 README 跨 3 个以上不同未审批群只读探测门状态（如过滤主动投递目标）；真实群配对请求随后在 1 小时过期前一直得到 "Too many pending pairing requests. Please try again later."。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| | `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. | |
There was a problem hiding this comment.
[Suggestion] This row still says only authorized unmentioned group messages are persisted, but this PR rewrote the sibling bullet below to include "members of an approved paired group" and made recordPendingGroupHistory skip the sender check for approved paired groups (the PR's own test persists Bob's ambient message although he is on no allowlist). The row was accurate pre-PR; the contradiction is introduced by this diff, and it concerns what data is written to disk. — Failure scenario: with groupPolicy: 'pairing' + senderPolicy: 'allowlist' and an approved group, an operator reading this table believes only individually authorized senders' ambient messages are persisted, when in fact every unmentioned message from any paired-group member is written to the group-history JSONL and later injected into model context — two contradicting persistence statements on the same page.
| | `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. | | |
| | `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. | |
中文说明
该行仍只说"已授权"发送者的未 @ 群消息会被持久化，但本 PR 已把下方的条目改写为包含"已批准配对群的成员"，并使 recordPendingGroupHistory 对已批准配对群跳过 sender 检查（本 PR 自己的测试就验证了 Bob 不在任何 allowlist 上时其普通消息仍被持久化）。该行在 PR 之前是准确的；矛盾由本 diff 引入，且涉及写入磁盘的数据范围。失败场景：groupPolicy: 'pairing' + senderPolicy: 'allowlist' 且某群已获批准时，运营者读该表格会以为只有单独授权发送者的普通消息被持久化，而实际上配对群任何成员的未 @ 消息都会写入群历史 JSONL 并在之后注入模型上下文——同一页面出现两处互相矛盾的持久化表述。
— qwen3.8-max via Qwen Code /review (v0.21.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| it('keeps group approvals separate from similarly named channel user approvals', () => { | ||
| const groupStore = new PairingStore('support', workspaceA); | ||
| const userStore = new PairingStore('support-group', workspaceA); |
There was a problem hiding this comment.
[Suggestion] R4-1: Same-store user/group allowlist separation is unpinned by tests. The test that looks like it covers this uses two DIFFERENT stores (support vs support-group), so it passes trivially regardless of the separation logic; 'approves a group without approving the member who requested it' only asserts isApproved('sender-1') is false — a group-1 id leaked into the user allowlist would not be caught. — Failure scenario: probe-confirmed — a mutant where approve()'s group branch additionally pushes request.subject.id into the user allowlist ships green across all suites (607 + 75 tests). On platforms where user and group IDs share a namespace, an ID approved only as a group then grants sender-level access. — Suggested fix (in 'approves a group without approving the member who requested it', after store.approve(code!)):
expect(store.isApproved('group-1')).toBe(false);
expect(store.getAllowlist()).toEqual([]);中文说明
同一 store 内用户/群 allowlist 的隔离没有测试固化。看似覆盖该性质的测试使用了两个不同的 store（support 与 support-group），无论隔离逻辑是否正确都会通过；而 'approves a group without approving the member who requested it' 只断言 isApproved('sender-1') 为 false——群 ID group-1 泄漏进用户 allowlist 不会被发现。— 失败场景：已用探针验证——approve() 群分支额外把 request.subject.id 推入用户 allowlist 的变异体在全部测试绿色下通过（607 + 75 个测试）。在用户与群 ID 共用命名空间的平台上，仅作为群被批准的 ID 会获得个人级访问权限。— 建议修复（在 'approves a group without approving the member who requested it' 中 store.approve(code!) 之后）：
expect(store.isApproved('group-1')).toBe(false);
expect(store.getAllowlist()).toEqual([]);— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
已修复 + 验证证据：同一 PairingStore 现在明确断言群组 ID 不会进入用户 allowlist，且用户撤销路径不会误删群组授权。npx vitest run src/PairingStore.test.ts 27/27 通过；Prettier 与 git diff --check 通过。
| await service.approvePairing('bot', code!); | ||
| await expect(service.pairingApprovals('bot')).resolves.toEqual({ | ||
| senderIds: [], | ||
| groupIds: ['group-1'], | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R4-2: approved.subject on the approval response is pinned by no test for group approvals. This new group test discards the approvePairing result; the user-variant asserts only objectContaining({senderId, code}) — fields a group request also carries; the route test mocks the service. — Failure scenario: probe-confirmed — a service-layer mutant stripping subject from group approvals ships green; the daemon then returns an approval response without approved.subject, web-shell's requestSubject() falls back to the initiating sender, and the success toast announces the individual user when access was actually granted to the whole group. — Suggested fix:
await expect(service.approvePairing('bot', code!)).resolves.toEqual({
approved: expect.objectContaining({
subject: { type: 'group', id: 'group-1', name: 'Release Team' },
}),
requests: [],
});中文说明
群批准的响应中 approved.subject 没有任何测试固化。新增的群测试丢弃了 approvePairing 的返回值；用户变体只断言 objectContaining({senderId, code})——群请求同样携带这些字段；路由测试 mock 了 service。— 失败场景：已用探针验证——service 层去掉群批准 subject 的变异体全绿通过；daemon 随后返回不含 approved.subject 的批准响应，web-shell 的 requestSubject() 回退到发起成员，成功提示会显示个人用户，而实际授权的是整个群。— 建议修复：
await expect(service.approvePairing('bot', code!)).resolves.toEqual({
approved: expect.objectContaining({
subject: { type: 'group', id: 'group-1', name: 'Release Team' },
}),
requests: [],
});— qwen3.8-max via Qwen Code /review (v0.21.5)
| expect(response.status).toBe(200); | ||
| expect(secondaryService.revokePairingApproval).toHaveBeenCalledWith('bot', { | ||
| type: 'group', |
There was a problem hiding this comment.
[Suggestion] R4-3: The pairing routes' success-path HTTP response bodies are asserted at no layer. This new test pins only status 200 and service call args — all 26 .body assertions in this file avoid pairing success responses, and the GET pairing-approvals body is equally unasserted despite the mock returning groupIds. — Failure scenario: probe-confirmed — a reshape that keeps the service call but serializes only {revoked} ships 12/12 green. Web-shell reads result.senderIds / result.groupIds ?? [] off the revoke response; with the fields missing, setApprovedSenderIds(result.senderIds) stores undefined and the approvals render breaks after any successful revoke while on-disk allowlists stay intact. — Suggested fix:
expect(response.body).toEqual({
revoked: 'group-1',
senderIds: [],
groupIds: [],
});and pin the GET body in the workspace test: expect(approvals.body).toEqual({ senderIds: ['sender-1'], groupIds: ['group-1'] });
中文说明
配对路由成功路径的 HTTP 响应体在任何层级都没有断言。新增测试只固化了 status 200 和 service 调用参数——本文件全部 26 处 .body 断言都不涉及配对成功响应；GET pairing-approvals 的响应体同样未断言（mock 明明返回了 groupIds）。— 失败场景：已用探针验证——保留 service 调用但只序列化 {revoked} 的改写在 12/12 测试绿色下通过。web-shell 从撤销响应读取 result.senderIds / result.groupIds ?? []；字段缺失时 setApprovedSenderIds(result.senderIds) 会存入 undefined，任何成功撤销后审批列表渲染都会出错，而磁盘 allowlist 并未变化。— 建议修复：
expect(response.body).toEqual({
revoked: 'group-1',
senderIds: [],
groupIds: [],
});并在工作空间测试中固化 GET 响应体：expect(approvals.body).toEqual({ senderIds: ['sender-1'], groupIds: ['group-1'] });
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const effectiveGroupPolicy = descriptorGroupPolicy | ||
| ? String(draft.values['groupPolicy'] ?? '') | ||
| : String(instance?.config.groupPolicy ?? ''); |
There was a problem hiding this comment.
[Suggestion] R4-4: The descriptor branch of this fixed effectiveGroupPolicy gate has zero test coverage — every dialog test renders with descriptors WITHOUT a groupPolicy field (DINGTALK/OPTIONAL_SECRET), so only the instance-config fallback branch is exercised. — Failure scenario: probe-confirmed — collapsing the ternary to the instance-config fallback ships green across all dialog tests. For descriptor-driven channel types (github/gitlab) with a hand-set groupPolicy: 'pairing' config, editing the policy draft back to a non-pairing value keeps the pairing panel visible during editing, inconsistent with senderPolicy's live-reacting path. — Suggested fix: add a dialog test with a descriptor declaring a groupPolicy field and instance config groupPolicy: 'open'; change the draft field to 'pairing' and assert the pre-save pairing affordance (the Save pairing mode first alert) appears.
中文说明
修复后的 effectiveGroupPolicy 门控中 descriptor 分支的测试覆盖为零——所有对话框测试都使用不含 groupPolicy 字段的 descriptor（DINGTALK/OPTIONAL_SECRET），因此只有 instance-config 回退分支被执行。— 失败场景：已用探针验证——把三元表达式坍缩为 instance-config 回退后全部对话框测试仍绿色。对于 descriptor 驱动的频道类型（github/gitlab），手工配置 groupPolicy: 'pairing' 后，把草稿中的策略改回非 pairing 值时配对面板在编辑期间仍然可见，与 senderPolicy 的实时响应行为不一致。— 建议修复：新增一个对话框测试，使用声明了 groupPolicy 字段的 descriptor，instance 配置为 groupPolicy: 'open'；把草稿字段改为 'pairing'，断言出现保存前提示（Save pairing mode first 警告）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| expect(container.textContent).toContain( | ||
| 'Group: Release Team can now use this Channel.', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-5: Pending-list replacement after group approval is unpinned. This group-approval test never asserts the approved group request is removed from the pending list; the user-side twin asserts 'No pending requests' and not.toContain('ABCD1234'). — Failure scenario: probe-confirmed — a mutant making the pending-list replacement conditional on subject type ships 34/34 green; the stale pending row stays visible with a live Approve button, and re-clicking it yields a channel_pairing_request_not_found error toast instead of the empty state. — Suggested fix (after the approve click):
expect(container.textContent).toContain('No pending requests');
expect(container.textContent).not.toContain('GROUP123');中文说明
群批准后待处理列表的替换未被固化。这个群批准测试从未断言已批准的群请求从待处理列表移除；用户侧的对应测试断言了 'No pending requests' 和 not.toContain('ABCD1234')。— 失败场景：已用探针验证——让待处理列表替换按主体类型有条件执行的变异体在 34/34 测试绿色下通过；过期的待处理行仍显示且 Approve 按钮可用，再次点击会得到 channel_pairing_request_not_found 错误提示而非空状态。— 建议修复（在批准点击之后）：
expect(container.textContent).toContain('No pending requests');
expect(container.textContent).not.toContain('GROUP123');— qwen3.8-max via Qwen Code /review (v0.21.5)
| const snapshot = await listApprovals(revokeChannel); | ||
| if (mounted.current && currentChannelName.current === revokeChannel) { | ||
| setApprovedSenderIds(snapshot.senderIds); | ||
| setApprovedGroupIds(snapshot.groupIds ?? []); | ||
| } | ||
| return; |
There was a problem hiding this comment.
[Suggestion] R4-6: The group mirror line added to this channel_pairing_approval_not_found recovery path has no group-target test — all three revoke-failure tests use the user target paired-user only, and the new group test covers only the success path. — Failure scenario: probe-confirmed — dropping the setApprovedGroupIds line on this 404 path ships 25/25 green. Two admins holding the panel: one revokes a group the other just revoked → daemon 404 → the refresh silently resets only the user list, desyncing displayed group approvals until a manual refresh. — Suggested fix: add a group variant of the 404-refresh test — seed {senderIds:['paired-user'], groupIds:['group-7','group-8']}, reject revokeApproval with code:'channel_pairing_approval_not_found', assert the revoked group-7 row disappears while Revoke Group: group-8 and Revoke paired-user survive.
中文说明
这条 channel_pairing_approval_not_found 恢复路径新增的群镜像行没有群目标测试——三个撤销失败测试都只使用用户目标 paired-user，新的群测试只覆盖成功路径。— 失败场景：已用探针验证——删除该 404 路径上的 setApprovedGroupIds 行后 25/25 测试绿色。两位管理员同时操作面板：一人撤销了另一人刚撤销的群 → daemon 返回 404 → 刷新只静默重置了用户列表，显示的群审批与实际状态脱节，直到手动刷新。— 建议修复：新增 404 刷新测试的群变体——预置 {senderIds:['paired-user'], groupIds:['group-7','group-8']}，让 revokeApproval 以 code:'channel_pairing_approval_not_found' 拒绝，断言已撤销的 group-7 行消失而 Revoke Group: group-8 与 Revoke paired-user 仍在。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| expect(revokeApproval).toHaveBeenCalledWith('release-bot', { | ||
| groupId: 'group-7', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R4-7: This group-revoke test asserts no SURVIVING entries from the revoke result, so the success-path setApprovedGroupIds(result.groupIds ?? []) mirror is unpinned (the user-path test pins its mirror via toContain('second-user')). — Failure scenario: probe-confirmed — mutants setApprovedGroupIds(result.senderIds) (cross-wire) and setApprovedGroupIds([]) both ship 25/25 green; after a successful group revocation, all other approved groups silently vanish from the displayed list until a manual refresh. — Suggested fix: seed groupIds: ['group-7','group-8'], revoke result { revoked: 'group-7', senderIds: ['paired-user'], groupIds: ['group-8'] }, and assert Revoke Group: group-8 and paired-user still render.
中文说明
这个群撤销测试没有断言撤销结果中幸存的条目，因此成功路径上 setApprovedGroupIds(result.groupIds ?? []) 的镜像未被固化（用户路径测试通过 toContain('second-user') 固化了其镜像）。— 失败场景：已用探针验证——变异体 setApprovedGroupIds(result.senderIds)（错接）与 setApprovedGroupIds([]) 均在 25/25 测试绿色下通过；一次成功的群撤销后，其他已批准群会从显示列表中静默消失，直到手动刷新。— 建议修复：预置 groupIds: ['group-7','group-8']，撤销结果设为 { revoked: 'group-7', senderIds: ['paired-user'], groupIds: ['group-8'] }，断言 Revoke Group: group-8 与 paired-user 仍然渲染。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| {!approvalsLoading && | ||
| !approvalsError && | ||
| approvedSenderIds.length === 0 ? ( | ||
| approvedTargets.length === 0 ? ( |
There was a problem hiding this comment.
[Suggestion] R4-8: The approvals empty-state condition and header badge derive from approvedTargets, but no test pins either against a group-inclusive snapshot — the only group-only scenario asserts the revoke button exists, never that 'No pairing approvals' is absent, and no test asserts the badge count. — Failure scenario: probe-confirmed — mutants regressing either derivation to approvedSenderIds ship 34/34 green; a channel with only group approvals renders the "No pairing approvals" empty state directly above the listed, revocable group row, and the header badge shows 0 next to a non-empty list. — Suggested fix (in 'identifies the group and requesting member…', after the reload with { senderIds: [], groupIds: ['group-7'] }):
expect(container.textContent).not.toContain('No pairing approvals');中文说明
审批空状态条件与头部计数徽标都取自 approvedTargets，但没有任何测试在含群快照上固化二者——唯一的纯群场景只断言撤销按钮存在，从未断言 'No pairing approvals' 不出现，也没有测试断言徽标数字。— 失败场景：已用探针验证——把任一派生回退为 approvedSenderIds 的变异体在 34/34 测试绿色下通过；只有群审批的频道会在已列出、可撤销的群行上方渲染 "No pairing approvals" 空状态，头部徽标在非空列表旁显示 0。— 建议修复（在 'identifies the group and requesting member…' 中，以 { senderIds: [], groupIds: ['group-7'] } 重新加载之后）：
expect(container.textContent).not.toContain('No pairing approvals');— qwen3.8-max via Qwen Code /review (v0.21.5)
| setApprovalsLoading(true); | ||
| setApprovedSenderIds([]); | ||
| setApprovedGroupIds([]); |
There was a problem hiding this comment.
[Suggestion] R4-9: This new setApprovedGroupIds([]) reset in the approvals reload effect is pinned by no test — the channel-switch test seeds only senderIds: ['paired-user']. — Failure scenario: probe-confirmed — deleting this line ships 34/34 green. Admin views Channel A with an approved group, switches to Channel B whose listApprovals is slow — Channel A's stale group row stays visible AND clickable; clicking sends the revocation against the WRONG channel's store (404, silent refresh), showing the admin a live approval the selected channel never had. — Suggested fix: extend the channel-switch test to also seed groupIds: ['group-7'] on the first snapshot (assert the Revoke Group: group-7 button exists before the switch), then assert after switching: expect(container.textContent).not.toContain('group-7').
中文说明
审批重新加载 effect 中新增的这行 setApprovedGroupIds([]) 重置没有测试固化——频道切换测试只预置了 senderIds: ['paired-user']。— 失败场景：已用探针验证——删除该行后 34/34 测试绿色。管理员查看有已批准群的频道 A，切换到 listApprovals 较慢的频道 B——频道 A 的过期群行仍然可见且可点击；点击会把撤销请求发到错误频道的 store（404，静默刷新），管理员看到的是所选频道从未拥有的审批。— 建议修复：扩展频道切换测试，在第一个快照中同时预置 groupIds: ['group-7']（切换前断言 Revoke Group: group-7 按钮存在），切换后断言：expect(container.textContent).not.toContain('group-7')。
— qwen3.8-max via Qwen Code /review (v0.21.5)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/31156145032)._ |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| 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'; |
There was a problem hiding this comment.
[Suggestion] Extending GroupPolicy with 'pairing' makes it a first-class documented value, but the GitLab surface still asserts pairing cannot dispatch todos: the connect-time warning (packages/channels/gitlab/src/GitlabAdapter.ts:79-83 — must be "open" (or "allowlist" ...) for todos to be dispatched), the descriptor help text (packages/channels/gitlab/src/index.ts:35), and docs/users/features/channels/gitlab.md (config-table row, the ⚠️ groupPolicy Must Be "open" or "allowlist" section, and How-It-Works step 6). Probe-confirmed at HEAD: a GitLab todo under groupPolicy: 'pairing' DOES dispatch after one-time group approval (prompt calls 0 → approve → 1), because the adapter force-mentions todos, so pairing requests are created exactly as overview.md now documents — Failure scenario: operator configures a GitLab channel with groupPolicy: 'pairing', the false warning fires on every connect and the docs declare pairing cannot work, so the operator reverts to open (weaker posture) or files a bogus bug.
Suggested fix: accept 'pairing' in the warning condition/message, update the descriptor help text, and sweep gitlab.md to state that pairing dispatches todos after one-time group approval. (Related-but-distinct from R1-6, which covers the descriptor options list under assertManagedConfig.)
中文说明
为 GroupPolicy 新增 'pairing' 后它已成为一等公民并有文档支持，但 GitLab 侧仍断言 pairing 无法分发 todos：连接时告警（GitlabAdapter.ts:79-83 —— must be "open" (or "allowlist" ...) for todos to be dispatched）、descriptor 帮助文案（gitlab/src/index.ts:35）以及 gitlab.md（配置表行、⚠️ groupPolicy Must Be "open" or "allowlist" 小节、How-It-Works 第 6 步）。已在 HEAD 用探针证实：groupPolicy: 'pairing' 下 GitLab todo 在一次性群审批后确实会分发（prompt 调用 0 → 批准 → 1），因为 adapter 对 todo 强制标记 mention，配对请求会按 overview.md 新文档所述正常创建。失败场景：运营者为 GitLab 频道配置 groupPolicy: 'pairing'，每次连接都触发虚假告警且文档声称 pairing 不可用，于是回退到 open（更弱的权限姿态）或提交错误 bug。建议修复：在告警条件/文案中接受 'pairing'，更新 descriptor 帮助文案，并同步修订 gitlab.md，说明 pairing 在一次性群审批后可分发 todos。（与 R1-6 相关但不同：R1-6 针对 assertManagedConfig 下 descriptor 的 options 列表。）
— qwen3.8-max via Qwen Code /review (v0.21.5)
| expect(revokeApproval).toHaveBeenCalledWith('release-bot', { | ||
| senderId: 'paired-user', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The group pairing flow this PR ships cannot be exercised by the channels Playwright suite: the e2e mock daemon (packages/web-shell/client/e2e/utils/mockDaemon.ts) still models pairing approvals as a sender-ID-only map — its approve handler records approved.senderId (the initiating member) into the user approval list instead of approving the group, GET pairing-approvals never returns groupIds, and the DELETE handler reads only senderId, so the { groupId } revocation this test asserts the UI sends would yield 404 channel_pairing_approval_not_found. The suite already exercises user pairing end-to-end, so this is live infrastructure left newly divergent by the PR's contract changes — Failure scenario: any spec extending web-shell.channels.spec.ts for group pairing observes silently wrong behavior (wrong identity approved in the mock, empty group-approvals section, 404 on group revoke) instead of a clear harness gap.
Suggested fix: model group approvals in the mock — approve by approved.subject.id when approved.subject?.type === 'group', return { senderIds, groupIds } from GET pairing-approvals, and in DELETE read groupId when senderId is absent and reply { revoked, senderIds, groupIds }.
中文说明
本 PR 交付的群配对流程无法被 channels Playwright 套件验证：e2e mock daemon（client/e2e/utils/mockDaemon.ts）仍把配对审批建模为仅 sender-ID 的 map——approve 处理器把 approved.senderId（发起成员）记入用户审批列表而不是批准群组；GET pairing-approvals 从不返回 groupIds；DELETE 处理器只读 senderId，因此本测试所断言的 UI 新请求 { groupId } 会得到 404 channel_pairing_approval_not_found。该套件已在端到端验证用户配对，属于被本 PR 契约变更新近拉开差距的在用基础设施。失败场景：任何为群配对扩展 web-shell.channels.spec.ts 的用例都会观察到静默的错误行为（mock 中错误的身份被批准、群审批列表为空、群撤销 404），而不是明确的 harness 缺口。建议修复：在 mock 中建模群审批——当 approved.subject?.type === 'group' 时按 approved.subject.id 批准；GET 返回 { senderIds, groupIds }；DELETE 在无 senderId 时读取 groupId 并回复 { revoked, senderIds, groupIds }。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| | `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) | |
There was a problem hiding this comment.
[Suggestion] This row was updated for 'pairing', but every sibling enumeration in the per-channel and plugin docs was missed and now contradicts it. Verified locations: plugins.md:57 (standard-options table — normative for custom channels, and ChannelBase applies group pairing generically), telegram.md:73/:108, feishu.md:124/:173, dingtalk.md:118/:164, qqbot.md:103/:162, wecom.md:82 — group-setup steps and/or "Bot doesn't respond in groups" troubleshooting bullets still say "allowlist" or "open" only — Failure scenario: operator sets groupPolicy: "pairing" (valid, runtime-supported) whose group is simply not yet approved, hits a troubleshooting bullet declaring the value must be "allowlist" or "open", concludes pairing is invalid or broken, and switches to "open" (weaker posture) or files a bug; extension authors mirroring plugins.md omit pairing support.
Suggested fix: add "pairing" to these enumerations, noting the group must be approved before responses start.
中文说明
本行已为 'pairing' 更新，但各频道页与插件文档中的同类枚举全部遗漏，现与 overview 矛盾。已核实的位置：plugins.md:57（标准选项表——对自定义频道是规范性文档，且 ChannelBase 对群配对是通用实现）、telegram.md:73/:108、feishu.md:124/:173、dingtalk.md:118/:164、qqbot.md:103/:162、wecom.md:82——群配置步骤和/或"机器人在群里不响应"的排查条目仍只写 "allowlist" 或 "open"。失败场景：运营者设置 groupPolicy: "pairing"（合法且运行时支持），其群只是尚未获批，却看到排查条目声称该值只能是 "allowlist" 或 "open"，便认为 pairing 非法或已损坏，转而改用 "open"（更弱姿态）或提交 bug；照抄 plugins.md 的扩展作者会漏掉 pairing 支持。建议修复：在上述枚举中补上 "pairing"，并注明群需获批后才会开始响应。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| 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})`; |
There was a problem hiding this comment.
[Suggestion] This command now renders and approves group subjects, and the docs this PR adds direct operators to qwen channel pairing approve for groups — but the parent registration in packages/cli/src/commands/channel.ts:16 still declares describe: 'Manage DM pairing requests', so qwen channel --help misdescribes the command's scope at the precise moment product messaging routes group operators to it — Failure scenario: operator reads the DM-only help line and concludes the Group: … entry shown by pairing list cannot be approved from the CLI, detouring to the raw daemon API or filing a bug.
Suggested fix: describe: 'Manage DM and group pairing requests' in channel.ts:16.
中文说明
该命令现在会渲染并批准群组主体，且本 PR 新增文档引导运营者用 qwen channel pairing approve 批准群组——但父级注册（channel.ts:16）仍声明 describe: 'Manage DM pairing requests'，导致 qwen channel --help 在产品文案正把群运营者引向此处时错误描述了命令范围。失败场景：运营者看到"仅 DM"的帮助文案，认为 pairing list 中的 Group: … 条目无法通过 CLI 批准，转而使用原始 daemon API 或提交 bug。建议修复：将 channel.ts:16 改为 describe: 'Manage DM and group pairing requests'。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
|
||
| const code = generateCode(); | ||
| active.push({ senderId, senderName, code, createdAt: now }); | ||
| active.push({ senderId, senderName, subject, code, createdAt: now }); |
There was a problem hiding this comment.
[Suggestion] Subject-keyed dedup + the shared MAX_PENDING = 3 cap let one sender occupy every pending slot: dedup now matches on (subject.type, subject.id), so one member mentioning the bot once in each of three different unapproved groups creates three distinct group subjects that fill the shared cap — pre-PR, sender-keyed dedup guaranteed one sender ≤ one slot, so this diff introduced the monopoly — Failure scenario: every subsequent pairing attempt on that channel (any DM pairing, any other group) gets 'Too many pending pairing requests' for up to the 1-hour expiry, and there is no deny/reject/clear path (CLI has only list/approve; no daemon route deletes pending requests), so the only remedies are approving the unvetted groups, hand-editing <scope>/<channel>-pairing.json, or waiting it out. Under groupPolicy: 'pairing' bot presence in groups is not operator-gated (e.g. on Telegram anyone can add a bot to a group they create), so queue-jamming stays a concrete cost to third parties.
Suggested fix: additionally cap pending requests per initiating senderId, or add a pairing deny <name> <code> command / daemon route so operators can clear unvetted pending requests without approving them.
中文说明
按 subject 去重 + 共享的 MAX_PENDING = 3 上限使单个发送者可占满所有待处理槽位：去重现在按 (subject.type, subject.id) 匹配，因此同一成员在三个不同未批准群里各 @ 一次机器人即可产生三个不同的群主体并填满共享上限——PR 之前按 senderId 去重保证一个发送者最多占一个槽位，是本 diff 引入了该垄断。失败场景：此后该频道上的一切配对尝试（任何私聊配对、任何其他群）都会在最长 1 小时过期前得到"Too many pending pairing requests"；且没有 deny/reject/clear 途径（CLI 只有 list/approve，daemon 也没有删除待处理请求的路由），唯一补救是批准这些未审查的群、手改 <scope>/<channel>-pairing.json 或干等。groupPolicy: 'pairing' 下机器人入群不受运营者控制（例如 Telegram 上任何人都可把机器人拉进自建群），因此堵塞队列对第三方仍是实际代价。建议修复：对同一发起 senderId 的待处理请求额外设上限，或新增 pairing deny <name> <code> 命令/daemon 路由，让运营者无需批准即可清理未审查的待处理请求。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action needed on PR #8440 this roundTriage result
DecisionNo code changes were made this round and no commit is required. The cancelled 中文说明本轮 PR #8440 无需改动分诊结果
决定本轮未做任何代码改动，也无需提交。被取消的 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Maintainer verification — real-stack run of group pairingI built this PR locally and exercised it end to end on a real stack: the real Commit tested: Environment
1. Runtime behaviour (Reviewer Test Plan steps 1–3)One continuous session on a channel with
2. Workspace isolation (Reviewer Test Plan step 4)Same channel name, same
3. Revocation, and silence on stored-loop checksRevoked over the daemon API ( The "stored loop authorization checks stay silent" claim is the one I most wanted to see for real, so I drove it with an actual scheduled loop:
4. Daemon API and SDKAgainst the live daemon:
5. Web Shell UI (real daemon, real browser)
The requests are those the real runtime produced. Approving "Group: Release Team" in the browser wrote 6. Mutation tests — are these hunks load-bearing?Each mutation reverts exactly one PR hunk on the PR tree, rebuilds, and replays the same real message:
All three regress, so all three are necessary. M2 is worth highlighting: without the one-line 7. Authorization-domain separationI also probed the security property called out in the PR description. A DM sender whose ID is literally 8. Suites on this tree
Notes for the merge decision (none are blockers)
Verdict from my side: the runtime, store, CLI, daemon API, SDK, and UI all behave as the description says, the authorization boundaries hold under adversarial probing, and the changed lines are demonstrably necessary. LGTM. 中文版本Maintainer 验证 —— 群配对的真实环境端到端跑测我在本地构建了这个 PR，并在真实栈上做了端到端验证：真实的 测试的 commit： 环境
1. 运行时行为（测试计划第 1–3 步）在
2. 工作空间隔离（测试计划第 4 步）同一个频道名、同一个 3. 撤销，以及定时任务检查的静默性通过 daemon API 撤销（ "定时任务授权检查保持静默"这条我最想看到真实证据，所以用真正的定时任务跑了一遍（见上文第三张截图）： 4. Daemon API 与 SDK对着运行中的 daemon：
5. Web Shell UI（真实 daemon、真实浏览器）见上文三张 UI 截图。列表里的请求就是真实运行时产生的。在浏览器里批准 "Group: Release Team" 后，磁盘上的 6. 变异测试 —— 这些改动是否必要每个变异只在 PR 树上回退一个 hunk，重新构建，再重放同一条真实消息（见上文变异截图）。三个都出现回归，说明三处都必要。其中 M2 值得强调：少了 7. 授权域隔离我还验证了 PR 描述里提到的安全性质：让一个 ID 恰好是 8. 本树上的测试套件
合入前值得留意的点（都不是阻塞项）
我这边的结论：运行时、存储、CLI、daemon API、SDK 和 UI 的行为都与描述一致，授权边界在对抗性探测下成立，改动的每一行都被证明是必要的。LGTM。 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2995 passed · 0 failed · 2995 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2995 通过 · 0 失败 · 2995 总计 Verification reportPR 8440 Deep Verification —
|
| # | Cell (observable oracle) | base (fca8f3c1f) |
head (f46e265dd) |
|---|---|---|---|
| S0 | ambient group msg → pending / sent / prompts | 0 / 0 / 0 (dropped by mention gate) | 0 / 0 / 0 (pairing_trigger_required) |
| S1 | alice mentions bot → pending subject; notice | 1 per-user request (alice); DM-wording notice | 1 group request group:g1, initiator alice retained; group notice with code |
| S2 | bob (2nd member) mentions → requests; notices | 2nd per-user request; 2nd notice | same request reused; no 2nd notice |
| S3 | approve first code | only alice approved; bob still pending | group g1 approved; user allowlist untouched; pending 0 |
| S4 | bob mentions again → dispatched? | blocked, 3rd code notice | dispatched to agent (prompts=1), no new notice |
| S5 | DM from carol | user pairing request (unchanged flow) | user pairing request, subject user:carol (DM flow intact) |
| S6/S7 | revokeGroup('g1'); bob mentions |
no group API exists on base | revoked=true; blocked again; new group request + re-notice |
| S8/S9 | user vs group approvals on disk | single user allowlist file | separate -allowlist.json / -groups.json; group approval never adds members to user list |
Result: head 34/34 assertions pass; base 20/20 pass — including the cells
where base is expected to show per-user granularity (bob blocked after alice's
approval). That expected-broken behavior is exactly what flips on head, so the
change is load-bearing. Evidence: 01-ab-head-vs-base.png, 02-ab-head-arm.png.
Control caveat: on base, groupPolicy:'pairing' is not a value the base settings
validator accepts (its enum lacks it — tmp/base-tree/packages/cli/src/serve/channel-settings-store.ts);
the base arm drives ChannelBase directly, which tolerates the unknown policy by
falling through to mention-gating + per-user sender pairing. That is the honest
runtime semantics a pre-PR build would apply to the same config object.
Config-surface acceptance at head was verified over real HTTP in the route oracle
(cell W1: PUT /workspace/channels/gh-bot with groupPolicy:'pairing' → 200 via
the real github plugin validation; groupPolicy:'bogus-value' → 400).
Reviewer Test Plan, step by step
- One pending request identifies group + initiating member — A/B S1/S2
(subjectgroup:g1Release Team, initiator alice; bob's mention reuses the
same request and does not post a second notice) and route cell W4. ✅ - Approve → another member can use it; unapproved DM sender stays blocked —
A/B S3/S4/S5 (bob dispatched after one approval; carol's DM still pairs per-user;
on base this step cannot complete — bob stays blocked, which is the point of the PR). ✅ - Revoke by
groupIdblocks again; ambient + stored-loop checks create no
requests — A/B S6/S7; loop-contract probe L1–L4 (5/5:createPairingRequest:false
never creates a request, approved groups pass for any member, revocation denies
silently); the suite itself pins loop-job behavior under pairing
(ChannelBase.test.ts≈L18969/L19051). ✅ - Group approvals isolated by workspace — store-boundary B6 (two cwds, same
channel name: approval in A invisible in B) and route cell W11 (same channel
name in two workspaces over the qualified/workspaces/:workspaceroutes:
primarygroupIds=['team-chat-1'], secondary[]). ✅
Mutation matrix (vacuity + pinning)
Suites: GroupGate + PairingStore + SenderGate + ChannelBase (644 tests).
Unmutated control: 644/644 green (logs/mutation-control.log).
| Mutant | Change | Result | Pinning test(s) (first) |
|---|---|---|---|
| M0 (positive control) | remove same-subject dedup in createSubjectRequest |
killed (5 red) | reuses one pending request per group regardless of the initiating sender |
| M1 | delete GroupGate pairing branch | killed (14 red) | pairs a mentioned group once and lets other members use it |
| M2 | delete sender_pending guard |
killed (4 red) | limits each sender to one pending request across subjects |
| M3 | approve() consumes request before persisting approval (pre-PR order) | killed (1 red) | refuses to rebuild an unreadable group allowlist on approve — fails the intended assertion: expected [] to have a length of 1 but got +0 (pending request burned) |
| M4 | disable strict refusal of unreadable group allowlist | killed (1 red) | same durability test |
| M5 | remove approved-group sender-gate bypass in preflight | killed (5 red) | backfills messages from members of an approved paired group |
| M6 | ambient messages create requests | killed (3 red) | does not create group pairing requests from ambient messages |
No survivors; the positive control (M0) proves the harness can land a kill, and M3's
failure message shows the revert fails the behavioral assertion (pending request
consumed despite failed allowlist write), not an import/setup break.
Evidence: 06-mutation-matrix.png; raw logs logs/mutation-*.log. Tree restored
after each mutant (git status clean).
Wire-oracle harnesses
- Daemon routes over loopback HTTP (
route-oracle.mjs, 35/35,
04-route-wire-oracle.png): real compiled routes + real
createChannelManagementService+ realWorkspaceChannelSettingsStore+ real
PairingStorefiles; only the worker-manager (process lifecycle, unrelated to
pairing) and auth middleware are faked, mirroring the repo's own route tests.
Cells: upsert acceptsgroupPolicy:'pairing'and rejects bogus values; empty
snapshot; typed group subject + initiator on the wire; unknown code → 404;
approve →approved.subject.type==='group'; approvals snapshot
{senderIds:[], groupIds:['team-chat-1']}; DELETE with both/neither ids → 400
invalid_channel_pairing_subject; non-string/emptygroupId→ 400
invalid_channel_pairing_group_id; revoke bygroupId→ 200 and store agrees;
repeat revoke → 404; user revoke viasenderIdleaves group domain untouched;
non-pairing channel → 409; reads require bearer token (401);Cache-Control: no-store; unknown instance → 404; workspace isolation on the qualified routes. - PairingStore boundary (
store-boundary.mjs, 33/33,03-store-boundary.png):
legacy pending entries normalize to user subjects and approve cleanly (migration
claim); identifier collisiondup-idapproved/revoked independently in both
domains; shared 3-slot cap across users+groups (cap_reached);sender_pending
in both directions while a second member still reuses the group's code; corrupted
groups.json→ non-strict read treats as empty with stderr warning, strict
approve path throwsrefusing to rewrite unreadable group allowlistand the
pending request survives (code still usable after repair); workspace isolation;
../evilchannel name stays inside the scope dir; group requests expire at 1 h. - Stored-loop contract (
/tmp/loop-contract-probe.mjs, 5/5,
05-loop-contract.png):GroupGate.check(env, {createPairingRequest:false})
denies unapproved groups aspairing_trigger_requiredwithout creating requests,
passes any member of an approved group, and denies silently after revocation —
the exact contractisStoredLoopTargetAuthorizedrelies on.
Targeted gates (exact counts, all at head)
| Suite | Result |
|---|---|
packages/channels/base full (19 files) |
1004/1004 |
packages/cli — pairing, management service, routes, settings store, registry (5 files) |
92/92 |
packages/sdk-typescript full (32 files) |
1464/1464 |
packages/web-shell — ChannelEditorDialog + ChannelPairingRequests |
37/37 |
packages/channels/github (adapter mention-synthesis guards) |
203/203 |
packages/channels/gitlab |
59/59 |
packages/web-shell Playwright e2e web-shell.channels.spec.ts (incl. group approve/revoke UI) |
1/1 |
The author's claimed counts (605 / 87 / 318) are subsets of these full-suite
numbers; the full suites are green. Base-side rebuild compiled cleanly with
tsc --build (exit 0). Repo-wide build/typecheck/lint were already run by the
PR's own CI and were not re-run here.
Findings
- Nit — warning wording vs strict behavior. When
*-groups.jsonexists but is
unreadable, the non-strict read warns that stored approvals "will be lost on the
next approve" (PairingStore.readGroupAllowlist). But while the file stays
unreadable,approve()throws instead (the strict path this PR adds) — so no
approve can succeed and nothing is lost until the file becomes readable again
(e.g., manual repair/truncation, after which only post-repair approvals remain —
measured in B5). The operator-visible consequence is the same (approvals inactive
until fixed); only the "next approve" phrasing is slightly inaccurate. Measured,
no behavioral impact; no fix required.
No blocking findings. Specifically checked and not problems: drain-discard in
drainPendingGroupHistory (only reachable via a concurrent-revocation race, where
discarding is the fail-closed direction); GithubAdapter's synthesized-mention guard
for unapproved pairing groups (pinned by the adapter's own +230 test lines);
sender_pending group rejection wording deliberately not attributing a member's
pending request to the group.
Not covered
- Per-commit attribution: checkout is depth 2 (
git rev-parse --is-shallow-repository= true;git rev-list --count HEAD^1..HEAD^2returns 1
at the shallow boundary vs 13 commits in the metadata snapshot). The aggregate
HEAD^1..HEADdiff is what was verified; individual review-round commits were
not exercised separately. - Live platform E2E (real Telegram/GitHub/etc. traffic) — declared out of scope
by the PR itself; adapter behavior verified via their unit suites only. - Repo-wide typecheck/lint/prettier — covered by the PR's CI; not re-run.
- QR-based channel auth, Web Shell config UI beyond the changed components —
deferred by the PR to later staged PRs. - Drain-discarding of group history under adversarial scheduling — analyzed
statically (fail-closed); no dedicated harness cell. - Web-shell Playwright run emits vite proxy
ECONNREFUSEDnoise for routes the
mock daemon intercepts; the spec passed regardless (this is the spec's designed
mock seam, not a failure).
Methodology
Environment: CI verify container (node:22-bookworm), working tree = merge commit
933f926600 (refs/pull/8440/merge), HEAD^1 = base tip, HEAD^2 = PR head.
npm ci + npm run build pre-ran at HEAD. Base control: git worktree add tmp/base-tree HEAD^1, root node_modules symlinked in (lockfile untouched by the
PR), packages/channels/base recompiled there with tsc --build (exit 0); base
dist asserted free of group-pairing code and of any @​qwen-code/* import (the
package's sole runtime dep is @​agentclientprotocol/sdk), realpath of the base
dist confirmed inside the base tree. Harnesses drove compiled dist/ output with
real temp QWEN_HOME filesystems and real loopback HTTP — no stubs of code under
test; the probe channel implements only abstract platform methods, the route oracle
fakes only worker-lifecycle and auth middleware (as the repo's own tests do).
Mutations applied to source, suites run, git checkout -- restored after each
(worktree verified clean at the end). Raw per-cell logs live in logs/
(mutation-*.log, sdk-suite.log); harness scripts in this directory
(ab-harness.mjs, store-boundary.mjs, route-oracle.mjs, run-mutations.sh).
Assertion accounting: harness assertions 127 (34+20+33+35+5) + gate tests 2860
(1004+92+1464+37+203+59+1) + mutation checks 8 (1 control-green + 7 kills) =
2995 pass, 0 fail.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round on PR #8440: no action neededThis round found no actionable feedback on PR #8440 (group pairing), so no Feedback triage
Why finding #6 was declinedThe sandboxed verification's only finding is a wording nit: the non-strict read Both verifications tested and approved the exact current head State
中文说明PR #8440 的 Autofix 评审轮次：无需处理本轮在 PR #8440（群配对）上没有发现任何可处理的反馈，因此未修改任何代码、未产生任何提交。分支保持在已验证的 head 反馈分类
为什么拒绝第 6 条沙箱验证唯一的发现是一个措辞 nit：当 两次验证测试并批准的都是当前这个 head（ 状态
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /review |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2996 passed · 0 failed · 2996 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2996 通过 · 0 失败 · 2996 总计 Verification reportPR 8440 Deep Verification (follow-up round) —
|
| # | Previous finding | Severity | Status at new head 9c7144211b |
|---|---|---|---|
| 1 | Warning wording vs strict behavior: readGroupAllowlist non-strict path warns stored group approvals "will be lost on the next approve", but while the file stays unreadable approve() throws (strict path) so no approve can succeed and nothing is lost |
nit | stands (unchanged), re-measured. Cell B9: the warning text is byte-identical at the new head, and 0/3 approve attempts succeeded while chan-groups.json was corrupted (each threw refusing to rewrite unreadable group allowlist); the pending code stayed usable and approved after repair (B5). Behaviorally harmless; wording still slightly inaccurate. |
Previously "checked and not problems" — re-verified at the new head: drain-discard in drainPendingGroupHistory remains fail-closed (static re-read of the unchanged hunk; no dedicated cell — see Not covered); GithubAdapter synthesized-mention guard re-pinned by the adapter gate (203/203, +230 test lines from the PR); sender_pending group rejection wording (deliberately not attributing a member's pending request to the group) unchanged in source and covered by the ChannelBase suite.
Delta since the previous round
The new head 9c7144211b is a pure merge whose parents are the previously verified head f46e265dd and the current base tip 650e085fec (a core transcript-pagination fix touching no channel/cli/sdk/web-shell surface). Three facts establish the verified surface is unchanged: tree(HEAD) == tree(HEAD^2) (c30cb603… — the CI pull-merge added nothing), the effective diff stat is identical to the previous round's aggregate (54 files, +2795/−308), and the PR touches no package.json/package-lock.json. The parent commits themselves are unreachable (depth-2 checkout), so the identity rests on those three facts plus full re-measurement rather than on a direct f46e265dd..9c7144211b diff.
Scope selection
- Central claim: with
groupPolicy: 'pairing', one mention/reply from an unapproved group creates exactly one group pairing request (stable chat ID + initiating member retained); approving the code grants every member access; revoking bygroupIdblocks the group again; DMs keep followingsenderPolicy. - Secondary 1: ambient (unmentioned, non-reply) messages and stored-loop authorization checks never create pairing requests.
- Secondary 2: user and group approvals are domain separated (no identifier collision crosses) and stay workspace-scoped.
Central claim — A/B load-bearing proof
Both arms run the same config (groupPolicy:'pairing', senderPolicy:'pairing') and the same envelope sequence against the compiled channel-base dist of each tree (HEAD vs a HEAD^1 worktree rebuilt with tsc --build, exit 0). Isolation asserted before trusting the control: base dist contains 0 occurrences of createGroupRequest/pairing_trigger_required (head dist contains both), and readlink -f of the loaded base ChannelBase.js resolves inside the base worktree. The package's only runtime dependency is @agentclientprotocol/sdk (no @qwen-code/* imports), and the PR leaves the lockfile untouched, so reusing the root node_modules is a clean control. Probe channel implements only the three abstract platform methods; all gating/pairing/store logic is each tree's own compiled code against a real temp QWEN_HOME. Harness: ab-harness.mjs. Witness: 01-ab-head-vs-base.png.
| # | Cell (observable oracle) | base (650e085fec) |
head (9c7144211b) |
|---|---|---|---|
| S0 | ambient group msg → pending / sent / prompts | 0 / 0 / 0 (mention-gate drop) | 0 / 0 / 0 (pairing_trigger_required) |
| S1 | alice mentions bot → pending subject; notice | 1 per-user request, no typed subject; DM wording | 1 group request group:g1 "Release Team", initiator alice/Alice retained; group-wording notice with code |
| S2 | bob (2nd member) mentions → requests; notices | 2nd per-user request; 2nd notice | same request reused (same code); no 2nd notice |
| S3 | approve first code | only alice approved; bob still pending (1 left) | group g1 approved; user allowlist untouched; pending 0 |
| S4 | bob mentions again → dispatched? | blocked, 3rd code notice, prompts 0 | dispatched (prompts 1); extra send is the agent reply, not a pairing notice |
| S5 | DM from carol | user pairing request; notice | user pairing request, subject user:carol; DM wording intact |
| S6 | revoke g1 |
no group API exists (typeof revokeGroup === 'undefined') |
revokeGroup('g1') === true; group no longer approved |
| S7 | bob mentions after revocation | still blocked per-user; re-noticed | blocked again; new group request + re-notice with new code |
| S8 | on-disk domains | no -groups.json ever written |
dedicated -groups.json; user allowlist file empty/absent — no cross-leak |
Result: head 26/26 assertions pass; base 18/18 pass — the base cells encode the expected per-user behavior (bob blocked after alice's approval), and that expected-broken behavior is exactly what flips on head, so the change is load-bearing.
Control caveat (same as previous round, re-verified): on base the settings validator's groupPolicy enum lacks 'pairing' (channel-settings-store.ts at HEAD^1); the base arm drives ChannelBase directly, which tolerates the unknown policy by falling through to mention-gating + per-user sender pairing — the honest runtime semantics a pre-PR build applies to the same config object. Config-surface acceptance at head was verified over real HTTP (cell W1: upsert with groupPolicy:'pairing' → 200 through the real github plugin descriptor; 'bogus-value' → 400 channel_settings_invalid_config).
Reviewer Test Plan, step by step
- One pending request identifies group + initiating member — A/B S1/S2 (subject
group:g1Release Team, initiator alice; bob's mention reuses the request, no second notice) and wire cell W3. ✅ - Approve → another member can use it; unapproved DM sender stays blocked — A/B S3/S4/S5 (bob dispatched after one approval; carol's DM still pairs per-user; on base this step cannot complete — bob stays blocked, which is the point of the PR). ✅
- Revoke by
groupIdblocks again; ambient + stored-loop checks create no requests — A/B S6/S7; loop-contract probe head 10/10 (the realisStoredLoopTargetAuthorizedpredicate denies unapproved groups, passes any member once approved, and never creates a request — even when the check synthesizes a mention/reply envelope); base contrast 6/6 (group loop authz required per-user approval). ✅ - Group approvals isolated by workspace — store cell B6 (two cwds, same channel name: approval in A invisible in B) and wire cells W16 (same channel name in two workspaces over the qualified
/workspaces/:workspaceroutes: secondarygroupIds=['other-team'], primary['team-chat-2']; qualified revoke works). ✅
All four steps were performable — no unreachable step found this round.
Mutation matrix (vacuity + pinning)
Suites: GroupGate + PairingStore + SenderGate + ChannelBase (644 tests). Unmutated control: 644/644 green in 14 s. Each mutant = one exact string replacement (asserted to match exactly once), suite run, git checkout -- restore; runner run-mutations.mjs, raw logs logs/mutation-*.log. The matrix was run twice (second run produced the capture); both runs byte-identical in red counts. Witness: 05-mutation-matrix.png.
| Mutant | Change | Result | First failing test (behavioral) |
|---|---|---|---|
| M0 (positive control) | remove same-subject dedup in createSubjectRequest |
killed (5 red) | reuses one pending request per group regardless of the initiating sender |
| M1 | delete GroupGate pairing branch | killed (14 red) | does not create a pairing request for ambient group messages |
| M2 | delete sender_pending guard |
killed (4 red) | limits each sender to one pending request across subjects |
| M3 | approve() consumes request before persisting approval (pre-PR order) | killed (1 red) | refuses to rebuild an unreadable group allowlist on approve — fails the intended assertion: expected [] to have a length of 1 but got +0 (pending request burned despite failed allowlist write) |
| M4 | disable strict refusal of unreadable group allowlist (readGroupAllowlist(true) → false) |
killed (1 red) | same durability test |
| M5 | remove approved-group sender-gate bypass in preflight | killed (5 red) | backfills messages from members of an approved paired group |
| M6 | ambient messages create requests | killed (3 red) | does not create a pairing request for ambient group messages |
No survivors. M0 proves the harness can land a kill; M3/M4 failures quote expected-versus-actual values (behavioral, not import/setup breaks). Tree verified clean after the run (git status --porcelain empty).
Wire-oracle harnesses (all mock-free against compiled dist/)
- Daemon routes over loopback HTTP (
route-oracle.mjs, 24/24, witness03-route-wire-oracle.png): real compiledregisterWorkspaceChannelManagementRoutes+ realcreateChannelManagementService+ realWorkspaceChannelSettingsStore+ realPairingStorefiles, driven withfetchover a real127.0.0.1socket; only the worker manager (process lifecycle, irrelevant to pairing —committedChannelNames() = []keeps every channelstoppedandreloadWorkspaceuncalled) and the auth middleware are faked, mirroring the repo's own route tests. Cells: upsert acceptsgroupPolicy:'pairing'via the real github descriptor / rejects bogus values; empty snapshot +Cache-Control: no-store; typed group subject + initiator on the wire; unknown code → 404channel_pairing_request_not_found(case accepted: approval used a lowercase code); approve →approved.subject.type==='group'; approvals snapshot{senderIds:[], groupIds:['team-chat-1']}; DELETE with both/neither id → 400invalid_channel_pairing_subject; empty/non-stringgroupId→ 400invalid_channel_pairing_group_id; revoke bygroupId→ 200 and the store file agrees; repeat revoke → 404; user revoke leaves group domain untouched; non-pairing channel → 409channel_pairing_not_enabled; no bearer → 401; unknown instance → 404; unknown workspace selector → 400workspace_mismatch(rejected before store access); workspace isolation on qualified routes both directions. - PairingStore boundary (
store-boundary.mjs, 27/27, witness02-store-boundary.png): legacy global files (pending without subject, user allowlist, group allowlist) all grandfathered into the scoped dir with a sentinel; legacy pending normalizes to a user subject and approves cleanly; identifier collisiondup-idapproved/revoked independently in both domains in all four orders; shared 3-slot cap across users+groups;sender_pendingboth directions and across domains while a second member still reuses the group code; corruptedgroups.json→ non-strict read empty with stderr warning, strict approve throwsrefusing to rewrite unreadable group allowlist, pending request survives (code approves after repair); workspace isolation;../evilchannel name stays inside the scope dir (..%2Fevil-*.json); group requests expire at 1 h and the slot is reusable; B9 nit re-measurement (wording stands, 0/3 approves succeed while unreadable). - Stored-loop contract (
loop-contract.mjs, head 10/10 + base contrast 6/6, witness04-loop-contract-both-arms.png): drives the real privateisStoredLoopTargetAuthorized(target, senderName)— the exact predicate loop jobs call — on both arms. Head: unapproved group denied with zero requests created; approved group passes for a member never seen; revoked group denied silently; DMs followsenderPolicy; group approval alone authorizes loops without any per-user pairing. Base contrast: group loop targets pass only for individually paired members, no group API exists — the control was run on both arms as required.
Targeted gates (exact counts, all at head)
| Suite | Result |
|---|---|
packages/channels/base full (19 files) |
1004/1004 |
packages/cli — pairing, management service, routes, settings store, registry (5 files) |
92/92 |
packages/sdk-typescript full (32 files) |
1464/1464 |
packages/web-shell — ChannelEditorDialog + ChannelPairingRequests |
37/37 |
packages/channels/github |
203/203 |
packages/channels/gitlab |
59/59 |
packages/webui — daemon workspace actions |
16/16 |
packages/web-shell Playwright e2e web-shell.channels.spec.ts |
1/1 |
Gate liveness proven, not assumed: for channels/base, the mutation matrix itself is the plant — the identical vitest invocation went red under 7 mutants (up to 14 failing tests). For the cli gate, an additional plant: changing the group subject line in pairing.ts from Group: … to Sender: … turned exactly 1 pairing.test.ts test red (1 failed | 4 passed); source restored, tree clean. The author's claimed counts (605 / 87 / 318) remain subsets of these full-suite numbers.
Findings
- Nit (carried from previous round, stands) — warning wording vs strict behavior in
PairingStore.readGroupAllowlist: while*-groups.jsonexists but is unreadable, the non-strict read warns approvals "will be lost on the next approve", butapprove()throws on the strict path until the file is readable, so nothing is lost to an approve (B9: 0/3 succeeded). Operator-visible consequence unchanged (approvals inactive until repair); only the phrasing is slightly inaccurate. No behavioral impact; no fix required.
No blocking findings. No new findings introduced by the delta merge.
Not covered
- Per-commit attribution: checkout is depth 2;
git rev-list --count HEAD^1..HEAD^2returns 1 at the shallow boundary vs 14 commits in the metadata snapshot, and the new head's parents (f46e265dd,650e085fec— the latter reachable only asHEAD^1) are otherwise unreachable. The aggregateHEAD^1..HEADdiff is what was verified; per-commit review-round attribution stays out of reach (same as previous round). - Direct diff against the previously verified head:
f46e265ddis not an object in this clone, sogit diff f46e265dd..9c7144211bcould not be computed; the "delta is only the main merge" conclusion rests ontree(HEAD)==tree(HEAD^2), identical diff stat, and the merge's parentage — plus full re-measurement regardless. - Live platform E2E (real Telegram/GitHub/etc. traffic) — declared out of scope by the PR; adapter behavior verified via their unit suites only.
- Repo-wide typecheck/lint/prettier — covered by the PR's own CI; not re-run.
- QR-based channel auth, Web Shell config/approval UI beyond the changed components — deferred by the PR to later staged PRs.
- Drain-discarding of group history under adversarial scheduling — static re-read only (fail-closed direction:
drain()runs, then entries are discarded if the group is not approved at that moment; a concurrent revocation can only discard, never leak). No dedicated harness cell. - Web-shell Playwright run emits vite proxy
ECONNREFUSEDnoise for routes the mock daemon intercepts; the spec passed regardless (its designed mock seam, not a failure).
Methodology
Environment: CI verify container (node:22-bookworm), working tree = merge commit 19595f0361 (refs/pull/8440/merge), HEAD^1 = base tip 650e085fec, HEAD^2 = PR head 9c7144211b. npm ci + npm run build pre-ran at HEAD. Base control: git worktree add tmp/base-tree HEAD^1, root node_modules symlinked (PR touches no lockfile; base package's sole runtime dep is @agentclientprotocol/sdk), packages/channels/base recompiled there with tsc --build (exit 0); base dist asserted free of group-pairing code and realpath-confirmed inside the base tree; worktree force-removed after the A/B cells and captures were complete. Harnesses drove compiled dist/ output with real temp QWEN_HOME filesystems and real loopback HTTP — no stubs of code under test; the probe channel implements only abstract platform methods, and the route oracle fakes only worker-lifecycle and auth middleware (as the repo's own tests do). Mutations were applied to source via exact single-occurrence string replacements, suites run, git checkout -- restored after each (worktree verified clean; matrix run twice, identical). Raw per-cell logs live in logs/ (mutation-*.log, gate-*.log, ab-*.stderr.log); harness scripts in this directory (ab-harness.mjs, store-boundary.mjs, route-oracle.mjs, loop-contract.mjs, run-mutations.mjs). Evidence images were produced with scripts/verify-capture.mjs. Assertion accounting: harness assertions 111 (26+18 A/B, 27 store, 24 routes, 16 loop [head 10 + base contrast 6]) + mutation checks 8 (1 control-green + 7 kills) + gate-liveness plant 1 + gate tests 2876 (1004+92+1464+37+203+59+16+1) = 2996 pass, 0 fail.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
BenGuanRan
left a comment
BenGuanRan
left a comment
There was a problem hiding this comment.
Reviewed the latest head. Verified all prior Critical findings against the current code: subject-aware Web Shell group approval/revocation, collision-free group allowlist storage, the effective group-policy UI gate, and GitHub aggregate-lane behavior for both approved follow-ups and unapproved ambient comments. The exact head passed the Ubuntu test job and Web Shell E2E; the skipped CLI no-sandbox integration job is noted. No blocking findings.
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Reset premature approval; code review and review comment must come first.
|
Review of head Prior Critical findings — all verified fixed at this head
Also confirmed addressed: R9-15 (group-specific Impact on existing flows
Memory / disk
CI at this head: 中文说明在 head 既有 Critical 均已修复：R1-1（Web Shell 面板已按 subject 渲染/审批/撤销群请求，含测试）；R1-10（群 allowlist 改为 对已有链路的影响：所有运行时行为变更均由新增的 内存/磁盘：均有界——pending 上限 3 且 1 小时过期清理；allowlist 为小 JSON（群 allowlist 原子写）；群历史受 该 head 的 CI：Ubuntu Node 22 测试通过（15m57s）；macOS/Windows/CLI 无沙箱集成 job 跳过。 |
BenGuanRan
left a comment
BenGuanRan
left a comment
There was a problem hiding this comment.
Approved after verifying all prior Critical findings fixed at head 9c71442 (details in the review comment above): subject-aware Web Shell panel, collision-free group allowlist storage, effective group-policy UI gate, and GitHub aggregate/directed lane behavior. Existing flows unaffected (all changes gated on the new groupPolicy: 'pairing'), memory/disk bounded. Ubuntu Node 22 test job green at this head; CLI no-sandbox integration job remains skipped.
5 participants
What this PR does
Adds
pairingas agroupPolicyvalue so a group chat can be approved once by its stable chat ID and then used by every member. Group pairing requests share the existing pairing-code flow, retain the initiating sender for audit context, and store approvals separately from user approvals within the selected workspace. The daemon API and TypeScript SDK now expose typed user/group subjects, group approval snapshots, and group revocation.The runtime only creates a group pairing request after a real mention or reply. Ambient messages and stored loop authorization checks stay silent. Once a group is approved, group history and scheduled loop authorization follow the group approval while direct messages continue to follow
senderPolicy.Why it's needed
User pairing is too granular for group chats: requiring every member of one group to pair separately is repetitive and does not match the group-level trust decision. This provides one approval per group without introducing another top-level policy field or weakening direct-message access control.
Reviewer Test Plan
How to verify
groupPolicy: "pairing", mention or reply to the bot from an unknown group, and confirm one pending request identifies the group plus the initiating member.groupId, confirm the group is blocked again, and verify ambient messages or stored loop checks do not create a new pairing request.Evidence (Before & After)
N/A — this PR adds runtime, daemon API, SDK, CLI, and documentation support. The Channel management UI is intentionally deferred to the next staged PR.
Tested on
Environment (optional)
Node.js workspace with package-local Vitest suites. Verified 605 Channel Base tests, 87 CLI service/route tests, and 318 TypeScript SDK tests, plus the repository build, typecheck, lint, Prettier check, and diff check.
Risk & Scope
Linked Issues
Part of #7209
中文说明
本 PR 做了什么
为
groupPolicy增加pairing值，使群聊可以按稳定的 chat ID 只审批一次，随后群内所有成员都能使用。群配对复用现有配对码流程，保留发起成员用于审计，并在所选工作空间内将群审批与用户审批分开存储。daemon API 和 TypeScript SDK 现在会暴露带类型的用户/群主体、群审批快照以及群撤销能力。运行时只会在真实 @ 机器人或回复机器人时创建群配对请求。普通群消息和后台定时任务授权检查都会保持静默。群审批后，群历史和定时任务授权会跟随群审批，而私聊仍继续遵循
senderPolicy。为什么需要
用户级配对不适合群聊：要求同一个群的每个成员分别配对既重复，也不符合按群建立信任的决策方式。本 PR 让每个群只需审批一次，同时不引入新的顶层策略字段，也不会放宽私聊访问控制。
Reviewer 测试计划
如何验证
groupPolicy: "pairing"，从未知群 @ 或回复机器人，确认只产生一个待审批请求，并同时标识群和发起成员。groupId撤销审批，确认该群重新被拦截，并确认普通群消息或定时任务检查不会自动生成新的配对请求。前后对比证据
N/A — 本 PR 只增加运行时、daemon API、SDK、CLI 和文档能力。频道管理 UI 会在下一个分阶段 PR 中处理。
测试平台
环境（可选）
Node.js workspace，使用各 package 的 Vitest。已验证 605 个 Channel Base 测试、87 个 CLI service/route 测试和 318 个 TypeScript SDK 测试，并通过仓库 build、typecheck、lint、Prettier 检查和 diff 检查。
风险与范围
关联 Issue
Part of #7209