Skip to content

refactor(dingtalk): remove obsolete background response aggregation - #11570

Merged
qqqys merged 1 commit into
QwenLM:mainfrom
qqqys:refactor/remove-dingtalk-background-aggregation
Sep 10, 2026
Merged

refactor(dingtalk): remove obsolete background response aggregation#11570
qqqys merged 1 commit into
QwenLM:mainfrom
qqqys:refactor/remove-dingtalk-background-aggregation

Conversation

@qqqys

@qqqys qqqys commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Removes the obsolete DingTalk background aggregation setting, its buffering, timers, retries, terminal/retirement flushes, dedicated tests and configuration documentation. Retains immediate, individually labeled Agent responses and the existing reply/proactive transports, attachments, message splitting and interactive cards.

This is the standalone cleanup prerequisite for #11561. It adds no output modes or shared output policy.

Why it's needed

The old concatenate-all-responses behavior is no longer needed. Removing it independently keeps deletion review and migration separate from the new output-policy design.

Reviewer Test Plan

How to verify

  • Channel management should expose credentials and interactive cards, with no aggregation toggle or new output-mode field.
  • In both group and private chats, Agent responses should be delivered immediately with their labels. Interleaved non-Agent text should remain unchanged, and empty terminal events should add no message.
  • Existing configurations that previously enabled aggregation should now behave like the old default: no waiting, combined result, delayed retry or retirement/disconnect flush.
  • Regular replies should retain source attribution, mentions, attachments and message splitting.

Evidence (Before & After)

The published 0.23.2 offline baseline held Agent text when aggregation was enabled, then emitted a combined result at the terminal event. The local built adapter sends the same sequence immediately. Eight configuration/route cases pass; empty terminals and retirement/disconnect add no extra messages. These are intercepted transport checks, not real DingTalk sends.

Local validation: 1,425 tests passed across DingTalk, the shared channel base and CLI configuration/management; full build, typecheck, bundle, changed-file lint, formatting and diff checks passed. Two self-audit passes and an independent review found no issues. See the separate verification report for details.

Tested on

OS Status
🍏 macOS ✅ Local tests and offline built-adapter checks
🪟 Windows ⚠️ Not tested locally
🐧 Linux ⚠️ Not tested locally

Environment (optional)

Node.js 22.17.0 in an isolated worktree. Published baseline and built-adapter checks intercept transport without credentials or network sends.

Risk & Scope

  • Main risk or tradeoff: users who enabled aggregation will receive immediate segments instead of a buffered result.
  • Not validated / out of scope: real DingTalk API/rendering, production deployment, new output modes and other channel adapters.
  • Breaking changes / migration notes: remove the obsolete aggregation setting from existing configurations; a leftover key is ignored and no longer changes delivery. Previously unconfigured channels keep their existing behavior.

Linked Issues

Cleanup prerequisite for #11561. The output-policy PR will absorb this deletion once it is merged into main.

中文说明

本 PR 的改动

删除旧钉钉后台聚合设置,以及对应的缓存、计时器、重试、终态和会话收尾补发、专属测试与配置文档。保留即时逐段发送且带 Agent 标签的响应,以及现有回复/主动投递通道、附件、消息分片和交互卡片。

这是 #11561 的独立前置清理 PR,不增加输出模式或共享输出策略。

为什么需要

旧的拼接全部回复逻辑已不再需要。单独删除可将清理评审与迁移影响同新的输出策略设计分开。

评审测试计划

如何验证

  • 频道管理应仅展示凭证和交互卡片配置,不再展示聚合开关,也没有新的输出模式字段。
  • 群聊与私聊中的 Agent 响应均应即时发送并保留标签。交错到达的非 Agent 文本保持原样,空终态事件不增加消息。
  • 以前启用聚合的配置应恢复旧默认行为:不等待、不合并结果、不进行延迟重试,也不在会话退出或频道断开时补发聚合消息。
  • 普通回复应保留来源归属、发送人提及、附件和消息分片。

前后对比证据

已发布 0.23.2 离线基线在启用聚合时缓存 Agent 文本,再于终态事件发出合并结果。本地构建后的适配器对相同序列即时发送。八种配置/路由组合全部通过;空终态和会话退出、频道断开不额外发消息。这些是拦截传输的验证,不是真实钉钉发送。

本地验证:钉钉、共享频道基类及 CLI 配置/管理共 1,425 个测试通过;全仓构建、类型检查、打包、改动文件 lint、格式及差异检查通过。两轮自审及一次独立评审均未发现问题。详情见独立验证报告评论。

测试平台

操作系统 状态
🍏 macOS ✅ 本地测试及离线构建适配器检查
🪟 Windows ⚠️ 未在本地测试
🐧 Linux ⚠️ 未在本地测试

环境

Node.js 22.17.0、独立 worktree。已发布版本基线及本地构建适配器检查均拦截传输,不使用凭证或发送网络请求。

风险与范围

  • 主要风险或取舍:启用过聚合的用户将收到即时分段消息,而不是缓存后的合并结果。
  • 未验证或范围外:真实钉钉 API/渲染、生产部署、新输出模式和其他频道适配器。
  • 兼容性及迁移:从既有配置中移除旧聚合设置;残留键会被忽略,不再改变投递行为。原本未配置该设置的频道保持现有行为。

关联事项

#11561 的前置清理。此 PR 合入 main 后,输出策略 PR 将吸收这部分删除。

@qqqys

qqqys commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Offline behavior verification report

Verified commit: 25dd9b6ec85ea5724918d084a58459744bc0fd17.

Published baseline: qwen 0.23.2. With aggregation enabled, Agent output was withheld until the terminal marker, and session retirement could flush a partial result. With the setting absent or false, output was immediate.

Built-adapter verification: Node.js 22.17.0 on macOS, isolated worktree, intercepted final transports. The eight combinations of omitted/false/true/string legacy values and group/DM targets all produce the same sequence:

  • First Agent response: one message immediately.
  • Interleaved Shell response and second Agent response: three separate messages before the terminal marker.
  • Empty terminal: still three messages.
  • Retirement or disconnect: no additional message.

Two harness runs passed, checking 54 intercepted messages total. Agent labels and Shell text were preserved; group delivery selected the proactive transport and DM selected the reply transport. Named-session labels and file markers remained intact at those seams. The retired catalog field and checked aggregation state/methods are absent; no output-mode configuration or capability was added.

Local checks passed:

  • DingTalk: 598 tests in 14 files.
  • Shared channel base: 692 tests.
  • CLI registry/configuration/management: 135 tests in 4 files.
  • Full build, typecheck, bundle, changed-file ESLint, Prettier and diff checks.
  • Two self-audit passes and an independent read-only review: no findings.

Limitations: the harness stops at the final transport seam. It does not verify live DingTalk delivery/rendering, file uploads or production behavior. No credentials, real DingTalk sends or deployment were used. Hosted CI is separate from these local results.

中文验证报告

验证提交:25dd9b6ec85ea5724918d084a58459744bc0fd17

已发布 qwen 0.23.2 基线:启用聚合时,Agent 输出会等待终态后合并发送,会话退出可能补发部分结果;未配置或设为 false 时即时发送。

在 macOS、Node.js 22.17.0、独立 worktree 中对最终构建适配器拦截传输验证。旧键省略、false、true、字符串四种值与群聊/私聊组合成八种场景,全部表现一致:第一条 Agent 响应立即形成一条消息;交错的 Shell 响应及第二条 Agent 响应使消息数在终态前达到三条;空终态、会话退出和频道断开均不增加消息。

两遍 harness 共核验 54 条拦截消息。Agent 标签和 Shell 原文保留;群聊走主动投递,私聊走回复;命名会话标签和文件标记保留到传输接口。旧管理字段与检查到的聚合状态/方法均已移除,也未增加输出模式字段或能力。

本地钉钉 598 个、共享频道基类 692 个、CLI 配置/管理 135 个测试,共 1,425 个测试通过。全仓构建、类型检查、打包、改动文件 ESLint、Prettier 和差异检查通过;两轮自审与一次独立只读评审未发现问题。

限制:harness 的证据止于最终传输接口,不证明真实钉钉投递/渲染、文件上传或生产行为。未使用凭证、发送真实钉钉消息或部署。远端 CI 与上述本地结果独立。

@qqqys
qqqys marked this pull request as ready for review September 10, 2026 12:39
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required heading is there, with a matching Chinese section.

Problem: this is a deletion, not a fix, so there is no reproduction to ask for. The justification is that the aggregation path is obsolete and that #11561 needs it gone first — and #11561 does say so explicitly ("isolated in #11570, which should land first"), so there is a named downstream consumer rather than a hypothetical one. What the description does not carry is evidence that anyone still runs with the setting enabled. That is fine for a cleanup, but it is the reason the migration note matters more than usual here.

Direction: aligned in principle — one delivery behaviour instead of two, and the buffering path was genuinely intricate. One thing worth saying out loud: this removes a documented, user-facing settings.json key, which is a public-contract change, not an internal tidy-up. It also lands ahead of its replacement. If #11561 slips or changes shape, aggregateBackgroundAgentResponses users are left with immediate segments and no way back. That sequencing risk is a maintainer call, not something the gate should wave through. CHANGELOG has no direct reference to this setting, but the DingTalk channel area is active and relevant.

Size: the core-module gate does not apply — nothing here matches the core paths (packages/channels/dingtalk/** is not one of them) and the change stays inside a single package plus its docs page. For transparency anyway: 1,013 production lines (DingtalkAdapter.ts 1,006, index.ts 7), 3,818 test lines, 20 docs lines, 0 generated/schema. Production lines cross the 1,000 informational threshold, so flagging it as large — though it is ~99% deletion, which is the cheap direction, and splitting a removal like this would only make it harder to review. No action needed on size.

Approach: the scope feels right, and I checked whether it was actually minimal rather than taking the description's word for it. Everything deleted traces back to the aggregation feature: the failOnHttpError plumbing through sendReply/deliverReplyText/sendResponseMessage, the two delivery-error wrapper classes, the pending-terminal parking structures, the deliverBackgroundReply override that existed only to widen the signature, and the onSessionRetiring override whose base implementation is a no-op. I grepped the head revision for all thirty removed symbols and found zero remaining references, so nothing is left dangling. No drive-by refactors either — the five tests that look "added" in the diff are byte-identical relocations past the deleted blocks; only 22 lines of test content are genuinely new. The one judgement call I would raise: keeping the aggregateBackgroundAgentResponses key silently ignored (rather than warning on it) is the right migration behaviour, and I confirmed it holds — parseChannelConfig spreads unknown keys through and the channels schema does not restrict them, so a leftover key really is inert.

Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. The real risk is the product one above, not a code path.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必需小标题齐全,中文说明也对应完整。

问题: 这是一个删除类改动,不是 bug 修复,所以没有"复现"可要求。它的理由是聚合路径已经过时,且 #11561 需要先移除它 —— #11561 里确实明确写了("isolated in #11570, which should land first"),因此存在一个明确的下游依赖方,而不是假想需求。描述中没有的是"当前是否还有用户启用该设置"的证据。对清理类改动来说这可以接受,但也正因为如此,这里的迁移说明比平时更重要。

方向: 原则上是对齐的 —— 用一种投递行为取代两种,而缓存路径本身确实非常复杂。但有一点需要明确说出来:这移除的是一个有文档、面向用户的 settings.json 配置键,属于对外契约变更,不是内部整理。而且它先于替代方案落地。如果 #11561 延期或形态改变,原本启用 aggregateBackgroundAgentResponses 的用户就只能收到即时分段消息,且没有回退路径。这个先后顺序的风险应由维护者判断,不适合由 gate 直接放行。CHANGELOG 中没有该设置的直接记录,但钉钉频道这一块是活跃且相关的。

规模: 核心模块 gate 不适用 —— 这里没有任何文件命中核心路径(packages/channels/dingtalk/** 不属于核心路径),改动也仅限单个 package 加它自己的文档页。为透明起见仍列出:生产代码 1,013 行DingtalkAdapter.ts 1,006、index.ts 7)、测试 3,818 行文档 20 行生成/schema 0 行。生产行数超过 1,000 的信息性阈值,所以标记为大型改动 —— 但其中约 99% 是删除,属于风险较低的方向,而且这类移除拆开反而更难评审。规模上无需采取行动。

方案: 范围是合理的,我也实际核查了它是否真的最小,而不是只采信描述。所有被删的内容都能追溯到聚合功能本身:sendReply/deliverReplyText/sendResponseMessage 上的 failOnHttpError 参数链、两个投递错误包装类、pending-terminal 暂存结构、仅为拓宽签名而存在的 deliverBackgroundReply override,以及基类实现本就是空操作的 onSessionRetiring override。我在 head 版本上检索了全部三十个被移除的符号,剩余引用为零,没有留下悬挂引用。也没有顺手重构 —— diff 里看起来像"新增"的五个测试,其实是被删除块之后位置移动的逐字节相同内容,真正新增的测试内容只有 22 行。唯一值得提出的判断点:把 aggregateBackgroundAgentResponses 保留为静默忽略(而不是告警)是正确的迁移行为,我也确认了它成立 —— parseChannelConfig 会把未知键原样透传,channels schema 也不做限制,所以残留键确实是惰性的。

风险: 无升级风险信号 —— 改动文件都没有命中与 revert 相关的路径。真正的风险是上面提到的产品层面风险,而不是某条代码路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Code review

No critical blockers. This is a clean deletion, and I checked the seams rather than trusting the diff shape.

The thing that makes a 1,000-line removal reviewable is whether anything still points at what went away. I grepped the head revision for all thirty removed symbols — the aggregation maps and detached sets, the drain/flush/schedule/complete/create/remove/refresh helpers, the pending-terminal structures, both delivery-error classes, failOnHttpError, BackgroundResponseDelivery, formatBackgroundResponseAggregation, the config field itself — and every one comes back with zero hits. The compile-level seams all land on their feet:

  • onSessionRetiring was overriding a base method whose implementation is an empty no-op, so dropping the override is exactly right once there is nothing to drain.
  • resolveBackgroundResponseDelivery is not an adapter method at all — it lives on ChannelBase and is still used by the base's own dispatch, so the adapter no longer calling it orphans nothing.
  • Removing the deliverBackgroundReply override falls through to the base implementation, which calls sendResponseMessage — and the adapter's override of that is retained.
  • failOnHttpError defaulted to false at every call site outside the aggregation flush, including the removed error-code branch that parsed errcode. The default path never entered it, so deleting it changes no observable behaviour.
  • DingtalkCardRequestError.retryable survives with a live reader in the interactive-card client, so the retry plumbing that was shared is not left dead.

The rewritten dispatch is semantically identical to the old non-aggregating branch: empty text falls through to the base, which returns early on an empty trim; non-empty text gets the same ## 🤖 Agent · <label> wrapper. Same guard order, same fallthrough target, one less level of branching.

I also looked for scope creep, since a diff this size is where it hides. There is none. Five tests that read as "added" are byte-identical relocations past the deleted blocks — I compared the removed and added line multisets, and only 22 lines of test content are genuinely new. Those 22 are one replacement test pinning the source label on a DM background reply, and a mock-harness fix so deliverBackgroundReply forwards sourceLabel instead of dropping it. The harness change is a strengthening, not a loosening. Three added assertions in an existing group-dispatch test now pin per-segment immediate delivery and that an empty terminal event adds no message — which is precisely the behaviour the PR claims to retain, so that is well spent.

One non-blocking observation, worth a maintainer's eye because it touches the migration note. The description says a leftover aggregateBackgroundAgentResponses key "is ignored and no longer changes delivery." That is true on the runtime path — I confirmed parseChannelConfig spreads unknown keys through and the channels settings schema does not constrain them, so the adapter simply never reads it. It is not unconditionally true on the management path. Once the descriptor is gone, the key stops being a managed field and starts being an unknown one, and the settings store's upsert validates those through a preserved-unknown-field assertion: a leftover key round-tripped unchanged passes, one round-tripped with a different value is now rejected as Channel field "aggregateBackgroundAgentResponses" is not manageable., and one omitted from the payload is silently dropped. Before this PR, changing that value through management succeeded. The read snapshot also still copies the stale key out to clients, since it forwards every non-secret key rather than only managed ones. None of this breaks delivery and I would not block on it — but the practical migration instruction is "delete the key from your config," not "leave it, it's inert," and the rejection message gives a user no hint that the option was removed. Related: nothing in the suite pins the leftover-key path in either direction. The new index.test.ts pins the management field list, which is the right test for the descriptor removal.

Skipped the sequence diagram and the changed-files table — a five-file pure deletion whose shape is already described above does not need either.

Test evidence

Evidence carried here is the PR's own CI, read through the API. This is an unattended run, so I did not build or execute anything from this PR; per the same rule there is no tmux capture (N/A — unattended CI, and there is no TUI surface here regardless). Zero checks are red on this commit.

Two things make the green lanes mean something here. The install step runs the repo's prepare script, which runs the full workspace build, so a green Lint & Static and a green Test lane together mean tsc --build accepted the head revision — that is the typecheck coverage for a package that has no typecheck script of its own. And the Test job log confirms the DingTalk workspace actually ran rather than being filtered out: src/DingtalkAdapter.test.ts 295 tests and the new src/index.test.ts 1 test, inside a 14-file / 598-test package run, all passed.

The honest gap is platform coverage: the macOS and Windows Test legs and the CLI integration leg are skipped on this PR, so the suite only ran on Linux. The author's own report claims 1,425 tests across DingTalk, the shared channel base, and CLI configuration/management on macOS — that is the author's claim from a local run, not something CI reproduced, and the counts are not comparable to the 598 CI ran for this package alone.

Final CI results for 25dd9b6 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle this: @qwen-code /verify — the load-bearing claim is an A/B one the author ran locally on macOS only, that the published 0.23.2 build buffers Agent text under an enabled aggregation setting and emits one combined result at the terminal event while this head sends the same sequence immediately. Nothing in the PR's suite can reproduce that comparison, because the tests that encoded the buffering behaviour are the 3,605 lines this PR deletes; what remains pins only the new side. A /verify run against the base build would show the delivery-sequence difference directly, and would also exercise the leftover-config-key path that no test currently covers. The author has write access, so this is an ordinary run rather than a sponsored one. /tmux is not the right lane — there is no terminal surface in a channel adapter.

中文说明

代码审查

没有发现阻塞性问题。这是一次干净的删除,我核查的是各个接缝,而不是只看 diff 的形状。

一千行的删除之所以可评审,关键在于是否还有东西指向被删掉的内容。我在 head 版本上检索了全部三十个被移除的符号 —— 聚合 Map 与 detached 集合、drain/flush/schedule/complete/create/remove/refresh 系列辅助方法、pending-terminal 结构、两个投递错误类、failOnHttpErrorBackgroundResponseDeliveryformatBackgroundResponseAggregation,以及配置字段本身 —— 每一个的剩余引用都是零。编译层面的接缝也都落到了正确位置:

  • onSessionRetiring 覆盖的基类方法本身就是空实现,因此在没有缓存可清理之后移除这个 override 完全正确。
  • resolveBackgroundResponseDelivery 根本不是适配器自己的方法 —— 它定义在 ChannelBase 上,并且仍被基类自身的 dispatch 使用,所以适配器不再调用它并不会造成孤立代码。
  • 移除 deliverBackgroundReply override 后会回落到基类实现,而基类实现调用的 sendResponseMessage 在适配器中的 override 被保留了。
  • failOnHttpError 在聚合 flush 之外的所有调用点默认都是 false,包括被删掉的那段解析 errcode 的分支。默认路径从来不会进入该分支,因此删除它不改变任何可观测行为。
  • DingtalkCardRequestError.retryable 仍然有活跃读取方(交互卡片客户端),所以真正共享的那部分重试机制没有变成死代码。

重写后的 dispatch 与旧的非聚合分支在语义上完全一致:空文本回落到基类,基类在 trim 为空时提前返回;非空文本走同样的 ## 🤖 Agent · <label> 包装。判断顺序相同、回落目标相同,只是少了一层分支。

我也专门找了范围蔓延,这种体量的 diff 最容易藏。结论是没有。看起来像"新增"的五个测试,其实是被删除块之后位置移动的逐字节相同内容 —— 我对比了删除行与新增行的多重集合,真正新增的测试内容只有 22 行。这 22 行是一个替代测试(固定 DM 后台回复上的来源标签),以及一处 mock 测试脚手架修正,让 deliverBackgroundReply 转发 sourceLabel 而不是丢弃它。这个脚手架改动是加强而非削弱。另一个已有群聊 dispatch 测试里新增的三条断言,现在固定了逐段即时投递、以及空终态事件不增加消息 —— 这正是本 PR 声称保留的行为,加得很值。

一条非阻塞的观察,因为涉及迁移说明,值得维护者看一眼。 描述里说残留的 aggregateBackgroundAgentResponses 键"会被忽略,不再改变投递"。在运行时路径上这是成立的 —— 我确认了 parseChannelConfig 会把未知键原样透传,channels 设置 schema 也不做约束,所以适配器根本不会读它。但在管理路径上并非无条件成立。描述符移除之后,这个键不再是受管理字段,而变成未知字段,设置存储的 upsert 会通过"保留型未知字段"断言来校验它:残留键原值回传可以通过,值发生变化时回传则会被拒绝并报 Channel field "aggregateBackgroundAgentResponses" is not manageable.,而载荷中不带该键则会被静默丢弃。在本 PR 之前,通过管理接口修改这个值是可以成功的。读取快照也仍会把该残留键复制给客户端,因为它转发的是所有非密钥字段,而不只是受管理字段。这些都不影响投递,我也不会因此阻塞 —— 但实际的迁移指引应该是"请从配置中删除该键",而不是"留着即可,它是惰性的",而且那条拒绝信息完全没有提示用户这个选项已被移除。相关地:测试套件中没有任何用例固定残留键这条路径(两个方向都没有)。新增的 index.test.ts 固定了管理字段列表,对于描述符移除来说这是对的测试。

没有加时序图和改动文件表 —— 一个五文件的纯删除、且形状上文已经说明,两者都不必要。

测试证据

这里携带的证据是本 PR 自己的 CI,通过 API 读取。这是无人值守运行,因此我没有构建或执行本 PR 的任何内容;基于同一规则也没有 tmux 截屏(N/A —— 无人值守 CI,而且这里本身也没有 TUI 界面)。该 commit 上没有任何红色检查。

有两点让绿色结果在这里具有实际含义。安装步骤会执行仓库的 prepare 脚本,而它会运行完整的 workspace 构建,因此 Lint & StaticTest 同时为绿,意味着 tsc --build 接受了 head 版本 —— 这就是一个自身没有 typecheck 脚本的 package 所获得的类型检查覆盖。并且 Test 作业日志确认 DingTalk workspace 确实执行了、没有被过滤掉:src/DingtalkAdapter.test.ts 295 个测试、新增的 src/index.test.ts 1 个测试,包含在一次 14 文件 / 598 测试的 package 运行中,全部通过。

诚实的缺口在平台覆盖:本 PR 的 macOS 与 Windows Test 分支以及 CLI 集成分支都是跳过状态,所以套件只在 Linux 上跑过。作者自己的报告称在 macOS 上钉钉、共享频道基类与 CLI 配置/管理共 1,425 个测试通过 —— 那是作者本地运行的声明,不是 CI 复现的结果,而且这个数字与 CI 为该 package 跑出的 598 并不可比。

(CI 结论表见上方英文部分的机器可读区域。)

沙箱验证可以定论这一点:@qwen-code /verify —— 本 PR 的关键主张是一个 A/B 主张,而作者只在 macOS 本地跑过:已发布的 0.23.2 在启用聚合设置时会缓存 Agent 文本、并在终态事件发出一个合并结果,而本 head 对相同序列即时发送。PR 自己的套件无法复现这个对比,因为编码了缓存行为的测试正是本 PR 删除的那 3,605 行;留下来的只固定了新的一侧。针对 base 构建跑一次 /verify 可以直接展示投递序列的差异,同时也会覆盖目前没有任何测试触及的残留配置键路径。作者具有写权限,因此这是普通运行而非受赞助运行。/tmux 不是合适的通道 —— 频道适配器没有终端界面。

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the review itself came back clean; the cap is the fork-refactor approval guardrail, plus two real-but-non-blocking reservations that are a maintainer's call rather than mine.

Before reading the diff I wrote down what I would do myself: drop the descriptor and the config type field, delete the buffering state machine (maps, timers, retry, terminal/retirement flushes), keep the immediate labelled path, delete the aggregation-only tests, update the docs page, and then go looking for the consumers that a removal like this usually strands — the management UI, the settings schema, anything that enumerates channel config keys. The PR does all of that, and it goes one step further in a way I would have adopted on seeing the code: it also removes the failOnHttpError plumbing and the two delivery-error wrapper classes, which existed only so an aggregation retry could capture a plan, and the deliverBackgroundReply and onSessionRetiring overrides, which existed only to serve the buffering path. So it matches my proposal and is more thorough about the second-order debris. I did not find a simpler path it missed.

The result is a genuinely better file. The dispatch method goes from roughly 810 lines of interleaved buffering, parking, retry and detachment logic down to twenty, and every branch that survived is one that was already reachable with the feature off. If I picked this up in six months I would thank whoever deleted it — the removed code was correct as far as I can tell, but it was the kind of correct that costs a reader an afternoon.

Where I am less settled is not the code, it is the decision. Three things a maintainer should weigh:

  • It deletes a documented user-facing setting ahead of its replacement. feat(channels): add shared output policy with DingTalk support #11561 is still open and unmerged, and this PR is explicitly the half that lands first. If feat(channels): add shared output policy with DingTalk support #11561 changes shape or stalls, anyone who had turned aggregation on is left with immediate segments and no way back. Splitting the removal out is good practice — I am not arguing for one big PR — but the sequencing means the capability goes away before the substitute exists, and that is a product judgement, not a review finding.
  • The migration note is slightly too optimistic. "A leftover key is ignored" holds on the runtime path and I verified it there. On the management upsert path the key becomes an unknown field, so round-tripping it unchanged is fine, omitting it is fine, but submitting it with a different value is now rejected as not manageable — where before it succeeded. Nobody hits that through the UI, since the toggle is gone, but the error text does not tell a user that the option was removed. Worth a line in the release note saying "delete the key."
  • The author's A/B evidence is macOS-only and not reproducible from this PR's own suite, because the tests that encoded the buffering behaviour are exactly the 3,605 lines being deleted. That is inherent to a removal, not a defect — but it means the "before" half of the before/after rests on the author's local run. The /verify lane named in my review comment is what would close that.

None of those is a blocker and I found no correctness bug, no regression on the default path, no security concern and no scope creep, so I am not requesting changes. I am also not approving: this is a cross-repository refactor, which the gate never auto-approves regardless of how the review went, and the first two points above are decisions I should not make on a maintainer's behalf.

⏸️ Deferring to a maintainer. I could not resolve one to @mention — $QWEN_MAINTAINER_HANDLE is not set in this run, the PR carries no area label (the owner policy matches on category/core / scope/core, and there is no channels area in it), and no human has reviewed yet, so the deterministic resolver returned nothing and I would rather leave the mention empty than guess a login. The PR is therefore unassigned. Whoever owns the channels area: the specific call needed is whether removing aggregateBackgroundAgentResponses should land before #11561 does, and whether the migration note should say "delete the key" rather than "it is ignored".

中文说明

Confidence: 3/5 —— 审查本身结果是干净的;这个分数上限来自 fork refactor 的审批护栏,另外加上两条真实但不阻塞、应由维护者判断的保留意见。

在读 diff 之前我先写下了自己会怎么做:移除字段描述符和配置类型字段,删掉缓存状态机(Map、计时器、重试、终态/会话收尾补发),保留即时带标签的路径,删除仅服务于聚合的测试,更新文档页,然后去找这类移除通常会遗漏的消费方 —— 管理界面、设置 schema,以及任何枚举频道配置键的地方。这个 PR 全部做到了,而且比我预想的更进一步,这一点我在看到代码后也会采纳:它还移除了 failOnHttpError 参数链和两个投递错误包装类(它们存在的唯一意义就是让聚合重试能捕获投递计划),以及 deliverBackgroundReplyonSessionRetiring 两个 override(它们只为缓存路径服务)。所以它与我的方案一致,并且在清理二级残留上更彻底。我没有找到它遗漏的更简路径。

结果是一个明显更好的文件。dispatch 方法从大约 810 行交织的缓存、暂存、重试与 detached 逻辑降到二十行,而留下来的每一个分支都是在该功能关闭时本来就会走到的。如果六个月后由我接手,我会感谢删掉它的人 —— 被删的代码据我判断是正确的,但那是那种需要读者花一个下午才能确认的正确。

我不那么确定的不是代码,而是这个决定。有三点需要维护者权衡:

  • 它在替代方案之前删除了一个有文档、面向用户的设置。 feat(channels): add shared output policy with DingTalk support #11561 仍处于 open 且未合并状态,而本 PR 明确是先落地的那一半。如果 feat(channels): add shared output policy with DingTalk support #11561 改变形态或停滞,原本启用聚合的用户就只能收到即时分段消息,且没有回退路径。把移除拆出来是好实践 —— 我不是在主张合成一个大 PR —— 但这个先后顺序意味着能力先消失、替代品还不存在,而这属于产品判断,不是审查结论。
  • 迁移说明略偏乐观。 "残留键会被忽略"在运行时路径上成立,我在那里确认过。但在管理接口的 upsert 路径上,该键变成了未知字段:原值回传没问题,不带该键也没问题,但带不同的值提交则会被拒绝为 not manageable —— 而这在本 PR 之前是可以成功的。通过 UI 不会触发,因为开关已经不存在,但那条错误信息并不会告诉用户这个选项已被移除。发布说明里值得加一句"请删除该键"。
  • 作者的 A/B 证据只在 macOS 上,且无法从本 PR 自己的测试套件复现,因为编码了缓存行为的那些测试正是被删除的 3,605 行。这是移除类改动的固有属性,不是缺陷 —— 但它意味着 before/after 中的"before"那一半依赖作者的本地运行。我在审查评论里点名的 /verify 通道正是用来补上这一环的。

这些都不是阻塞项,我也没有发现正确性 bug、默认路径上的回归、安全问题或范围蔓延,所以我不会提交 request changes。我同样不会批准:这是一个跨仓库(fork)的 refactor,无论审查结果如何,gate 都不会自动批准;而且上面前两点是我不应代替维护者做出的决定。

⏸️ 转交维护者。 我没能解析出一个可以 @ 的人 —— 本次运行未设置 $QWEN_MAINTAINER_HANDLE,PR 上没有 area 标签(owner 策略只匹配 category/core / scope/core,其中没有 channels 领域),而且还没有人评审过,所以确定性解析器返回为空;与其猜一个登录名,我宁愿把这个提及留空。因此该 PR 目前没有被指派。频道领域的负责人:这里需要的具体判断是——移除 aggregateBackgroundAgentResponses 是否应该先于 #11561 落地,以及迁移说明是否应该写成"请删除该键"而不是"它会被忽略"。

Qwen Code · qwen3.8-max-2026-09-02

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved at head 25dd9b6e.

Required CI is green — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and web-shell E2E Smoke completed successfully; only review-pr (the reviewer's own job) is still running.

For a deletion of this size the only question that matters is whether anything still expects the removed thing, and I checked that rather than sampling it:

  • aggregateBackgroundAgentResponses has zero references anywhere at this head — it existed in exactly four places at the base (docs/users/features/channels/dingtalk.md, DingtalkAdapter.ts, DingtalkAdapter.test.ts, index.ts) and all four were updated in the same commit. No orphaned helper, timer or map entry is left behind: the aggregation type, both registries (backgroundResponseAggregations, detachedBackgroundResponseAggregations) and every drain…/schedule…Flush call site are gone together, and the remaining delivery paths are the two plain helpers (deliverReplyText, deliverProactiveText) plus a dispatchBackgroundResponse that forwards immediately.
  • Dropping the settings field does not break an existing config. plugin.management.fields is a descriptor list for the management UI, not a validating schema, and there is no strict unknown-key rejection anywhere in the channel option path — so a stored aggregateBackgroundAgentResponses: true is inert after this change, which is the behavior the description claims ("existing configurations … behave like the old default").
  • The one test added (index.test.ts) pins the management field list by exact equality (clientId, clientSecret, interactiveCards), so re-adding a field to that list is caught rather than drifting silently, and requiredConfigFields is asserted alongside it.
  • The +7/-999 adapter shape is a net simplification on a path that had real hazards: the ten-minute retirement flush, the disconnect/drain flushes and the retry-after-failed-delivery branch are all gone, so there is no longer a buffer whose lifetime is coupled to session retirement, which was the same family of defect as the queued-delivery work elsewhere in this channel.

No new Critical found. One non-blocking note: docs/users/features/channels/dingtalk.md loses the whole section without a line telling readers who still have the key set that it is now ignored. That is a one-sentence follow-up rather than a gate, and it pairs with the same choice in the sibling prefix removal.

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the single PR commit 25dd9b6e (+220/-4631, 5 files). No blocking finding. Approving, with three non-blocking notes.

For a deletion this size the only question is whether anything removed is still reachable, still configured, or still documented. I checked each.

Nothing dangles. git grep at head for aggregateBackgroundAgentResponses, BackgroundResponseAggregation, PendingBackgroundResponseTerminal, BACKGROUND_RESPONSE_AGGREGATION, backgroundResponseAggregations and pendingBackgroundResponseTerminals returns zero hits across packages/, scripts/, docs/ and .github/. The only Background* survivors are channel-base's unrelated BackgroundResponseDeliveryTarget / resolveBackgroundResponseDelivery (ChannelBase.ts:98,612,619,652). index.ts lost a management.fields entry, not a named export — DingtalkChannel and downloadMedia are still exported — so no other package's import breaks.

Background delivery still works, which was my main worry going in: DingtalkAdapter.ts:2694-2713 keeps override async dispatchBackgroundResponse with the same non-agent/empty-text early return as base, then calls super.dispatchBackgroundResponse(sessionId, this.formatBackgroundAgentResponse(text, context?.label), context), and the base hook chain (resolveBackgroundResponseDeliverydeliverBackgroundResponseToTargetpushProactive/deliverBackgroundReply, ChannelBase.ts:609-676) is intact. So the worst case — background runs can no longer reply at all — does not occur. What changes is combined-vs-segmented delivery, and that was opt-in and defaulted off at base (rawAggregateBackgroundAgentResponses === true), so the population that sees a behaviour change is the set that explicitly enabled it.

No orphaned persisted state. Everything removed was in-memory Maps/Sets plus timers; the adapter's only fs usage (mkdirSync/writeFileSync/tmpdir) belongs to the surviving file-upload path.

The added tests are substantive, not a token replacement. The +202 lines add a test subclass that delegates to the real base deliverBackgroundReply instead of stubbing it, a new preserves the resolved source label on a DM background reply case asserting the actual markdown body (the surviving coverage for dispatchBackgroundResponse / formatBackgroundAgentResponse / formatBackgroundAgentLabel), tightened pushProactive call-count assertions, and five file-projector/status-card cases. 207 test blocks survive in the head file, and 20 references to the surviving background-reply surface remain.

Three notes, none blocking

1. A documented, user-settable config key was deleted with no deprecation path. Base both validated aggregateBackgroundAgentResponses and rejected a malformed value with Channel "<name>" aggregateBackgroundAgentResponses must be a boolean.; head neither reads it, warns about it, nor documents its removal (docs/users/features/channels/dingtalk.md lost 20 lines and gained none). A user whose settings still carry "aggregateBackgroundAgentResponses": true silently moves from one combined message to one message per segment, with no stderr line and nothing in the docs to explain it. Startup is not broken — I checked that no strict unknown-key validator exists (channel-registry.ts:291-295 validates only the plugin's declared management fields, and routes/workspace-channel-management.ts has no unknown-field rejection) — so this is silence, not failure. A one-time warning when the key is present, or one line in the docs under "Background Agent output", would close it.

2. The commit subject carries no ! even though the PR body files this under "Breaking changes / migration notes" and a user-settable key stops working. The sibling PR #11571, removing a config key in the same area, uses feat(channels)!:. Worth aligning so release tooling and changelog readers see both removals the same way.

3. index.test.ts:6-10 pins the whole management field list with toEqual(['clientId','clientSecret','interactiveCards']). That couples this deletion guard to every future field addition — #11561 (the shared output-policy work) will add an output-mode field and trip this test, with a failure message pointing at a deletion guard rather than at the real edit. Asserting the negative (not.toContain('aggregateBackgroundAgentResponses') plus toContain for the three retained keys) would keep the guard and drop the coupling.

Two things I could not verify

The deleted deliverBackgroundReply and onSessionRetiring overrides: DingTalk now falls back to ChannelBase's implementation, and I did not read the deleted bodies within budget. The plausible unconfirmed failure mode is an over-limit long background reply, if the deleted override used the adapter's own chunk-splitting machinery (createReplyTextDelivery / deliverReplyText, head :1596-1700) rather than the base's single-shot send. Session-scoped cleanup of sessionMentionTargets and reaction state survives in a neighbouring method (:2690-2692), which suggests the deleted onSessionRetiring was aggregation-only, but I did not confirm it.

Also checked, since both PRs touch DingtalkAdapter.test.ts: #11570 and #11571 do not conflictgit merge-tree --write-tree --name-only pr-11570-review pr-11571-review exits 0 with no conflicted-file section, and the two edit disjoint regions (#11570 the vi.mock('@qwen-code/channel-base') delegation block and a DM background-reply case; #11571 only the DingtalkChannel quoted media describe block). Landing order between them does not matter.

CI at head: green apart from review-pr (bot orchestration). No unresolved review threads. I did not run the suite locally — no node_modules in this checkout.

中文说明

审了唯一的 PR 提交 25dd9b6e(+220/-4631,5 个文件)。没有阻塞项。 批准,附三条非阻塞说明。

这么大的删除量,唯一的问题是「被删的东西是否仍可达、仍被配置、仍被文档描述」。我逐项查了。

没有任何悬空引用。 在 head 上 grep aggregateBackgroundAgentResponsesBackgroundResponseAggregationPendingBackgroundResponseTerminalBACKGROUND_RESPONSE_AGGREGATIONbackgroundResponseAggregationspendingBackgroundResponseTerminals,在 packages/scripts/docs/.github/全部零命中;唯一存活的 Background* 是 channel-base 里无关的 BackgroundResponseDeliveryTarget / resolveBackgroundResponseDeliveryindex.ts 少的是一个 management.fields 条目而不是具名导出——DingtalkChanneldownloadMedia 仍在——所以不会破坏其它包的 import。

后台投递仍然可用,这是我进来时最担心的一点:DingtalkAdapter.ts:2694-2713 保留了 override async dispatchBackgroundResponse,早退条件与 base 相同,随后调用 super.dispatchBackgroundResponse(...),而 base 的钩子链(resolveBackgroundResponseDeliverydeliverBackgroundResponseToTargetpushProactive/deliverBackgroundReplyChannelBase.ts:609-676)完整。所以最坏情况(后台运行彻底无法回复)没有发生。变化的是「合并 vs 分段」投递,而它在 base 上是可选且默认关闭的(rawAggregateBackgroundAgentResponses === true),因此行为变化的群体正是显式开启过它的人。

没有孤立的持久化状态。 被删的全是内存里的 Map/Set 加定时器;adapter 唯一的 fs 使用(mkdirSync/writeFileSync/tmpdir)属于存活的上传路径。

新增测试是实质的,不是象征性替换。 +202 行加了一个委派给真实 base deliverBackgroundReply(而不是 stub)的测试子类、一个断言真实 markdown 正文的 preserves the resolved source label on a DM background reply 用例(这是 dispatchBackgroundResponse / formatBackgroundAgentResponse / formatBackgroundAgentLabel 存活的覆盖)、收紧的 pushProactive 调用次数断言,以及五个 file-projector/status-card 用例。head 文件里存活 207 个测试块。

三条说明,都不阻塞

1. 一个有文档、用户可设的配置键被删除且没有任何弃用路径。 base 既校验 aggregateBackgroundAgentResponses、也会以 Channel "<name>" aggregateBackgroundAgentResponses must be a boolean. 拒绝非法值;head 既不读它、不告警、也不记录它的移除(docs/users/features/channels/dingtalk.md 删了 20 行、一行未加)。settings 里仍写着 "aggregateBackgroundAgentResponses": true 的用户会静默地从「一条合并消息」变成「每段一条」,stderr 没有一行、文档也没有任何解释。启动不会坏——我确认过不存在严格的未知键校验(channel-registry.ts:291-295 只校验插件声明的管理字段,routes/workspace-channel-management.ts 没有未知字段拒绝)——所以这是沉默,不是失败。键存在时给一次性告警,或在文档「Background Agent output」下加一行,就能补上。

2. 提交标题没有 !,尽管 PR 正文把它归在「Breaking changes / migration notes」下、且一个用户可设的键失效了。同区域删配置键的姊妹 PR #11571 用的是 feat(channels)!:。对齐一下,发布工具与读 changelog 的人才能同样看待这两次移除。

3. index.test.ts:6-10toEqual(['clientId','clientSecret','interactiveCards']) 钉死了整个管理字段列表。 这把删除守卫和今后每一次字段新增耦合起来——#11561(共享输出策略)会加一个 output-mode 字段并触发这个测试,而失败信息会指向一个删除守卫而不是真正的改动处。改成断言否定式(not.toContain('aggregateBackgroundAgentResponses') 加对三个保留键的 toContain)能既保住守卫又去掉耦合。

两点我未能验证

被删的 deliverBackgroundReplyonSessionRetiring 覆写:DingTalk 现在回落到 ChannelBase 的实现,而我在预算内没有读被删的函数体。未确认的可能失败形态是超长的后台回复——如果被删的覆写用的是 adapter 自己的分块机制(createReplyTextDelivery / deliverReplyText,head :1596-1700)而不是 base 的单次发送。会话级清理(sessionMentionTargets 与 reaction 状态)在相邻方法里存活(:2690-2692),这暗示被删的 onSessionRetiring 只服务于聚合,但我没有确认。

另外,因为两个 PR 都改 DingtalkAdapter.test.ts,我查了:#11570#11571 不冲突——git merge-tree --write-tree --name-only pr-11570-review pr-11571-review 退出 0 且没有冲突文件段,两者改的是不相交区域(#11570vi.mock('@qwen-code/channel-base') 委派块与一个 DM 后台回复用例;#11571 只改 DingtalkChannel quoted media 那个 describe 块)。两者之间的合并顺序无所谓。

head 上的 CI:除 review-pr(机器人编排)外全绿。无未解决的 review 线程。我没有在本地跑测试套件——这个 checkout 没有 node_modules

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants