feat(web-shell): redesign Channel policy and workspace management - #8848
Conversation
github-actions
Bot
added
the
review/self-reported
E2E test report
|
|
Re-running the gate on the current head, at @wenshao's request. The branch has moved through three rounds of maintainer real-stack review since the first pass; this comment reflects the state at the reviewed commit.
Moving on to code review. 🔍 中文说明应 @wenshao 的请求，在当前 head 上重新跑门禁。自首次检查以来，该分支已经过维护者三轮真实环境评审；本条评论反映的是被审查提交时的状态。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent read of the diff finds no critical blockers. The risky parts of this PR are exactly the parts the maintainer already mutation-tested on the real stack, and my static check of them agrees:
Residuals — all explicitly non-blocking per the maintainer's round-3 report, recorded here so they aren't silently dropped: GitHub/GitLab descriptors declare their own Files changed (all 27)
Test evidence — the PR's own CI, via APIAll checks on the reviewed commit are settled; nothing is pending and nothing failed. The fork PR's CI runs the Node suite on ubuntu only (windows/macos unit legs and the sandbox-less integration leg are skipped for forks, as usual), which is exactly why the real-stack and sandboxed lanes below matter.
On the behavioural claims, this run is unusually well evidenced, and I'm saying what is evidence versus claim: the three rounds of real-stack verification in this thread are the maintainer's own A/B and mutation results (base-vs-head daemons, captured wire bodies, on-disk settings, guard removed → regression observed), not the author's self-report. The author's claimed suite numbers are secondary to that. A sandboxed Not verified here: live DingTalk/Feishu/WeCom platform traffic (no real credentials anywhere in the loop so far) — same as every prior round. 中文说明代码审查我独立通读 diff，未发现阻塞性问题。这个 PR 风险最高的部分恰好是维护者已在真实栈上做过变异测试的部分，我的静态检查与其结论一致：
遗留项——维护者第三轮报告已明确均不阻塞，记录在此以免被静默遗忘：GitHub/GitLab 描述符自带 测试证据 —— 来自 PR 自身 CI（API 读取）被审查提交上的所有检查均已结束：无进行中、无失败。Fork PR 的 CI 只在 ubuntu 上跑 Node 套件（windows/macos 单测与无沙箱集成测试按惯例跳过）——这正是真实栈与沙箱验证通道重要的原因。 关于行为性结论：本轮的证据 unusually 充分，并区分「证据」与「声明」：本主题中三轮真实环境验证是维护者本人的 A/B 与变异结果（base 与 head 双 daemon、抓取的请求体、磁盘配置、删除守卫即观察到回归），不是作者自报。作者自述的用例数字相对次要。本次 triage 重跑同时触发的沙箱 本轮未验证：真实 DingTalk/Feishu/WeCom 平台流量（到目前为止所有轮次都未使用真实凭据）。 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ 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 |
|
Confidence: 3/5 — clean review and a thoroughly verified PR; the cap is policy, not doubt. Stepping back: my independent proposal for this problem was essentially what this PR does — inject the shared access/scope fields into the manageable catalog, render them descriptor-driven, scope management operations through the existing workspace-qualified SDK client, and preserve legacy values instead of migrating them. The PR matches that and goes further in the right places: the defaults mirror the runtime fallbacks exactly, the store learns to preserve legacy Every blocking concern raised on this PR has been fixed and re-verified on the real stack by the maintainer — including the safety-critical one (legacy configs without a stored policy are not widened, proven by removing the guard and watching the regression). CI on the reviewed commit is fully green, and the sandboxed verification report will land in this thread when the run completes. The three residual notes (GitHub/GitLab defaults, raw labels, grid wrap) are non-blocking per the maintainer's own round-3 verdict and are recorded in my Stage 2 comment so they aren't lost. The only reason this run stops short of approving is the core-infrastructure gate: this is a fork PR whose change spans five packages with ~1,715 production logic lines, which the triage policy escalates to a maintainer for the final sign-off regardless of how clean the review is. That gate exists for exactly this moment — the review is done, the evidence is in, and the human verdict is the only missing piece. ⏸️ Deferring to @wenshao for the final call. To the extent this gate's purpose is maintainer awareness, it is already overwhelmingly satisfied by your three rounds of real-stack verification; per policy the bot's approval is withheld for you (or another maintainer) to cast. Nothing in this run found a reason to say no. 中文说明置信度：3/5 —— 审查干净、验证充分；分数上限来自政策，而非疑虑。 退一步看：我对这个问题的独立方案与本 PR 基本一致——把共享准入/会话字段注入可管理目录、用描述符驱动渲染、通过既有的 workspace-qualified SDK client 执行管理操作、保留而非迁移旧值。PR 与之一致，且在正确之处走得更远：默认值与运行时回落逐字一致、store 学会保留旧 本 PR 上提出过的每一个阻塞性问题都已修复并由维护者在真实栈上复验——包括最关键的一条（未存储策略的旧配置不会被放宽，且通过删除守卫亲见回归来证明）。被审查提交的 CI 全绿，沙箱验证报告会在运行完成后发到本主题。三条遗留注记（GitHub/GitLab 默认值、原始标签、网格换行）按维护者第三轮结论均不阻塞，已记录在我的 Stage 2 评论中以免丢失。 本轮未走到批准的唯一原因是核心基础设施门禁：这是一个跨 5 个包、约 1,715 行生产逻辑的 fork PR，triage 政策要求此类改动无论审查多干净都交由维护者最终拍板。这个门禁正是为此刻而设——审查已完成、证据已齐备，只差人类的结论。 ⏸️ 转交 @wenshao 做最终决定。 就该门禁「维护者知悉」的目的而言，您三轮真实环境验证已充分满足；按政策，bot 的批准票留给您（或其他维护者）投出。本轮没有发现任何反对合并的理由。 — 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 |
|
Follow-up UI refinement at |
|
Workspace follow-up at |
qqqys
changed the title
|
The latest commit redesigns the full Channel manager surface in addition to the policy/editor work already in this PR.
Verification for the combined change:
|
|
The latest commit replaces the bespoke Channel dashboard styling with the established Web Shell tab language, using Scheduled Tasks as the direct reference.
Verification after the alignment change:
|
|
按最新交互反馈调整配置面板顺序：会话管理现在位于访问控制之前，让用户先决定会话隔离方式，再配置谁可以访问。新增 DOM 顺序回归断言；ChannelEditorDialog 16 个测试、ESLint 和 Prettier 均通过。 |
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.
Not explored to full depth (tool budget reached): PR #8848 (QwenLM/qwen-code) redesigns Web Shell Channel m...: none — all checks above completed within budget.; chunk 9: full typecheck of ChannelsManagerPage.tsx (blocked by unbuildable @qwen-code/webui dist in this sandbox; the one line in my territory there was verified by …; chunk 9: Playwright e2e execution of the two spec files (fixtures verified against SDK types instead).; chunk 2: channel-settings-store.test.ts could not be executed — vitest collection fails with Failed to resolve entry for package "ajv" (ajv 6.12.6 is installed and …; You are review agent reverse-audit — Reverse audit agen...: none** — all checks above completed within budget (~15 tool calls)., and 2 more.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #8848 (QwenLM/qwen-code) redesigns Web Shell Channel m...：none — all checks above completed within budget.；chunk 9：full typecheck of ChannelsManagerPage.tsx (blocked by unbuildable @qwen-code/webui dist in this sandbox; the one line in my territory there was verified by …；chunk 9：Playwright e2e execution of the two spec files (fixtures verified against SDK types instead).；chunk 2：channel-settings-store.test.ts could not be executed — vitest collection fails with Failed to resolve entry for package "ajv" (ajv 6.12.6 is installed and …；You are review agent reverse-audit — Reverse audit agen...：none** — all checks above completed within budget (~15 tool calls).，另有 2 条。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| return instance && field.key !== 'sessionScope' | ||
| ? '' | ||
| : (field.default ?? field.options?.[0]?.value ?? ''); |
There was a problem hiding this comment.
[Critical] The catalog now injects senderPolicy/groupPolicy into every manageable channel with required: true (channel-registry.ts), but this default-fill carve-out covers only sessionScope. For an existing instance with no stored value the draft gets '', validateChannelEditorDraft flags 'required', and submit returns early — so every pre-existing dingtalk/wecom/feishu channel (the pre-PR editor never wrote groupPolicy) can no longer be saved: even rotating a token fails until the user manually picks a policy. The runtime default (disabled) is neither shown nor pre-filled, so picking open just to unblock silently widens group access on save. Probe-verified end-to-end: a draft for a groupPolicy-less instance yields errors={groupPolicy:'required'}; extending the carve-out flips it clean. Note the sessionScope branch itself is also untested — reverting it keeps all tests green. — Failure scenario: editing any pre-existing dingtalk/wecom/feishu channel → blocked by "Group Policy is required"; choosing open to unblock silently opens group traffic.
| return instance && field.key !== 'sessionScope' | |
| ? '' | |
| : (field.default ?? field.options?.[0]?.value ?? ''); | |
| if (instance && field.key !== 'sessionScope' && field.default === undefined) { | |
| return ''; | |
| } | |
| return field.default ?? field.options?.[0]?.value ?? ''; |
中文说明
目录现在为所有可管理频道注入 required: true 的 senderPolicy/groupPolicy（channel-registry.ts），但这里的默认值回填例外只覆盖了 sessionScope。已有实例若没有存储值，草稿取值为 ''，validateChannelEditorDraft 报 'required'，提交提前返回——于是所有既有的 dingtalk/wecom/feishu 频道（PR 之前的编辑器从未写入 groupPolicy）都无法再保存：连轮换 token 都会失败，直到用户手动选择一个策略。运行时默认值（disabled）既未展示也未预填，用户为了取消阻塞而选 open 会在保存时悄悄放开群聊访问。已端到端探针验证：无 groupPolicy 的实例草稿产生 errors={groupPolicy:'required'}；扩展该例外后恢复干净。另外 sessionScope 分支本身也没有测试——回退它全部测试仍然为绿。— 失败场景：编辑任何既有 dingtalk/wecom/feishu 频道 → 被 "Group Policy is required" 阻塞；为解除阻塞选择 open 会悄悄开放群聊流量。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
已修复 + 验证证据：f8ac4192d7；回归先红后绿，channel-editor-state.test.ts 29/29 通过；ESLint、Prettier、git diff --check 通过。
| configuredSessionScope === 'thread' | ||
| ? 'chat_thread' | ||
| : configuredSessionScope, |
There was a problem hiding this comment.
[Suggestion] Normalizing legacy sessionScope: 'thread' to chat_thread changes the routing-key shape for already-configured channels with no migration of persisted routes. SessionRouter persists routes keyed by the routing key (${channel}:${threadId || chatId} under thread vs ${channel}:${chatId}:${threadId} under chat_thread), so after this change no persisted entry matches the new key: the first inbound message in each pre-existing thread silently starts a brand-new session (losing accumulated context), while old routes linger dormant and are restored on every restart. isSharedSessionTarget also flips legacy thread DMs from private to shared — a paired DM user not in allowedUsers is newly rejected from /clear, /who, destructive-command confirmation and the ! host-shell gate on a session still exclusively theirs. — Failure scenario: any channel configured today with sessionScope: 'thread' (the documented recipe) silently splits its threaded sessions and tightens DM command gating after upgrade. Consider migrating persisted routes on first load (the old key is re-derivable from the stored target), keeping thread semantics for pre-existing configs until re-saved, or at least documenting the one-time reset and the DM-gating change.
中文说明
将旧 sessionScope: 'thread' 归一化为 chat_thread 会改变已配置频道的路由键形状，但没有迁移已持久化的路由。SessionRouter 按路由键持久化路由（thread 下为 ${channel}:${threadId || chatId}，chat_thread 下为 ${channel}:${chatId}:${threadId}），因此升级后没有任何已持久化条目能匹配新键：每个既有 thread 的首条入站消息会悄悄开启全新会话（丢失累积上下文），旧路由则残留并在每次重启时被恢复。isSharedSessionTarget 也把旧 thread 私聊从私有翻转为共享——不在 allowedUsers 中的已配对私聊用户会在仍完全属于他们的会话上被新拒绝执行 /clear、/who、破坏性命令确认和 ! host-shell 门控。— 失败场景：今天以 sessionScope: 'thread' 配置的频道升级后会话被悄悄拆分、私聊命令门控收紧。建议在首次加载时迁移已持久化路由（旧键可由存储的 target 重新推导），或对既有配置保留 thread 语义直到其被重新保存，至少也应在文档中说明一次性重置和私聊门控变化。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| case 'chat_thread': | ||
| return threadId | ||
| ? `${this.name}:${chatId}:${threadId}` | ||
| : `${this.name}:${chatId}`; |
There was a problem hiding this comment.
[Suggestion] This added switch is now a second full copy of SessionRouter.routingKey (base/src/SessionRouter.ts) — the two must stay byte-for-byte in sync by hand, and this duplication is precisely how the defect this PR fixes arose (the missing chat_thread case misrouted attachments). Any future scope addition or key-format change silently diverges unless both switches are edited together; the compiler can't help because both have default branches. — Concrete cost: the next scope change repeats this bug class. Consider exposing the router's key computation (or extracting a shared sessionScopeRouteKey(name, scope, senderId, chatId, threadId) in channel-base) and reusing it here.
中文说明
新增的这段 switch 现在是 SessionRouter.routingKey（base/src/SessionRouter.ts）的第二份完整拷贝——两者必须靠手工保持逐字节同步，而这种重复正是本 PR 修复的缺陷的成因（缺失 chat_thread 分支导致附件误路由）。未来任何 scope 新增或键格式变更，除非同时修改两处，否则会悄悄分叉；由于两者都有 default 分支，编译器无法帮忙。— 具体代价：下一次 scope 变更会重演这类 bug。建议暴露 router 的键计算（或在 channel-base 提取共享的 sessionScopeRouteKey(name, scope, senderId, chatId, threadId)）并在此复用。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(qualifiedWorkspace.workspaceChannels).toHaveBeenCalledTimes(2); | ||
| expect(Object.keys(result?.channels ?? {})).toEqual(['bot-b']); |
There was a problem hiding this comment.
[Suggestion] The new workspace-qualified pairing branch in useDaemonChannels (all four operations: list/approve/approvals/revoke) has zero test coverage — this test creates and resets the qualifiedWorkspace.workspaceChannelPairing* mocks but never asserts them. Probe: replacing the pairing memo body with actions.channelPairing keeps the whole suite green (10/10). — Failure scenario: a refactor ships cross-workspace pairing — approving a pairing code while viewing a secondary workspace sends the approval to the primary workspace's endpoint (wrong channel, or silent 404), and no test catches it. Extend this test to call pairing.list/approve with the secondary workspace and assert the qualifiedWorkspace mocks are called while actions.channelPairing is not.
中文说明
useDaemonChannels 中新的 workspace-qualified 配对分支（全部四个操作：list/approve/approvals/revoke）零测试覆盖——本测试创建并重置了 qualifiedWorkspace.workspaceChannelPairing* mock，但从未对其断言。探针：将配对 memo 主体替换为 actions.channelPairing 后整个套件仍为绿（10/10）。— 失败场景：一次重构把跨工作区配对带上线——在查看二级工作区时批准配对码会把批准请求发到主工作区的端点（错误的频道，或静默 404），且没有测试能捕获。请扩展本测试：在二级工作区下调用 pairing.list/approve，断言调用了 qualifiedWorkspace 的 mock 且未调用 actions.channelPairing。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (UNSAFE_OBJECT_KEYS.has(groupId) || !isRecord(groupConfig)) { | ||
| throw invalidConfig(`Channel field "${key}.${groupId}" is invalid.`); |
There was a problem hiding this comment.
[Suggestion] The new groups validation has no negative test for unsafe group ids or wrong-typed nested values. Probe: deleting UNSAFE_OBJECT_KEYS.has(groupId) || at this line keeps all 67 store tests green — a request with groups: { "__proto__": { "dispatchMode": "collect" } } would then be persisted, re-opening the prototype-pollution class this guard exists to block; requireMention: 'yes' / groupHistoryLimit: '25' would also pass unnoticed. (Not exploitable through the store today — JSON.parse materializes __proto__ as an own property and consumers only read — so this is a missing-test gap, not a live vulnerability.) Add invalid-config cases: unsafe group id (__proto__, constructor), requireMention: 'yes', groupHistoryLimit: '25'.
中文说明
新的 groups 校验对不安全的群 ID 和类型错误的嵌套值没有负向测试。探针：删除此行的 UNSAFE_OBJECT_KEYS.has(groupId) || 后全部 67 个 store 测试仍为绿——此时 groups: { "__proto__": { "dispatchMode": "collect" } } 的请求会被持久化，重新打开该守卫本要阻断的原型污染类别；requireMention: 'yes' / groupHistoryLimit: '25' 也会无声通过。（目前无法通过 store 利用——JSON.parse 会把 __proto__ 实体化为自有属性且消费方只读——因此这是缺测试的缺口，不是现实漏洞。）请补充非法配置用例：不安全群 ID（__proto__、constructor）、requireMention: 'yes'、groupHistoryLimit: '25'。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| workspaces={WORKSPACES} | ||
| workspaceCwd="/workspace/main" | ||
| onWorkspaceChange={vi.fn()} |
There was a problem hiding this comment.
[Suggestion] The PR-added workspaceLoading guards in ChannelEditorDialog.tsx (workspace Select disabled, Save disabled) have zero coverage — no test passes workspaceLoading: true, so removing either disjunct keeps the suite green (the one disabled-Select assertion holds via Boolean(instance) alone). In production, while the workspace snapshot is mid-fetch, Save would otherwise be live and the Select interactive against an empty/stale workspace list. Add a renderDialog({ workspaceLoading: true }) case asserting the Workspace select and the Save button are disabled (and re-enable when it goes back to false).
中文说明
ChannelEditorDialog.tsx 中本 PR 新增的 workspaceLoading 守卫（工作区 Select 禁用、保存禁用）零覆盖——没有测试传入 workspaceLoading: true，因此移除任一析取项套件仍绿（唯一的 Select 禁用断言仅靠 Boolean(instance) 成立）。生产中在工作区快照拉取中途，保存本应不可用、Select 本应对空/陈旧列表不可交互。请补一个 renderDialog({ workspaceLoading: true }) 用例，断言 Workspace 选择器和保存按钮被禁用（并在恢复 false 后重新可用）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(qualifiedWorkspace.workspaceChannels).toHaveBeenCalledTimes(2); | ||
| expect(Object.keys(result?.channels ?? {})).toEqual(['bot-b']); |
There was a problem hiding this comment.
[Suggestion] This test of the workspace-qualified load path never asserts the catalog half: nothing checks workspaceChannelTypes is called or that result.catalog is populated. Deleting that call from the load Promise.all keeps every assertion green — in production the platform grid (availablePlatforms = catalog.filter(...)) would render empty for every non-primary workspace (no new channels addable there) while the channel list and lifecycle keep working. The e2e catches only the gross mutant (its Edit-button step); subtler content mutants slip through both. Add expect(qualifiedWorkspace.workspaceChannelTypes).toHaveBeenCalledTimes(2) and a result.catalog assertion.
中文说明
这个针对 workspace-qualified 加载路径的测试从未断言 catalog 一半：没有检查 workspaceChannelTypes 被调用，也没检查 result.catalog 有值。把该调用从加载 Promise.all 中删掉，所有断言仍绿——生产中平台网格（availablePlatforms = catalog.filter(...)）会在每个非主工作区渲染为空（无法新增频道），而频道列表和生命周期照常工作。e2e 只能抓住粗粒度的突变（靠其 Edit 按钮步骤）；更细的内容突变两边都漏。请补 expect(qualifiedWorkspace.workspaceChannelTypes).toHaveBeenCalledTimes(2) 和对 result.catalog 的断言。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const canManage = | ||
| supportsManagement && | ||
| Boolean(workspace.token) && | ||
| Boolean(activeWorkspaceCwd) && | ||
| activeWorkspace?.trusted === true; |
There was a problem hiding this comment.
[Suggestion] The new trust gate disables everything silently: capabilities computes trusted: state === 'active' && runtime.trusted, so bootstrapping/draining/inactive or never-trusted workspaces arrive as trusted: false; defaultWorkspace prefers primary over trusted, so the page can default-select an untrusted primary with every control and add-platform card disabled — and the only alerts are channels.unsupported.*/channels.readOnly.* (no trust-related key in either locale). The channel load fails with the generic "Channels could not be loaded" + "Workspace is not trusted.", suggesting nothing about switching workspaces; when all entries are untrusted every picker option is disabled too — permanently inert with no remediation path. Add an explanatory alert branch for supportsManagement && workspace.token && !canManage (naming the workspace and the trust requirement), and consider preferring a trusted entry over a non-trusted primary in defaultWorkspace.
中文说明
新的信任门会静默禁用一切：capabilities 计算 trusted: state === 'active' && runtime.trusted，因此 bootstrapping/draining/inactive 或从未受信任的工作区会以 trusted: false 到达；defaultWorkspace 优先 primary 而非 trusted，页面可能默认选中一个未受信任的主工作区，所有控件和添加平台卡片都被禁用——而仅有的告警是 channels.unsupported.*/channels.readOnly.*（两种语言都没有信任相关的 key）。频道加载以笼统的 "Channels could not be loaded" + "Workspace is not trusted." 失败，对“切换工作区”毫无提示；当所有条目都未受信任时选择器每个选项也都被禁用——永久不可用且没有补救路径。请为 supportsManagement && workspace.token && !canManage 增加解释性告警分支（指出工作区和信任要求），并考虑让 defaultWorkspace 优先选择受信任条目而非未受信任的主工作区。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const configuredSessionScope = | ||
| (rawConfig['sessionScope'] as ChannelConfig['sessionScope']) || | ||
| plugin.defaultSessionScope || | ||
| 'user'; |
There was a problem hiding this comment.
[Suggestion] The thread→chat_thread normalization of a plugin-supplied defaultSessionScope has zero coverage. Probe: normalizing only the explicitly configured value keeps all 159 channel tests green — the shipped regression would be channels of a plugin declaring defaultSessionScope: 'thread' (a legal SessionScope value) silently keeping the deprecated scope when sessionScope is unconfigured, skipping the routing-key migration (the silent session-split shape of the normalization finding above). No builtin declares 'thread' (github/gitlab declare 'chat_thread'), so the trigger is an extension plugin. Add a parseChannelConfig test registering a mock plugin with defaultSessionScope: 'thread' and no configured sessionScope, asserting the result is 'chat_thread'.
中文说明
对插件提供的 defaultSessionScope 做 thread→chat_thread 归一化这一路径零覆盖。探针：只对显式配置的值归一化时全部 159 个 channel 测试仍绿——会带上线的回归是：声明 defaultSessionScope: 'thread'（合法的 SessionScope 值）的插件，在未配置 sessionScope 的频道上悄悄保留废弃 scope，跳过路由键迁移（即上面归一化发现中的静默会话拆分形态）。没有内置插件声明 'thread'（github/gitlab 声明 'chat_thread'），触发需要扩展插件。请补一个 parseChannelConfig 测试：注册 defaultSessionScope: 'thread' 且未配置 sessionScope 的 mock 插件，断言结果为 'chat_thread'。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| dmPolicy: new Set(['open', 'disabled']), | ||
| groupPolicy: new Set(['disabled', 'allowlist', 'pairing', 'open']), | ||
| sessionScope: new Set(['user', 'thread', 'single']), | ||
| sessionScope: new Set(['user', 'thread', 'chat_thread', 'single']), |
There was a problem hiding this comment.
[Suggestion] The back-compat retention of 'thread' in this whitelist is load-bearing but untested. Probe: removing 'thread' keeps all 67 store tests green while 400-ing every upsert that re-sends a legacy stored config (Channel field "sessionScope" has an invalid value.) — stored 'thread' values genuinely exist (pre-PR configs; normalization is parse-time only and never rewritten back), and any client re-sending raw stored config (SDK/CLI/scripts; the web editor coerces on open) hits the lockout — the same failure mode as the groups-validation finding, with a real producer of the value. Add a store test upserting a config whose sessionScope is 'thread' and assert it is accepted and round-trips.
中文说明
该白名单对 'thread' 的向后兼容保留是承重的，但没有测试。探针：移除 'thread' 后全部 67 个 store 测试仍绿，而任何重新提交旧存储配置的 upsert 都会 400（Channel field "sessionScope" has an invalid value.）——存储的 'thread' 值真实存在（PR 之前的配置；归一化只发生在解析时，不会写回），任何重新提交原始存储配置的客户端（SDK/CLI/脚本；web 编辑器在打开时才强制转换）都会撞上这个锁定——与 groups 校验发现相同的失败模式，且该值有真实来源。请补一个 store 测试：upsert sessionScope 为 'thread' 的配置，断言被接受且可往返。
— qwen3.8-max via Qwen Code /review (v0.21.8)
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 explored to full depth (tool budget reached): PR #8848 重构 Web Shell 的 Channel 管理（开放共享的私聊/群聊准入策略、会话隔离与工作...: none — all eight checklist items for my dimension (mutable fields × every exit path, timers × every teardown, collection insert/delete pairing and ordering) wer…; PR #8848 重构 Web Shell 的 Channel 管理（第 11 轮评审，HEAD ec7f42f...: none — all checks above completed within budget.; PR #8848 重构 Web Shell 的 Channel 管理（开放共享的私聊/群聊准入策略、会话隔离与工作...: none — all checks above were run to completion within budget.; PR #8848 重构 Web Shell 的 Channel 管理（第 11 轮评审，HEAD ec7f42f...: none — all checks I opened were completed within budget (~13 tool calls).; chunk 4: none — all planned checks were completed.Typecheck is clean and all tests pass. My review of the territory is complete — I have no findings to report. Here is t…, and 5 more.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：PR #8848 重构 Web Shell 的 Channel 管理（开放共享的私聊/群聊准入策略、会话隔离与工作...：none — all eight checklist items for my dimension (mutable fields × every exit path, timers × every teardown, collection insert/delete pairing and ordering) wer…；PR #8848 重构 Web Shell 的 Channel 管理（第 11 轮评审，HEAD ec7f42f...：none — all checks above completed within budget.；PR #8848 重构 Web Shell 的 Channel 管理（开放共享的私聊/群聊准入策略、会话隔离与工作...：none — all checks above were run to completion within budget.；PR #8848 重构 Web Shell 的 Channel 管理（第 11 轮评审，HEAD ec7f42f...：none — all checks I opened were completed within budget (~13 tool calls).；chunk 4：none — all planned checks were completed.Typecheck is clean and all tests pass. My review of the territory is complete — I have no findings to report. Here is t…，另有 5 条。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| <Input | ||
| id={`${formId}-allowedGroupIds`} | ||
| value={draft.allowedGroupIds} |
There was a problem hiding this comment.
[Suggestion] The new Allowed group IDs input omits aria-invalid, unlike every other validation-capable control in this dialog — renderTextInput (~line 603), the name input (~line 680), and the sessionScope/senderPolicy RadioGroups (~lines 752/850) all set aria-invalid={Boolean(error)}. — Failure scenario: under group policy Allowlist, typing a reserved group ID (__proto__/constructor/prototype) and submitting sets errors['allowedGroupIds'] and the FieldShell renders the message as role="alert", but the input itself reports no invalid state — a screen-reader user tabbing back into the field gets no invalid announcement on the control, while sibling fields (Allowed user IDs, credential fields) do announce it.
| <Input | |
| id={`${formId}-allowedGroupIds`} | |
| value={draft.allowedGroupIds} | |
| <Input | |
| id={`${formId}-allowedGroupIds`} | |
| value={draft.allowedGroupIds} | |
| aria-invalid={Boolean(errors['allowedGroupIds'])} |
中文说明
新增的 Allowed group IDs 输入框缺少 aria-invalid，而本对话框中其他所有可校验控件都有——renderTextInput（约 603 行）、名称输入框（约 680 行）、sessionScope/senderPolicy 单选组（约 752/850 行）都设置了 aria-invalid={Boolean(error)}。— 失败场景：群组策略为白名单时，输入保留群组 ID（__proto__/constructor/prototype）并提交后，errors['allowedGroupIds'] 被置位，FieldShell 以 role="alert" 渲染错误文案，但输入框本身不报告无效状态——使用读屏器的用户 tab 回到该字段时控件没有任何无效播报，而同级字段（允许的用户 ID、凭据字段）都有。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
已修复。验证证据：保留群组 ID 提交后，Allowed group IDs 输入框现会报告 aria-invalid=true；聚焦 Web Shell 测试 72/72、Playwright Channel 流程 1/1 通过。修复提交：6c532feca5。
| workspaces: [] as Array<{ | ||
| id: string; | ||
| cwd: string; |
There was a problem hiding this comment.
[Suggestion] This diff adds workspaces as a required property of the hoisted mock's inferred type, but the pre-existing untouched assignment capabilities: { features: [] } at line 748 (test 'does not load Channel routes when the capability is unavailable') lacks it — a strict-tsc TS2741 introduced by the diff on a line the diff does not touch. — Concrete cost: probe-verified with the repo's TypeScript (strict, web-shell compilerOptions): ChannelsManagerPage.test.tsx(748,7): error TS2741: Property 'workspaces' is missing in type '{ features: never[]; }'; applying the fix below flips the probe. CI stays green only because web-shell's typecheck targets exclude client/**/*.test.tsx, but the TS language server reports a red error at line 748 in every editor session from this PR on, and the file fails type checking the moment tests are ever added to a type-check target.
Fix at line 748:
capabilities: { features: [], workspaces: [] },(or declare the hoisted mock's workspaces property optional)
中文说明
本 diff 把 workspaces 加成了 hoisted mock 推断类型中的必需属性，但第 748 行（测试 'does not load Channel routes when the capability is unavailable'）中既有的、未被本 diff 触碰的赋值 capabilities: { features: [] } 缺少该属性——这是 diff 在一行未改动代码上引入的 strict-tsc TS2741。— 具体代价：已用仓库自带 TypeScript（strict、web-shell 编译选项）探针验证：ChannelsManagerPage.test.tsx(748,7): error TS2741: Property 'workspaces' is missing in type '{ features: never[]; }'；应用下方修复后探针翻转。CI 之所以仍为绿，仅因 web-shell 的 typecheck 目标排除了 client/**/*.test.tsx，但从本 PR 起每个编辑器会话的 TS 语言服务器都会在 748 行报红；一旦测试文件被纳入某个 type-check 目标即会失败。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
已修复。验证证据：capability-unavailable fixture 已补齐 workspaces 空列表；精确 TypeScript probe 不再报告 workspaces 缺失，聚焦 Web Shell 测试 72/72、全量 typecheck 通过。修复提交：6c532feca5。
| if ( | ||
| field.key === 'sessionScope' && | ||
| field.default === 'thread' && | ||
| field.options?.some((option) => option.value === 'chat_thread') | ||
| ) { | ||
| return 'chat_thread'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] This coercion applies to edits of existing instances too, not only new channels: paired with the dialog's visibility filter (ChannelEditorDialog.tsx ~line 239 offers 'thread' only when instance?.config.sessionScope === 'thread'), an instance whose stored config omits sessionScope while its plugin declares defaultSessionScope: 'thread' is silently migrated on any save, with no UI path to keep its runtime-effective scope. — Failure scenario (probe-verified end-to-end at this commit): an extension channel plugin declares defaultSessionScope: 'thread' (a shape the registry explicitly supports and tests via the valid-optional-required-object fixture); an instance created without an explicit sessionScope routes via 'thread' (name:<threadId>). Opening the editor seeds the draft with 'chat_thread' and hides the thread option, so saving an unrelated change (e.g. rotating a credential) persists sessionScope: 'chat_thread'; SessionRouter.routingKey then produces name:<chatId>:<threadId> and every existing per-thread session on that channel is orphaned. The probe flips with the fix below. No built-in plugin defaults to 'thread' (github/gitlab use 'chat_thread'), so only extension plugins are affected; the shipped docs wording (legacy 'thread' is preserved "when already configured") endorses neither migrating nor hiding this third state.
| if ( | |
| field.key === 'sessionScope' && | |
| field.default === 'thread' && | |
| field.options?.some((option) => option.value === 'chat_thread') | |
| ) { | |
| return 'chat_thread'; | |
| } | |
| if ( | |
| field.key === 'sessionScope' && | |
| field.default === 'thread' && | |
| field.options?.some((option) => option.value === 'chat_thread') | |
| ) { | |
| return instance ? 'thread' : 'chat_thread'; | |
| } |
(or offer the legacy option whenever the runtime-effective scope is 'thread' — stored value or plugin default)
中文说明
该强制转换对编辑已有实例同样生效，而不仅限新建频道：配合对话框的可见性过滤（ChannelEditorDialog.tsx 约 239 行，仅当 instance?.config.sessionScope === 'thread' 时才提供 'thread' 选项），一个存储配置中没有 sessionScope、而其插件声明了 defaultSessionScope: 'thread' 的实例，在任何一次保存时都会被静默迁移，且 UI 上没有任何途径保持其运行时实际生效的范围。— 失败场景（已在当前提交端到端探针验证）：扩展频道插件声明 defaultSessionScope: 'thread'（registry 明确支持并用 valid-optional-required-object 夹具测试了该形状）；未显式设置 sessionScope 的实例按 'thread' 路由（name:<threadId>）。打开编辑器时草稿被填入 'chat_thread' 且 thread 选项被隐藏，因此保存任何无关改动（如轮换凭据）都会持久化 sessionScope: 'chat_thread'；SessionRouter.routingKey 随之变为 name:<chatId>:<threadId>，该频道所有既有的按 thread 会话全部孤立。应用下方修复后探针翻转。内置插件均不以 'thread' 为默认（github/gitlab 用 'chat_thread'），故仅扩展插件受影响；随 PR 发布的文档措辞（旧 'thread' 在"已配置"时保留）既不支持迁移、也不支持隐藏这第三种状态。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
已修复。验证证据：已有实例继承 extension plugin 的 legacy thread 默认值时，编辑草稿与保存请求均保留 thread，且 UI 显示并选中 legacy 选项；回归 mutation 撤掉修复后按预期失败，聚焦测试 72/72 通过。修复提交：6c532feca5。
| key: 'senderPolicy', | ||
| label: 'Sender Policy', | ||
| kind: 'enum', | ||
| required: true, | ||
| default: 'pairing', |
There was a problem hiding this comment.
[Suggestion] This fixture declares senderPolicy with default: 'pairing', but the descriptor actually served for builtin channels — created by this same PR in channel-registry.ts (SHARED_ACCESS_FIELDS, pinned by channel-registry-builtins.test.ts) — declares default: 'allowlist'; the dialog tests therefore exercise a non-production initial state. — Failure scenario (probe-verified at this commit): with the production 'allowlist' default, a freshly opened create dialog renders the "Allowed user IDs" input immediately, so the initial-state assertion expect(inputByLabel('Allowed user IDs')).toBeNull() (~line 548) is only true because of this divergent default — the production create-flow initial state is untested, and a regression in the 'allowlist' default's seeding/visibility handling ships green. The same divergence exists at the two e2e mock descriptors (web-shell.channels.spec.ts ~line 221 and visuals/screenshots.spec.ts ~line 265), already flagged in an open prior-round comment.
| key: 'senderPolicy', | |
| label: 'Sender Policy', | |
| kind: 'enum', | |
| required: true, | |
| default: 'pairing', | |
| key: 'senderPolicy', | |
| label: 'Sender Policy', | |
| kind: 'enum', | |
| required: true, | |
| default: 'allowlist', |
After aligning the default, update the dependent assertions — e.g. assert the input is visible initially, or start the "hidden until Allowlist" check from an explicit 'Pairing' selection.
中文说明
该夹具把 senderPolicy 声明为 default: 'pairing'，但内置频道实际下发的描述符——由本 PR 自己在 channel-registry.ts 中创建（SHARED_ACCESS_FIELDS，且 channel-registry-builtins.test.ts 已固定）——声明的是 default: 'allowlist'；因此对话框测试运行在一个非生产的初始状态上。— 失败场景（已在当前提交探针验证）：按生产的 'allowlist' 默认值，新建对话框一打开就会渲染"允许的用户 ID"输入框，所以初始状态断言 expect(inputByLabel('Allowed user IDs')).toBeNull()（约 548 行）仅因这个分歧的默认值才为真——生产新建流程的初始状态无人测试，'allowlist' 默认值的种子/可见性处理若发生回归会绿灯溜走。同样的分歧还存在于两个 e2e mock 描述符（web-shell.channels.spec.ts 约 221 行、visuals/screenshots.spec.ts 约 265 行），此前轮次已有未关闭的评论指出。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
已修复。验证证据：单元、E2E 与 visual descriptor 的 sender policy 默认值已与生产 allowlist 对齐，并显式覆盖 Allowlist→Pairing→Allowlist 可见性；聚焦测试 72/72、Playwright Channel 流程 1/1 通过。修复提交：6c532feca5。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "PR #8848 redesigns Web Shell Channel management: it…": none — all checks above were completed within budget.; "───── auditor 2 of 14 — chunk 2 ─────": none — all checks above completed within budget; the remainder of channel-settings-store.test.ts (diff lines 602–713) is chunk 3's territory and was deliberat…; "PR #8848 redesigns Web Shell Channel management: it…": none — all planned checks completed within budget.; "───── auditor 2 of 13 — chunk 2 ─────": none — all checks above completed within budget.; "───── auditor 12 of 14 — chunk 12 ─────": none — all checks I started were completed within budget., and 7 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："PR #8848 redesigns Web Shell Channel management: it…"：none — all checks above were completed within budget.；"───── auditor 2 of 14 — chunk 2 ─────"：none — all checks above completed within budget; the remainder of channel-settings-store.test.ts (diff lines 602–713) is chunk 3's territory and was deliberat…；"PR #8848 redesigns Web Shell Channel management: it…"：none — all planned checks completed within budget.；"───── auditor 2 of 13 — chunk 2 ─────"：none — all checks above completed within budget.；"───── auditor 12 of 14 — chunk 12 ─────"：none — all checks I started were completed within budget.，另有 7 条。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (key === 'groups') { | ||
| if (!isRecord(value)) { | ||
| throw invalidConfig(`Channel field "${key}" must be an object.`); |
There was a problem hiding this comment.
[Suggestion] The new groups branch rejects a stored non-record groups value (e.g. null or []) even when the client re-sends it completely unchanged — a shape the pre-diff preserve-verbatim path (assertPreservedUnknownField fallthrough) accepted, and contrary to this file's own documented grace for unchanged stored objects. Probe-verified at this commit: stored groups: null / groups: [] re-sent on an unrelated edit is rejected with channel_settings_invalid_config, while the identical shape under an unknown key is accepted via the deep-equal fallthrough; adding the deep-equal preservation carve-out below flips both probes. — Concrete cost: a hand-written or externally produced settings.json with "groups": null on a channel locks every UI save of that channel (even unrelated edits like rotating a token) until someone hand-edits the file — the same lockout class this PR's review history treated as merge-blocking (R2-3, R5-2). The entry-level sibling check is the open R7-15 thread; this is the container-level check.
| if (key === 'groups') { | |
| if (!isRecord(value)) { | |
| throw invalidConfig(`Channel field "${key}" must be an object.`); | |
| if (key === 'groups') { | |
| if (!isRecord(value)) { | |
| if ( | |
| containsUnsafeObjectKey(value) || | |
| !isDeepStrictEqual(previous, value) | |
| ) { | |
| throw invalidConfig(`Channel field "${key}" must be an object.`); | |
| } | |
| return true; |
中文说明
新的 groups 分支会拒绝已存储的非 record groups 值（如 null 或 []）——即使客户端原样重发。PR 之前这类形状走 assertPreservedUnknownField 的原样保留路径是被接受的，且与本文件对"未改动存储对象"的书面宽限相矛盾。已在当前提交探针验证：存储 groups: null / groups: [] 在任何无关编辑中被原样重发时抛 channel_settings_invalid_config，而未知键下的相同形状经深相等路径通过；加上下方的深相等保留例外后两个探针均翻转。— 具体代价：手写或外部生成的 settings.json 中某频道含 "groups": null 时，该频道的每一次 UI 保存（包括轮换 token 这类无关编辑）都会抛错，直到有人手改配置文件——与本 PR 评审历史中被当作合并阻塞的锁死同类（R2-3、R5-2）。条目级的同类检查是未关闭的 R7-15 线程；本条是容器级检查。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
已修复。验证证据：fd6241b10d；channel-settings-store.test.ts 75/75，通过 unchanged null/[]，changed/unsafe 值仍拒绝；focused ESLint/Prettier 与 git diff --check 通过。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…": I did not execute the web-shell/cli/webui unit tests in this worktree; verification was by code reading and cross-file tracing only (CI result not inspected).; "本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…": there's one incomplete check—the daemon route-level trust enforcement for workspace channel mutation routes (I didn't read the route handler; the service assert…; "本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…": daemon route-handler layer for /workspaces/{cwd}/channels* (trust/registration enforcement at the route, not the service) — not read; that territory overlaps …; "本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…": did not execute web-shell unit tests; verification was by source reading only.; "本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…": none — all checks I started were completed within budget., and 12 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…"：I did not execute the web-shell/cli/webui unit tests in this worktree; verification was by code reading and cross-file tracing only (CI result not inspected).；"本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…"：there's one incomplete check—the daemon route-level trust enforcement for workspace channel mutation routes (I didn't read the route handler; the service assert…；"本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…"：daemon route-handler layer for /workspaces/{cwd}/channels* (trust/registration enforcement at the route, not the service) — not read; that territory overlaps …；"本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…"：did not execute web-shell unit tests; verification was by source reading only.；"本 PR 重构 Web Shell 的 Channel 管理：为所有可管理适配器开放共享的私聊/群聊准入策略、会话隔离…"：none — all checks I started were completed within budget.，另有 12 条。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if ( | ||
| field.key === 'sessionScope' && | ||
| field.default === 'thread' && | ||
| field.options?.some((option) => option.value === 'chat_thread') | ||
| ) { | ||
| return instance ? 'thread' : 'chat_thread'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-2: A plugin that declares its own sessionScope enum with default: 'thread' and options lacking chat_thread passes assertManagementDescriptor; for a NEW channel the draft then initializes to 'thread' (this migration branch requires a chat_thread option), while the dialog's sessionScopeOptions filter hides 'thread' for new instances — the RadioGroup value matches no rendered option, and saving as-is persists the legacy scope the filter exists to hide. — Failure scenario: a third-party plugin registers sessionScope {default: 'thread', options: [user, thread]} (admissible today) → creating a new channel renders "Conversation isolation" with zero checked radios (probe-verified at this commit), and saving as-is persists sessionScope: 'thread', contradicting the PR's documented "not offered when creating a new Channel" intent; the probe flips when the new-channel branch falls back to the first non-thread option. No builtin plugin triggers this today.
| if ( | |
| field.key === 'sessionScope' && | |
| field.default === 'thread' && | |
| field.options?.some((option) => option.value === 'chat_thread') | |
| ) { | |
| return instance ? 'thread' : 'chat_thread'; | |
| } | |
| if (field.key === 'sessionScope' && field.default === 'thread') { | |
| if (instance) return 'thread'; | |
| if (field.options?.some((option) => option.value === 'chat_thread')) { | |
| return 'chat_thread'; | |
| } | |
| return ( | |
| field.options?.find((option) => option.value !== 'thread')?.value ?? | |
| 'user' | |
| ); | |
| } |
中文说明
插件若自行声明 sessionScope 枚举且 default: 'thread'、选项中又不含 chat_thread，仍可通过 assertManagementDescriptor；此时新建频道的草稿会初始化为 'thread'（该迁移分支要求存在 chat_thread 选项），而弹窗的 sessionScopeOptions 过滤器对新建实例隐藏 'thread'——RadioGroup 的值与任何已渲染选项都不匹配，直接保存会把该过滤器本要隐藏的旧版 scope 持久化。— 失败场景：第三方插件注册 sessionScope {default: 'thread', options: [user, thread]}（当前可通过校验）→ 新建频道时 "Conversation isolation" 中没有任何选中项（已在本提交探针验证），直接保存会持久化 sessionScope: 'thread'，与 PR 文档"新建 Channel 不提供该选项"的意图矛盾；将新建分支改为回退到首个非 thread 选项后探针翻转。当前所有内置插件均不会触发。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
已修复。验证证据：fd6241b10d；Web Shell state 37/37，独立 post-fix state+dialog 59/59；Web Shell build/typecheck、focused ESLint/Prettier 与 git diff --check 通过。
| await act(async () => finishPrimaryStart()); | ||
| expect(secondaryStart?.disabled).toBe(true); |
There was a problem hiding this comment.
[Suggestion] R1-3: Per-workspace ERROR isolation — the actionErrorKey(workspaceCwd, name) keying on both the write side (runAction's catch) and the read side (the card's runtimeError lookup) — has zero cross-workspace test coverage; every error-path test in this file runs in a single workspace. The code is correct today. — Failure scenario: a future refactor symmetrically degrades both the writer and the reader back to name-only keys → all tests stay green, while in production a failed Start in workspace A renders its destructive error banner on workspace B's same-named channel, attributing the failure to the wrong workspace. Suggested fix: add a sibling to the independence test — make start reject for the primary workspace, switch to secondary and assert primary's error text is absent from secondary's card, then switch back and assert it is shown only there.
// sketch: reject start on the primary workspace, switch to secondary,
// expect secondary's card to show no error text, switch back to primary,
// expect the error banner visible only on primary's card中文说明
R1-3：按工作区隔离的错误状态——写侧（runAction 的 catch）与读侧（卡片 runtimeError 查找）都使用 actionErrorKey(workspaceCwd, name) 键——完全没有跨工作区测试覆盖；本文件中所有错误路径测试都只在单一工作区运行。当前代码是正确的。— 失败场景：未来某次重构把写侧和读侧对称地退回到仅按名称的键 → 所有测试仍然为绿，而生产中工作区 A 的一次失败启动会把其错误横幅渲染到工作区 B 的同名频道上，把失败归因到错误的工作区。建议修复：为独立性测试补一个姊妹用例——让主工作区的 start 拒绝，切到次工作区并断言主工作区的错误文案不出现在次工作区卡片上，再切回主工作区断言错误只在那里显示。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| result = useDaemonChannels({ | ||
| autoLoad: true, | ||
| workspaceCwd: '/workspace-b', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R1-5: The explicit workspaceCwd mode has exactly one test with one static workspace; switching the explicit option between two non-provider workspaces — the path ChannelsManagerPage actually drives — is untested, so the workspaceClient memo deps and the current staleness filter are unpinned for explicit-mode switches. The code is correct today. — Failure scenario (probe-verified at this commit): removing workspaceCwd from the memo deps keeps the entire existing suite green (27/27), while a probe that re-renders with /workspace-b → /workspace-c observes workspace B's channels exposed as C's (Object.keys(result.channels) === ['bot-b'] after the switch) and mutations would hit the stale client — the exact cross-workspace blast the qualified routes exist to prevent. Distinct from the open coverage comments on these files (pairing branch, five mutation branches, load-path catalog assertion): none of them re-renders with a different explicit cwd. Suggested fix: render with workspaceCwd: '/workspace-b', resolve a load, re-render with workspaceCwd: '/workspace-c' (a second qualified mock), and assert B's data is hidden while loading, C's data lands, client.workspaceByCwd was called for both cwds, and no mutation reaches the stale client.
中文说明
R1-5：显式 workspaceCwd 模式只有一个使用静态工作区的测试；在两个非 provider 工作区之间切换显式选项——正是 ChannelsManagerPage 实际驱动的路径——没有测试覆盖，因此显式模式切换下的 workspaceClient memo 依赖与 current 过期过滤都未被固定。当前代码是正确的。— 失败场景（已在本提交探针验证）：从 memo 依赖中移除 workspaceCwd 后整个现有测试套件仍为绿（27/27），而一个把 /workspace-b → /workspace-c 重新渲染的探针观察到工作区 B 的频道被当作 C 的暴露（切换后 Object.keys(result.channels) === ['bot-b']），且变更操作会打到陈旧的客户端——正是 workspace-qualified 路由要防止的跨工作区串扰。与这些文件上已有的覆盖类评论（pairing 分支、五个变更分支、load 路径 catalog 断言）不同：它们都不会用不同的显式 cwd 重新渲染。建议修复：以 workspaceCwd: '/workspace-b' 渲染并完成一次加载，再以 workspaceCwd: '/workspace-c'（第二个 qualified mock）重新渲染，断言加载中 B 的数据被隐藏、C 的数据落地、client.workspaceByCwd 对两个 cwd 均被调用，且任何变更操作都不会到达陈旧客户端。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Follow-up real-stack verification (maintainer) — the two findings from my previous reviewRe-verified the PR after the five commits that followed my earlier report (
Verdict: finding 1 is fixed and holds on the real stack, and the new Finding 1 (four-option control / no
|
| Claim | Evidence |
|---|---|
New Channels don't offer thread |
Fresh WeCom create dialog renders exactly user / chat_thread / single, user preselected; no "By thread (legacy)" label anywhere |
| Existing non-thread instances don't offer it either | dingtalk-open editor: 3 options, user selected |
Existing thread config keeps its behavior |
wecom-legacy editor shows By thread (legacy) as a fourth option, selected; saving writes "sessionScope": "thread" back verbatim (PUT body and settings.json both confirmed), with groups."*", group-a.groupHistoryLimit: 25 and the stored secret preserved |
| Round-trip is stable | Re-opening after save still shows the legacy option selected; a second save is byte-identical on disk |
| Fixtures now mirror the daemon | The e2e descriptor now carries all four options incl. thread (default: 'user'), so the earlier fixture/catalog divergence that hid this bug is gone |
Two small notes, neither blocking:
GET /workspace/channel-typesstill advertises['user','thread','chat_thread','single']for every manageable adapter; the filtering lives inChannelEditorDialog. That's a reasonable place for it (the daemon must keep accepting storedthread), just worth knowing that any other future consumer of the catalog would seethreadagain.- The
.sessionScopeControlgrid is stillrepeat(3, …), so the four-option legacy case wraps "Share all" onto a lone second row (screenshots below). Now only reachable for legacy instances, so cosmetic — fine to leave as-is in my view.
New groups store guard (fd6241b1) — verified on the wire ✅
Seeded wecom-oldgroups with groups: null (a legacy non-record value the previous revision of this branch would have rejected unconditionally on any edit):
- Editing the channel in the UI (changed
botIdonly) →PUTcarriesgroups: nullunchanged → 200, and disk still hasgroups: nullafterwards. - Direct API probe with a changed non-record value (
groups: []) → 400channel_settings_invalid_config("must be an object"), as intended. - The unsafe-key rejection branch is covered by the new store unit tests (416 cli tests pass, see below).
Finding 2 (new Channels default to a closed allowlist) — still open, unchanged ⚠️
Same flow as last time on the new head: create a WeCom Channel, touch nothing but name/credentials, save. Persisted config:
{ "type": "wecom", "botId": "bot-x", "senderPolicy": "allowlist", "groupPolicy": "disabled", "sessionScope": "user" }No allowedUsers, no pairing fallback in SenderGate.check() → such a Channel still rejects every direct message with no way for a sender to request access. The editor doesn't require any IDs when Allowlist is selected, and the Risk section still doesn't mention the default change from the old Web Shell path (pairing). As said before, this may well be intentional (it matches the CLI default) — but please either default the descriptor to pairing or state the behavior change in the description before merge.
Scope notes
- The "inherited defaults" slice of
6c532fec(descriptordefault: 'thread'via a plugin'sdefaultSessionScope) is not reachable with any built-in adapter, so it was verified through the unit tests plus the new registry assertions (assertManagementDescriptornow validates the catalog's scope consistency at load), not on the live stack. - Workspace-qualified routes, group-allowlist round-trip, and the WeCom
chat_threadrouting fix were verified in my previous round; that code is unchanged since, and this round's create/edit flows ran on the post-main-merge head without regressions.
Focused suites (head)
web-shell channels 101 ✓ · cli channel + settings-store 416 ✓ (includes the new legacy-groups tests) · webui useDaemonChannels 10 ✓ · wecom 135 ✓
Screenshots
| New WeCom — three-way control, no legacy option | Legacy instance — fourth option shown & selected (light) |
 |
 |
| Legacy instance (dark) — note "Share all" wrapping | Manager (dark) |
 |
 |
| Manager (light) | Manager at 420px |
 |
 |
Environment: macOS 26.6 (arm64), Node 24.18.1, Chromium (Playwright 1.58.2), no real platform credentials.
中文版本
复验报告（维护者）—— 针对我上一轮提出的两个问题
在上一份报告之后，作者推了五个提交（013b31ea、合并 main、ec7f42f0、6c532fec、fd6241b1）。我在同样的真实环境上复验：PR head 自行构建并由真实 qwen serve daemon 提供服务（独立 bundle、独立 QWEN_HOME、工作区 ws-a/ws-b），用 Chromium 驱动。
| PR head | fd6241b10d |
| 栈 | dist/cli.js serve --workspace ws-a --workspace ws-b，端口 :5991 |
| 预置数据 | wecom-legacy（sessionScope: thread、私聊+群白名单、groups: { "*": {…}, "group-a": {…} }）、wecom-oldgroups（groups: null，旧版非对象值）、dingtalk-open |
| 判据 | 浏览器 DOM · 抓取的 PUT 请求体 · 工作区磁盘上的 .qwen/settings.json · 直接对 store 的 API 探针 |
结论：问题 1 已修复且在真实栈上成立；新加的 groups store 守卫行为与设计完全一致。问题 2 代码未变、描述中也仍未提及——这个文档层面的点是我仅剩的诉求。
问题 1（四选项控件 / thread 未归一）—— 已修复 ✅
修复方式是客户端过滤（daemon 目录仍下发全部四个 scope，见下面的说明），加上对已有 legacy 配置的「保存时原样保留」语义。PR 声明的行为在真实 daemon 上全部复现：
| 结论 | 证据 |
|---|---|
新建频道不提供 thread |
全新 WeCom 创建弹窗只渲染 user / chat_thread / single，默认选中 user；任何位置都没有「By thread (legacy)」 |
| 已有非 thread 实例同样不提供 | dingtalk-open 编辑器：3 个选项，选中 user |
已有 thread 配置保留原行为 |
wecom-legacy 编辑器显示第四个选项 By thread (legacy) 且为选中态；保存后 "sessionScope": "thread" 原样写回（PUT 请求体与 settings.json 双重确认），groups."*"、group-a.groupHistoryLimit: 25 与密钥均保留 |
| 往返稳定 | 保存后重开仍显示并选中 legacy 选项；再次保存磁盘内容不变 |
| fixture 已对齐 daemon | e2e descriptor 现在带全部四个选项（含 thread，default: 'user'），之前掩盖此 bug 的 fixture/真实目录脱节已消除 |
两个小注记，均不阻塞：
GET /workspace/channel-types对所有可管理适配器仍下发['user','thread','chat_thread','single']，过滤在ChannelEditorDialog里。这个位置是合理的（daemon 必须继续接受已存的thread），只是将来目录若有其他消费方，会再次看到thread。.sessionScopeControl仍是repeat(3, …)网格，legacy 的四选项场景下「Share all」会换行独占一行（见截图）。现在只有 legacy 实例才会遇到，属外观问题，我认为可以不改。
新的 groups store 守卫（fd6241b1）—— 已在真实链路上验证 ✅
预置 wecom-oldgroups 的 groups: null（旧版非对象值，本分支上一版对任何编辑都会无条件拒绝）：
- UI 里编辑该频道（只改
botId）→PUT原样携带groups: null→ 200，磁盘上仍是groups: null。 - 直接 API 探针发送变更后的非对象值（
groups: []）→ 400channel_settings_invalid_config（"must be an object"），符合设计。 - 不安全键名的拒绝分支由新增 store 单测覆盖（cli 416 个用例全过，见下）。
问题 2（新建频道默认封闭白名单）—— 仍未处理，行为未变 ⚠️
在新 head 上重复上次的流程：新建 WeCom 频道，只填名称和凭据，直接保存。落盘配置：
{ "type": "wecom", "botId": "bot-x", "senderPolicy": "allowlist", "groupPolicy": "disabled", "sessionScope": "user" }没有 allowedUsers，SenderGate.check() 也没有配对兜底 → 这样的频道仍会拒绝所有私聊，对方没有任何申请入口。选中 Allowlist 时编辑器不要求填写任何 ID，Risk 一节也仍未提及相对旧 Web Shell 路径（pairing）的默认值变化。如上次所说，这可能是有意为之（与 CLI 默认一致）——但请在合并前，要么把 descriptor 默认改为 pairing，要么在描述中明确说明这个行为变化。
范围说明
6c532fec的「继承默认值」分支（插件通过defaultSessionScope使 descriptordefault: 'thread'）任何内置适配器都无法触达，因此通过单测与新的注册期断言（assertManagementDescriptor现在在加载时校验目录 scope 一致性）验证，而非真实栈。- workspace-qualified 路由、群白名单往返、WeCom
chat_thread路由修复在我上一轮已验证；相关代码此后未变，本轮的新建/编辑流程在合并main后的 head 上运行无回归。
聚焦用例（head）
web-shell channels 101 ✓ · cli channel + settings-store 416 ✓（含新增 legacy-groups 用例） · webui useDaemonChannels 10 ✓ · wecom 135 ✓
环境
macOS 26.6（arm64）、Node 24.18.1、Chromium（Playwright 1.58.2），未使用任何真实平台凭据。截图同上方英文部分。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3 (rounds 1-2 reported findings; round 3 dry after dedup; chunk 4 retired dry in rounds 1 and 2 with no cold-check round left under the cap).
Not explored to full depth (tool budget reached): "PR #8848 redesigns Web Shell Channel management — exposing…": none — all checks above completed. The dialog-internal rendering beyond access-section visibility and ChannelsManagerPage.tsx belong to other chunks and were …; chunk 7: running ChannelsManagerPage.test.tsx to confirm green — blocked in this worktree because packages/webui/dist is absent (sibling package unbuilt, @qwen-code…; chunk 2: none — all planned checks completed within budget.; "PR #8848 redesigns Web Shell Channel management — exposing…": none — all checks above completed within budget.; "PR #8848 redesigns Web Shell Channel management — exposing…": did not run the full web-shell test suite (only ChannelsManagerPage.test.tsx ), so cross-file interactions with the unchanged tests elsewhere are unverified., and 9 more.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 3 (rounds 1-2 reported findings; round 3 dry after dedup; chunk 4 retired dry in rounds 1 and 2 with no cold-check round left under the cap)。
未探索到全部深度（达到工具调用预算）："PR #8848 redesigns Web Shell Channel management — exposing…"：none — all checks above completed. The dialog-internal rendering beyond access-section visibility and ChannelsManagerPage.tsx belong to other chunks and were …；chunk 7：running ChannelsManagerPage.test.tsx to confirm green — blocked in this worktree because packages/webui/dist is absent (sibling package unbuilt, @qwen-code…；chunk 2：none — all planned checks completed within budget.；"PR #8848 redesigns Web Shell Channel management — exposing…"：none — all checks above completed within budget.；"PR #8848 redesigns Web Shell Channel management — exposing…"：did not run the full web-shell test suite (only ChannelsManagerPage.test.tsx ), so cross-file interactions with the unchanged tests elsewhere are unverified.，另有 9 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| disabled={ | ||
| !supportsManagement || | ||
| Boolean(editor) || | ||
| loading || | ||
| busy !== null || | ||
| deleting | ||
| } |
There was a problem hiding this comment.
[Suggestion] The toolbar workspace Select is disabled while busy !== null, so while any lifecycle action is in flight the only direct workspace navigation control is locked — even though busyByWorkspace deliberately supports per-workspace independence and this PR's own test ('allows an independent lifecycle action after switching workspaces') proves switching mid-action is safe. runAction's entry guard reads the same per-workspace busy, so after switching, actions in the other workspace work — unlocking the picker would be fully functional, not moot. The only reachable switch route while busy is the non-obvious detour the test itself uses: open the Add-channel dialog (platform cards are not busy-gated) and change workspace inside it. — Failure scenario: operator starts a slow action (e.g. restart) in workspace A and wants to check or manage workspace B during it → toolbar picker disabled until A's action finishes → must take the dialog detour or wait out the action.
| disabled={ | |
| !supportsManagement || | |
| Boolean(editor) || | |
| loading || | |
| busy !== null || | |
| deleting | |
| } | |
| disabled={ | |
| !supportsManagement || | |
| Boolean(editor) || | |
| loading || | |
| deleting | |
| } |
中文说明
工具栏的工作区 Select 在 busy !== null 时被禁用，因此任何生命周期操作进行中时，唯一的直接工作区导航控件都会被锁定——尽管 busyByWorkspace 刻意支持按工作区独立，且本 PR 自己的测试（'allows an independent lifecycle action after switching workspaces'）已证明操作进行中切换工作区是安全的。runAction 的入口守卫读取的是同样的按工作区 busy，切换到另一工作区后该工作区的操作可正常进行——解锁该选择器完全可用，并非无意义。busy 期间唯一可达的切换路径是该测试自己使用的迂回方式：打开"添加频道"对话框（平台卡片不受 busy 门控），在对话框内切换工作区。— 失败场景：运维者在工作区 A 启动一个较慢的操作（如 restart），期间想查看或管理工作区 B → 工具栏选择器被禁用，直到 A 的操作完成 → 只能走对话框迂回路径或一直等待。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
已修复：工具栏 Workspace 选择器不再被其他 workspace 的 lifecycle busy 状态全局禁用，现有测试改为直接通过工具栏切换并验证两个 workspace 的 busy 状态彼此独立。验证：ChannelsManagerPage.test.tsx 14/14 通过；Web Shell typecheck、production build、focused ESLint、Prettier、git diff --check 均通过；独立 test-engineer 复现修复前失败并复验修复后通过。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 7: running ChannelsManagerPage.test.tsx under vitest — the review worktree has no node_modules (no root .bin/vitest , no package-level install), so verificati…; chunk 4: none — all checks I intended completed within budget.; chunk 13: did not execute web-shell.channels.spec.ts under Playwright (static verification only; the mock-daemon request arithmetic was verified against the full test s…; chunk 9: none — all checks I intended completed within budget..
[Critical] R14-2 Maintainer's outstanding merge condition (real-stack verification, @wenshao) still unaddressed at this head: creating a new Channel with untouched defaults persists senderPolicy: 'allowlist' with no allowedUsers (SHARED_ACCESS_FIELDS in packages/cli/src/commands/channel/channel-registry.ts still declares default: 'allowlist'), and SenderGate.check() has no pairing fallback under allowlist — a brand-new Channel therefore rejects every direct message with no way for a sender to request access, where the old Web Shell path persisted senderPolicy: 'pairing'. The maintainer verified this twice on a real qwen serve daemon and stated: "please either default the descriptor to pairing or state the behavior change in the description before merge", and in the follow-up: "Finding 2 is unchanged in code and still not mentioned in the description — that documentation-level point is my only remaining ask." Neither the descriptor default nor the description's Risk section changed since (see also the open R5-4 thread on channel-registry.ts:40). — Failure scenario: an operator creates a DingTalk/Feishu/WeCom Channel through the Web Shell, fills only name/credentials and saves → every direct message is silently rejected, senders get no pairing path, and nothing in the UI indicates the channel is unreachable. / 中文说明：维护者在真实栈验证中提出的合并条件在本 head 上仍未解决：按默认值新建频道会落盘 senderPolicy: 'allowlist' 且无 allowedUsers（channel-registry.ts 的 SHARED_ACCESS_FIELDS 仍声明 default: 'allowlist'），而 SenderGate.check() 在 allowlist 下没有配对兜底——新建频道会拒绝所有私聊且对方没有任何申请入口，而旧 Web Shell 路径落盘的是 senderPolicy: 'pairing'。维护者已在真实 qwen serve daemon 上两次验证并明确表示："合并前要么把 descriptor 默认改为 pairing，要么在描述中说明该行为变化"，复验时重申"这是我仅剩的诉求"。此后 descriptor 默认值与描述的风险一节均未改动（另见 channel-registry.ts:40 上未关闭的 R5-4 线程）。— 失败场景：运维通过 Web Shell 新建 DingTalk/Feishu/WeCom 频道，只填名称/凭据即保存 → 所有私聊被静默拒绝，发送者无配对入口，界面也无任何提示。
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 7：running ChannelsManagerPage.test.tsx under vitest — the review worktree has no node_modules (no root .bin/vitest , no package-level install), so verificati…；chunk 4：none — all checks I intended completed within budget.；chunk 13：did not execute web-shell.channels.spec.ts under Playwright (static verification only; the mock-daemon request arithmetic was verified against the full test s…；chunk 9：none — all checks I intended completed within budget.。
[Critical] R14-2 Maintainer's outstanding merge condition (real-stack verification, @wenshao) still unaddressed at this head: creating a new Channel with untouched defaults persists senderPolicy: 'allowlist' with no allowedUsers (SHARED_ACCESS_FIELDS in packages/cli/src/commands/channel/channel-registry.ts still declares default: 'allowlist'), and SenderGate.check() has no pairing fallback under allowlist — a brand-new Channel therefore rejects every direct message with no way for a sender to request access, where the old Web Shell path persisted senderPolicy: 'pairing'. The maintainer verified this twice on a real qwen serve daemon and stated: "please either default the descriptor to pairing or state the behavior change in the description before merge", and in the follow-up: "Finding 2 is unchanged in code and still not mentioned in the description — that documentation-level point is my only remaining ask." Neither the descriptor default nor the description's Risk section changed since (see also the open R5-4 thread on channel-registry.ts:40). — Failure scenario: an operator creates a DingTalk/Feishu/WeCom Channel through the Web Shell, fills only name/credentials and saves → every direct message is silently rejected, senders get no pairing path, and nothing in the UI indicates the channel is unreachable. / 中文说明：维护者在真实栈验证中提出的合并条件在本 head 上仍未解决：按默认值新建频道会落盘 senderPolicy: 'allowlist' 且无 allowedUsers（channel-registry.ts 的 SHARED_ACCESS_FIELDS 仍声明 default: 'allowlist'），而 SenderGate.check() 在 allowlist 下没有配对兜底——新建频道会拒绝所有私聊且对方没有任何申请入口，而旧 Web Shell 路径落盘的是 senderPolicy: 'pairing'。维护者已在真实 qwen serve daemon 上两次验证并明确表示："合并前要么把 descriptor 默认改为 pairing，要么在描述中说明该行为变化"，复验时重申"这是我仅剩的诉求"。此后 descriptor 默认值与描述的风险一节均未改动（另见 channel-registry.ts:40 上未关闭的 R5-4 线程）。— 失败场景：运维通过 Web Shell 新建 DingTalk/Feishu/WeCom 频道，只填名称/凭据即保存 → 所有私聊被静默拒绝，发送者无配对入口，界面也无任何提示。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
已修复：新建 Channel 的共享私聊策略默认值已从 allowlist 恢复为 pairing；旧配置缺省 senderPolicy 时仍保留 allowlist 兼容语义。验证证据：registry 回归 44/44、Web Shell editor-state 37/37、SenderGate 16/16、目标 Chromium E2E 1/1、全仓 build 与 typecheck、ESLint/Prettier、git diff --check 均通过。Commit: ad365b8 |
已被后续 commit 取代，当前 head 需重新 review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — reached the round cap of 3 without two consecutive dry rounds (the rounds 1+2 pair reported findings; round 3 reported 1 finding).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — the walk above completed within budget; no check left unfinished.; chunk 4: none — could not run the vitest suite locally (no node_modules in the worktree), so CI status was used as the test-pass evidence instead., and 7 more.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — reached the round cap of 3 without two consecutive dry rounds (the rounds 1+2 pair reported findings; round 3 reported 1 finding)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — the walk above completed within budget; no check left unfinished.；chunk 4：none — could not run the vitest suite locally (no node_modules in the worktree), so CI status was used as the test-pass evidence instead.，另有 7 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| label: 'Group Policy', | ||
| kind: 'enum', | ||
| required: true, | ||
| default: 'open', |
There was a problem hiding this comment.
[Suggestion] The added default: values in the GITHUB fixture (default: 'open' here, default: 'allowlist' below) strip the only coverage of initialFieldValue's no-default enum fallback (field.default ?? field.options?.[0]?.value ?? '' → the options[0] branch). The test titled 'defaults enum fields to the first option for new channels' now passes via the explicit-default branch, which is already covered by 'uses an explicit enum default over the first option for new channels'. — Failure scenario: probe-verified at this commit: deleting the options[0] fallback branch keeps all 101 channel tests green, so a future regression there ships unseen; extension plugins may declare enum management fields without a default (managementFieldsWithSharedControls only normalizes sessionScope), and such a channel would render its required enum field empty in the editor.
| label: 'Group Policy', | |
| kind: 'enum', | |
| required: true, | |
| default: 'open', | |
| label: 'Group Policy', | |
| kind: 'enum', | |
| required: true, | |
| options: [ |
(drop the added default: keys — the runtime-fallback-when-editing test does not depend on them — or keep them and add a dedicated test that drives a descriptor with a no-default enum field through createChannelEditorDraft)
中文说明
GITHUB 夹具中新增的 default: 值（此处 default: 'open'，下方 default: 'allowlist'）消除了 initialFieldValue 无默认值枚举回退（field.default ?? field.options?.[0]?.value ?? '' → options[0] 分支）的唯一覆盖。名为 'defaults enum fields to the first option for new channels' 的测试现在实际验证的是显式 default 分支，而该分支已被 'uses an explicit enum default over the first option for new channels' 覆盖。— 失败场景：已在当前提交探针验证：删除 options[0] 回退分支后全部 101 个 channel 测试仍为绿，未来该处的回归将在无人察觉的情况下合入；扩展插件可以声明不带 default 的枚举管理字段（managementFieldsWithSharedControls 只归一化 sessionScope），此类频道会在编辑器中渲染出空的必填枚举字段。建议修复：移除新增的 default: 键（编辑时运行时回退测试并不依赖它们），或保留它们并新增一个用无 default 枚举字段的 descriptor 驱动 createChannelEditorDraft 的专门测试。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| await act(async () => save?.click()); | ||
| expect(channelState.current.createOrUpdate).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
[Suggestion] This test exercises an in-dialog workspace switch (editor open on Secondary repo) but never pins the hook binding during that window: the only useChannelsMock assertions occur after the later toolbar switch and after finishSave(). — Failure scenario: probe-verified at this commit: reordering the override to activeWorkspaceCwd = selectedManagementWorkspace?.cwd ?? editor?.workspaceCwd routes the createOrUpdate mutation to the toolbar workspace's daemon endpoint (the channel would be created in the wrong workspace) and all 14 tests still pass — createOrUpdate is a shared mock and the only assertion here is toHaveBeenCalledTimes(1).
| await act(async () => save?.click()); | |
| expect(channelState.current.createOrUpdate).toHaveBeenCalledTimes(1); | |
| await act(async () => save?.click()); | |
| expect(channelState.current.createOrUpdate).toHaveBeenCalledTimes(1); | |
| expect(useChannelsMock).toHaveBeenLastCalledWith( | |
| expect.objectContaining({ workspaceCwd: '/workspace/secondary' }), | |
| ); |
中文说明
该测试演练了弹窗内切换工作区（编辑器打开在 Secondary repo），但从未在该窗口内固定 hook 绑定：useChannelsMock 的断言只出现在之后工具栏切换和 finishSave() 之后。— 失败场景：已在当前提交探针验证：把覆盖顺序改为 activeWorkspaceCwd = selectedManagementWorkspace?.cwd ?? editor?.workspaceCwd 会把 createOrUpdate 变更路由到工具栏工作区的 daemon 端点（频道被创建到错误的工作区），而全部 14 个测试仍然通过——createOrUpdate 是共享 mock，此处唯一断言只有 toHaveBeenCalledTimes(1)。建议修复：在弹窗内切换后立即固定 useChannelsMock 的 workspaceCwd 参数。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Round-3 real-stack verification (maintainer) — the pairing-default fixRe-verified after the two commits that followed my previous report (
Verdict: the remaining finding from my last report is fixed, and it is fixed safely — existing configurations that never stored a Finding 2 (new Channels defaulted to a closed allowlist) — fixed ✅
The part that mattered most: legacy configs are not widened ✅The runtime falls back to On the real stack the editor gets this right: To confirm the guard is load-bearing rather than incidentally satisfied, I removed the compat line in
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Pairing management after saving a new (pairing-default) Channel | Legacy thread instance — fourth option shown & selected |
 |
 |
| Manager (light) | Manager on the secondary workspace |
 |
 |
| Manager at 430px (dark) | |
 |
|
Environment: Debian (Linux 6.12, x86_64), Node 22.22.2, Chromium via Playwright 1.58.2, no real platform credentials. Reproduction note: sibling package dists in my checkout were older than the branch, so I rebuilt core, acp-bridge, web-templates and webui inside the worktree before npm run bundle; web-shell's tsc -p tsconfig.lib.json then fails on files unrelated to this PR (transcriptToMessages.ts, useQueuedPrompts.ts) purely because of a stale @qwen-code/sdk dist — CI is green on all of these.
中文版本
第三轮真实环境验证（维护者）—— 针对「默认改为配对」的修复
在上一轮报告之后的两个提交（1bd4a488 保持工作区切换可用、ad365b8c 新建频道默认配对）上重新验证。方式与前几轮一致：把 PR head 构建成自己的 CLI bundle，由真实的 qwen serve daemon（不是浏览器 E2E 用的 mock daemon）提供服务，并用 Chromium 驱动。
| PR head | ad365b8c（merge base 02f1e90a0a） |
| 运行栈 | dist/cli.js serve --port 5991 --workspace ws-a --workspace ws-b，独立 QWEN_HOME，独立 web-shell 构建 |
| 工作区 | ws-a（主）与 ws-b（次），/capabilities 显示均受信任 |
| 预置数据 | wecom-nopolicy / wecom-mut（没有 senderPolicy、没有 groupPolicy，即旧配置形态）、wecom-legacy（sessionScope: thread、私聊与群聊白名单、groups: { "*": {…}, "group-a": {…} }）、dingtalk-open |
| 判据 | 浏览器 DOM · 抓取的 PUT 请求体 · 各工作区磁盘上的 .qwen/settings.json · 对源码做变异后重新构建并重新驱动 |
结论：我上一轮遗留的问题已修复，而且修得稳妥 —— 从未存储过 senderPolicy 的旧配置不会被静默放宽。我通过删除这处兼容守卫、亲眼看到放宽发生来验证它确实承重。仅剩一处不一致（GitHub/GitLab），我认为不阻塞合并。
Finding 2（新建频道默认落到空白名单）—— 已修复 ✅
| 结论 | 证据 |
|---|---|
| 默认值改动确实到达真实目录 | 运行中的 daemon 上 GET /workspace/channel-types：dingtalk、wecom、feishu 的 senderPolicy.default = pairing（原为 allowlist） |
| 新建频道按配对写盘 | 在 ws-a 新建 WeCom，除名称与凭据外不改任何字段 → PUT 请求体 {"type":"wecom","botId":"bot-new","senderPolicy":"pairing","groupPolicy":"disabled","sessionScope":"user"}，磁盘内容完全一致 |
| 次级工作区同样如此 | 切换到 ws-b 后新建 Feishu → PUT /workspaces/%2Fopt%2Fwt8848%2Fws-b/channels/feishu-b，senderPolicy: pairing，且只写入 ws-b/.qwen/settings.json |
| 配对不只是一个标签，链路是通的 | 重新打开该频道后，「先保存再配对」的提示被替换为 daemon 提供的真实配对管理（待处理请求 0、配对授权 0） |
最关键的一点：旧配置不会被放宽 ✅
运行时对缺省 senderPolicy 的回落是 allowlist（parseChannelConfig：rawConfig['senderPolicy'] || 'allowlist'），groupPolicy 回落 disabled。若只是简单地把默认值一改，编辑器打开这类旧配置就会显示配对，并在下次保存时写回 —— 把原本封闭的频道变成可以接受陌生人配对申请。
真实栈上编辑器的处理是正确的：wecom-mut（存储中没有 senderPolicy）打开显示 Allowlist / Disabled，保存写入 "senderPolicy":"allowlist","groupPolicy":"disabled"，与运行时语义完全一致；旧字段 webhookUrl 与已存密钥也都保留。
为确认这处守卫是真正承重、而非被巧合满足，我删掉 channel-editor-state.ts 里的兼容分支（if (field.key === 'senderPolicy') return 'allowlist' 改为落到 descriptor 默认值），重新构建 Web Shell 后用同一份配置重跑：编辑器显示配对，保存把 "senderPolicy":"pairing" 写进了 settings.json。这正是本 PR 避免掉的回归。
1bd4a488（操作进行中仍可切换工作区）—— 已验证 ✅
我把启动请求的响应挂起 9 秒，在这一时刻观察管理页：
- 工作区选择器可用，而
ws-a中其余 4 个「启动」按钮都被禁用 —— busy 锁是按工作区隔离的（busyByWorkspace），不是全局的。 - 操作进行中切到
ws-b正常，ws-b自己的「启动」未被禁用；切回后ws-a的状态仍在。 - 把
busy !== null加回禁用条件（即该提交之前的状态），同一时刻选择器就会变灰 —— 这一行改动确实名副其实。
之前几轮验证过的行为没有回归 ✅
wecom-legacy 仍然逐字节原样回写：编辑器把 By thread (legacy) 作为第四个选项并选中；保存后 sessionScope: "thread"、groups."*"（requireMention、dispatchMode）与 group-a.groupHistoryLimit: 25 全部保留，密钥保持 {"operation":"preserve"}，工作区选择器锁定在 ws-a · Primary。workspace-qualified 路由、群白名单回写、白名单输入的条件显示也都与此前一致。
遗留但不阻塞：新默认值只覆盖 5 个可管理适配器中的 3 个 ⚠️
GitHub 与 GitLab 自行声明了 senderPolicy（packages/channels/github/src/index.ts），既没有 default，选项顺序又是 allowlist, pairing, open，因此 initialFieldValue 落到 options[0]。在本 PR 修复的同一条 Web Shell 路径上实测：
// 新建 GitHub 频道，默认值不动，写入 settings.json 的内容
{ "type": "github", "useLocalGh": false, "groupPolicy": "open",
"senderPolicy": "allowlist", "sessionScope": "chat_thread" }没有 allowedUsers，编辑器也不要求填写。SenderGate.isAllowed() 在 allowlist 下就是 allowedUsers.has(senderId)，而 GithubAdapter.processAggregateLane 会用 this.gate.isAllowed(sender) 过滤新评论 —— 在运维填入 ID 之前，聚合链路是静默的。
这不是回归（base 的编辑器解析枚举默认值的方式相同，field.default ?? options[0]），而且对公开仓库而言「白名单优先」很可能是有意为之，descriptor 自身的说明文案也是这么写的。但「新建频道默认配对」目前只对 DingTalk/WeCom/Feishu 成立，新增的 registry 断言也只覆盖了这三个。建议要么给这两个 descriptor 补上显式默认值，要么在描述中说明。
顺带两处外观问题，都不阻塞：
- GitHub/GitLab 的编辑器渲染的是 descriptor 原始标签（Sender Policy、Allowed Users (comma-separated)），而共享字段的适配器用的是打磨过的文案（Direct message policy、Allowed user IDs），上面的对比图里可以并排看到。
.sessionScopeControl仍是repeat(3, …)，所以四选项的旧配置会把「Share all」挤到单独一行（上一轮已提过，且只有旧实例才会遇到）。
聚焦用例（head）
web-shell channels 101 ✓ · cli channel + settings-store 416 ✓ · webui useDaemonChannels 10 ✓ · wecom 134 ✓ / 1 条环境导致的既有失败
那条失败是 continues attachment cleanup when one dir removal fails，在 origin/main 上同样复现，且该测试未被本 PR 触碰（我的沙箱以 root 运行，用例期望失败的目录删除反而成功了）。
环境
Debian（Linux 6.12、x86_64）、Node 22.22.2、Chromium（Playwright 1.58.2），未使用任何真实平台凭据。复现提示：我 checkout 中的兄弟包 dist 比分支旧，因此在 worktree 内先重建了 core、acp-bridge、web-templates、webui 再执行 npm run bundle；此后 web-shell 的 tsc -p tsconfig.lib.json 会在与本 PR 无关的文件（transcriptToMessages.ts、useQueuedPrompts.ts）上失败，纯粹是 @qwen-code/sdk dist 过旧所致 —— 这些在 CI 上都是绿的。
|
@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: 58 passed · 0 failed · 58 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：58 通过 · 0 失败 · 58 总计 Verification reportPR #8848 Deep Verification —
|
| Cell | Oracle | Base (HEAD^1) |
Head (HEAD^2) |
|---|---|---|---|
S1 add group id, keep * + per-group |
upsert result + stored file | ❌ rejected groups is not manageable (as expected) |
✅ accepted; gid-new={} written, */gid-keep byte-preserved |
| S2 sibling edit, groups byte-identical | upsert + file | ✅ passes | ✅ passes (control) |
S3 edit known nested key (requireMention true→false) |
upsert + file | ❌ rejected (as expected) | ✅ accepted, sibling key preserved |
S4 tamper unknown nested key (advancedRouting changed) |
rejection | ❌ rejected | ❌ rejected groups.gid-keep.advancedRouting is not manageable (safety holds) |
S5 reserved group id (constructor) |
rejection | ❌ rejected | ❌ rejected (safety holds) |
S6 legacy sessionScope: 'thread' round-trip |
stored value | ✅ thread stored |
✅ thread stored (control) |
| S7 stale revision | rejection | ❌ channel_settings_conflict |
❌ channel_settings_conflict (control) |
Store: head 8/8, base 7/7 — witness 01-store-ab-base-vs-head.png.
Secondary claim 1 — catalog exposes shared access fields (new channels default to pairing): supportedChannelCatalog() per arm:
| Cell | Base | Head |
|---|---|---|
| C2/C3 dingtalk, feishu, wecom | no senderPolicy/groupPolicy fields (as expected) |
injected: senderPolicy enum, default pairing, options pairing/allowlist/open; groupPolicy enum, default disabled, options disabled/pairing/allowlist/open |
| C4 sessionScope | present (pre-existing injection) | options user, thread (legacy), chat_thread, single, default user |
| C6 github/gitlab plugin-owned descriptors | groupPolicy default open, senderPolicy no default |
byte-identical (plugin-declared fields win over shared injection; PR doesn't touch these plugins) |
| C1/C5/C7/C8 manageable set, dingtalk plugin fields, unmanageable types | invariant | invariant |
Catalog: head 18/18, base 12/12 — witness 02-catalog-ab-base-vs-head.png. Note the editor pairs with the store here: for an existing instance missing a stored policy, the editor falls back to allowlist/disabled (matching runtime parseChannelConfig defaults), not to the catalog create-defaults — consistent, verified against config-utils.ts.
Secondary claim 2 — WeCom chat_thread attachment routing: real compiled WeComChannel.attachmentRouteKey:
| Cell | Base | Head |
|---|---|---|
W1/W2 chat_thread key |
bot:alice:chat-1 ≠ bot:bob:chat-1 — sender leaked (fell through to user branch), as expected |
bot:chat-1:topic-1 shared across senders |
| W3/W4 isolation | n/a | thread/chat isolation + threadless fallback to bot:chat-1 hold |
W5/W6/W7 user/single/legacy thread scopes |
invariant | invariant (controls) |
WeCom: head 7/7, base 4/4 — witness 03-wecom-ab-base-vs-head.png.
Browser e2e (mock daemon): web-shell.channels.spec.ts under headless Chromium — 2/2 passed, including create/delete of a typed Channel with workspace-qualified request bodies — witness 04-e2e-channels-browser-flow.png.
Mutation matrix (vacuity) — witness 05-mutation-matrix.png
| Mutant | Reverted guard | Suite (size) | Result |
|---|---|---|---|
| M1 | delete whole groups branch (≡ base behavior) |
store (75) | KILLED 2/75 — accepts env-resolvable descriptor fields… and preserves unchanged legacy values in known group fields fail with Channel field "groups" is not manageable. (accept→reject mismatch, same error the base A/B cell produced) |
| M2 | remove the preserved-legacy-value exception only | store (75) | KILLED 1/75 — …legacy values in known group fields fails: groups.*.requireMention is invalid. |
| M3 | remove SHARED_ACCESS_FIELDS injection |
registry (44) | KILLED 2/44 — field list [settings, sessionScope] ≠ expected [settings, senderPolicy, allowedUsers, groupPolicy, sessionScope]; pairing options assertion gets undefined |
| M4 | revert WeCom chat_thread case |
wecom (135) | KILLED 1/135 — expected 'bot:alice:chat-1' to be 'bot:bob:chat-1' |
| M5 | remove wildcard * preservation in assignGroups |
editor-state (37) | KILLED 1/37 — stored * entry missing from upsert config |
| PC1 | positive control: drop chat_thread from pre-existing sessionScope enum (region untouched by PR) |
store (75) | KILLED 2/75 — proves the harness can fail the suite |
Every reverted run failed the intended behavioral assertion with expected-versus-actual output (no import/compile artifacts).
Survivors, classified (completeness reporting, not merge conditions): M1 left the other five groups tests green because base's assertPreservedUnknownField fallthrough already produced identical outcomes for unchanged or unsafe values — those tests pin safety/round-trip properties that hold under both implementations, so surviving a whole-branch revert is correct there, not a vacuity. The one genuine coverage gap found: no unit test adds a new group id beside existing preserved groups in one edit (my S1 shape); the load-bearing parts of that behavior (accept-any-change at all, legacy preservation) are pinned by M1/M2 kills, and my A/B cell S1 covers it. Also unpinned: github/gitlab descriptor precedence (covered here by A/B cell C6) and the typed-* input edge (see Findings).
Targeted gates (head)
| Gate | Result |
|---|---|
cli: channel-settings-store (75), channel-registry (42), channel-registry-builtins, config-utils (4 files) |
169/169 passed |
channels/wecom: WeComAdapter.test.ts |
135/135 passed |
web-shell: ChannelEditorDialog (22), ChannelsManagerPage (14), channel-editor-state (37) |
73/73 passed |
webui: useDaemonChannels.test.tsx |
10/10 passed |
browser e2e: web-shell.channels.spec.ts (chromium) |
2/2 passed |
Suite liveness is proven by PC1 above and by every mutant going red. config-utils.ts change verified behavior-equivalent: parseChannelConfig throws on unknown plugin before the touched line, so plugin?. → plugin. cannot change outcomes (new tests pin chat_thread and legacy thread parsing). The dialog's thread-filter (ChannelEditorDialog.tsx:239) offers the legacy scope only when the existing instance uses it — pinned by the dialog tests.
Findings
Nit (non-blocking) — typed * in “Allowed group IDs” is silently discarded. assignGroups skips * in the entered list (if (groupId === '*') continue;) and validation only rejects UNSAFE_OBJECT_KEYS, so an operator who types * (plausibly meaning “all groups”) gets no groups written and no error. Scripted probe via buildChannelUpsertRequest: new channel + allowedGroupIds='*' → config.groups === undefined; existing channel with groups:{'gid-1':{}} + input replaced by * → also undefined (membership cleared). Reproduce: npx tsx probe in packages/web-shell (see logs/); no test pins either direction. Not blocking: runtime semantics are unaffected (* remains the wildcard config key server-side, which the editor correctly never emits as membership), and the behavior is input-drop, not data corruption.
Measured minimal fix (preserves the PR's intent)
In validateChannelEditorDraft, extend the existing allowlist check:
splitList(draft.allowedGroupIds).some(
(groupId) => UNSAFE_OBJECT_KEYS.includes(groupId) || groupId === '*',
)Applied in a scratch copy and driven through the same probes: hostile input now yields allowedGroupIds: 'invalidGroupId' (was {}); benign input (gid-a, gid-b) produces byte-identical output (groups = {"gid-a":{},"gid-b":{}}) and identical validation errors; channel-editor-state.test.ts stays 37/37. The suite is green both with and without the patch, so this axis is unpinned — the fix should ship with a fixture asserting that '*' yields invalidGroupId. Alternative: state in the field description that “all groups” means groupPolicy: Open.
No other findings: safety controls (reserved keys, tampered unknown nested keys, stale revisions, secret-key discipline) hold on head wherever they held on base, and every PR-introduced guard is test-pinned.
Not covered
- Per-commit attribution: 23 commits in the metadata snapshot, but the depth-2 merge-ref checkout makes only the aggregate
HEAD^1..HEADdiff reachable (git rev-list HEAD^1..HEAD^2= 1 grafted commit). Verified the aggregate. - Playwright visual regression (
visuals/screenshots.spec.ts, light/dark manager flows) and the mobile-chromium project: not run (visual comparison needs the repo's golden snapshots on a calibrated renderer; out of budget/scope). Desktop-Chromium functional e2e did run. - Real platform credentials / live DingTalk/Feishu/WeCom traffic — same as the PR's own scope note; browser path exercised against the mock daemon only. The e2e run reproduces the wire shape the UI sends, not real-platform behavior.
- Daemon HTTP routes end-to-end: the store was driven directly (the seam the workspace-qualified routes call into); route handlers themselves unchanged by this PR.
- Repo-wide typecheck/lint: not re-run;
npm ci && npm run build(full tsc) completed at HEAD before this round per the job contract, and the affected packages were rebuilt from source here. - i18n key completeness beyond what the rendered component tests consume; docs/ changes (prose only).
- Base-side e2e: not run — the spec's new assertions target UI that does not exist on base (A/B at the store/catalog/adapter level covers the behavioral delta instead).
Methodology
Environment: CI verify container (node v22.23.2), working tree = refs/pull/8848/merge (depth 2). Base arm: git worktree add tmp/base-tree HEAD^1, root node_modules + per-package node_modules symlinked in (lockfile untouched by the PR), packages/cli and packages/channels/wecom rebuilt in-tree (~90 s). Realpath audit confirmed @qwen-code/* links from the base tree resolve into the head tree; that is a clean control because the PR leaves all plugin runtime code unchanged except WeComAdapter.attachmentRouteKey (comments-only in channel-base), and the WeCom cell loaded each tree's own compiled dist by file path. Harnesses (harness-store.mjs, harness-catalog.mjs, harness-wecom.mjs, kept in this directory) drive compiled dist/ output against temp workspaces with isolated QWEN_HOME, encode per-arm expectations so predicted base failures count as passing A/B assertions, and emit CELL … PASS/FAIL lines (raw logs in logs/). Mutation runs patched source in place, ran the affected vitest files, and restored via git checkout (tree verified clean after). Browser e2e used the repo's Playwright config with a freshly installed headless Chromium. Evidence images were produced with scripts/verify-capture.mjs.
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. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Released in v0.21.12. |
5 participants
What this PR does
Web Shell Channel management now exposes the shared direct-message, group-access, session-routing, and workspace-ownership controls for every manageable adapter. Operators can select all supported sender and group policies, manage the matching user and group allowlists, choose one of the three active Channel session scopes, and bind a Channel to a registered workspace.
The Channel manager now follows the established full-page tab structure used by Scheduled Tasks and the other Web Shell management surfaces: a flat divided header, brief introduction, count-and-actions toolbar, full-width configured list, and neutral add-connection cards. Runtime state remains easy to scan, with one primary lifecycle action, visible editing, and an accessible overflow menu for restart and deletion, without introducing separate dashboard chrome, gradients, or platform-specific styling.
The editor groups these settings into Access control and Conversation management panels. Allowed user IDs appear only for the direct-message Allowlist policy, allowed group IDs appear only for the group Allowlist policy, and session isolation uses a three-way segmented control with a plain-language explanation of each conversation boundary.
The Channel manager defaults to the primary workspace and can switch among currently registered, trusted workspaces. New Channel dialogs expose the same workspace selection explicitly; existing instances keep their workspace binding locked because stored credentials are redacted and cannot be moved safely without re-entry. Secondary-workspace operations use the daemon's workspace-qualified management routes.
Group allowlist edits round-trip the runtime groups shape without discarding wildcard defaults or per-group settings. Pairing management appears when either direct-message or group pairing is active. Legacy Channel
sessionScope: threadremains accepted and is preserved for existing configurations; it is not offered when creating a new Channel. This is intentionally separate from the daemon bridge's ownthreadcreation scope.The session-scope editor slice overlaps with #8457. This PR keeps only the shared Channel management control; #8457 remains responsible for its broader session sidebar and settings experience.
Why it's needed
The Channel runtime already supports these policies, but Web Shell previously exposed only Pairing/Open for direct messages and no group policy controls. New configurations therefore omitted
groupPolicyand defaulted to disabled group traffic, while existing allowlist configurations could open blank and fail validation. Workspace ownership was also implicit in whichever session opened the manager, making secondary-workspace Channel configuration difficult to discover and control.Reviewer Test Plan
How to verify
Open Channel management and compare its header, content padding, introduction, toolbar, button sizing, card radius, and list density with Scheduled Tasks. With more than one registered workspace, confirm the primary workspace is selected by default. Confirm configured Channels show an accurate status explanation and a single primary Start, Stop, or Retry action; Edit should remain visible, while Restart and Delete should be available from the named overflow menu. Confirm available platforms read as neutral add-connection choices and that the layout remains usable at narrow widths and in both light and dark themes. Switch the manager to another trusted workspace, then create a DingTalk, Feishu, or WeCom Channel and confirm the dialog shows the same workspace. Confirm Direct message policy offers Pairing, Allowlist, and Open, while Group policy offers Disabled, Pairing, Allowlist, and Open. Confirm Allowed user IDs is hidden until direct-message Allowlist is selected, and Allowed group IDs is hidden until group Allowlist is selected. Under Conversation management, switch among By user, By chat or thread, and Share all, and confirm the explanatory sentence updates with the selected isolation boundary. Save and reopen to confirm the workspace, selected policies, IDs, and
sessionScoperound-trip; the workspace selector should be locked for the existing instance. For an existing configuration withsessionScope: thread, confirm the editor shows the legacy option and savesthreadunchanged; confirm a new Channel does not offer that option. For pairing, confirm the save-first prompt appears before persistence and pending request/approval management appears afterward.Evidence (Before & After)
Before: Web Shell exposed only Pairing/Open for direct messages, no group access controls, no understandable session-isolation control, and no explicit workspace ownership. After: the manager uses the same flat page hierarchy and neutral visual language as the other Shell tabs, while the editor exposes all shared policies in two focused panels, reveals allowlist inputs only when relevant, explains the three active session boundaries, and selects from registered workspaces with the primary as default. Browser E2E verifies the visible choices and workspace-qualified create/edit/delete request bodies, and dedicated light/dark visual runs were inspected against the Scheduled Tasks layout.
Tested on
Environment (optional)
Node.js 25.9, npm 11.12.1, Chromium Playwright, mock daemon.
Focused verification: 55 Channel state/component tests, one functional Channel browser flow, two light/dark manager visual flows, Web Shell production build, repository build, and repository typecheck.
Risk & Scope
sessionScope: threadpreserves its existing routing behavior; new Channels use the active scope catalog.threadremains accepted and round-trips for compatibility. New management surfaces expose onlyuser,chat_thread, andsingle.Linked Issues
Closes #8845
中文说明
本 PR 做了什么
Web Shell 的 Channel 管理现在为所有可管理适配器开放共享的私聊准入、群聊准入、会话路由和工作区归属配置。用户可以选择完整的私聊与群聊策略，维护对应的用户和群聊白名单，在三种有效的 Channel 会话范围中选择，并把频道绑定到已注册的工作区。
频道管理页改为遵循“定时任务”和其他 Web Shell 管理页的既有结构：扁平分隔页头、简短说明、数量与操作工具栏、全宽频道列表，以及中性的“添加连接”卡片。运行状态仍然清晰，并保留唯一的主要启停操作、始终可见的编辑入口和具名且可访问的重启/删除菜单，但不再引入独立 dashboard 外观、渐变或平台品牌样式。
编辑器将这些设置重新组织为“访问控制”和“会话管理”两张面板。只有私聊策略为“白名单”时才显示允许的用户 ID，只有群聊策略为“白名单”时才显示允许的群聊 ID；会话隔离改为三段式选择，并用一句直白说明解释每种选择对应的对话边界。
频道管理默认选择主工作区，并可在当前已注册且受信任的工作区之间切换。新建频道时会明确展示同一工作区选择；已有频道会锁定工作区归属，因为已保存凭据经过脱敏，无法在不重新输入的情况下安全迁移。二级工作区操作使用 daemon 的 workspace-qualified 管理路由。
群聊白名单编辑会按运行时 groups 结构无损回写，不会丢失通配默认配置或单群高级配置。当私聊或群聊任一策略为配对时都会展示配对管理。旧的 Channel
sessionScope: thread仍兼容读取、编辑并原样保存，但新建 Channel 不再提供该选项；这与 daemon bridge 自身的thread创建语义明确分离。会话范围编辑控件与 #8457 有一小段重叠。本 PR 只保留共享 Channel 管理字段；#8457 继续负责更大的会话侧栏和设置体验。
为什么需要
Channel 运行时已经支持这些策略，但 Web Shell 之前只开放私聊 Pairing/Open，且没有群聊策略配置。新配置会遗漏
groupPolicy，从而默认禁用群消息；已有 allowlist 配置也可能打开为空并校验失败。频道归属此前还隐含依赖打开管理页的会话工作区，二级工作区的频道配置难以发现和控制。Reviewer Test Plan
如何验证
打开频道管理，先与“定时任务”对比页头、内容留白、说明、工具栏、按钮尺寸、卡片圆角和列表密度。在注册了多个工作区时确认默认选择主工作区。确认已配置频道显示准确的状态说明和唯一的“启动”“停止”或“重试”主操作；编辑入口应始终可见，重启和删除应位于具名的更多操作菜单。确认可连接平台表现为中性的“添加连接”入口，页面在窄宽度以及明暗主题下都可正常使用。切换到另一个受信任工作区后，新建 DingTalk、Feishu 或 WeCom Channel，确认弹窗显示相同的工作区；私聊策略包含配对、白名单和开放，群聊策略包含禁用、配对、白名单和开放。确认只有私聊选择白名单时才出现“允许的用户 ID”，只有群聊选择白名单时才出现“允许的群聊 ID”。在“会话管理”中切换“按用户隔离”“按群/话题隔离”“全部共享”，确认下方说明随会话边界同步变化。保存后重新打开，确认工作区、策略、ID 与
sessionScope完整回显，并确认已有实例的工作区选择被锁定。对已有sessionScope: thread的配置，确认编辑器显示旧版选项并原样保存thread；同时确认新建 Channel 不提供该选项。配对模式下确认首次保存前显示先保存提示，保存后显示待处理请求和批准管理。证据（前后对比）
改动前：Web Shell 仅提供私聊 Pairing/Open，没有群聊准入、易于理解的会话隔离配置或明确的工作区归属。改动后：管理页与其他 Shell Tab 使用相同的扁平层级和中性视觉语言；编辑器用两张聚焦面板承载完整共享策略，仅在相关策略下展示白名单输入，解释三种有效会话边界，并从已注册工作区中选择且默认主工作区。浏览器 E2E 验证可见选项和 workspace-qualified 的新建、编辑、删除请求体，明暗主题视觉用例也已对照“定时任务”逐一检查。
测试平台
环境（可选）
Node.js 25.9、npm 11.12.1、Chromium Playwright、mock daemon。
聚焦验证：55 个 Channel 状态/组件测试、1 条 Channel 浏览器功能链路、2 条明暗主题管理页视觉链路、Web Shell 生产构建、仓库构建和仓库类型检查。
风险与范围
sessionScope: thread的既有路由语义；新建 Channel 使用当前 scope 目录。thread仍兼容读取并原样回写；新的管理界面只开放user、chat_thread和single。关联 Issue
Closes #8845