Repository navigation
fix(cli): honor channel approval mode without webhooks - #15754
Conversation
|
Verification report for
Boundary: the independent integration harness mocked configuration loading/parsing, IM transport construction, event-relay registration, and the SDK network boundary. It verifies request propagation, not HTTP/ACP acceptance or real DingTalk transcript/card behavior. No real IM messages were sent and no production service was changed. Remote CI is separate from these local results. 该提交的验证结果:六项更新后的回归预期在原上游实现上失败,修复后全部通过;相关完整测试文件共 578/578 通过。独立本地集成验证使用真实编译后的 worker/router/bridge,共享 router 的七个 channel 在新建会话和重启恢复时各有 7/7 请求正确,覆盖无 webhook 的五种模式、未配置模式和 webhook 独立集成验证模拟了配置载入/解析、IM transport、事件 relay 注册和 SDK 网络边界,确认的是请求传递;未验证 HTTP/ACP 接受结果或真实钉钉 transcript/审批卡片。未发送真实 IM 消息或更改生产服务;远端 CI 结果与本地验证分别记录。 |
yiliang114
left a comment
There was a problem hiding this comment.
No findings — this is a clean single-line fix.
Change: Removes the config["webhooks"] guard before setChannelApprovalMode so that channel approval mode is always honored, not just when a webhook is configured.
Testing: The parameterized it.each covers all five approval-mode values and verifies both webhook-enabled (telegram) and non-webhook (feishu) channels. The original test still covers the undefined-approvalMode path.
No correctness, security, or regression concerns.
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Final gate review at head e3e27ca (re-read immediately before submitting).
- No prior review rounds or unresolved threads on this PR.
- Required CI per the repository ruleset (
Lint & Static (ubuntu-latest, Node 22.x)): COMPLETED/SUCCESS at this head. Non-required lanes (precheck, Bun acceptance) were still queued at submission time. - Independent check found no Critical. The one-line production change matches the standalone path exactly (
start.tsregisterssetChannelApprovalMode(name, config.approvalMode)unconditionally at both call sites).undefinedsemantics are safe: the router deletes the map entry, andsessionOptions()— the single read funnel for fresh and restored routes — omits the field, preserving default behavior; registration also runs beforerestoreRoutes(), so restoration is covered. The worker's session-request builder still forwards only a truthyapprovalMode, so noundefinedleaks to the wire. - Test updates pin the exact call sequence (including
['feishu', undefined]) and parametrize all five supported modes across webhook/no-webhook channels.
|
One thing up front: this run started before the merge and the PR landed while it was in flight (merge commit Template looks good ✓ Problem: real, and evidenced in the code rather than by a user report. The daemon worker registered a channel's Direction: aligned. The channel-adapter developer doc describes the channel approval mode as the fallback policy for permission requests with no webhook precondition, and the daemon accepts exactly the value set the channel parser produces — Worth a release note, though: this is permission gating. A daemon-managed channel configured with Size: not a core path ( Approach: minimal — removing the guard is the fix, and I'd cut nothing. Expanding one regression case into a parameterized run over all five modes is the only extra scope and it earns its place. Risk: no high-risk path match (Stage 1e). Non-blocking follow-up, and pre-existing rather than introduced here: Moving on to code review. 🔍 中文说明先说明一点:本次运行在合并之前就已启动,PR 在运行过程中已被合并(合并提交 模板完整 ✓ 问题: 真实存在,且由代码本身证明,而非用户报告。daemon worker 仅在 方向: 一致。channel adapter 的开发文档把 channel 审批模式描述为权限请求的兜底策略,并没有以 webhook 为前提;daemon 接受的取值集合与 channel 解析器产出的集合完全一致—— 不过值得写进 release note:这属于权限管控。配置了 规模: 未触及核心路径( 方案: 已是最小改动——删掉这个判断本身就是修复,没有可裁剪的部分。把单个回归用例扩展为覆盖全部五种模式的参数化用例是唯一的额外范围,且是必要的。 风险: 未命中高风险路径(Stage 1e)。 一个非阻塞的后续项,且是既有问题而非本 PR 引入: 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo critical findings. The production change is one deleted condition, and the interesting question is what falls out of calling the setter unconditionally — so that is what I traced.
The restore claim holds. Registration happens before No new server-side rejection surface. This was the one non-obvious risk: values that were previously never forwarded for non-webhook channels now reach the daemon. Channel config validates against The tests are load-bearing, not decorative. Under the old guard the Two non-blocking notes:
Test evidenceThis was an unattended CI run, so per the gate's rules I executed nothing from the PR — no build, no test run, no checkout of the head. The evidence below is the PR's own CI, read through the API at the reviewed commit, plus the static trace above. 91 check-runs on The cancelled lanes ( The author's comment on this thread reports 578 local tests passing and a local integration harness covering seven new sessions and seven restores. That is the author's own claim on macOS — attributed, not re-run here, and not counted as evidence. The independent signal is the green ubuntu unit lane, which does include
Sandboxed verification would settle the remaining gap: 中文说明代码审查没有发现严重问题。生产代码只删掉了一个条件判断,真正值得追问的是"无条件调用这个 setter 会带来什么",所以我沿着这条链读了一遍。 传 "重启恢复"的说法成立。 注册发生在同一个启动块中 不会新增服务端拒绝的情况。 这是唯一不那么显然的风险:此前从未为非 webhook channel 下发的取值,现在会到达 daemon。channel 配置按 测试是有效的,不是摆设。 在旧的判断下, 两点非阻塞的观察:
测试证据这是一次无人值守的 CI 运行,因此按门禁规则我没有执行 PR 中的任何代码——没有构建、没有跑测试、没有 checkout head 分支。下面的证据来自 API 读取的该 PR 自身 CI(针对被审查的提交),以及上述静态阅读。 被取消的那些 lane( 作者在本线程的评论中报告本地 578 项测试通过,以及一个覆盖七个新建会话与七个恢复请求的本地集成验证。这是作者在 macOS 上的自述——此处仅做归属说明,未重跑,也不作为证据计入。独立信号是 ubuntu 单测 lane 的绿色结果,其中确实包含 (CI 结果表格见上方英文部分,由 finalize 任务在 CI 稳定后就地更新。) 如果要把剩下的缺口补上,沙箱验证可以做到: — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the fix is right and minimal; the only reservation is that the end-to-end effect is inferred from a mock boundary rather than observed, and the change flips permission-gating behaviour for existing configs. Asked what I would have done before reading the diff: delete the webhook condition so the daemon path matches the two standalone paths, then make the regression test say something about modes rather than about webhooks. That is what this PR does, so there is no simpler path I can offer — the diff is already at the floor for this fix. What I checked rather than assumed, since "one-line fix" is where things hide:
Where I land: this is a consistency fix with the right blast radius, and the author flagged the one behaviour change that matters ( Verdict, and why no action follows it. This would have been an approve: every stage clean, no Stage 0 escalation, the fork- If anything here is worth acting on after the fact, it is the two non-blocking items in the Stage 2 comment: the stale channel-adapter doc note claiming the approval mode is never read, and the one untested webhook + non- 中文说明信心度:4/5 —— 修复方向正确且已是最小改动;唯一的保留意见是端到端效果是从 mock 边界推断出来的而非实测观察到的,并且这次改动改变了既有配置下的权限管控行为。 在读 diff 之前先问自己会怎么做:删掉 webhook 这个条件,让 daemon 路径与两条 standalone 路径一致,然后让回归测试断言"模式"而不是"是否有 webhook"。这正是本 PR 做的事,所以我没有更简单的方案可提——就这个修复而言,diff 已经到底了。 因为是"一行修复",最容易藏问题的地方,我逐项核实而不是默认:
我的结论:这是一个影响范围恰当的一致性修复,作者主动标出了唯一重要的行为变化(没有 webhook 的 daemon channel 上 结论,以及为什么没有后续动作。 这本来会是一次 approve:各阶段干净,没有 Stage 0 升级,fork 如果其中还有什么值得事后处理,就是 Stage 2 评论里的两个非阻塞项:channel adapter 文档中"审批模式从未被读取"的过时说明,以及唯一未覆盖的"带 webhook + 非 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Released in v0.26.3. |
What this PR does
Daemon-managed channels now honor their configured
approvalModeeven when no webhook sources are configured, matching standalone channel startup. Regression coverage checks all five supported modes, omitted configuration, and channels with different modes sharing a worker.Why it's needed
A channel configured with
approvalMode: "yolo"could still request tool approval when started throughqwen serve --channel: the value passed configuration validation but was omitted when the channel had no webhooks. The documented setting applies to every session on the channel, so requiring webhook configuration made the two startup paths inconsistent.Reviewer Test Plan
How to verify
qwen serve --channel. Confirm a fresh session receives that mode, matchingqwen channel start.Evidence (Before & After)
Tested on
Environment (optional)
Node.js 24.14.1; pnpm 11.24.0. Tests use isolated local state and mocked IM/network boundaries.
Risk & Scope
yolo.Linked Issues
No linked issue.
中文说明
此 PR 的改动
daemon 管理的 channel 现在会应用配置的
approvalMode,不再要求配置 webhook,与 standalone channel 启动行为一致。回归测试覆盖全部五种支持的模式、未配置模式,以及共享同一个 worker 的不同审批模式 channel。修改原因
配置了
approvalMode: "yolo"的 channel 经qwen serve --channel启动后,仍可能请求工具审批:该值通过了配置校验,却在没有 webhook 时未被传递。文档约定该设置适用于 channel 的每个会话,因此以 webhook 配置为生效条件导致两个启动入口行为不一致。审阅者测试计划
如何验证
qwen serve --channel启动。确认新建会话收到该模式,与qwen channel start一致。修改前后证据
测试平台
环境
Node.js 24.14.1;pnpm 11.24.0。测试使用隔离的本地状态,并模拟 IM 和网络边界。
风险与范围
yolo。关联 Issue
无关联 Issue。