Skip to content

feat(channels): add pairing approval management API - #8045

Merged
wenshao merged 2 commits into
QwenLM:mainfrom
qqqys:codex/issue-7209-channel-pairing-access-api
Jul 29, 2026
Merged

feat(channels): add pairing approval management API#8045
wenshao merged 2 commits into
QwenLM:mainfrom
qqqys:codex/issue-7209-channel-pairing-access-api

Conversation

@qqqys

@qqqys qqqys commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR adds workspace-scoped APIs for listing senders approved through pairing mode and revoking one approval. It exposes the same contract for the primary workspace and an explicitly selected workspace, and adds matching TypeScript SDK helpers.

Revocation updates only the selected workspace's pairing allowlist. It never falls back to another runtime or mutates the legacy process-global allowlist.

Why it's needed

The Channel management UI can already review and approve pending pairing requests, but it has no supported backend contract for showing previously approved senders or removing their access. This API is the backend slice required for a follow-up UI PR.

Reviewer Test Plan

How to verify

  1. Configure a Channel instance in pairing mode and approve one sender. Listing pairing approvals should return that sender ID.
  2. Revoke the sender. The response should contain the revoked ID and an updated empty approval snapshot; repeating the revocation should return 404 channel_pairing_approval_not_found.
  3. Configure the same Channel name in two workspaces and approve the same sender in both. Revoking it from one workspace must leave the other workspace approved.
  4. Verify that missing bearer authentication, an invalid daemon client ID, an untrusted workspace, a non-pairing Channel, and an empty sender ID all fail before the selected allowlist is mutated.

Evidence (Before & After)

N/A — this is a daemon and SDK contract with no UI changes.

Automated verification passed locally: 23 pairing-store tests, 43 daemon service/route tests, 2 focused SDK route tests, full build, full typecheck, and full lint.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Node.js 22 workspace install; daemon route tests used Supertest. No live IM provider was required for this backend contract.

Risk & Scope

  • Main risk or tradeoff: the existing allowlist stores sender IDs only, so the API cannot return display names without a separate storage migration.
  • Not validated / out of scope: Web Shell UI, QR setup, CLI commands, and a live DingTalk/WeCom/Feishu message round trip.
  • Breaking changes / migration notes: none; the routes and SDK methods are additive, and revocation preserves the legacy migration baseline.

Linked Issues

Part of #7209.

中文说明

本 PR 做了什么

本 PR 新增了 workspace-scoped 的配对授权接口,用于列出通过配对模式获准的 sender,以及撤销某个 sender 的授权。主工作区和显式指定的工作区使用同一套契约,同时补齐对应的 TypeScript SDK helper。

撤销只更新所选工作区的配对 allowlist,不会回退到其他 runtime,也不会修改进程级 legacy 全局 allowlist。

为什么需要

频道管理 UI 已经能够查看并批准待处理的配对请求,但还没有受支持的后端契约来展示历史已授权 sender 或移除其访问权限。这个 API 是后续 UI PR 所需的后端切片。

Reviewer 测试计划

如何验证

  1. 将一个频道实例配置为配对模式并批准一个 sender。查询配对授权列表时应返回该 sender ID。
  2. 撤销该 sender。响应应包含被撤销的 ID 和更新后的空授权快照;再次撤销应返回 404 channel_pairing_approval_not_found
  3. 在两个工作区中配置同名频道,并在两边批准同一个 sender。只在一个工作区撤销时,另一个工作区仍应保持授权。
  4. 验证缺少 bearer 认证、无效 daemon client ID、不可信工作区、非配对模式频道和空 sender ID 都会在修改所选 allowlist 之前失败。

前后证据

N/A——这是 daemon 和 SDK 契约变更,没有 UI 改动。

本地自动化验证已通过:23 个 pairing-store 测试、43 个 daemon service/route 测试、2 个聚焦 SDK route 测试,以及完整 build、typecheck 和 lint。

测试平台

系统 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境

Node.js 22 workspace 安装;daemon route 测试使用 Supertest。本后端契约不需要真实 IM provider。

风险与范围

  • 主要风险或取舍:现有 allowlist 只保存 sender ID,因此如果不做额外存储迁移,API 无法返回展示名称。
  • 未验证 / 不在范围内:Web Shell UI、扫码配置、CLI 命令,以及真实钉钉、企业微信或飞书消息回环。
  • 破坏性变更 / 迁移说明:无;路由和 SDK 方法均为增量变更,撤销会保留 legacy 迁移基线。

关联 Issue

#7209 的一部分。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: this is a feature addition, not a bug fix — the Channel management UI can approve pairing requests but has no backend contract for listing or revoking previously approved senders. Part of #7209. The gap is real and the follow-up UI work depends on this slice.

Direction: aligned. Additive workspace-scoped API for the daemon's channel management surface, consistent with the existing pairing-request routes. CHANGELOG has no direct reference but the channel management area is actively developed.

Size: cross-package (channels, cli, sdk-typescript) but only 176 production logic lines — 14 in PairingStore, 86 in service + routes, 76 in SDK. Test lines: 178. Well within bounds.

Approach: scope feels right. Each layer (store → service → routes → SDK → docs) gets exactly the additions it needs. No unrelated changes, no drive-by refactors. The revoke method is a simple filter-and-write, the service wraps it with proper error codes, and the routes follow the existing pairing-request pattern exactly.

Risk: no elevated risk signals.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:这是功能新增,不是 bug 修复——频道管理 UI 已能批准配对请求,但缺少列出或撤销已授权 sender 的后端契约。属于 #7209 的一部分,后续 UI 工作依赖此切片。

方向:对齐。为 daemon 频道管理面增加 workspace-scoped 的增量 API,与现有配对请求路由一致。

规模:跨包(channels、cli、sdk-typescript)但仅 176 行生产逻辑代码。测试 178 行。远在限制之内。

方案:范围合理。每一层(store → service → routes → SDK → docs)都恰好只加了所需的增量。无无关改动。

风险:无升级风险信号。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at dbe6d7312193317ae21c202758807a7ab782b08f · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Code review

Independent proposal first: given "add workspace-scoped APIs for listing and revoking pairing approvals," I would add a revoke(senderId) method to PairingStore, extend the channel management service with pairingApprovals / revokePairingApproval, register GET + DELETE routes at /channels/:name/pairing-approvals, add SDK types and client methods, and update the protocol doc. Exactly what this PR does.

The implementation follows existing patterns at every layer — no surprises:

  • PairingStore.revoke is a clean filter-and-write with a boolean return. The comment update in migrateLegacyState correctly reflects the new semantics: revocation touches only the scoped allowlist, never the legacy global baseline. The test proves workspace isolation and legacy preservation.
  • Service layer wires revoke through inMutationLane() (consistent with approvePairing), throws channel_pairing_approval_not_found on miss, and returns the updated snapshot.
  • Routes mirror the existing pairing-request routes: same auth middleware, same noStore, same parseInstanceNamevalidateClient → handler flow. The new parsePairingSenderId validates non-empty string, matching the parsePairingCode pattern. Error code maps to 404 in ERROR_STATUS.
  • SDK adds three types and two methods on each client class. The DELETE method on DaemonClient uses CHANNEL_CONTROL_DEFAULT_TIMEOUT_MS, consistent with other mutations. WorkspaceDaemonClient delegates through channelRequest with { method: 'DELETE', body }.
  • Tests cover the important paths: workspace isolation, legacy baseline preservation, 404 on double-revoke, auth rejection, invalid client ID, empty sender ID, untrusted workspace, and the public type surface.

No correctness issues, no security concerns, no convention violations.

Testing

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped
Test (windows-latest, Node 22.x) ⏭️ skipped
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
precheck-pr / precheck ✅ success
Classify PR ✅ success

macOS and Windows tests are skipped (fork PR limitation). The Ubuntu suite and the Serve A/B job both pass. The Serve A/B result is particularly relevant — it exercises the daemon routes this PR adds. A maintainer also verified locally against a real daemon (see comments below).

中文说明

代码审查

独立方案:给定"添加 workspace-scoped 的配对授权管理 API",我的方案与 PR 完全一致——在 PairingStore 加 revoke,扩展 service 层,注册 GET + DELETE 路由,补齐 SDK 类型和客户端方法,更新协议文档。

实现逐层遵循现有模式,无意外:

  • PairingStore.revoke 是简洁的 filter-and-write,返回布尔值。migrateLegacyState 中的注释更新正确反映了新语义:撤销只触及 scoped allowlist,不修改 legacy 全局基线。测试证明了 workspace 隔离和 legacy 保留。
  • Service 层 通过 inMutationLane() 包装 revoke(与 approvePairing 一致),未找到时抛出 channel_pairing_approval_not_found,返回更新后的快照。
  • 路由 镜像现有配对请求路由:相同的认证中间件、noStoreparseInstanceNamevalidateClient → handler 流程。
  • SDK 新增三个类型和每个客户端类两个方法。DELETE 方法使用 CHANNEL_CONTROL_DEFAULT_TIMEOUT_MS,与其他变更操作一致。
  • 测试 覆盖重要路径:workspace 隔离、legacy 基线保留、重复撤销 404、认证拒绝、无效 client ID、空 sender ID、不可信工作区、公共类型面。

无正确性问题、无安全隐患、无规范违反。

测试

Ubuntu 测试套件和 Serve A/B 均通过。macOS/Windows 因 fork PR 限制跳过。维护者已在本地对真实 daemon 进行了验证(见下方评论)。

Qwen Code · qwen3.8-max-preview

Reviewed at dbe6d7312193317ae21c202758807a7ab782b08f · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — clean, convention-following additive API; every layer does exactly what it needs to and nothing more.

This is what a well-scoped backend slice looks like. The PR adds two endpoints, threads them through three layers, and stops. No abstractions that aren't already there, no scope creep, no "while I'm here" edits. The workspace isolation semantics are correct and tested — revoking in one workspace leaves the other (and the legacy baseline) untouched, which is the whole point of the scoping work in #7017.

The maintainer's local verification against a real daemon (comments below) confirms the contract works end-to-end, and the Serve A/B CI job exercises the new routes on the base-vs-head comparison. Ubuntu suite is green; macOS/Windows skips are a fork CI limitation, not a PR concern.

Would merge without hesitation.

中文说明

置信度:5/5 — 干净、遵循规范的增量 API;每一层恰好只做所需的事。

这是一个范围良好的后端切片。PR 新增两个端点,贯穿三层,然后停止。没有多余的抽象,没有范围蔓延。Workspace 隔离语义正确且经过测试——在一个工作区撤销不影响另一个(也不影响 legacy 基线),这正是 #7017 scoping 工作的核心。

维护者已在本地对真实 daemon 验证了端到端契约,Serve A/B CI 在 base-vs-head 比较中执行了新路由。Ubuntu 套件通过;macOS/Windows 跳过是 fork CI 限制。

毫不犹豫可以合并。

Qwen Code · qwen3.8-max-preview

Reviewed at dbe6d7312193317ae21c202758807a7ab782b08f · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head dbe6d73, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-preview via Qwen Code /review

Comment on lines +7337 to +7340
[
'DELETE',
'http://daemon/workspaces/%2Ftmp%2Fwork%20space/channels/bot/pairing-approvals',
],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The WorkspaceDaemonClient test asserts the HTTP method and URL for the new revokeWorkspaceChannelPairingApproval call but never asserts the request body — unlike the primary DaemonClient test, which pins JSON.parse(calls[11]!.body!) to { senderId: 'sender/1' }. — Concrete cost: if body: request were accidentally dropped from the channelRequest call in WorkspaceDaemonClient.revokeWorkspaceChannelPairingApproval, the workspace-scoped DELETE would be sent with no body, the server's parsePairingSenderId would read body['senderId'] as undefined and return 400 invalid_channel_pairing_sender_id for every workspace-scoped revocation, and this test would still pass because it only checks calls.map(({ method, url }) => ...). Add a body assertion after the header checks (confirm the workspace DELETE call index), e.g.:

expect(JSON.parse(calls[6]!.body!)).toEqual({ senderId: 'sender-1' });
中文说明

WorkspaceDaemonClient 的测试只断言了新 revokeWorkspaceChannelPairingApproval 调用的 HTTP 方法和 URL,却没有断言请求体——而主 DaemonClient 测试用 JSON.parse(calls[11]!.body!) 固定为 { senderId: 'sender/1' }。具体代价:如果 WorkspaceDaemonClient.revokeWorkspaceChannelPairingApprovalchannelRequest 调用里意外漏掉 body: request,workspace-scoped 的 DELETE 将不带请求体发出,服务端 parsePairingSenderId 会把 body['senderId'] 读成 undefined,并对每一次 workspace-scoped 撤销返回 400 invalid_channel_pairing_sender_id,而本测试仍会通过,因为它只检查 calls.map(({ method, url }) => ...)。建议在 header 断言之后补一条 body 断言(确认 workspace DELETE 的调用索引):

expect(JSON.parse(calls[6]!.body!)).toEqual({ senderId: 'sender-1' });

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复:commit dbe6d73121 固定了 qualified workspace 撤销请求的 DELETE body。验证通过:SDK public-surface 与 DaemonClient focused tests 314/314、仓库 build、SDK typecheck、Prettier、git diff --check

Comment on lines +373 to +375
DaemonChannelPairingApprovalsSnapshot,
DaemonChannelPairingRevocationRequest,
DaemonChannelPairingRevocationResult,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] These three new types are exported from daemon/index.ts but are not re-exported from the top-level barrel packages/sdk-typescript/src/index.ts, breaking the pattern of the four pre-existing sibling pairing types (DaemonChannelPairingRequest, DaemonChannelPairingRequestsSnapshot, DaemonChannelPairingApprovalRequest, DaemonChannelPairingApprovalResult), which are re-exported there. — Concrete cost: a consumer of @qwen-code/sdk-typescript who calls client.workspaceChannelPairingApprovals('bot') and writes import type { DaemonChannelPairingApprovalsSnapshot } from '@qwen-code/sdk-typescript' gets a compile error, even though the four sibling types resolve from the same path. Add the three types to the re-export block in packages/sdk-typescript/src/index.ts:

type DaemonChannelPairingApprovalResult,
type DaemonChannelPairingApprovalsSnapshot,
type DaemonChannelPairingRevocationRequest,
type DaemonChannelPairingRevocationResult,
中文说明

这三个新类型已从 daemon/index.ts 导出,但没有从顶层 barrel packages/sdk-typescript/src/index.ts 再导出,打破了四个既有同级 pairing 类型(DaemonChannelPairingRequestDaemonChannelPairingRequestsSnapshotDaemonChannelPairingApprovalRequestDaemonChannelPairingApprovalResult)的模式——它们都在那里被再导出。具体代价:使用 @qwen-code/sdk-typescript 的消费者调用 client.workspaceChannelPairingApprovals('bot') 并写 import type { DaemonChannelPairingApprovalsSnapshot } from '@qwen-code/sdk-typescript' 时会编译报错,尽管四个同级类型都能从同一路径解析。建议在 packages/sdk-typescript/src/index.ts 的再导出块中补上这三个类型:

type DaemonChannelPairingApprovalResult,
type DaemonChannelPairingApprovalsSnapshot,
type DaemonChannelPairingRevocationRequest,
type DaemonChannelPairingRevocationResult,

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复:commit dbe6d73121 从顶层 SDK entry 再导出 3 个 pairing approval/revocation 类型,并在 public-surface 测试中固定该契约。验证通过:SDK public-surface 与 DaemonClient focused tests 314/314、仓库 build、SDK typecheck、Prettier、git diff --check

@wenshao

wenshao commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Local verification against a real daemon — merge reference

I built this locally and ran it end to end against a live qwen serve, not just the unit suites. Head verified: daf316fb0ad97d7ed30513225bc002005717a641, merge-base d0481ad8843d9b113c4aba4b1c1e5d2961bbbd3a.

Verdict: behaves as the description claims. 88/88 end-to-end checks pass on the PR head, and the driver demonstrably discriminates the change (14/60 at merge-base, 40/60 against a deliberately broken build). Three non-blocking notes below for the follow-up UI PR — none of them are defects introduced here.

How I ran it

The daemon runs straight from this branch's TypeScript source (a loader that maps every workspace exports entry to src/*.ts, so no build is needed), with an isolated QWEN_HOME and three registered workspaces — two trusted, one deliberately untrusted.

The important part is the oracle. Every pairing request is produced by the same production code an inbound DingTalk/WeCom/Feishu message runs through — new PairingStore(name, cwd) + SenderGate.check(senderId, senderName), which is literally what ChannelBase constructs — and it runs in a separate process from the daemon. That gate object is created before the first approval and never rebuilt, so it stands in for a running channel worker. So the checks below measure the effect an operator actually cares about ("can this person still talk to the bot?"), not merely that a JSON field changed.

What passed

Group Checks Covers
A. Lifecycle (/workspace/...) 13 pair → approve → list → revoke → 404 on repeat; Cache-Control: no-store on both new routes; the live worker gate flips allow → deny with no restart, and the revoked sender is re-issued a fresh pairing code as if unknown
B. Cross-workspace 6 same channel name in two workspaces, same sender approved in both; revoking in A leaves B approved on the API and on B's live gate; verified in both directions
C. Legacy baseline 6 grandfathered legacy sender listed and revocable; the legacy global file is byte-identical afterwards; deleting the scoped file does not resurrect (sentinel holds)
D. Guards 20 18 rejection cases (no/bad token, bad client id, untrusted workspace, unknown channel, non-pairing channel, empty · missing · non-string · array senderId, reserved name all, path-separator name, name-error-wins, malformed JSON) — each with the documented status+code, then the allowlist re-read byte-for-byte unchanged and the sender still allowed by the live gate
E. senderId round-trip 6 ids containing / % # ? &, unicode, spaces, tabs, and a 512-char id all revoke correctly through the JSON body; exact match, no case folding
G. Concurrency 5 12 concurrent DELETEs → 12×200, empty allowlist, live gate denies all 12 (no lost update on the read-modify-write)
H. SDK vs. real daemon 8 both helper pairs driven against the live daemon (the PR's SDK tests stub fetch): correct URLs, DELETE carries a JSON body, unicode/space channel name encodes correctly, repeat revoke rejects as DaemonHttpError 404
I–L. Adversarial 20 token-less daemon → 401 token_required on both new routes (with a non-strict route as positive control); corrupt/non-array allowlist files; __proto__ in the body; 11 MB body → 413; no-body DELETE → 400; 10 concurrent DELETEs split across both route prefixes for the same workspace → no lost update

Does the harness actually discriminate the change?

  • Merge-base daemon, same driver, fresh state: 14/60. Every new-route call gets Express' 404 Cannot GET|DELETE …/pairing-approvals. The 14 survivors are exactly the pre-existing behaviours (pairing-code issue, approve, the auth-before-routing 401s, and the two allowedUsers observations) — i.e. every check that measures this PR flips.
  • Mutant: PairingStore.revoke() rewritten to report success without writing → 40/60. 20 checks die, including every "did it take effect" assertion (A10/A11, B4, C4, G3/G4). That is the proof the effect checks are not vacuous.
  • PR's own suites with only the six production files reverted to merge-base: PairingStore.test.ts 2 failed, cli service+routes 9 failed, DaemonClient.test.ts -t workspace 2 failed. Six of the nine cli failures are behavioural, not import errors (expected 404 to be 403, expected 401, got 404, expected undefined to be 'invalid_channel_pairing_sender_id'). Restored: 23 + 43 + 92 pass, eslint --max-warnings 0 clean, prettier --check clean, tsc --noEmit clean on channels/base and identical error counts at head and merge-base for cli/sdk (stale sibling-dist artefacts, not this PR).

Design decisions I checked and agree with

  • pairingStoreFor() runs assertWorkspaceConfig() before touching the store, so a channel whose cwd differs from the selected workspace can't be used to read or mutate another scope's allowlist.
  • revokePairingApproval goes through inMutationLane while pairingApprovals does not — correct, and the WeakMap-per-runtime service memoization means both route prefixes for the same workspace share one lane (check L3 proves this empirically).
  • Both new routes sit behind mutate({ strict: true }), so a token-less loopback daemon refuses them with 401 token_required while non-strict routes stay open (I1–I3).
  • SenderGate is consulted on every message including group messages (preflightInbound → group → dm → sender), and isApproved() re-reads from disk each call — which is why revocation takes effect on a running worker with no restart and no reload.

Three non-blocking notes for the follow-up UI PR

  1. senderPolicy: "pairing" + a config allowedUsers entry makes "revoked" a lie. SenderGate checks the static allowedUsers list first, so for a sender present in both places the API returns 200 {"revoked": "...", "senderIds": []} while the live gate still admits them (checks F2/F3). A sender that exists only in allowedUsers is invisible to the approvals list and DELETE returns 404 (F4). The API is not wrong — it manages the pairing allowlist and nothing else — but a UI that renders this as "access removed" will mislead the operator. Worth either documenting on the route or having the UI surface config-based entries separately.
  2. A corrupt allowlist is indistinguishable from "nobody approved." readAllowlist() swallows the parse error, so a truncated file returns {"senderIds": []} with no diagnostic and every previously-approved sender is silently denied (J1/J2). It also does not check the shape: a non-array JSON value is echoed straight back as {"senderIds": {...}}, breaking the declared string[] contract, and DELETE against it 500s with channel_management_failed (J4/J5). This is pre-existing in PairingStore, but this PR is the first to expose it through a typed public API — an Array.isArray + string filter in getAllowlist() would close it cheaply. Related: writeAllowlist() writes in place, while the migration path a few lines above deliberately uses temp-file + rename for exactly this reason.
  3. Revocation is per-workspace, including against the legacy baseline. Not mutating the legacy global file is the right call and the README says so, but the consequence is that revoking in every existing workspace does not stop a new workspace from grandfathering that sender back in on first construction (check C5). Worth one sentence in qwen-serve-protocol.md next to the approvals section.

Minor: the SDK surfaces the daemon error code only via err.body.code, not a typed field — consistent with the existing approveWorkspaceChannelPairing helper, so no change requested, just something the UI will need to reach for.

Environment

Linux (Debian 13, kernel 6.12), Node 22.22. Daemon run from source via tsx; three daemons used (PR head with token, merge-base with token, PR head token-less). No live IM provider involved — the pairing/allowlist path is entirely local, and the platform adapters are not on it.

中文说明

基于真实 daemon 的本地验证 —— 合并参考

我在本地把这个 PR 完整跑起来了,不只是跑单元测试,而是对着一个真实运行的 qwen serve 做端到端验证。验证的 head 是 daf316fb0ad97d7ed30513225bc002005717a641,merge-base 是 d0481ad8843d9b113c4aba4b1c1e5d2961bbbd3a

结论:行为与 PR 描述一致。 PR head 上 88/88 项端到端检查全部通过,并且这套 driver 确实能区分该改动(merge-base 上 14/60,故意打坏的构建上 40/60)。下面有三条不阻塞合并的说明,供后续 UI PR 参考——都不是本 PR 引入的缺陷。

怎么跑的

daemon 直接从这个分支的 TypeScript 源码启动(用一个 loader 把每个 workspace 包的 exports 映射到 src/*.ts,因此完全不需要 build),使用隔离的 QWEN_HOME,注册三个工作区——两个受信任,一个故意不受信任。

关键在于 oracle。每一个配对请求都由真实入站消息(钉钉/企业微信/飞书)所走的同一份生产代码产生——new PairingStore(name, cwd) + SenderGate.check(senderId, senderName),也就是 ChannelBase 实际构造的那套——而且它运行在与 daemon 不同的进程里。这个 gate 对象在第一次批准之前创建,之后从不重建,因此它扮演的就是一个正在运行的 channel worker。所以下面这些检查衡量的是运维人员真正关心的效果("这个人现在还能不能跟 bot 说话?"),而不仅仅是某个 JSON 字段变了。

通过了哪些

分组 检查数 覆盖内容
A. 生命周期(/workspace/... 13 配对 → 批准 → 列表 → 撤销 → 重复撤销 404;两个新路由都带 Cache-Control: no-store;运行中的 worker gate 无需重启即从允许翻转为拒绝,并且被撤销的 sender 会像陌生人一样重新拿到新的配对码
B. 跨工作区 6 两个工作区使用同名频道,同一 sender 在两边都被批准;在 A 撤销后,B 在 API 与其真实 gate 上仍然保持授权;双向都验证过
C. Legacy 基线 6 被 grandfather 的 legacy sender 可列出、可撤销;撤销后 legacy 全局文件逐字节未变;删除 scoped 文件不会复活(sentinel 生效)
D. 前置校验 20 18 种拒绝场景(无/错 token、非法 client id、不受信任工作区、未配置频道、非配对模式频道、senderId 为空 · 缺失 · 非字符串 · 数组、保留名 all、含路径分隔符的名字、名字错误优先、畸形 JSON)——每一种都返回文档所述的 status+code,随后 allowlist 逐字节重读未变,且该 sender 仍被真实 gate 放行
E. senderId 往返 6 包含 / % # ? &、unicode、空格、制表符以及 512 字符的 id,都能通过 JSON body 正确撤销;精确匹配,不做大小写折叠
G. 并发 5 12 个并发 DELETE → 12×200,allowlist 清空,真实 gate 全部拒绝(读-改-写没有丢更新)
H. SDK 对真实 daemon 8 两对 helper 都对着真实 daemon 调用(PR 自带的 SDK 测试是 stub fetch 的):URL 正确、DELETE 确实携带 JSON body、unicode/空格频道名编码正确、重复撤销以 DaemonHttpError 404 抛出
I–L. 对抗性 20 无 token 的 daemon 上两个新路由都返回 401 token_required(并用一个非 strict 路由做正对照);损坏/非数组的 allowlist 文件;body 中的 __proto__;11 MB body → 413;无 body 的 DELETE → 400;同一工作区经两个路由前缀发起的 10 个并发 DELETE → 无丢更新

这套验证真的能区分改动吗

  • merge-base daemon,同一 driver,全新状态:14/60。 所有新路由调用都拿到 Express 的 404 Cannot GET|DELETE …/pairing-approvals。存活的 14 项恰好是原有行为(发配对码、批准、路由前的鉴权 401,以及两条关于 allowedUsers 的观察)——也就是说,凡是衡量本 PR 的检查全部翻转。
  • 变异体:把 PairingStore.revoke() 改成"报告成功但不写盘" → 40/60。 20 项检查死掉,包含所有"是否真的生效"的断言(A10/A11、B4、C4、G3/G4)。这证明这些效果断言不是空转。
  • 只把六个生产文件回退到 merge-base 后跑 PR 自带测试: PairingStore.test.ts 2 失败,cli service+routes 9 失败,DaemonClient.test.ts -t workspace 2 失败。cli 的 9 个失败里有 6 个是行为性断言失败而非 import 错误(expected 404 to be 403expected 401, got 404expected undefined to be 'invalid_channel_pairing_sender_id')。恢复后:23 + 43 + 92 全部通过,eslint --max-warnings 0 干净,prettier --check 干净,tsc --noEmitchannels/base 干净,cli/sdk 的报错数在 head 与 merge-base 完全一致(是同级目录 dist 过期造成的,与本 PR 无关)。

我确认并认可的几个设计取舍

  • pairingStoreFor() 在接触 store 之前先执行 assertWorkspaceConfig(),所以 cwd 与所选工作区不一致的频道无法用来读取或修改另一个 scope 的 allowlist。
  • revokePairingApprovalinMutationLanepairingApprovals 不走,这是对的;并且服务按 runtime 用 WeakMap 记忆化,因此同一工作区的两个路由前缀共享同一条 mutation lane(检查 L3 用实测证明了这一点)。
  • 两个新路由都在 mutate({ strict: true }) 之后,所以无 token 的 loopback daemon 会用 401 token_required 拒绝它们,而非 strict 的路由仍然开放(I1–I3)。
  • SenderGate 对每一条消息都会生效,群消息也不例外(preflightInbound → group → dm → sender),并且 isApproved() 每次调用都重新读盘——这正是撤销能在运行中的 worker 上立即生效、既不用重启也不用 reload 的原因。

三条不阻塞合并的说明(供后续 UI PR)

  1. senderPolicy: "pairing" 与配置里的 allowedUsers 同时存在时,"已撤销"会变成一句假话。 SenderGate 检查静态 allowedUsers,所以对于同时存在于两处的 sender,API 会返回 200 {"revoked": "...", "senderIds": []},而真实 gate 仍然放行(检查 F2/F3)。只存在于 allowedUsers 的 sender 对授权列表完全不可见,DELETE 返回 404(F4)。API 本身没错——它管理的就是配对 allowlist——但如果 UI 把这个结果渲染成"已移除访问权限",就会误导运维。建议要么在路由文档里写明,要么让 UI 单独展示来自配置的条目。
  2. 损坏的 allowlist 与"无人获批"无法区分。 readAllowlist() 吞掉解析错误,因此一个被截断的文件会返回 {"senderIds": []} 且没有任何诊断信息,同时所有此前已获批的 sender 被静默拒绝(J1/J2)。它也不校验类型:非数组的 JSON 值会被原样回显成 {"senderIds": {...}},破坏了声明的 string[] 契约,而针对它的 DELETE 会以 channel_management_failed 500(J4/J5)。这是 PairingStore 里原有的问题,但本 PR 是第一个把它暴露到有类型的公开 API 上的改动——在 getAllowlist() 里加一个 Array.isArray + 字符串过滤就能低成本收口。相关:writeAllowlist() 是就地写入,而上方几行的迁移逻辑正是出于同样的顾虑而刻意采用了临时文件 + rename。
  3. 撤销是按工作区生效的,对 legacy 基线也一样。 不修改 legacy 全局文件是正确的选择,README 也是这么写的,但其后果是:即使在所有已存在的工作区里都撤销了,一个新的工作区在首次构造 store 时仍会把该 sender grandfather 回来(检查 C5)。建议在 qwen-serve-protocol.md 的授权列表章节旁补一句。

小点:SDK 只通过 err.body.code 暴露 daemon 的错误码,没有独立的类型化字段——这与既有的 approveWorkspaceChannelPairing helper 保持一致,所以不要求改动,只是 UI 需要知道去哪里取。

环境

Linux(Debian 13,内核 6.12),Node 22.22。daemon 通过 tsx 从源码运行;共用到三个 daemon(带 token 的 PR head、带 token 的 merge-base、无 token 的 PR head)。全程没有涉及真实 IM 服务——配对/allowlist 路径完全是本地的,平台适配器不在这条链路上。


🤖 Generated with Claude Code — Claude Opus 5 (1M context)

Re-export the new approval and revocation types from the public SDK entry, and pin the qualified workspace DELETE request body in regression coverage.
@wenshao

wenshao commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Local verification — behavior confirmed on a real daemon, recommend merge

I built this PR locally and verified it against a live qwen serve daemon with real channel workers, rather than only re-running the unit tests. This fills the gap the triage bot flagged (the sandboxed /verify and /tmux lanes are unavailable for a fork PR).

Result: the contract behaves exactly as documented. 50/50 live checks pass, plus 4,092 automated tests, and a 5-mutant matrix confirms the harness is not vacuous. No blocking issues found.

Verified at head dbe6d73 (merge-base d0481ad), macOS 15.7.7, Node v22.23.1, isolated worktree, full npm run build.

Why a live run mattered here

The PR's own tests assert that the allowlist file changes. What they cannot assert is the thing that actually matters: the channel worker is a separate OS process that enforces the pairing gate. A revocation issued through the daemon API is only real if that worker starts refusing the sender afterwards. So the harness drives real inbound messages through a real mock IM server and uses the worker's own reply as the oracle — a pairing-code prompt means refused, the agent's answer means admitted.

That end-to-end loop is confirmed: approve → sender reaches the agent → revoke via DELETE → the same sender is refused again and issued a fresh pairing code (checks C07, C12, C13).

Live E2E — 50/50

Topology: one daemon registering three workspaces serves the API under test; a second daemon hosts the enforcing worker. They share QWEN_HOME, so the only thing linking the API to the enforcement point is the on-disk scoped allowlist — precisely the boundary under test.

Phase What it proves Checks
1 — live pairing gate list → approve → admitted → revoke → refused again → repeat DELETE 404s C01–C15
2 — workspace isolation revoking in A leaves B approved; legacy global allowlist untouched C16–C22
3 — rejection matrix 15 probes; allowlist provably unchanged afterwards C23–C40
4 — SDK over the wire both new SDK helpers against the real routes, incl. 404 surfacing C41–C46
5 — worker handover a sender revoked in A is still admitted by B's live worker, then revoked there C47–C50

live E2E part 1

live E2E part 2

Point-by-point against your Reviewer Test Plan:

  1. Approve then list → C05, C06 ✅
  2. Revoke, empty snapshot, repeat → 404 → C08–C10, C14, C15 ✅
  3. Same channel name in two workspaces, revoke one → C16–C22, C47 ✅ (see the caveat below)
  4. Auth / client ID / untrusted workspace / non-pairing / empty senderId all fail before mutation → C24–C39 ✅ — and C39 is the important one: after all 15 rejected probes the allowlist is byte-identical, and C40 confirms the guard sender still reaches the agent.

DELETE-with-body works over real HTTP through the shipped SDK (C42, C48), not just against a mocked fetch.

Mutation matrix — 5/5 killed

Each mutant was applied to the built artifact the live daemon actually loads, then the full 50-check E2E was re-run. No survivors, so no check is vacuous.

mutation matrix

M3 is the most informative: reverting pairingStoreFor to an unscoped new PairingStore(name) flips C20, C22 and C50 red — confirming the workspace scoping and the "never mutate the legacy baseline" guarantee are both load-bearing, not incidental.

Automated suites

Suite Result
packages/channels/base (full) 958 passed, 19 files
packages/cli src/serve (full) 3,735 passed, 1 skipped, 126 files
SDK DaemonClient + daemon-public-surface 314 passed
Prettier / ESLint on all 14 changed files clean
SDK tsc --noEmit, full monorepo npm run build clean

One caveat on test-plan step 3 — not a defect in this PR

While building the harness I hit two pre-existing daemon constraints that make the "same channel name in two workspaces" scenario unreachable with two simultaneously-running workers:

  1. A daemon with two registered workspaces that both define bot refuses to start it — ambiguous_channel_workspace — both at boot via --channel bot and via PUT /workspace/channel.
  2. A second channel-hosting daemon on the same QWEN_HOME is refused: "Channel service is already running under qwen serve (PID …)".

This does not weaken the PR. The pairing routes only require the channel to be configured, so both workspaces' approvals remain fully addressable from one daemon — verified live (C17, C20, C45) — and M3 proves the scoping is load-bearing for exactly that path, which is what the follow-up UI PR will use. It only means the scenario has to be verified as I did (sequential worker handover, C47–C49) rather than with two live workers. Worth knowing when writing the UI PR's test plan.

Minor observations (non-blocking, no change requested)

  • revoke() writes the allowlist with a plain writeFileSync, so it adds a second unsynchronised writer alongside approve(). Identical to the existing approve() shape and not introduced here, but if the follow-up UI ever issues bulk revocations it may be worth revisiting.
  • senderId is deliberately not trimmed (correct for opaque IDs, as the bot review noted) — so a UI that pads the value will get a 404 rather than a revocation. Worth a trim on the client side.
中文说明

本地验证 —— 已在真实 daemon 上确认行为,建议合并

我在本地构建了本 PR,并针对运行真实 channel worker 的真实 qwen serve daemon 做了验证,而不仅仅是重跑单元测试。这补上了 triage bot 指出的空缺(fork PR 无法使用沙箱化的 /verify/tmux 通道)。

结论:契约行为与文档完全一致。50/50 项实时检查通过,另有 4,092 个自动化测试通过,5 个变异体全部被杀死,证明验证harness 非空洞。未发现阻塞问题。

验证于 head dbe6d73(merge-base d0481ad),macOS 15.7.7,Node v22.23.1,隔离 worktree,完整 npm run build

为什么必须做实时验证

本 PR 自带的测试断言的是 allowlist文件发生了变化。但它们无法断言真正关键的一点:channel worker 是一个独立的操作系统进程,配对门禁由它执行。通过 daemon API 发出的撤销,只有在该 worker 随后真的开始拒绝该 sender 时才算真实生效。因此本 harness 通过真实的 mock IM server 推送真实入站消息,并以 worker 自己的回复作为判据 —— 回复配对码提示即为拒绝,回复 agent 答案即为放行。

该端到端闭环已确认:批准 → sender 可达 agent → 通过 DELETE 撤销 → 同一 sender 再次被拒绝并收到新的配对码(检查项 C07、C12、C13)。

实时 E2E —— 50/50

拓扑:一个注册了三个 workspace 的 daemon 提供被测 API;第二个 daemon 承载执行门禁的 worker。二者共享 QWEN_HOME,因此连接 API 与执行点的唯一媒介就是磁盘上的 scoped allowlist —— 这正是被测边界。

阶段 验证内容 检查项
1 —— 实时配对门禁 列表 → 批准 → 放行 → 撤销 → 再次拒绝 → 重复 DELETE 返回 404 C01–C15
2 —— workspace 隔离 在 A 撤销后 B 仍保持授权;legacy 全局 allowlist 未被修改 C16–C22
3 —— 拒绝矩阵 15 项探测;事后可证 allowlist 未变 C23–C40
4 —— SDK 真实链路 两个新 SDK helper 打到真实路由,含 404 错误透传 C41–C46
5 —— worker 交接 在 A 被撤销的 sender 仍被 B 的实时 worker 放行,随后在 B 撤销 C47–C50

逐条对照你的 Reviewer 测试计划:

  1. 批准后查询列表 → C05、C06 ✅
  2. 撤销、空快照、重复撤销 → 404 → C08–C10、C14、C15 ✅
  3. 两个 workspace 同名频道,撤销其一 → C16–C22、C47 ✅(见下方说明)
  4. 认证 / client ID / 不可信 workspace / 非配对模式 / 空 senderId 均在修改前失败 → C24–C39 ✅ —— 其中 C39 最关键:15 项探测全部被拒后 allowlist 逐字节未变,C40 进一步确认哨兵 sender 仍可达 agent。

DELETE 带 body 通过随仓库发布的 SDK 在真实 HTTP 上可用(C42、C48),而非仅在 mock fetch 下成立。

变异矩阵 —— 5/5 被杀死

每个变异体都施加于实时 daemon 真正加载的构建产物,然后重跑完整的 50 项 E2E。无幸存者,说明没有任何一项检查是空洞的。

M3 信息量最大:将 pairingStoreFor 退回为无 scope 的 new PairingStore(name) 会让 C20、C22、C50 变红 —— 证明 workspace scoping 与"绝不修改 legacy 基线"这两项保证都是实际承载语义的,而非顺带产物。

自动化测试

测试套件 结果
packages/channels/base(全量) 958 通过,19 个文件
packages/cli src/serve(全量) 3,735 通过,1 跳过,126 个文件
SDK DaemonClient + daemon-public-surface 314 通过
全部 14 个改动文件的 Prettier / ESLint 干净
SDK tsc --noEmit、完整 npm run build 干净

关于测试计划第 3 条的一点说明 —— 并非本 PR 的缺陷

在搭建 harness 时我遇到了两个既有的 daemon 约束,它们使得"两个 workspace 同名频道"这一场景无法以两个同时运行的 worker 复现

  1. 当两个已注册 workspace 都定义了 bot 时,daemon 拒绝启动它 —— 报 ambiguous_channel_workspace,无论是启动时的 --channel bot 还是 PUT /workspace/channel
  2. 同一 QWEN_HOME 下的第二个承载频道的 daemon 会被拒绝:"Channel service is already running under qwen serve (PID …)"

并不削弱本 PR。配对路由只要求频道处于已配置状态,因此两个 workspace 的授权都可以从同一个 daemon 完整访问 —— 已实时验证(C17、C20、C45)—— 且 M3 证明 scoping 对这条路径确实承载语义,而后续 UI PR 走的正是这条路径。它只意味着该场景需要像我这样验证(顺序交接 worker,C47–C49),而非用两个同时运行的 worker。写 UI PR 的测试计划时值得留意。

次要观察(不阻塞,无需修改)

  • revoke() 使用普通 writeFileSync 写 allowlist,因此在 approve() 之外新增了一个未加锁的写入方。这与既有 approve() 的形态一致、并非本 PR 引入,但如果后续 UI 会发起批量撤销,或许值得重新审视。
  • senderId 有意不做 trim(对不透明 ID 而言是正确的,bot review 也已指出)—— 因此若 UI 传入带空格的值会得到 404 而非撤销。建议在客户端做一次 trim。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-preview via Qwen Code /review

@wenshao

wenshao commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 29, 2026
Merged via the queue into QwenLM:main with commit c970260 Jul 29, 2026
113 of 114 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants