feat(dingtalk): make background agent aggregation optional - #10899
feat(dingtalk): make background agent aggregation optional#10899qqqys wants to merge 12 commits into
Conversation
|
|
|
Thanks for the PR — and for reworking #10807 into an opt-in design instead of letting it sit.
Moving on to code review. 🔍 中文说明感谢贡献——也感谢把 #10807 重新设计为显式开启的方案，而不是搁置。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff, my independent proposal for this was: flow a bounded Agent label from the background-task registry through the notification turn into the channel boundary, parse it strictly in the bridges, prefix it in the DingTalk adapter, and — since opt-in aggregation needs to know when a turn ends — mark the final response segment with a terminal flag the adapter can buffer against. The PR does essentially this, and adds a bounded delivery retry for aggregated sends, which earns its keep: aggregation concentrates a whole turn into one send, so the comment explaining why a failed send is retried instead of dropped is exactly right. No critical blockers found. What I verified against the base branch, since this spans four packages:
One non-blocking observation: if a same-task segment somehow arrived while the terminal flush's delivery is still in flight, it would land in the still-mapped buffer and schedule a second message for that turn. Rare, bounded, no data loss — noting it only so a maintainer can decide if it deserves a guard. Tests are substantial (~893 lines): default vs aggregated delivery, interleaved agents, timeout flush, session-death flush, retry, legacy and malformed metadata, label validation, and the Session turn-boundary semantics including ordering. This PR reshapes a multi-step runtime flow, so a map of the key path: sequenceDiagram
participant P1 as Background task registry (core)
participant P2 as ACP Session
participant P3 as Channel bridge (ACP or daemon)
participant P4 as DingTalk adapter
participant P5 as DingTalk API
P1->>P2: notification callback with label
P2->>P2: run response turn, mark final segment
P2->>P3: response segment (taskId, label, turnComplete)
P3->>P3: strict-parse backgroundTask meta
P3->>P4: backgroundResponse with context
alt default - no aggregation
P4->>P5: send each labeled segment immediately
else aggregateBackgroundAgentResponses enabled
P4->>P4: buffer per session and task
P4->>P5: one labeled result on turn end, partial, or bounded timeout
end
Files changed (24 of 24 shown)
Testing evidence — the PR's own CI (unattended run; no local execution of PR code)As of this snapshot the unit suite and static checks are still in flight on the reviewed commit — this repo's unit suite alone runs ~30 minutes, so I am reporting what is there rather than polling. No red checks so far; the macOS/Windows test jobs and the CLI integration job read as skipped, matching the fork-PR pattern where secret-gated jobs don't run — the no-AK integration variant stands in and is green. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: live DingTalk delivery. The author states no real bot message was sent and tested on macOS only — that is the author's claim, not independently re-run here (this CI path never executes PR code). Sandboxed verification would settle this: 中文说明代码审查在读 diff 之前，我的独立方案是：把长度受限的 Agent 名称从后台任务注册表沿通知回合传到 Channel 边界，在 bridge 严格解析，在钉钉适配器加前缀；由于显式聚合需要知道回合何时结束，给最终响应分段打终止标记供适配器缓冲使用。PR 基本就是这么做的，并额外加了聚合发送的有界重试——聚合把整个回合集中到一次发送，注释解释"失败重发而非丢弃"的理由完全成立。 未发现 Critical 阻塞问题。由于横跨四个包，我对照基础分支核实了：
一个非阻塞观察：如果同一任务的分段在终止 flush 的发送仍在途中时到达，它会落入尚未移除的缓冲并为该回合安排第二条消息。罕见、有界、不丢数据——仅供维护者判断是否需要防护。 测试相当充分（约 893 行）：默认与聚合发送、并发 Agent 交错、超时清空、会话死亡清空、重试、旧版与畸形元数据、标签校验，以及 Session 回合边界语义（含顺序断言）。 测试证据无人值守 CI 运行，不执行 PR 代码。截至快照，单元套件与静态检查仍在运行（本仓库单元套件约需 30 分钟），只报告现状不轮询。目前无红色检查；macOS/Windows 测试与 CLI 集成任务显示 skipped，符合 fork PR 的密钥受限模式——no-AK 集成变体代为运行且为绿色。 未验证：真实钉钉发送。作者声明未通过真实机器人发送消息、仅在 macOS 测试——这是作者的陈述，非本处独立复现。 沙箱验证可以定论： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, well-tested implementation that matches my independent proposal; what remains is non-blocking notes, and the PR's own CI is still in flight. Stepping back: this is the stronger of the two designs #10807's history leaves on the table. Immediate labeled delivery fixes the attribution gap on The non-blocking items, named so nothing is silently dropped:
Approval is deferred until CI lands green on 中文说明置信度：4/5 —— 干净、测试充分的实现，与我的独立方案一致；其余均为非阻塞事项，且 PR 自身 CI 仍在运行。 退一步看：这是 #10807 留下的两种设计中更强的一个。即时带标注发送修复了当前 非阻塞事项，逐条点名以免被静默丢弃：
批准推迟到 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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 3: executing the six new tests under vitest (worktree has no node_modules; a full monorepo install + workspace build was disproportionate to the remaining budget —….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）：chunk 3：executing the six new tests under vitest (worktree has no node_modules; a full monorepo install + workspace build was disproportionate to the remaining budget —…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.22.3)
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 reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：reverse audit — reached the 5-round cap without two consecutive dry rounds.
— qwen3.8-max via Qwen Code /review (v0.23.0)
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
Every other session-retirement path already notifies adapters through the retiring hook, so an adapter that buffers a background turn can flush it before the routing target disappears. Closing a named task skipped that notification, so a task closed while an Agent turn had buffered output kept the buffer alive with no route: the output arrived minutes later against a stale target, or never at all when the process exited first. The hook now fires once the close resolves, so a refused close (a busy or already-closed task) still leaves the live task's buffer untouched.
…lled Three ways a background Agent turn could be misreported once its responses are aggregated: A completion marker that arrived while the first segment was still resolving its delivery target found no buffer yet, so it took the empty-text early return and the finished turn only surfaced ten minutes later, stamped as a partial. Such a marker is now parked and applied to the buffer as soon as it exists. A turn longer than the bounded wait had its buffer deleted by the flush that delivered it, so the tail of the same turn started over as a fresh turn and was presented as the complete result. The buffer now survives until the turn actually ends, with the bounded wait re-armed so a silent turn is still reaped, and the tail stays marked partial. A turn whose text was fully drained by that bounded wait then completed with nothing left to send, so no card ever retracted the partial label. It now closes with a header-only completion card, and only when the turn really completed with everything delivered. Separately, retrying a failed delivery to a direct-message target re-ran the whole reply, including the file uploads that had already reached the chat, so each retry posted every attachment again. The retry now reuses the body prepared by the first attempt.
…ntless retries A card composed before the turn ended froze the icon and the partial mark at that moment, so a turn that crossed the bounded wait and then completed was announced as a stopped, partial one even though the send that carried it went out after completion. The pending card now picks up the turn's final state. A delivery given up on after three failures also left the aggregation looking untouched, so the next card presented the tail of a turn whose earlier text never reached the chat as the complete result. Dropped text now marks the turn partial for every card that follows. Finally, a send the API has permanently rejected — an invalid credential, a missing app — was still re-attempted twice more at thirty-second intervals, spending the chat's send quota on attempts that cannot succeed. Only failures that could plausibly clear are retried now. Also pins behaviour that had no coverage: the map entry a successful flush removes, a turn whose terminal status differs from its earlier segments, two overlapping segments of one task joining a single aggregation, a parked retry surviving later output, the background route disappearing mid-resolution, retirement on /clear of a named task, the completion marker on a cancelled notification turn, and the label a shell response takes from its registry entry.
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 reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.ts:10295 — [probe] D3-1 the label.length > 256 rejection branch of sessionBackgroundNotification has no test (code unchanged since round 2 — deferred by the code-age rule)packages/channels/dingtalk/src/DingtalkAdapter.ts:2429 — [probe] D3-2 canAggregate fallback (aggregation enabled + agent segment lacking turnComplete) has no test (code unchanged since round 2 — deferred by the code-age rule)packages/cli/src/acp-integration/session/Session.ts:9830 — [probe] D3-3 the no-responseStream exit's partial terminal marker has no test (code unchanged since round 2 — deferred by the code-age rule)
Convergence: round 3 posted 8 inline comment(s), 8 of them reported for the first time; the previous round posted 12 (12 new). Findings keep coming back to the same files: packages/channels/dingtalk/src/DingtalkAdapter.ts (findings in round 2; 8 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：reverse audit — reached the 5-round cap without two consecutive dry rounds.
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 8 条行内评论，其中 8 条是首次提出；上一轮发布了 12 条（其中 12 条首次提出）。发现反复回到同一批文件：packages/channels/dingtalk/src/DingtalkAdapter.ts（第 2 轮已出过发现，本轮又有 8 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
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 reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/channels/dingtalk/src/DingtalkAdapter.ts:2471 — [probe] the two new routing conditions in dispatchBackgroundResponse (non-agent kind fallback; canAggregate's typeof turnComplete check) are exercised by no test — both mutations surv…packages/cli/src/acp-integration/acpAgent.ts:10290 — [probe] the label.length > 256 and non-string branches of sessionBackgroundNotification have no test — removing the bound leaves the target test green (code unchanged since round 3 — defe…packages/channels/dingtalk/src/DingtalkAdapter.ts:2527 — [probe] the route-changed-during-resolution revalidation inside the aggregation branch is unwitnessed — deleting it survives 291/291; a stale target captured before the suspend would …packages/channels/dingtalk/src/DingtalkAdapter.ts:2718 — [probe] the give-up branch's parts.length > 0 arms (flush the tail / re-arm the bounded wait) are unwitnessed — a probe shows segments buffered during the retry window silently lost w…packages/cli/src/acp-integration/session/Session.test.ts:10410 — [probe] the structured.description side of the label derivation (Session.ts:10066-10067) — the only label source for monitor response updates — is asserted nowhere; deleting i…packages/channels/dingtalk/src/DingtalkAdapter.ts:2766 — [probe] the dropped !== true condition in owesTerminalBackgroundResponseCard — the only guard against a bare completed card after a give-up discarded buffered text — is unwitnessed; t…packages/cli/src/acp-integration/session/Session.test.ts:10243 — [probe] the partial markers of the !sendResult.responseStream and aborted-catch stop branches (Session.ts:9825-9831, 10006-10010) are never exercised with a buffered segment —…
Convergence: round 4 posted 8 inline comment(s), 8 of them reported for the first time; the previous round posted 8 (8 new). Findings keep coming back to the same files: packages/channels/dingtalk/src/DingtalkAdapter.ts (findings in rounds 2, 3; 7 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 8 条行内评论，其中 8 条是首次提出；上一轮发布了 8 条（其中 8 条首次提出）。发现反复回到同一批文件：packages/channels/dingtalk/src/DingtalkAdapter.ts（第 2、3 轮已出过发现，本轮又有 7 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
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.
2 Suggestion(s) and 3 fails-closed, new-surface Critical(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 5, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round; 3 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/channels/dingtalk/src/DingtalkAdapter.ts:2612 — [review] Critical [fails-closed] [new-surface] R4-2: (fix-induced) The round-4 fix for R4-2 parks the terminal until every resolver exits and lets the last one apply it — but this fi…packages/channels/dingtalk/src/DingtalkAdapter.ts:2881 — [review] Critical [fails-closed] [new-surface] R5-5: Lifecycle teardown ( disconnect at 2145, onSessionDied at 2255, onSessionRetiring at 2277) drains backgroundResponseAggregat…packages/channels/dingtalk/src/DingtalkAdapter.ts:2954 — [review] Critical [fails-closed] [new-surface] R2-1: (fix-induced) The round-4 fix for R2-1 added this resolution-retry callback whose .finally() guards on the stale pending objec…packages/channels/dingtalk/src/DingtalkAdapter.ts:2613 — [review] R5-6: Deleting the added cleanup this.pendingBackgroundResponseTerminals.delete(key) in this resolver-drain finally leaves every affected test green — the cleanup is pinn…packages/channels/dingtalk/src/DingtalkAdapter.ts:2951 — [review] R5-7: Deleting the cleanup inside this resolution-retry .finally() leaves every affected test green — no test pins cleanup on the retry-completion path (mutant survived; ha…packages/channels/dingtalk/src/DingtalkAdapter.ts:2506 — [review] D5-1 non-agent-kind delegation branch of dispatchBackgroundResponse untested — code unchanged since round 4, already deferred in round 4 (re-deferred)packages/channels/dingtalk/src/DingtalkAdapter.ts:2511 — [review] D5-2 canAggregate fallback for agent segments lacking turnComplete untested in aggregation mode — code unchanged since round 4, already deferred in round 4 (re-deferred)packages/channels/dingtalk/src/DingtalkAdapter.ts:2878 — [test] D5-3 detached-set cleanup in removeBackgroundResponseAggregation unpinned (mutant survived) — code unchanged since round 4, deferred by the code-age rule
Convergence: round 5 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 8 (8 new). Findings keep coming back to the same files: packages/channels/dingtalk/src/DingtalkAdapter.ts (findings in rounds 2, 3, 4; 3 more now); packages/channels/dingtalk/src/DingtalkAdapter.test.ts (findings in round 4; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
2 条 Suggestion和3 条 fails-closed 且 new-surface 的 Critical 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 5 轮，非阻断）——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录，本轮不要求修改；其中 3 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 8 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 8 条（其中 8 条首次提出）。发现反复回到同一批文件：packages/channels/dingtalk/src/DingtalkAdapter.ts（第 2、3、4 轮已出过发现，本轮又有 3 条）；packages/channels/dingtalk/src/DingtalkAdapter.test.ts（第 4 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
Qwen Code review did not complete successfully. Run review failed. See workflow logs for details. A transient error is retried automatically; if you are seeing this, retry with |
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 reviewed: build-and-test — packages/cli unit suite timed out on the review runner (result undetermined); 13 dependent-workspace suites were not reached within budget; the test-efficacy probe was inconclusive for infrastructure reasons (probe-tree prerequisite guard).
Not reviewed: coverage proof — the Step 3D check-coverage gate was not run after the fan-out in this session, and the run's prompt/transcript records were destroyed mid-review when a concurrent review session's cleanup swept .qwen/tmp; the 19 finder agents and 5 reverse-audit rounds did run and report (their outputs are in this run's transcript), but diff coverage can no longer be certified from records.
Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3 and 5 each reported a new finding; the round-5 finding was verified after the cap).
Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read.
Not reviewed: verification — could not check that Step 4 and Step 5 ran (no subagent transcripts at /home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.).
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/channels/dingtalk/src/DingtalkAdapter.ts:2940 — [review] Critical [fails-closed] [new-surface] Teardown drain never sweeps pendingBackgroundResponseTerminals — parked entries leak permanently, post-teardown timers drop parked text …packages/channels/dingtalk/src/DingtalkAdapter.ts:2844 — [review] Permanent-rejection give-up predicate can only match on the first attempt; saved-plan retries throw unwrapped errors and retry to the cappackages/channels/dingtalk/src/DingtalkAdapter.ts:2286 — [review] onSessionDied drain runs after the mention-target delete, so drained reply-path cards lose the sender @-mention (fix must capture the mention synchronously, not reorder)
Convergence: round 6 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/channels/dingtalk/src/DingtalkAdapter.ts (findings in rounds 4, 5; 6 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (6 Critical(s)), the rate of first-time findings is not falling (this round 6, previous 5), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — packages/cli unit suite timed out on the review runner (result undetermined); 13 dependent-workspace suites were not reached within budget; the test-efficacy probe was inconclusive for infrastructure reasons (probe-tree prerequisite guard).
未审查（原文为英文）：coverage proof — the Step 3D check-coverage gate was not run after the fan-out in this session, and the run's prompt/transcript records were destroyed mid-review when a concurrent review session's cleanup swept .qwen/tmp; the 19 finder agents and 5 reverse-audit rounds did run and report (their outputs are in this run's transcript), but diff coverage can no longer be certified from records.
未审查（原文为英文）：reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3 and 5 each reported a new finding; the round-5 finding was verified after the cap).
未审查：覆盖情况——无法读取 agent 的运行记录（no subagent transcripts at /home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.），本次运行无法证明 diff 的任何部分被读过。
未审查：验证——无法检查步骤 4 与步骤 5 是否运行（no subagent transcripts at /home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-14/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-14--work-qwen-code-qwen-code/subagents/14ad2535-7571-42a4-b954-31d5c247597d'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.）。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 6 条行内评论，其中 6 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/channels/dingtalk/src/DingtalkAdapter.ts（第 4、5 轮已出过发现，本轮又有 6 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 6 条 Critical），首次发现的速率没有下降（本轮 6，上一轮 5），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| } else if (context.turnComplete) { | ||
| current.turnComplete = true; | ||
| current.completionPartial = context.partial === true; | ||
| } else if (parked?.turnComplete && parked.resolvers === 0) { |
There was a problem hiding this comment.
[Critical] R4-4: (fix-induced) [certifies-falsely] [new-surface] The round-5 fix for R4-4 (f598b41) correctly retains the parked terminal entry after resolution-retry exhaustion instead of deleting it — but nothing ever resets that retained entry. The revival condition at ~2565 fires only while parked.retryTimer is armed, which is never true after exhaustion, so the next turn's first segment silently reuses the stale entry: it inherits the previous turn's turnComplete/status through this apply branch and is flushed immediately as a whole-turn card under turn 1's status icon with a bogus （部分） label, its later segments fragment into separate aggregations, and its own terminal marker is dropped. The inherited maxed retryAttempts also makes the new turn's first transient resolution failure drop that segment outright.
Trigger: multiSession + aggregateBackgroundAgentResponses: true; turn-1 first segment fails target resolution 3x (give-up sets resolutionDropped, entry retained); turn-1's empty terminal parks turnComplete/status on it; turn-2's first segment arrives, finds no revival (no retryTimer), reuses the stale entry — turn 2's content ships as ## ❌ Agent · Turn one（部分）.
Witness:
probe (unmodified PR code):
after exhaustion: pending = {resolvers:0, retryAttempts:3, resolutionDropped:true}
after turn-1 terminal: {..., turnComplete:true, status:'failed', label:'Turn one'}
after turn-2 first segment:
pushProactive = ["## ❌ Agent · Turn one（部分）\n\nTurn two result."]
pending size = 0; turn-2 terminal dropped, no further card
Fix: when a NEW turn's non-empty segment arrives, replace a parked entry that is already terminated and resolution-dropped (parked?.turnComplete === true && parked.resolutionDropped && !parked.retryTimer) with a fresh { resolvers: 0 } as the revival branch does — while distinguishing that case from a same-turn retry re-dispatch, which arrives with the timer already cleared and must keep the parked terminal; per-turn identity on parked entries is the robust shape.
The fix must not break the same-turn retention pinned by marks a segment partial when another resolution retry is pending (expects '## ✅ Agent · Worker one（部分）\n\nFirst result.') or retains resolution loss until the next segment can report it (DingtalkAdapter.test.ts:8443) — a naive broadening of the revival condition to parked.resolutionDropped was measured to fail the former. Please add a regression test: mock resolution to reject 3x then resolve; dispatch turn-1 segment + empty terminal, then turn-2 segment + terminal; assert turn-2's card is a clean ## ✅ Agent · Turn two without （部分） or turn-1 icon/label — removing the new reset must turn it red.
中文说明
第 5 轮对 R4-4 的修复（f598b415bf）正确地把解析重试耗尽后的 parked terminal 表项保留下来而不是删除——但这个被保留的表项没有任何重置路径。~2565 处的复活条件只在 parked.retryTimer 武装时触发，而耗尽后定时器已不存在，于是下一回合的首个分段会静默复用这个陈旧表项：经由本处的应用分支继承上一回合的 turnComplete/status，立即以整回合卡片的形式冲出——带着上一回合的状态图标和虚假的 （部分） 标记；该回合后续分段碎裂成独立聚合，其自身的终止标记被丢弃。继承来的已达上限的 retryAttempts 还会让新回合第一次瞬时解析失败直接丢弃分段。
探针（未改动的 PR 代码）：耗尽后 pending = {resolvers:0, retryAttempts:3, resolutionDropped:true}；turn-1 终止标记后追加 turnComplete:true/status:'failed'；turn-2 首分段后投出 ## ❌ Agent · Turn one（部分）\n\nTurn two result.，pending 清空，turn-2 终止标记被丢弃。
修复：当新回合的非空分段到达时，把已终止且 resolutionDropped 的 parked 表项（parked?.turnComplete === true && parked.resolutionDropped && !parked.retryTimer）替换为新的 { resolvers: 0 }——同时区分同回合重试再派发（定时器同样已清除、必须保留 parked 终止标记）；为 parked 表项引入每回合身份是更稳妥的形态。约束：不能破坏 marks a segment partial when another resolution retry is pending（期望 '## ✅ Agent · Worker one（部分）\n\nFirst result.'）与 retains resolution loss until the next segment can report it（DingtalkAdapter.test.ts:8443）钉住的同回合保留行为——实测把复活条件简单放宽到 parked.resolutionDropped 会让前者变红。请补充回归测试：mock 解析连续拒绝 3 次后成功；派发 turn-1 分段 + 空终止标记，再派发 turn-2 分段 + 终止标记；断言 turn-2 卡片为干净的 ## ✅ Agent · Turn two——移除新增重置后测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Exhausted resolution now records that the turn ended, clears stale terminal metadata, gives the next turn fresh pending state, and session drain clears both mapped and detached pending entries. Regression coverage: does not apply an exhausted terminal to the next turn; clears pending resolution state when a session dies. The old behavior failed these contracts; the DingTalk suite now passes 317/317.
| if ( | ||
| !current && | ||
| text.trim().length > 0 && | ||
| parked?.turnComplete === true && | ||
| parked.retryTimer | ||
| ) { |
There was a problem hiding this comment.
[Critical] R5-4: (fix-induced) [certifies-falsely] [new-surface] Still-standing successor of carried R5-4: the f598b41 armed-timer revival branch below covers the case where the resolution retry is still armed — but its sibling state survives. Once the timer has fired and the retry's resolution is in flight (suspended on the named-session owner lock, pending.retryTimer already cleared at ~3010), a next-turn segment fails this revival check, joins the stale parked cohort (parked.resolvers++ at ~2594), and both resolvers race into one aggregation — two turns' content is flushed as a single completed card, violating the separation that keeps a completed turn retry separate from the next turn pins for the armed case.
Witness:
probe (unmodified PR code):
resolverCalls after timer fire: 2
resolverCalls after turn-2 segment: 3 <- joined, not revived
pushProactive = ["## ✅ Agent · Turn one\n\nTurn one result.\n\nTurn two result."] <- two turns, one card
Fix: give parked entries per-turn identity so a segment joins only its own turn's cohort (or detach a terminated parked cohort whenever a new non-empty segment arrives, including while a retry resolution is in flight). Note: extending the revival condition to parked.resolvers > 0 alone likely does NOT close this — the in-flight retry re-fetches the map after its await (~2651-2652) and joins whatever aggregation the new turn created.
Same-turn overlapping resolvers must still merge into one aggregation — pinned by waits for every overlapping resolver before completing a turn and keeps a parked terminal until every overlapping resolver exits. Please add a regression test: park turn-1's terminal, advance the retry timer into a gated resolveBackgroundResponseDelivery mock (deferredPromise), dispatch turn-2's segment while gated, resolve the gates; assert two separate cards with their own labels — removing the per-turn separation must turn it red.
中文说明
承接 R5-4 的遗留缺陷：f598b415bf 新增的「定时器武装时复活」分支只覆盖解析重试仍在武装的情形——其兄弟状态仍然存在：定时器已触发、重试的解析正在飞行中（挂在命名会话所有者锁上，pending.retryTimer 已在 ~3010 清除）时，下一回合的分段无法通过本复活条件，会加入陈旧的 parked 队列（~2594 处 parked.resolvers++），两个 resolver 竞态进入同一个聚合——两个回合的内容被作为单张完成卡片冲出，违背 keeps a completed turn retry separate from the next turn 为武装情形钉住的隔离。探针：定时器触发后 resolverCalls=2，turn-2 分段后为 3（加入而非复活），投出合并卡片 ## ✅ Agent · Turn one\n\nTurn one result.\n\nTurn two result.。修复：为 parked 表项引入每回合身份，使分段只加入自己回合的队列（或在任何新非空分段到达时脱离已终止的 parked 队列，含重试解析飞行中）。注意：仅把复活条件放宽到 parked.resolvers > 0 很可能不够——飞行中的重试在 await 之后（~2651-2652）会重新读取映射并加入新回合已创建的聚合。约束：同回合重叠 resolver 仍须合并——由 waits for every overlapping resolver before completing a turn 与 keeps a parked terminal until every overlapping resolver exits 钉住。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Resolution retries now expose their in-flight state; a later turn replaces the map identity while the old pending entry stays tracked and is delivered through the detached path. Regression coverage: separates a resolution retry in flight from the next turn. The old behavior merged the turns; the DingTalk suite now passes 317/317.
| if (context.turnComplete) { | ||
| parked.turnComplete = true; | ||
| parked.status = context.status; |
There was a problem hiding this comment.
[Critical] R6-1: [certifies-falsely] [new-surface] An empty-text terminal for a NEW turn overwrites the parked terminal of the PREVIOUS turn in place — this park branch sets parked.status/label/completionPartial unconditionally — so when the armed resolution retry fires, the apply at ~2687 stamps the new turn's status and label onto the previous turn's aggregation: turn 1's successful result ships as ## ❌ Agent · Turn two (wrong icon, wrong label).
Trigger: turn-1 segment A fails resolution once and is parked with an armed retry; turn-1's empty terminal parks (turnComplete = true); within the retry window turn 2 emits only an empty-text {turnComplete: true, status: 'failed', label: 'Turn two'} segment on the same [sessionId, taskId] key — this branch overwrites the parked state; the timer fires, resolution succeeds, and turn-1's content is flushed under turn-2's identity.
Witness:
probe (unmodified PR code):
pushProactive = ["## ❌ Agent · Turn two\n\nTurn one result."] <- turn 1's success under turn 2's identity
with guard `if (context.turnComplete && !parked.turnComplete)`:
pushProactive = ["## ✅ Agent · Turn one\n\nTurn one result."]
pinned 'applies a completion marker that races the first target resolution' stays green
| if (context.turnComplete) { | |
| parked.turnComplete = true; | |
| parked.status = context.status; | |
| if (context.turnComplete && !parked.turnComplete) { | |
| parked.turnComplete = true; | |
| parked.status = context.status; |
The guard stops the overwrite; if a later-newer terminal must also be honored, park it behind the old one or give parked entries per-turn identity so a terminal matches its own turn. Same-turn parking must keep working — a turn's terminal may arrive before its aggregation exists, pinned by applies a completion marker that races the first target resolution. Please add a regression test: park turn-1's terminal via one rejected resolution (timer armed), dispatch turn-2's empty-text terminal with a different label/status, advance 30s; assert the delivered card is '## ✅ Agent · Turn one\n\nFirst result.' — removing the guard makes it deliver ❌/Turn two and turns red.
中文说明
新回合的空文本终止标记会就地覆盖上一回合停放的终止状态——此停放分支无条件设置 parked.status/label/completionPartial——武装的解析重试触发后，~2687 的应用分支把新回合的状态与标签盖到上一回合的聚合上：turn 1 的成功结果以 ## ❌ Agent · Turn two 投出（错误图标、错误标签）。触发：turn-1 分段 A 解析失败一次、带武装定时器停放；turn-1 空终止标记停放（turnComplete=true）；重试窗口内 turn 2 仅发出同键的空文本 {turnComplete:true, status:'failed', label:'Turn two'} 分段，覆盖停放状态；定时器触发、解析成功后，turn-1 的内容以 turn-2 的身份冲出。探针：未改动代码投出 ## ❌ Agent · Turn two\n\nTurn one result.；加上 if (context.turnComplete && !parked.turnComplete) 守卫后投出 ## ✅ Agent · Turn one\n\nTurn one result.，且 applies a completion marker that races the first target resolution 保持绿。约束：同回合停放必须继续工作（终止标记可能先于聚合到达）。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Any later blank or whitespace turn now separates the terminated pending cohort without overwriting the earlier terminal, and terminal-only fresh pending state is discarded. Regression coverage also starts a following nonblank turn to prove no stale label or status survives. The DingTalk suite passes 317/317.
| if ( | ||
| this.pendingBackgroundResponseTerminals.get(key) === parked && | ||
| parked.resolvers === 0 && | ||
| !parked.retryTimer && | ||
| !parked.resolutionDropped | ||
| ) { | ||
| this.pendingBackgroundResponseTerminals.delete(key); | ||
| } |
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] When the last of several concurrent first-segment resolvers fails after the aggregation already exists, this finally deletes the parked terminal marker while it still carries turnComplete — the turn's completion is silently discarded and never transferred to the aggregation. Distinct surviving defect beside fixed R5-3: the f598b41 fix propagated resolutionDropped to the existing aggregation (probe-observed working), but not the completion marker.
Trigger: two early segments suspend as resolvers (resolvers=2); the empty-text turnComplete marker parks on the pending entry. The first resolver succeeds and creates the aggregation, but its transfer chain skips (resolvers !== 0). The second (last) resolver fails: its else-branch moves resolutionDropped onto the aggregation and clears it on the parked entry; the throw/return skips the transfer chain, and this finally deletes the parked entry (resolvers===0, no retryTimer, resolutionDropped cleared) — discarding the parked turnComplete. The terminal being the turn's last message, no later dispatch ever runs the transfer branch: the aggregation never gets turnComplete, the bounded wait flushes its text as ## ⏹️ Agent · Worker one（部分） with the running status, owesTerminalBackgroundResponseCard never fires, and the completed turn is permanently presented as partial.
Witness:
probe (unmodified PR code):
parked while resolvers in flight: [{resolvers:2, turnComplete:true, status:'completed', label:'Worker one', ...}]
after settlement: parked = [] (deleted with turnComplete still set)
aggregation = {..., resolutionDropped:true, status:'running', parts:['First result.']} <- no turnComplete
after bounded wait: ["## ⏹️ Agent · Worker one（部分）\n\nFirst result."] <- completed turn permanently partial
Fix: in this finally, when parked.resolvers === 0 and parked.turnComplete is set, transfer the parked terminal to the existing aggregation before deleting — mirror the third transfer-chain branch (set turnComplete, status = parked.status ?? current.status, label, completionPartial) and let the completion flush run. Do not simply skip the delete when turnComplete is set — a kept entry becomes reachable by the next turn's first segment and reintroduces cross-turn reuse (the confirmed R4-4/R5-4 findings).
The revival guard at DingtalkAdapter.ts:2565-2570 is the only reset a parked entry gets from a later turn, which is why transfer-and-delete (not keep) is required. Please add a regression test: suspend two concurrent segments' resolution, park an empty-text turnComplete marker during the suspension, let the first resolver succeed and the second fail (target mismatch); assert the aggregation is flushed as a completed turn (completion card sent, no permanent （部分） outcome) — removing the finally-transfer branch must turn it red.
中文说明
当聚合已存在、最后一个并发首段 resolver 失败时，这个 finally 会在 parked 终止标记仍携带 turnComplete 时删除它——回合的完成状态被静默丢弃、从未转移到聚合上。这是已修复的 R5-3 旁边的独立残留缺陷：f598b415bf 把 resolutionDropped 传播到了已存在聚合（探针证实生效），但没有传播完成标记。触发：两个早段作为 resolver 挂起（resolvers=2），空文本 turnComplete 标记停放在 pending 表项上；第一个 resolver 成功建聚合但转移链因 resolvers !== 0 跳过；第二个（最后一个）resolver 失败——其 else 分支把 resolutionDropped 移到聚合上并清除 parked 上的标志，throw/return 跳过转移链，本 finally 删除 parked 表项——连同停放的 turnComplete。终止标记是该回合最后一条消息，后续不会再有派发执行转移分支：聚合永远拿不到 turnComplete，bounded wait 以运行中状态冲出 ## ⏹️ Agent · Worker one（部分），完成卡永不触发。修复：在本 finally 中，当 parked.resolvers === 0 且 parked.turnComplete 已置时，先把停放的终止状态转移到已存在聚合（镜像第三条转移分支）再删除；不要在 turnComplete 置位时简单跳过删除——保留的表项会被下一回合首段复用，重新引入跨回合污染。约束：2565-2570 的复活守卫是 parked 表项唯一的重置路径，因此必须「转移后删除」而非「保留」。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Both resolver-failure branches now transfer a parked terminal to the existing aggregation only when the failing resolver is the last one, then complete and flush it immediately. Regression coverage: applies a parked terminal when the last resolver fails. The DingTalk suite passes 317/317.
| const key = JSON.stringify([sessionId, context.taskId]); | ||
| let current = this.backgroundResponseAggregations.get(key); | ||
| let parked = this.pendingBackgroundResponseTerminals.get(key); | ||
| if (current?.turnComplete === true && text.trim().length > 0) { |
There was a problem hiding this comment.
[Critical] R6-3: [certifies-falsely] [new-surface] Every turn-separation guard keys on text.trim().length > 0, so a whitespace-only (or empty-text) event from the NEXT turn falls through into a completed-but-not-yet-removed aggregation — the armed delivery-retry window — and overwrites its status/label/completionPartial and mutates its pending delivery: turn 1's text is immediately re-delivered under turn 2's icon and label, and turn 1's owed retry accounting is consumed. With partial: true, the overwritten completionPartial also suppresses the header-only completion card turn 1 was owed.
Trigger: turn 1 completes and its card send fails once (armed 30s retry, turnComplete: true, delivery set). Turn 2's model response is whitespace-only — Session.ts emits it whenever responseText.length > 0 (whitespace included) and DaemonChannelBridge emits it because text is truthy. The event fails this detach condition, skips the empty-text branch (requires !current), overwrites current.status/label, mutates current.delivery, rewrites the header via refreshBackgroundResponseDelivery, and the flush immediately re-delivers turn 1's text as ## ❌ Agent · Turn two.
Witness:
probe (unmodified PR code):
after whitespace event:
pushProactive = ["## ✅ Agent · Turn one\n\nTurn one result.",
"## ❌ Agent · Turn two\n\nTurn one result."] <- mislabeled immediate re-delivery
with the trim clause dropped from the detach + parked-reset conditions:
no immediate re-delivery; after the retry window the armed retry delivers the correct card
via detachedBackgroundResponseAggregations admission; pinned separation test stays green
| if (current?.turnComplete === true && text.trim().length > 0) { | |
| if (current?.turnComplete === true) { |
Also drop the same text.trim().length > 0 clause from the parked-reset branch at ~2565 — whitespace/empty events then reach the !current && text.trim().length === 0 branch and no-op through super's empty-text early return.
The detached aggregation must be added to detachedBackgroundResponseAggregations, not merely deleted from the map: flushBackgroundResponseAggregation's entry guard admits an aggregation only while this.backgroundResponseAggregations.get(key) === aggregation or this.detachedBackgroundResponseAggregations.has(aggregation) (DingtalkAdapter.ts:2721-2724), and the armed retry still needs that admission. Please add a regression test next to DingtalkAdapter.test.ts:8364: complete turn 1 with pushProactive mocked to reject once (arming the delivery retry), dispatch a whitespace-only segment and an empty-text turnComplete event for the same key with a different status/label, advance the retry timer; assert the retried card is still ## ✅ Agent · Turn one... — removing the guard change makes it pick up turn 2's status/label and turns red.
中文说明
所有回合隔离守卫都以 text.trim().length > 0 为条件，因此下一回合的纯空白（或空文本）事件会落入「已完成但尚未移除」的聚合（武装的投递重试窗口），覆盖其 status/label/completionPartial 并改动其待投递状态：turn 1 的文本立即以 turn 2 的图标与标签重发，turn 1 应得的重试记账被消耗；若带 partial: true，被覆盖的 completionPartial 还会抑制 turn 1 应得的仅表头完成卡。触发：turn 1 完成且卡片发送失败一次（武装 30s 重试）；turn 2 的模型输出为纯空白——Session.ts 在 responseText.length > 0（含空白）时仍会发出，DaemonChannelBridge 因 text 为真值而发出；该事件通不过本脱离条件、又跳过空文本分支（要求 !current），覆盖状态后经 refreshBackgroundResponseDelivery 改写表头并立即重发为 ## ❌ Agent · Turn two。探针证实：去掉脱离与 parked 重置条件中的 trim 子句后不再立即重发，武装重试经 detachedBackgroundResponseAggregations 准入正确投递，隔离测试保持绿。约束：脱离的聚合必须加入 detachedBackgroundResponseAggregations 而非仅从映射删除（DingtalkAdapter.ts:2721-2724 的准入守卫），武装重试仍需要该准入。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. A completed aggregation now detaches on every later dispatch for the same key, including blank or whitespace events, so its retry keeps the original status and label. Regression coverage: keeps a completed delivery retry separate from later whitespace. The old guard fails this test; the DingTalk suite passes 317/317.
| aggregation: BackgroundResponseAggregation, | ||
| ): void { | ||
| const delivery = aggregation.delivery; | ||
| if (!delivery || aggregation.flushing) return; |
There was a problem hiding this comment.
[Critical] R6-4: [certifies-falsely] [new-surface] refreshBackgroundResponseDelivery is skipped while a flush is in flight (this guard) and is never re-run — its single call site is dispatch's turnComplete path at ~2713. When the turn completes mid-flush and that flush then fails, the retry reuses the stored plan/preparedReplyBody with the frozen pre-completion header: the retried content card delivers ## ⏹️ Agent · Worker one（部分） for a turn that completed. With completionPartial/dropped/resolutionDropped set, owesTerminalBackgroundResponseCard returns false and no correction card follows — the frozen card is the final report. This contradicts the diff's own invariant comment (~2703): 'its card must still report the completed turn'. The round-3 R3-2 fix does not cover this sequence — its zero-progress refresh runs only inside the terminal dispatch, when flushing is already false.
Trigger: a background turn outruns the 10-minute bounded wait; the flush composes a delivery with the running status (⏹️, partial), sets flushing = true, and awaits the send. The terminal event arrives during that await: dispatch updates delivery.status/label/partial, but refresh returns early on aggregation.flushing and the re-entrant flush returns early too. The in-flight send then fails; the catch stores the plan built from the frozen text and arms the 30s retry. Nothing calls refresh again — the retry delivers the frozen header.
Witness:
probe (unmodified PR code):
sends = [
{title:'⏹️ Agent · Worker on', text:"## ⏹️ Agent · Worker one（部分）\n\nFirst result."} <- attempt 1 (rejected)
{title:'⏹️ Agent · Worker on', text:"## ⏹️ Agent · Worker one（部分）\n\nFirst result."} <- RETRY, DELIVERED, WRONG
{title:'✅ Agent · Worker one', text:"## ✅ Agent · Worker one"} <- header-only correction
]
with `this.refreshBackgroundResponseDelivery(aggregation)` re-run after the flush entry guard:
{title:'✅ Agent · Worker one', text:"## ✅ Agent · Worker one\n\nFirst result."} <- RETRY, DELIVERED, CORRECT
full DingtalkAdapter.test.ts suite: 308/308 green with the fix
Fix: re-run the refresh inside flushBackgroundResponseAggregation before a stored delivery is reused — call this.refreshBackgroundResponseDelivery(aggregation) right after the flush entry guard (which already returns early when aggregation.flushing, so mid-send protection is preserved; refresh is idempotent — the header replace is a fixed point — so the existing dispatch call site can stay).
Keep the already-sent-chunk guard if (plan && plan.nextChunk > 0) return; (DingtalkAdapter.ts:2914) intact — chunks already delivered cannot be rewritten, and retries only the unsent proactive chunks of an aggregation (DingtalkAdapter.test.ts:10271) relies on resume-from-nextChunk semantics. Please add a regression test: dispatch a running segment, advance the 10-minute bounded wait, hold pushProactive on a deferred promise, dispatch the completion terminal (empty text, status: 'completed', turnComplete: true), reject the deferred send, advance the 30s retry timer; assert the retried send text contains ## ✅ Agent · and not （部分） — without the fix it reads ## ⏹️ Agent · …（部分）.
中文说明
refreshBackgroundResponseDelivery 在 flush 飞行中会被本守卫跳过，且永远不会再执行——它唯一的调用点是 ~2713 的 dispatch turnComplete 路径。当回合在 flush 途中完成、而该 flush 随后失败时，重试会复用携带冻结前完成表头的已存计划/preparedReplyBody：重投的内容卡对已完成的回合显示 ## ⏹️ Agent · Worker one（部分）；若置位了 completionPartial/dropped/resolutionDropped，owesTerminalBackgroundResponseCard 返回 false、不再有纠正卡——冻结的卡片成为最终报告。这与 diff 自身的不变量注释（~2703：「卡片必须仍报告已完成的回合」）矛盾；第 3 轮 R3-2 的修复不覆盖该时序（其零进度刷新只在终止派发内执行，彼时 flushing 已为 false）。修复：在 flushBackgroundResponseAggregation 复用已存投递之前重跑刷新——在 flush 入口守卫之后调用 this.refreshBackgroundResponseDelivery(aggregation)（入口守卫在 aggregation.flushing 时已提前返回，发送中保护保留；刷新是幂等的）。约束：保持 if (plan && plan.nextChunk > 0) return;（DingtalkAdapter.ts:2914）原样——已送达分片不可改写，retries only the unsent proactive chunks of an aggregation（DingtalkAdapter.test.ts:10271）依赖 nextChunk 续传语义。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Every flush attempt now refreshes a zero-progress stored delivery before reuse, while the existing nextChunk greater than zero guard still preserves already-sent chunk resume semantics. Regression coverage: refreshes a failed in-flight flush before retrying it. The DingTalk suite passes 317/317.
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- onSessionDied drain loses the sender @-mention on drained reply-path cards (DingtalkAdapter.ts:2286) — already recorded in round 6's deferred list (D6-3)
Not reviewed: build-and-test — the test phase never ran: the build phase consumed the 600s call budget (builds were green across all 21 workspaces); the runner disk then exhausted (ENOSPC), making installs and probes impossible.
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 — stopped after round 4 of the 5-round cap: the runner disk exhausted (ENOSPC) and a concurrent session's cleanup swept the review worktree mid-round; round 5 was not built, round-4 chunk 11 was never audited, and several round-4 checks were cut short.
Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read.
Not reviewed: verification — could not check that Step 4 and Step 5 ran (no subagent transcripts at /home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.).
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/channels/dingtalk/src/DingtalkAdapter.ts:2940 — [review] Critical [fails-closed] [new-surface] teardown drain never sweeps pendingBackgroundResponseTerminals — armed retry timers fire post-teardown, resolutionDropped entries leak p…packages/cli/src/acp-integration/session/Session.ts:9948 — [probe] loopDetected stop-path partial marker unpinned; stopAfterPermissionCancel-without-abort untestedpackages/channels/dingtalk/src/DingtalkAdapter.ts:2539 — [probe] non-agent-kind dispatch guard untested (308/308 green under mutation)packages/channels/dingtalk/src/DingtalkAdapter.test.ts:7377 — [probe] $&-label test never reaches replaceHeader; string-form replace mutation survivespackages/channels/base/src/ChannelAgentBridge.ts:112 — [review] parseBackgroundResponseContext reject branches (kind allowlist, status, toolUseId) untested
Convergence: round 7 posted 6 inline comment(s), 2 of them reported for the first time; the previous round posted 6 (6 new). Findings keep coming back to the same files: packages/channels/dingtalk/src/DingtalkAdapter.ts (findings in rounds 4, 5, 6; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — the test phase never ran: the build phase consumed the 600s call budget (builds were green across all 21 workspaces); the runner disk then exhausted (ENOSPC), making installs and probes impossible.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：reverse audit — stopped after round 4 of the 5-round cap: the runner disk exhausted (ENOSPC) and a concurrent session's cleanup swept the review worktree mid-round; round 5 was not built, round-4 chunk 11 was never audited, and several round-4 checks were cut short.
未审查：覆盖情况——无法读取 agent 的运行记录（no subagent transcripts at /home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.），本次运行无法证明 diff 的任何部分被读过。
未审查：验证——无法检查步骤 4 与步骤 5 是否运行（no subagent transcripts at /home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-9/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-9--work-qwen-code-qwen-code/subagents/f42c07ea-673c-4a06-8c26-04bdef5b590c'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.）。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 5 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 6 条行内评论，其中 2 条是首次提出；上一轮发布了 6 条（其中 6 条首次提出）。发现反复回到同一批文件：packages/channels/dingtalk/src/DingtalkAdapter.ts（第 4、5、6 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
| this.pendingBackgroundResponseTerminals.get(key) === parked && | ||
| parked.resolvers === 0 && | ||
| !parked.retryTimer && | ||
| !parked.resolutionDropped |
There was a problem hiding this comment.
[Critical] R4-4: [certifies-falsely] [new-surface] Still standing — re-verified this round at the same commit. A parked terminal that survives exhausted resolution retries (resolutionDropped set, no retryTimer) is never reset or deleted: both delete sites require !resolutionDropped, and the new-turn revival branch fires only while parked.retryTimer is armed. The next turn of the same task reuses the stale entry and inherits the dead turn's turnComplete/status/label.
Failure scenario: aggregation mode on; turn 1's first segment fails resolveBackgroundResponseDelivery three times (give-up sets resolutionDropped with no timer); turn 1's empty terminal parks (turnComplete, completed). When turn 2 arrives and resolution succeeds, the transfer branch (~2687) stamps turn 2's fresh aggregation with turn 1's turnComplete/status/label — turn 2's first segment flushes immediately as a COMPLETED card under turn 1's icon and label with a spurious （部分）, and turn 2's remaining segments detach into a second card.
Witness:
probe (this round, intact PR code):
after exhaustion: pendingBackgroundResponseTerminals = {resolvers:0, retryAttempts:3, resolutionDropped:true}
after turn-1 terminal parks: {turnComplete:true, status:'completed', label:'Turn one'}
after turn-2 FIRST segment: pushProactive = ["## ✅ Agent · Turn one（部分）\n\nTurn two result."] <- turn-2 content under turn-1's label
flip (revival condition widened to (parked.retryTimer || parked.resolvers === 0)):
calls after turn-2 first segment: []; single correct card "## ✅ Agent · Turn two…"
Fix: reset or replace the exhausted parked entry when a new turn's segment arrives — widen the revival condition at ~2566 to also replace an entry with no retryTimer, resolvers === 0 and resolutionDropped set (or clear the parked turnComplete/status/label at exhaustion while keeping resolutionDropped for the （部分） transfer). Constraint: retryBackgroundResponseResolution routes on map identity (DingtalkAdapter.ts:3040) and the revival branch requires parked.retryTimer so a still-armed retry's closure keeps its object — cleanup must run only at exhaustion, never while a retry is pending. Please add a regression test (fail resolution 3×, park the terminal, dispatch turn 2; assert turn 2's first delivery carries its own label/status and is not marked complete on segment one) and confirm it reds when the reset is removed.
中文说明
遗留未决——本轮在相同提交上复核确认。解析重试耗尽后幸存的 parked terminal（resolutionDropped 置位、无 retryTimer）永远不会被重置或删除：两个删除点都要求 !resolutionDropped，而新回合复活分支只在 parked.retryTimer 武装时触发；同一任务的下一回合会复用这条陈旧表项，继承已死回合的 turnComplete/status/label。触发：聚合模式开启，回合 1 首分段解析连续失败 3 次（放弃分支置 resolutionDropped、不武装定时器），回合 1 的空终止 marker 停放（turnComplete、completed）；回合 2 到达且解析成功时，转移分支（~2687）把回合 1 的 turnComplete/status/label 盖到回合 2 的新聚合上——回合 2 的首分段立即以「完成」卡片发出，却顶着回合 1 的图标与标签、带多余（部分），其余分段脱离成第二张卡。探针输出见英文部分。修复：新回合分段到达时重置或替换已耗尽的 parked 表项——把 ~2566 的复活条件放宽为同时覆盖「无 retryTimer、resolvers === 0、resolutionDropped 置位」的表项（或在耗尽时清除停放的 turnComplete/status/label、保留 resolutionDropped 用于（部分）传递）。约束：重试按映射身份路由（DingtalkAdapter.ts:3040），复活分支依赖 parked.retryTimer 以保证武装中重试的闭包持有原对象——清理只能在耗尽时执行。请补回归测试（解析连续失败 3 次、停放终止 marker、派发回合 2，断言回合 2 首次投递使用自身标签/状态且首分段不被标记完成），并确认移除该重置后测试变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Exhausted resolution now records that the turn ended, clears stale terminal metadata, gives the next turn fresh pending state, and session drain clears both mapped and detached pending entries. Regression coverage: does not apply an exhausted terminal to the next turn; clears pending resolution state when a session dies. The old behavior failed these contracts; the DingTalk suite now passes 317/317.
| if (context.turnComplete) { | ||
| parked.turnComplete = true; | ||
| parked.status = context.status; |
There was a problem hiding this comment.
[Critical] R6-1: [certifies-falsely] [new-surface] Still standing — the code is byte-identical to the round-6 review. An empty-text terminal for a NEW turn overwrites the parked terminal of the PREVIOUS turn in place — this park branch sets parked.status/label/completionPartial unconditionally — so when the armed resolution retry fires, the apply at ~2687 stamps the new turn's status and label onto the previous turn's aggregation: turn 1's successful result is delivered under turn 2's icon/label.
Witness: not run this round — carried work-list entry re-asserted at byte-identical PR content; this round's code trace confirms the park branch still writes parked.turnComplete/status/label/completionPartial with no turn-identity check.
Fix: park with per-turn identity — refuse to overwrite a parked entry that already carries turnComplete for a different turn (detach/replace instead), or key the parked record by turn generation so a new turn's terminal cannot land on the previous turn's record. Please add a regression test (park turn 1's terminal with the retry armed, dispatch turn 2's empty terminal, advance the retry; assert the delivered card carries turn 1's own status/label) and confirm it reds when the identity guard is removed.
中文说明
遗留未决——代码与第 6 轮审查时逐字节相同。新回合的空文本终止 marker 会原地覆盖上一回合停放的终止记录——该停放分支无条件写入 parked.status/label/completionPartial——武装中的解析重试触发时，~2687 的应用会把新回合的状态与标签盖到上一回合的聚合上：回合 1 的成功结果顶着回合 2 的图标/标签送达。证据：本轮未重新运行探针——在逐字节相同的 PR 内容上复验，本轮代码走查确认停放分支仍无回合身份检查。修复：按回合身份停放——拒绝覆盖已携带其他回合 turnComplete 的停放表项（改为脱离/替换），或按回合代际为停放记录建键。请补回归测试并做移除变红验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Any later blank or whitespace turn now separates the terminated pending cohort without overwriting the earlier terminal, and terminal-only fresh pending state is discarded. Regression coverage also starts a following nonblank turn to prove no stale label or status survives. The DingTalk suite passes 317/317.
| if (current?.turnComplete === true && text.trim().length > 0) { | ||
| this.detachedBackgroundResponseAggregations.add(current); |
There was a problem hiding this comment.
[Critical] R6-3: [certifies-falsely] [new-surface] Still standing — re-verified this round at the same commit. Every turn-separation guard keys on text.trim().length > 0, so an empty or whitespace-only event from the NEXT turn falls through into a completed-but-not-yet-removed aggregation — the armed delivery-retry window — and overwrites its status/label/completionPartial and mutates its pending delivery.
Failure scenario: turn 1 completes but its card send failed once, so the aggregation stays with turnComplete and a retained delivery awaiting the 30s retry. Turn 2 of the same task completes silently and dispatches ('', {status:'stopped', turnComplete:true}); the detach guard requires non-empty text and the park branch requires !current, so the event lands on turn 1's aggregation — turn 1's completed card is re-delivered instantly under turn 2's ⏹️ icon: a successfully completed turn reported as stopped.
Witness:
probe (this round, intact PR code):
after turn-1 failed flush: calls = ["## ✅ Agent · Worker one\n\nWorking."] (rejected)
after turn-2 empty terminal: calls = [..., "## ⏹️ Agent · Worker one\n\nWorking."] <- completed turn 1 re-delivered under turn 2's ⏹️ icon
flip (guard widened to if (current?.turnComplete === true)):
one call only; the 30s retry delivers the ✅ card
Fix: drop the length clause so any event after turn completion starts a new turn — if (current?.turnComplete === true) { detach }; the empty event then reaches the existing !current empty-text branch and routes to super. Constraint: the detached empty terminal must not produce a card — ChannelBase.dispatchBackgroundResponse early-returns on empty text (ChannelBase.ts:550). Please add a regression test (complete turn 1 with a failed first send, dispatch turn 2's empty terminal, advance the retry; assert the delivered card still carries turn 1's ✅ header) and confirm it reds when the widened detach is removed.
中文说明
遗留未决——本轮在相同提交上复核确认。所有回合隔离守卫都以 text.trim().length > 0 为条件，因此下一回合的空/纯空白事件会落入已完成但尚未移除的聚合——武装中的投递重试窗口——覆盖其 status/label/completionPartial 并改动待投递。触发：回合 1 完成但卡片发送失败一次，聚合带着 turnComplete 与保留的投递等待 30 秒重试；同任务回合 2 静默完成并派发 ('', {status:'stopped', turnComplete:true})——脱离守卫要求非空文本、停放分支要求 !current，事件于是落在回合 1 的聚合上：回合 1 的完成卡片立即以回合 2 的 ⏹️ 图标重发——成功完成的回合被报告为已停止。探针输出见英文部分。修复：去掉长度条件，使回合完成后的任何事件都开启新回合（if (current?.turnComplete === true) { 脱离 }）；空事件随后进入既有的 !current 空文本分支并路由给 super。约束：脱离的空终止不得产卡——ChannelBase.dispatchBackgroundResponse 对空文本提前返回（ChannelBase.ts:550）。请补回归测试并做移除变红验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. A completed aggregation now detaches on every later dispatch for the same key, including blank or whitespace events, so its retry keeps the original status and label. Regression coverage: keeps a completed delivery retry separate from later whitespace. The old guard fails this test; the DingTalk suite passes 317/317.
| const delivery = aggregation.delivery; | ||
| if (!delivery || aggregation.flushing) return; |
There was a problem hiding this comment.
[Critical] R6-4: [certifies-falsely] [new-surface] Still standing — the code is byte-identical to the round-6 review. refreshBackgroundResponseDelivery is skipped while a flush is in flight (this guard) and is never re-run — its single call site is dispatch's turnComplete path. When the turn completes mid-flush and that flush then fails, the retry reuses the stored plan/preparedReplyBody with the frozen pre-completion header: the retried card delivers ## ⏹️ Agent · …（部分） for a turn that completed, and with completionPartial/dropped/resolutionDropped set, owesTerminalBackgroundResponseCard returns false and no correction card follows — the frozen card is the final report. This contradicts the diff's own invariant comment (~2703): 'its card must still report the completed turn'.
Witness: not run this round — carried work-list entry re-asserted at byte-identical PR content; the round-6 probe stands (retry delivered the frozen ⏹️（部分） card; re-running refresh after the flush entry guard flipped it to the correct ✅ card with 308/308 green), and this round's trace confirms the guard and the single call site.
Fix: re-run this.refreshBackgroundResponseDelivery(aggregation) inside flushBackgroundResponseAggregation right after the flush entry guard, before a stored delivery is reused — refresh is idempotent (fixed-point header replace), so the existing dispatch call site can stay. Constraint: keep the already-sent-chunk guard if (plan && plan.nextChunk > 0) return; (DingtalkAdapter.ts:2914) intact — delivered chunks cannot be rewritten, and retries only the unsent proactive chunks of an aggregation relies on resume-from-nextChunk semantics. Please add a regression test (running segment, advance the 10-minute bounded wait, hold pushProactive on a deferred promise, dispatch the completion terminal, reject the deferred send, advance the 30s retry; assert the retried send text contains ## ✅ Agent · and not （部分）) and confirm it reds without the fix.
中文说明
遗留未决——代码与第 6 轮审查时逐字节相同。refreshBackgroundResponseDelivery 在 flush 飞行中被该守卫跳过，且永远不会再执行——其唯一调用点是 dispatch 的 turnComplete 路径。当回合在 flush 途中完成、而该 flush 随后失败时，重试会复用携带冻结前完成表头的已存计划/preparedReplyBody：重投的卡片对已完成的回合显示 ## ⏹️ Agent · …（部分）；若置位了 completionPartial/dropped/resolutionDropped，owesTerminalBackgroundResponseCard 返回 false、不再有纠正卡——冻结的卡片成为最终报告，与 diff 自身的不变量注释（~2703）矛盾。证据：本轮未重新运行探针——第 6 轮探针仍然成立（重试投出冻结的 ⏹️（部分）卡；在 flush 入口守卫后重跑刷新即变为正确的 ✅ 卡，308/308 绿），本轮走查确认守卫与唯一调用点。修复：在 flushBackgroundResponseAggregation 的入口守卫之后、复用已存投递之前重跑 this.refreshBackgroundResponseDelivery(aggregation)（刷新是幂等的）。约束：保持 if (plan && plan.nextChunk > 0) return;（DingtalkAdapter.ts:2914）原样——已送达分片不可改写，续传用例依赖 nextChunk 语义。请补回归测试并做移除变红验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Every flush attempt now refreshes a zero-progress stored delivery before reuse, while the existing nextChunk greater than zero guard still preserves already-sent chunk resume semantics. Regression coverage: refreshes a failed in-flight flush before retrying it. The DingTalk suite passes 317/317.
| if (pending.retryTimer) { | ||
| pending.resolutionDropped = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Critical] R7-1: [certifies-falsely] [new-surface] When a resolution retry is already armed, this dedupe branch drops a TEXT-BEARING terminal segment's turnComplete marker — unlike an empty-text terminal it never passes the park branch (~2574), so the terminal state is recorded nowhere and the throw error discards it together with the segment's text. The turn never completes: the bounded wait re-arms forever, the chat never learns the turn finished, and the next turn's segments merge into the stale open aggregation (the detach guard requires current?.turnComplete === true).
Failure scenario: the producer's final response segment carries both text and turnComplete: true (Session.ts: turnComplete = functionCalls.length === 0 — a text-only final response IS the terminal). Segment 1 fails resolution, arming the 30s retry; within the window the text-bearing terminal arrives and its resolution also fails; the catch hits this armed branch, sets only resolutionDropped, and returns. The identical drop exists in the !delivery || target mismatch sibling branch, which shares this scheduler. The existing test at DingtalkAdapter.test.ts:8070 exercises this interleave but sends the terminal as a separate EMPTY-text event (which the park branch saves), so the text-bearing-terminal shape is untested.
Witness:
probe (this round, scratch tree, intact PR code):
resolveCalls=3
cards=["## ⏹️ Agent · Worker one（部分）\n\nPart one."] <- one partial card; terminal text lost; no ✅ ever ships
aggCount=1 aggStates=[{"resolutionDropped":true}] <- turnComplete never recorded; entry never drains
baseline suite: Tests 308 passed (308) (bug present, uncaught)
fixed arm (record the terminal state in the dedupe branch):
cards=["## ✅ Agent · Worker one（部分）\n\nPart one."], aggCount=0
Fix: in the armed-timer branch, park the terminal marker the way the empty-text park branch does before returning (pending.turnComplete/status/label/completionPartial from context when context.turnComplete), so the retry-driven re-dispatch's existing transfer branch applies it. Keep this scoped to the armed branch — parking in the exhaustion branch would hand the stale marker to the next turn (the R4-4 shape). Constraint: DingtalkAdapter.test.ts:8131-8134 pins the empty-terminal interleave to exactly ['## ✅ Agent · Worker one（部分）\n\nFirst result.'] — the fix must keep marking the dropped segment's turn partial via resolutionDropped and must not change that outcome. Please add the regression variant (terminal dispatched with non-empty text and turnComplete: true while the first retry is armed; assert the completion card is eventually delivered with the terminal status and the aggregation drains empty) and confirm it reds when the parked-terminal guard is removed.
中文说明
当解析重试仍在武装时，该去重分支会丢弃携带文本的终止分段的 turnComplete marker——与空文本终止不同，它从不经过停放分支（~2574），终止状态无处记录，throw error 连同分段文本一起丢弃。回合永远不完成：bounded wait 永远重新武装，聊天永远不知道回合已结束，下一回合的分段会并入陈旧的开放聚合（脱离守卫要求 current?.turnComplete === true）。触发：生产者的最终响应分段同时携带文本与 turnComplete: true；分段 1 解析失败武装 30 秒重试，窗口内携带文本的终止分段到达且其解析也失败，catch 命中该武装分支，只置 resolutionDropped 后返回。!delivery || 目标不匹配 的兄弟分支存在同样的丢弃。既有测试（DingtalkAdapter.test.ts:8070）以独立的空文本事件发送终止（会被停放分支保存），因此携带文本的终止形态未被测试。探针：见英文部分（一张 ⏹️（部分）卡、终止文本丢失、永不发 ✅；表项不排空；基线套件 308/308 绿；修复臂翻转为 ✅ 且表项排空）。修复：在武装分支返回前像空文本停放分支一样停放终止状态，使重试重派发的既有转移分支得以应用；仅限武装分支——在耗尽分支停放会把陈旧 marker 交给下一回合（R4-4 形态）。约束：DingtalkAdapter.test.ts:8131-8134 把空终止交织钉死为 ['## ✅ Agent · Worker one（部分）\n\nFirst result.']——修复必须继续通过 resolutionDropped 标记被丢分段的回合为部分，且不改变该结果。请补回归变体并做移除变红验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Resolution-retry scheduling now records terminal metadata from a text-bearing terminal before the armed-retry dedupe branch, while exhaustion still clears stale terminal fields. Regression coverage: preserves a text-bearing terminal while a resolution retry is armed. Removing this transfer makes the test fail; the DingTalk suite passes 317/317.
| if (parked.retryTimer) { | ||
| clearTimeout(parked.retryTimer); | ||
| parked.retryTimer = undefined; |
There was a problem hiding this comment.
[Critical] R7-2: [certifies-falsely] [new-surface] A next-turn segment that resolves successfully cancels the previous turn's armed resolution retry and silently discards it — the retry's text/context exist only in the timer closure. When the superseded segment was the previous turn's TEXT-BEARING TERMINAL, that turn's entire content AND completion vanish (nothing is even logged), and resolutionDropped is stamped onto the next turn's aggregation — every turn-2 card carries （部分） although turn 2 lost nothing.
Failure scenario: turn 1 is a single text-bearing segment with turnComplete: true (short background task). Its resolution fails, arming the 30s retry; scheduleBackgroundResponseResolutionRetry never copies context.turnComplete onto the parked entry, so the revival branch (requires parked.turnComplete) cannot fire. Turn 2's first segment resolves inside the window: this branch clears the armed timer — the closure holding turn 1's only text becomes unreachable — and sets parked.resolutionDropped, which transfers to turn 2's fresh aggregation (~2655-2659). The asymmetry reveals the intent: when the terminal WAS parked, the revival branch + the detached path deliver the superseded segment late under its own status; this branch is the hole for the terminal-inside-the-retry shape. Distinct from R7-1 (schedule-time dedupe, same turn) and R5-4 (timer already fired, resolution in flight).
Witness: not run — the unit probe was prepared in the scratch tree but the review environment was swept by a concurrent session's cleanup mid-verification (worktree, scratch tree and diff deleted before the probe could be written); confirmation rests on the line-by-line trace of the revival condition, the clear-and-mark branch, and the resolutionDropped transfer.
Fix: when the success path finds an armed retry, do not just clear it — capture the superseded segment's text/context on the parked marker before clearing and flush it as a detached aggregation under its own status (the same literal shape retryBackgroundResponseResolution's detached path uses); do not set resolutionDropped on the new turn's aggregation for a supersession that carried a terminal. Constraint: a superseded segment must not be re-dispatched through dispatchBackgroundResponse while the next turn's aggregation exists — the else if (context.turnComplete) branch (~2697-2700) would complete turn 2's aggregation under turn 1's status; route via the detached path. Please add a regression test (turn 1's single text-bearing terminal while resolution throws once; turn 2's first segment resolves inside the retry window; assert turn 1's text is delivered in its own card with turn 1's completion status and turn 2's cards carry no （部分）) and confirm restoring the bare clearTimeout drop reds it.
中文说明
下一回合的分段解析成功时会取消上一回合武装中的解析重试并静默丢弃——重试的文本/上下文只存在于定时器闭包中。当被取代的分段是上一回合携带文本的终止分段时，该回合的全部内容连同完成状态一起消失（甚至没有日志），且 resolutionDropped 会被盖到下一回合的聚合上——回合 2 的每张卡都带（部分），尽管回合 2 没有丢失任何内容。触发：回合 1 是单个携带文本且 turnComplete: true 的分段（短任务），解析失败武装 30 秒重试；scheduleBackgroundResponseResolutionRetry 从不把 context.turnComplete 拷贝到停放表项，复活分支（要求 parked.turnComplete）无法触发；回合 2 首分段在窗口内解析成功：该分支清除武装中的定时器（持有回合 1 唯一文本的闭包不可达），并置 parked.resolutionDropped，随后转移到回合 2 的新聚合（~2655-2659）。不对称性揭示了意图：终止曾被停放时，复活分支+脱离路径会在之后以自身状态送达被取代分段；该分支是「终止在重试内部」形态的漏洞。与 R7-1（调度期去重、同回合）和 R5-4（定时器已触发、解析飞行中）不同。证据：本轮未能运行探针——验证中途审查环境被并发会话的清理扫除（工作树、临时树与 diff 在探针写入前被删除）；确认基于对复活条件、清除并标记分支、resolutionDropped 转移的逐行走查。修复：成功路径发现武装中的重试时不要只清除——先把被取代分段的文本/上下文捕获到停放标记，再以其自身状态作为脱离聚合冲出；不要为携带终止的被取代在下一回合聚合上置 resolutionDropped。约束：被取代分段不得在下一回合聚合存在时经由 dispatchBackgroundResponse 重派发（else if (context.turnComplete) 分支会以回合 1 的状态完成回合 2 的聚合），应走脱离路径。请补回归测试并做恢复裸 clearTimeout 丢弃的变红验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. A text-bearing terminal resolution retry is now retained and delivered through its own detached aggregation when a later turn takes the key; resolution loss no longer leaks onto the later turn. Regression coverage: keeps a text-bearing terminal retry separate from the next turn. Removing the terminal capture makes the test fail; the DingTalk suite passes 317/317.
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): "agent reverse-audit (round 2)": did not execute the new/changed Session.test.ts cases under vitest — verified them by reading against the implementation instead.; "agent reverse-audit (round 2)": whether an agent -kind notification can reach parseBackgroundResponseContext with a status outside 'completed' | 'failed' | 'cancelled' — I confirmed the…; "agent reverse-audit (round 2)": whether a messageRewriter -intercepted turn-complete terminal is re-emitted with rewritten: true and thereby dropped by AcpBridge , leaving the DingTalk agg….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/channels/dingtalk/src/DingtalkAdapter.ts:2176 — [probe] Critical [fails-closed] [new-surface] disconnect() drain is fire-and-forget while callers process.exit() in the same tick, so buffered aggregated output is silently lost on sh…packages/channels/base/src/ChannelAgentBridge.ts:88 — [probe] BackgroundResponseContext.toolUseId is declared and populated but has zero read sites repo-wide (dead field on a new public interface)packages/cli/src/acp-integration/acpAgent.ts:10583 — [probe] the label.length>256 branch of the new background-notification label gate is untested (only the blank-label rejection is exercised)packages/channels/dingtalk/src/DingtalkAdapter.ts:2940 — [probe] the detached-aggregation half of the drainBackgroundResponseAggregations union is never exercised (no test combines detach with drain)packages/channels/dingtalk/src/DingtalkAdapter.ts:2534 — [probe] no DingTalk test dispatches a non-'agent' context, so the kind guard keeping shell/monitor/workflow replies out of Agent-labeling is unpinnedpackages/channels/dingtalk/src/DingtalkAdapter.ts:933 — [probe] retryable/permanent classification never reaches the give-up check on resumed attempts (ReplyTextDeliveryError has no retryable; proactive resume bypasses the wrapper)packages/cli/src/acp-integration/session/Session.ts:9896 — [probe] turnComplete is derived only from functionCalls.length===0 and never consults finishReason, so a MAX_TOKENS-truncated reply is stamped complete with no partialpackages/cli/src/acp-integration/session/Session.ts:10038 — [probe] the non-agent branch of the new rawLabel fallback is write-only (DingtalkAdapter diverts non-agent contexts before any label use)packages/channels/dingtalk/src/DingtalkAdapter.test.ts:7021 — [probe] the prepended Agent header becomes the markdown title for every background reply, but the rewritten tests assert only markdown.text, so the title change ships unpinnedpackages/channels/dingtalk/src/DingtalkAdapter.ts:2307 — [probe] the onSessionRetiring drain is fire-and-forget and its retry timer outlives retirement, so the (部分) card can land after the confirmation/fresh conversationpackages/cli/src/acp-integration/session/Session.ts:9976 — [probe] the post-loop finishBackgroundNotificationTurn exit is the only one not passing partial=true and is currently an unobservable dead switchpackages/cli/src/acp-integration/session/Session.ts:9294 — [probe] the entry?buildBackgroundEntryLabel fallback is unreachable in production yet the new test pins only that dead branch (test-locked duplication)packages/cli/src/serve/create-sub-session.ts:290 — [probe] a blank-sanitized info.name drops attribution instead of falling back to info.prompt, so the card carries no sub-session namepackages/channels/base/src/AcpBridge.test.ts:954 — [probe] parseBackgroundResponseContext's eight validation branches are exercised by only two inputs; the typeof turnComplete==='boolean' gate is unpinnedpackages/channels/dingtalk/src/DingtalkAdapter.ts:3062 — [probe] the stale-target/!delivery guard (2622 and 3060) is never exercised on its true sidepackages/channels/dingtalk/src/DingtalkAdapter.ts:2645 — [probe] the successful-resolution branch that cancels an armed retry timer and sets resolutionDropped is unreachable in testspackages/cli/src/acp-integration/session/Session.test.ts:10472 — [probe] the response-side label sanitizer (Session.ts:10043) is unpinned by any testpackages/cli/src/acp-integration/session/Session.test.ts:9895 — [probe] only three of eight finishBackgroundNotificationTurn partial:true exits are pinned with buffered textpackages/channels/dingtalk/src/DingtalkAdapter.test.ts:7193 — [probe] the typeof context.turnComplete==='boolean' clause of canAggregate is never exercised (all aggregation dispatches pass an explicit turnComplete)packages/channels/dingtalk/src/DingtalkAdapter.test.ts:9158 — [probe] sourceLabel is always undefined in aggregation tests, so the named-session attribution plumbing is unexercised
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：did not execute the new/changed Session.test.ts cases under vitest — verified them by reading against the implementation instead.；"agent reverse-audit (round 2)"：whether an agent -kind notification can reach parseBackgroundResponseContext with a status outside 'completed' | 'failed' | 'cancelled' — I confirmed the…；"agent reverse-audit (round 2)"：whether a messageRewriter -intercepted turn-complete terminal is re-emitted with rewritten: true and thereby dropped by AcpBridge , leaving the DingTalk agg…。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 20 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (current?.turnComplete === true && text.trim().length > 0) { | ||
| this.detachedBackgroundResponseAggregations.add(current); |
There was a problem hiding this comment.
[Critical] R6-3: [certifies-falsely] [new-surface] Still standing (re-confirmed by probe this round). The turn-isolation detach guard keys on text.trim().length > 0, so a whitespace-only segment from a LATER turn is not detached — it folds into the previous turn's already-completed aggregation and overwrites the status/label of its frozen pending-retry delivery. Turn 1's completed answer is then re-sent under turn 2's header (stamped ⏹️/❌ with turn 2's label, no （部分）), and the aggregation is removed so no card corrects it.
Witness:
producer (real Session): a stream whose only text part is '\n' emits {text:'\n',trimLength:0,turnComplete:false}
consumer (aggregation on, turn-1 send 429'd, retry armed): after the whitespace turn-2 segment,
sends=['## ✅ Agent · Worker one\n\nTurn one answer.', '## ⏹️ Agent · Worker two\n\nTurn one answer.'] (no clock advanced, no correcting card)
fix flip (guard on text.length>0): turn 1's retry delivers '## ✅ Agent · Worker one' correctly
Fix: make the detach guard text-independent — once an aggregation has turnComplete === true, detach on ANY later dispatch for that key (drop the text.trim().length > 0 clause), so a whitespace-only segment falls into the empty-text parking branch instead of mutating the completed turn's card.
Keep the !current && text.trim().length === 0 parking branch (DingtalkAdapter.ts:2575) intact for a terminal that races the first segment's suspended resolution; Session.ts:9713/9908 emit at most one terminal per turn, so detaching on any post-turnComplete dispatch cannot discard the same turn's terminal. Please add a regression test (turn-1 segment+terminal with the send failing once, then a whitespace-only turn-2 segment for the same key; assert the delivered card keeps turn 1's icon/label) and confirm it reds when text.trim().length > 0 is reinstated.
中文说明
[Critical] R6-3：[certifies-falsely] [new-surface] 仍然存在（本轮经探针重新确认）。回合隔离的 detach 守卫以 text.trim().length > 0 为条件，因此来自后续回合的纯空白 segment 不会被 detach——它会并入上一回合已完成的 aggregation，并覆盖其冻结待重试投递的 status/label。于是回合 1 已完成的答案会以回合 2 的表头重新发出（被打上 ⏹️/❌ 与回合 2 的 label、且无 （部分）），随后该 aggregation 被移除，没有卡片纠正它。
证据：生产者（真实 Session）仅含 '\n' 文本的流发出 {trimLength:0,turnComplete:false}；消费者（开启聚合、回合 1 发送被 429、重试已挂载）在收到空白的回合 2 segment 后发出两张卡片，第二张把回合 1 的内容打成回合 2 的 ⏹️ 表头；将守卫改为 text.length>0 后回合 1 的重试正确发出 ✅ 表头。
修复：让 detach 守卫与文本无关——一旦 aggregation 的 turnComplete === true，对该 key 的任何后续派发都 detach（去掉 text.trim().length > 0），使纯空白 segment 落入空文本 park 分支。约束：保留 DingtalkAdapter.ts:2575 的 park 分支；Session.ts:9713/9908 每回合至多发一个终止标记。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. A completed aggregation now detaches on every later dispatch for the same key, including blank or whitespace events, so its retry keeps the original status and label. Regression coverage: keeps a completed delivery retry separate from later whitespace. The old guard fails this test; the DingTalk suite passes 317/317.
| parked?.turnComplete === true && | ||
| parked.retryTimer | ||
| ) { | ||
| parked = { resolvers: 0 }; |
There was a problem hiding this comment.
[Critical] R5-4: [certifies-falsely] [new-surface] Still standing (re-confirmed by probe this round). The turn-isolation replacement guard fires only while parked.retryTimer is armed. Once the resolution-retry timer has fired and its re-dispatched resolver is in flight (retryTimer cleared, resolve suspended on the named-session owner lock), a new turn's first segment for the same taskId is NOT isolated — it joins the old turn's pending entry, and the old turn's parked terminal then completes a mixed cross-turn card.
Witness:
probe (intact tree): after the retry fired, entry={resolvers:1,retryTimer:undefined,turnComplete:true,status:'completed',label:'Worker one'}
turn 2's first segment dispatched during the gated re-resolution joins it (resolvers=2);
on gate release a SINGLE mixed card is delivered: '## ✅ Agent · Worker one\n\nTurn one text.\n\nTurn two text.' and turn 2's own terminal is dropped
fix flip (drop && parked.retryTimer): the mixed card disappears — but that one-line patch fails two pre-existing tests, so the viable fix needs turn identity, not blanket relaxation
Fix: mark the pending entry when its retry re-dispatch is in flight (e.g. pending.retryInFlight = true in the timer callback, cleared when it settles) and widen the replacement condition to parked?.turnComplete === true && (parked.retryTimer || parked.retryInFlight), so the orphaned re-dispatch falls into the detached path.
The detached path is selected solely by the identity check this.pendingBackgroundResponseTerminals.get(key) === pending (DingtalkAdapter.ts:3040), and 'keeps a completed turn retry separate from the next turn' pins the order [turn two, turn one] — so the fix must replace the map entry, not mutate it. Please extend that test to dispatch turn 2's first segment while the retry's re-dispatched resolve is gated in flight, asserting two separate cards; removing the widened guard must red it. Note the production trigger is a timing window whose natural width (owner-lock hold duration) was not measured.
中文说明
[Critical] R5-4：[certifies-falsely] [new-surface] 仍然存在（本轮经探针重新确认）。回合隔离的 replacement 守卫只在 parked.retryTimer 挂载时生效。一旦解决重试计时器已触发、其重新派发的 resolver 正在飞行（retryTimer 已清除、resolve 挂起于命名会话 owner 锁），同一 taskId 的新回合首个 segment 就不会被隔离——它会并入旧回合的 pending 条目，旧回合 parked 的终止标记随后完成一张混合的跨回合卡片。
证据：探针显示重试触发后条目为 {resolvers:1,retryTimer:undefined,turnComplete:true,...}；在受门控的重新解决期间派发回合 2 的首个 segment 会并入（resolvers=2），门控释放后发出单张混合卡片 '## ✅ Agent · Worker one\n\nTurn one text.\n\nTurn two text.'，回合 2 自己的终止标记被丢弃。去掉 && parked.retryTimer 可消除混合卡片，但该一行补丁会使两个既有测试失败，故可行修复需要回合标识而非简单放宽守卫。
修复：在重试重新派发飞行时标记 pending 条目（如 pending.retryInFlight = true），并将 replacement 条件放宽为 parked?.turnComplete === true && (parked.retryTimer || parked.retryInFlight)。约束：detached 路径仅由 DingtalkAdapter.ts:3040 的身份检查选择，且既有测试钉住了 [turn two, turn one] 的顺序——修复必须替换 map 条目而非原地修改。请扩展该测试并做变异验证；生产触发为时间窗口，其自然宽度未测量。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Fixed in 872f5e4. Resolution retries now expose their in-flight state; a later turn replaces the map identity while the old pending entry stays tracked and is delivered through the detached path. Regression coverage: separates a resolution retry in flight from the next turn. The old behavior merged the turns; the DingTalk suite now passes 317/317.
| if (context.turnComplete) { | ||
| parked.turnComplete = true; | ||
| parked.status = context.status; |
There was a problem hiding this comment.
[Critical] R6-1: [certifies-falsely] [new-surface] Still standing (re-confirmed by probe this round). The empty-text parking branch sets parked.status/label/completionPartial unconditionally, so a later turn's whitespace-only/empty-text event overwrites the terminal fields of a parked entry that already holds an EARLIER turn's completed terminal with a resolution retry pending. When the retry fires it delivers the earlier turn's preserved text under the later turn's status/label.
Witness:
probe (whitespace-only turn-2 segment): parked after turn 1 = {status:'cancelled',label:'Worker one',completionPartial:true,retryTimer:armed}
after turn 2's '\n' segment + empty terminal, parked becomes {status:'completed',label:'Worker two',completionPartial:false}
the retry delivers '## ✅ Agent · Worker two\n\nTurn one text.' — turn 1's cancelled output certified as turn 2's success
fix flip (if context.turnComplete && !parked.turnComplete): card reads '## ⏹️ Agent · Worker one（部分）\n\nTurn one text.'
(The literal 'a turn whose only event is an empty-text terminal' is gated out by Session, but a whitespace-only segment — which R6-3 confirms Session emits — reaches this same branch.)
Fix: guard the park branch against overwriting an already-parked terminal — only set the terminal fields when !parked.turnComplete, or mirror the non-empty replacement guard for the empty-text case (replace the map entry with a fresh {resolvers:0} when parked.turnComplete && parked.retryTimer).
The fix must replace the map entry, not mutate the old object or its timer: the old retry closure depends on this.pendingBackgroundResponseTerminals.get(key) === pending becoming false (DingtalkAdapter.ts:3040) to take the detached-delivery branch. Please add a regression test (reject resolution once, dispatch turn-1 segment+terminal status A, then a second empty-text terminal status B within the retry window, advance 30s, assert the header uses status A); removing the guard must red it.
中文说明
[Critical] R6-1：[certifies-falsely] [new-surface] 仍然存在（本轮经探针重新确认）。空文本 park 分支无条件设置 parked.status/label/completionPartial，因此后续回合的纯空白/空文本事件会覆盖一个已持有更早回合完成终止标记（且有解决重试待处理）的 parked 条目的终端字段。重试触发时会以后续回合的 status/label 发出更早回合保留的文本。
证据：探针（空白的回合 2 segment）显示回合 1 后 parked={status:'cancelled',label:'Worker one',...}，回合 2 的 '\n' segment + 空终止标记后 parked 变为 {status:'completed',label:'Worker two',...}，重试发出 '## ✅ Agent · Worker two\n\nTurn one text.'——回合 1 被取消的输出被认证为回合 2 的成功；加入 !parked.turnComplete 守卫后卡片正确显示 ⏹️ Worker one（部分）。（字面的「仅含空终止标记的回合」被 Session 排除，但 R6-3 已确认 Session 会发出纯空白 segment，它会到达此分支。）
修复：仅在 !parked.turnComplete 时设置终端字段，或对空文本情形镜像非空 replacement 守卫（当 parked.turnComplete && parked.retryTimer 时用新的 {resolvers:0} 替换 map 条目）。约束：必须替换 map 条目而非修改旧对象或计时器（旧重试闭包依赖 DingtalkAdapter.ts:3040 的身份检查变为 false）。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| parked.resolvers === 0 && | ||
| !parked.retryTimer && | ||
| !parked.resolutionDropped | ||
| ) { | ||
| this.pendingBackgroundResponseTerminals.delete(key); |
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] Still standing (re-confirmed by probe this round). When the last in-flight target resolver fails AFTER the aggregation already exists, this finally deletes the pending entry — including a parked turn-complete terminal — without merging that terminal into the existing aggregation. The failing else branch transfers only resolutionDropped, so the turn's completion is silently discarded.
Witness:
probe (intact tree), two resolver gates: at terminal time cards=0 agg.turnComplete=undefined parked.turnComplete=true
after the last resolver rejects: pending.size=0 agg.turnComplete=undefined cards=0 (entry deleted, terminal lost)
after the 10-min bound: cards=['## ✅ Agent · Worker one（部分）\n\nFirst result.'] ('Second result.' never delivered); the lingering aggregation then mislabels the NEXT turn
fix flip (merge parked terminal in both resolver-failure else branches): pending.size=0 agg.turnComplete=true cards=1 immediately; all 312 pre-existing tests still pass
Fix: in both resolver-failure else branches (DingtalkAdapter.ts:2612-2619 and 2634-2641), when the failing resolver is the last in flight (parked.resolvers === 1) and an aggregation exists, transfer the parked terminal (turnComplete/status/label/completionPartial) into it and kick a flush when turnComplete became true.
'keeps a parked terminal until every overlapping resolver exits' asserts a single card, so the transfer must fire only when the failing resolver is the last in flight, never while others await. Please add a collocated test (two gated resolvers, first resolves, terminal dispatched while the second gate is in flight, second rejects — assert the completion card is sent WITHOUT advancing timers); removing the transfer must red it.
中文说明
[Critical] R6-2：[certifies-falsely] [new-surface] 仍然存在（本轮经探针重新确认）。当最后一个飞行中的目标 resolver 在 aggregation 已存在之后失败时，此 finally 会删除 pending 条目——连同其 parked 的回合完成终止标记——而没有把该终止标记并入已存在的 aggregation。失败的 else 分支只转移 resolutionDropped，于是回合的完成被静默丢弃。
证据：探针（两个 resolver 门控）显示终止时刻 cards=0、agg.turnComplete=undefined、parked.turnComplete=true；最后一个 resolver 拒绝后 pending.size=0、cards=0（条目被删、终止标记丢失）；10 分钟上限后才发出 '## ✅ Agent · Worker one（部分）'（'Second result.' 从未发出），且残留的 aggregation 会污染下一回合。在两个 resolver 失败 else 分支并入 parked 终止标记后，cards 立即为 1，且 312 个既有测试全部通过。
修复：在两个 resolver 失败 else 分支（DingtalkAdapter.ts:2612-2619 与 2634-2641）中，当失败的 resolver 是最后一个飞行中的（parked.resolvers === 1）且 aggregation 存在时，把 parked 终止标记并入并触发 flush。约束：'keeps a parked terminal until every overlapping resolver exits' 断言只发一张卡片，故转移只能在该 resolver 是最后一个时触发。请补充同位测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| } else if (parked?.turnComplete && parked.resolvers === 0) { | ||
| current.turnComplete = true; | ||
| current.status = parked.status ?? current.status; |
There was a problem hiding this comment.
[Critical] R4-4: [certifies-falsely] [new-surface] Still standing (re-confirmed by probe this round). A pendingBackgroundResponseTerminals entry whose resolution retries are exhausted (resolutionDropped=true, no retryTimer) is never removed by any path — both finally cleanups gate deletion on !resolutionDropped, and drainBackgroundResponseAggregations sweeps only the two aggregation maps. The entry leaks for the channel's lifetime, and its stale parked terminal is consumed by the NEXT turn's first segment through this apply, completing that turn prematurely under the previous turn's status/label.
Witness:
probe (intact tree): after give-up the pending map holds {resolvers:0,retryAttempts:3,resolutionDropped:true,turnComplete:true,status:'completed',label:'Worker one'} size=1
after onSessionDied('session-1'): size=1 (the drain swept aggregations, not the pending map)
with resolution then succeeding, the next turn's first segment flushes immediately as '## ✅ Agent · Worker one（部分）\n\nNew turn text.' under the PRIOR turn's header
fix flip: drain-sweep patch -> size=0; replacement-guard patch -> next turn buffered in its own aggregation
Fix: clear the stale terminal fields when the give-up branch latches resolutionDropped with no resolver/timer, and have drainBackgroundResponseAggregations also sweep pendingBackgroundResponseTerminals for the drained session (clearTimeout(retryTimer), delete(key)).
resolutionDropped must stay on the entry for transfer to a later aggregation of the same turn (DingtalkAdapter.ts:2658-2661 and :2693 label a card （部分） when a sibling segment was dropped), so cleanup clears only the terminal fields at give-up and deletes only on drain. Please add a regression test (reject resolveBackgroundResponseDelivery 3x, park an empty-text turnComplete terminal, then resolve and dispatch a new same-key segment — assert pushProactive is NOT called immediately, and that onSessionDied empties pendingBackgroundResponseTerminals); removing the sweep/clear must red it.
中文说明
[Critical] R4-4：[certifies-falsely] [new-surface] 仍然存在（本轮经探针重新确认）。解决重试耗尽（resolutionDropped=true、无 retryTimer）的 pendingBackgroundResponseTerminals 条目永远不会被任何路径移除——两个 finally 清理都以 !resolutionDropped 为删除条件，而 drainBackgroundResponseAggregations 只清扫两个 aggregation 集合。该条目在 channel 生命周期内一直泄漏，其陈旧的 parked terminal 会被下一回合的首个 segment 经此 apply 消费，从而以上一回合的 status/label 提前完成新回合。
证据：探针显示 give-up 后 pending map 仍持有该条目（size=1），onSessionDied 后仍为 1（drain 只清扫了 aggregation）；当后续解决成功时，新回合首个 segment 会以上一回合表头 ## ✅ Agent · Worker one（部分） 立即刷出。修复（drain 清扫 pending map）后 size=0。
修复：在 give-up 分支置位 resolutionDropped 且无 resolver/timer 时清除陈旧终端字段，并让 drainBackgroundResponseAggregations 一并清扫被 drain 会话的 pendingBackgroundResponseTerminals。约束：必须保留 resolutionDropped 以便转移给同回合后续 aggregation（DingtalkAdapter.ts:2658-2661 与 :2693）。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const delivery = aggregation.delivery; | ||
| if (!delivery || aggregation.flushing) return; |
There was a problem hiding this comment.
[Critical] R6-4: [certifies-falsely] [new-surface] Still standing (confirmed by code read this round). refreshBackgroundResponseDelivery is skipped while a flush is in flight (this aggregation.flushing guard) and is never re-run inside flushBackgroundResponseAggregation — its only call site is dispatch's turnComplete path. When a turn completes mid-flush and that flush then fails, the retry reuses the stored proactivePlan/replyPlan/preparedReplyBody with the frozen pre-completion header, so the retried content card reports ## ⏹️ Agent · …（部分） for a turn that completed.
Witness:
code read: flushBackgroundResponseAggregation (2717-2870) contains NO refreshBackgroundResponseDelivery call;
the retry branches at 2765-2776 reuse delivery.proactivePlan/replyPlan (frozen chunks) while the recomputed body (2760) is used only on the non-plan branches
carried round-6 probe: sends=[attempt1 ⏹️（部分） rejected, retry ⏹️（部分） DELIVERED WRONG, header-only ✅ correction];
with refresh re-run after the flush entry guard the retry delivers '## ✅ Agent · Worker one\n\nFirst result.' and the suite stays 308/308
Fix: call this.refreshBackgroundResponseDelivery(aggregation) inside flushBackgroundResponseAggregation right after the entry guard (before a stored delivery is reused); refresh is idempotent and the entry guard already returns early when flushing, so mid-send protection is preserved.
Keep the already-sent-chunk guard if (plan && plan.nextChunk > 0) return; (DingtalkAdapter.ts:2914) intact — chunks already delivered cannot be rewritten, and 'retries only the unsent proactive chunks of an aggregation' (DingtalkAdapter.test.ts:10271) relies on resume-from-nextChunk semantics. Please add a regression test (dispatch a running segment, advance the 10-min bounded wait, hold pushProactive on a deferred promise, dispatch the completion terminal, reject the deferred send, advance the 30s retry; assert the retried text contains ## ✅ Agent · and not （部分）); without the refresh re-run it reads ## ⏹️ Agent · …（部分）.
中文说明
[Critical] R6-4：[certifies-falsely] [new-surface] 仍然存在（本轮经代码阅读确认）。refreshBackgroundResponseDelivery 在 flush 飞行中被本 aggregation.flushing 守卫跳过，且永远不会在 flushBackgroundResponseAggregation 内重新运行——它唯一的调用点是 dispatch 的 turnComplete 路径。当回合在 flush 途中完成、而该 flush 随后失败时，重试会复用携带冻结前完成表头的已存 proactivePlan/replyPlan/preparedReplyBody，于是重投的内容卡对已完成的回合显示 ## ⏹️ Agent · …（部分）。
证据：代码阅读确认 flushBackgroundResponseAggregation（2717-2870）不含任何 refreshBackgroundResponseDelivery 调用；2765-2776 的重试分支复用冻结的 plan，而重新计算的 body（2760）只在非 plan 分支使用。沿用的第 6 轮探针显示重投发出错误的 ⏹️（部分）卡片；在 flush 入口守卫后重跑 refresh 后，重投正确发出 ✅ 卡片且测试套件 308/308 通过。
修复：在 flushBackgroundResponseAggregation 入口守卫之后、复用已存投递之前调用 this.refreshBackgroundResponseDelivery(aggregation)（refresh 幂等，入口守卫在 flushing 时已提前返回）。约束：保持 DingtalkAdapter.ts:2914 的 if (plan && plan.nextChunk > 0) return; 原样（已送达分片不可改写）。请补充回归测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.23.0)
wenshao
left a comment
wenshao
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 reviewed: verifier-incidental finding L6-3 (cross-turn re-attribution at DingtalkAdapter.ts:2676) — arrived after the reverse-audit round cap stopped the loop, so no verifier ruled on it.
Not explored to full depth (tool budget reached): chunk 11: could not execute the three touched test files — npx vitest run src/agents/background-tasks.test.ts was stopped by the unit-test build guard ( packages/core/d…; "agent reverse-audit (round 1)": did not trace packages/acp-bridge/src/bridge.ts event recording/forwarding to confirm an empty-text agent_message_chunk carrying _meta.backgroundTask.turnC…; chunk 2: I did not execute ChannelBase.test.ts , DaemonChannelBridge.test.ts or DingtalkAdapter.test.ts ; the pass/fail reasoning above is from reading the code, not….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/channels/dingtalk/src/DingtalkAdapter.ts:2176 — [probe] Critical [fails-closed] [new-surface] The disconnect/shutdown drain is fire-and-forget while…packages/channels/base/src/ChannelBase.ts:95 — [probe] BackgroundResponseDeliveryTarget, the type of the two new…packages/channels/base/src/DaemonChannelBridge.ts:1021 — [probe] The two bridges added by the same diff admit the new…packages/channels/dingtalk/src/DingtalkAdapter.test.ts:150 — [probe] The suite's mocked ChannelBase cannot reach the…packages/channels/dingtalk/src/DingtalkAdapter.test.ts:7135 — [probe] The 44 new aggregation tests were inserted mid-describe,…packages/channels/dingtalk/src/DingtalkAdapter.test.ts:7417 — [probe] The test named for the $& hazard mocks pushProactive, so…packages/channels/dingtalk/src/DingtalkAdapter.ts:927 — [probe] retryable is only ever populated from a token error, so…packages/channels/dingtalk/src/DingtalkAdapter.ts:2308 — [probe] The retiring drain's per-session scoping is unpinned, so…packages/channels/dingtalk/src/DingtalkAdapter.ts:2537 — [probe] The kind !== 'agent' escape hatch is exercised by no…packages/channels/dingtalk/src/DingtalkAdapter.ts:2560 — [probe] The next turn's trim-empty dispatch also merges into a…packages/channels/dingtalk/src/DingtalkAdapter.ts:2622 — [probe] The 19-line resolution give-up block is duplicated…packages/channels/dingtalk/src/DingtalkAdapter.ts:2644 — [probe] The only branch that cancels an armed resolution retry is…packages/channels/dingtalk/src/DingtalkAdapter.ts:2767 — [probe] The flush's retry branch bypasses the…packages/channels/dingtalk/src/DingtalkAdapter.ts:2778 — [probe] The deferred flush sends to the target cached at resolve…packages/channels/dingtalk/src/DingtalkAdapter.ts:2820 — [probe] A content-free completion card is emitted even when the…packages/channels/dingtalk/src/DingtalkAdapter.ts:2844 — [probe] ReplyTextDeliveryError carries no retryable at all, so…packages/channels/dingtalk/src/DingtalkAdapter.ts:2913 — [probe] The flushing guard skips the header refresh in exactly the…packages/channels/dingtalk/src/DingtalkAdapter.ts:2921 — [probe] The refresh path's plan.title update is unpinned, so the…packages/channels/dingtalk/src/DingtalkAdapter.ts:2937 — [probe] No test reads detachedBackgroundResponseAggregations, so…packages/channels/dingtalk/src/DingtalkAdapter.ts:2993 — [probe] A second segment failing resolution while the single retry…- …and 11 more (see the run report)
Convergence: round 6 posted 4 inline comment(s), 4 of them reported for the first time. Findings keep coming back to the same files: packages/channels/dingtalk/src/DingtalkAdapter.ts (findings in rounds 4, 5; 4 more now). (Evidence: the previous round was recovered from a marker this account did not post, so those rounds may not be this account's own.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：verifier-incidental finding L6-3 (cross-turn re-attribution at DingtalkAdapter.ts:2676) — arrived after the reverse-audit round cap stopped the loop, so no verifier ruled on it.
未探索到全部深度（达到工具调用预算）：chunk 11：could not execute the three touched test files — npx vitest run src/agents/background-tasks.test.ts was stopped by the unit-test build guard ( packages/core/d…；"agent reverse-audit (round 1)"：did not trace packages/acp-bridge/src/bridge.ts event recording/forwarding to confirm an empty-text agent_message_chunk carrying _meta.backgroundTask.turnC…；chunk 2：I did not execute ChannelBase.test.ts , DaemonChannelBridge.test.ts or DingtalkAdapter.test.ts ; the pass/fail reasoning above is from reading the code, not…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 31 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 4 条行内评论，其中 4 条是首次提出。发现反复回到同一批文件：packages/channels/dingtalk/src/DingtalkAdapter.ts（第 4、5 轮已出过发现，本轮又有 4 条）。（证据说明：上一轮的数据来自并非本账号发布的标记，上述轮次可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.23.0)
| // chatId is a conversationId — resolve to the latest sessionWebhook. | ||
| const webhook = this.webhooks.get(chatId); |
There was a problem hiding this comment.
[Critical] R6-1: [certifies-falsely] [new-surface] The new failOnHttpError switch — the signal the aggregation flush relies on to make a failed background delivery retryable — only converts HTTP outcomes into errors. sendReply's "no webhook for this chatId" early return two lines below still resolves successfully, so the flush records a wholly undelivered turn as delivered / completionDelivered, removes the aggregation entry, arms no retry, and never sends the completion card.
With aggregateBackgroundAgentResponses: true and a DM target (supportsProactiveTarget requires isGroup === true), the flush delivers through deliverBackgroundReply → sendResponseMessage → sendReply, whose first act is this lookup-and-return. this.webhooks is in-memory and only ever set from an inbound message, so it is empty for any chat that has not messaged since the worker started — reachable when createdRouter.restoreRoutes() repopulates routes from disk after a restart (dispatchBackgroundResponse gates only on route / channel / kind, never on webhook presence), or when a DM was ingested without a conversationId, making the chatId the webhook URL while nothing was stored under it. Nothing upstream rescues it: prepareReplyOutput's preflight throw runs only inside deliverFiles' per-file loop, so a text-only body — the common case — never reaches it. The flush then takes its success branch, and three things go wrong at once: up to ten minutes of buffered turn text is lost with only a generic stderr line, and the three-attempt retry budget plus dropped bookkeeping this PR added for exactly this purpose never engage; owesTerminalBackgroundResponseCard becomes permanently false because completionDelivered is true, so the promised ## ✅ Agent · … card is never sent; and if the turn is still open, isPartialBackgroundResponseDelivery returns true from delivered === true, so every later card is stamped （部分） — telling the user earlier text went out when nothing was ever sent. deliverReplyText's own leading if (!webhook) return; has the same hole on the captured-plan retry branch.
Throw when the caller asked for failures, in both silent-return sites, reusing the message already present at DingtalkAdapter.ts:1511:
const webhook = this.webhooks.get(chatId);
if (!webhook) {
process.stderr.write(
`[DingTalk:${this.name}] No webhook for chatId ${chatId}, cannot send.\n`,
);
if (failOnHttpError) {
throw new Error('DingTalk session webhook unavailable');
}
return;
}with the equivalent guard on deliverReplyText's leading if (!webhook) return;, which already receives failOnHttpError. Keep the log-and-return default for in-turn sends, which have no retry machinery to hand the error to.
Witness:
probe (DM target isGroup:false, no webhooks entry, aggregation on, one segment + empty turnComplete terminal)
PRISTINE PR
fetchCalls=0 urls=[] <- zero webhook POSTs
aggSize=0 entry={"present":false,"retryArmed":false} <- entry deleted, no retry armed
stderr=["[DingTalk:test-dingtalk] No webhook for chatId dm-cid, cannot send.\n"]
after90s fetchCalls=0 aggSize=0
WITH THE FIX (throw when failOnHttpError && !webhook)
aggSize=1 entry={"present":true,"parts":[],"retryArmed":true}
stderr=[..., "background response delivery failed (attempt 1): DingTalk session webhook unavailable\n"]
after90s fetchCalls=0 aggSize=0 <- gives up after the 3-attempt budget
Tests 314 passed (314) <- whole dingtalk suite still green
false-certification arm (open turn, bounded-wait flush with no webhook; webhook then repopulated)
afterBoundedWait bodies=[] delivered=true retained=true
final bodies=["## ✅ Agent · Worker one（部分）\n\nLater segment.","## ✅ Agent · Worker one"]
<- 'Lost segment.' was never sent anywhere, yet the next card is stamped （部分）
The guard must test the same lookup sendReply uses — this.webhooks.get(chatId) — not resolveSessionWebhook(chatId), which additionally returns undefined for any stored URL that is not https: on a ROBOT_MESSAGE_HOSTS host (DingtalkAdapter.ts:1384-1397); a resolveSessionWebhook-based guard would abort sends that sendReply would otherwise attempt successfully. Note also that the throw now enters the bounded retry budget at :2838-2841, so it must stay a plain Error rather than a ProactiveTextDeliveryError with retryable === false, unless a permanent give-up is intended. Please add a case beside the aggregation tests: a target with isGroup: false and no inbound message for that chatId (so no webhooks entry), one agent segment plus its empty-text turnComplete terminal, then drive the bounded-wait flush and assert the turn was not accepted as sent — no webhook POST happened, backgroundResponseAggregations.get(key)?.delivered is not true, and a retry timer was armed. Removing the new throw must turn that red; today no test in the file drives the flush with an uncached chatId.
中文说明
新增的 failOnHttpError 开关是聚合冲刷用来「让投递失败可重试」的信号，但它只把 HTTP 结果转成错误。下方两行处 sendReply 的「该 chatId 没有 webhook」提前返回仍然正常 resolve，于是冲刷会把一个完全没投出去的回合记为 delivered / completionDelivered，删除聚合表项、不武装任何重试，也永远不发完成卡。
当 aggregateBackgroundAgentResponses: true 且目标为私聊（supportsProactiveTarget 要求 isGroup === true）时，冲刷经 deliverBackgroundReply → sendResponseMessage → sendReply 投递，而 sendReply 的第一步就是这里的查表返回。this.webhooks 是内存态、只在收到入站消息时才 set，因此对自 worker 启动后没有发过消息的会话它是空的——两种可达状态：worker 重启后 createdRouter.restoreRoutes() 从磁盘恢复路由（而 dispatchBackgroundResponse 只校验路由/频道/kind，从不校验 webhook 是否存在）；或私聊入站时没有 conversationId，使 chatId 变成 webhook URL，而其下没有存任何条目。上游也救不了：prepareReplyOutput 的预检抛错只在 deliverFiles 的逐文件循环内运行，因此纯文本正文（常见情况）根本走不到它。于是冲刷进入成功分支，同时出现三个错误结果：最多十分钟的缓冲正文丢失、只留下一条泛化的 stderr，而本 PR 专门为此加上的三次重试预算与 dropped 记账完全没有启用；由于 completionDelivered 为真，owesTerminalBackgroundResponseCard 永久为假，承诺的 ## ✅ Agent · … 卡片永远不会发出；若回合仍未结束，isPartialBackgroundResponseDelivery 会因 delivered === true 返回真，导致后续每张卡都被标上 （部分）——告诉用户此前已有文本发出，而实际上什么都没发出去。deliverReplyText 自己的开头 if (!webhook) return; 在「已捕获计划」的重试分支上有同样的漏洞。
修复：在两个静默返回点上当调用方要求严格失败时抛错（复用 DingtalkAdapter.ts:1511 已有的错误文案），并保留回合内普通发送的「记日志后返回」默认行为——那些路径没有重试机制可以接手这个错误。
约束：该判断必须使用与 sendReply 相同的查表方式 this.webhooks.get(chatId)，而不是 resolveSessionWebhook(chatId)——后者对任何不是 ROBOT_MESSAGE_HOSTS 上 https: 的已存 URL 也会返回 undefined（DingtalkAdapter.ts:1384-1397），基于它的判断会中止 sendReply 本来能成功尝试的发送。另外该抛错会进入 :2838-2841 的有界重试预算，因此它必须保持为普通 Error，而不是带 retryable === false 的 ProactiveTextDeliveryError，除非确实希望永久放弃。
验收：请在聚合测试旁补一条用例——目标 isGroup: false 且该 chatId 没有入站消息（因此 webhooks 无条目），发送一个 agent 分段加其空文本 turnComplete 终止标记，然后驱动等待上限冲刷，断言该回合没有被记为已投递：没有 webhook POST、backgroundResponseAggregations.get(key)?.delivered 不为 true、并且武装了重试定时器。移除新增的抛错后该断言必须变红；目前文件中没有任何测试会用未缓存的 chatId 驱动冲刷。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.23.0)
| if (current?.turnComplete === true && text.trim().length > 0) { | ||
| this.detachedBackgroundResponseAggregations.add(current); | ||
| this.backgroundResponseAggregations.delete(key); | ||
| current = undefined; | ||
| } |
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] The new-turn detach is gated on text.trim().length > 0, so a dispatch whose text is truthy but trim-empty is not treated as a new turn: it falls through to the merge below and overwrites the previous turn's still-retained, retry-awaiting aggregation — its status, label, completionPartial and its pending delivery, including the chunk-0 header that refreshBackgroundResponseDelivery then rewrites.
The aggregation key is JSON.stringify([sessionId, context.taskId]) with taskId: meta.agentId, i.e. per-agent, so every turn of one background agent shares a key. Turn 1 completes with text and its first proactive chunk fails — a DingTalk 429, exactly the burst the retry exists for — so the entry stays in the map with turnComplete: true, a captured delivery at nextChunk === 0 and a 30s retry armed. Turn 2 then dispatches text that is truthy but trim-empty. That is reachable, not hypothetical: responseText accumulates behind only if (!part.text) continue; (Session.ts:9816), so an iteration whose only non-thought text is '\n' or ' ' yields responseText.length > 0 with responseText.trim().length === 0, and both relays gate on truthiness (text || / content.text ||) while this detach gate trims. Turn 2's dispatch therefore merges into turn 1's entry, and two things go wrong: the retry re-posts turn 1's buffered output under turn 2's label and status (a cancelled turn's ## ⏹️ Agent · …（部分） over text that completed, or a different worker's name over the first worker's report), and turn 2's own completion is consumed by the same entry — completionDelivered = true makes owesTerminalBackgroundResponseCard false and the entry is removed — so no card ever reports turn 2's real status. An in-flight flush's bookkeeping is corrupted too, since completionDelivered is computed from the mutated delivery.partial.
Gate the merge on turn identity rather than on text, so any further dispatch on a completed entry starts a new one:
| if (current?.turnComplete === true && text.trim().length > 0) { | |
| this.detachedBackgroundResponseAggregations.add(current); | |
| this.backgroundResponseAggregations.delete(key); | |
| current = undefined; | |
| } | |
| if (current?.turnComplete === true) { | |
| this.detachedBackgroundResponseAggregations.add(current); | |
| this.backgroundResponseAggregations.delete(key); | |
| current = undefined; | |
| } |
Equivalently, refuse to overwrite status / label / completionPartial / delivery on an entry whose turnComplete is already true.
Witness:
probe (turn 1 'Working.' completed with the first proactive chunk throwing, then a trim-empty next-turn dispatch)
PRISTINE PR
turn1 retained=true turnComplete=true nextChunk=0 retryArmed=true
afterMarker texts=["## ✅ Agent · Worker one\n\nWorking.",
"## ⏹️ Agent · Worker one（部分）\n\nWorking."] aggSize=0
<- turn 1's COMPLETED output re-posted under a cancelled/（部分）header; entry consumed
whitespace-segment variant ('\n' as turn 2's first text):
afterWsSegment texts=["## ✅ Agent · Worker one\n\nWorking.",
"## ⏹️ Agent · Worker two\n\nWorking."] aggSize=0 detached=0
<- turn 1's output re-posted under a DIFFERENT agent's label and status, immediately, no retry needed
WITH THE FIX (drop the `text.trim().length > 0` conjunct)
afterMarker texts=["## ✅ Agent · Worker one\n\nWorking."]
afterRetry texts=["## ✅ Agent · Worker one\n\nWorking.","## ✅ Agent · Worker one\n\nWorking."]
whitespace variant: aggSize=0 detached=1 <- retry obligation preserved
Tests 314 passed (314) — including `keeps a completed turn retry separate from the next turn`,
`isolates a restarted task turn from the prior turn retry`,
`isolates a single-segment restarted turn from the prior retry`
Two things the fix has to keep working. flushBackgroundResponseAggregation admits an entry only when it is still the map's value or a member of the detached set (DingtalkAdapter.ts:2718-2722), and owesTerminalBackgroundResponseCard requires aggregation.delivered === true && aggregation.completionDelivered !== true (:2899-2900) — which is what pins the header-only completion card asserted at DingtalkAdapter.test.ts:8667-8670; detaching earlier must leave the detached entry's own retry and completion obligation running, or that card disappears. And a measured side effect worth an explicit decision: with the conjunct dropped, a turn whose only dispatch is an empty terminal now detaches and then falls to super.dispatchBackgroundResponse, whose empty-text guard returns — so that turn produces no completion card of its own. All 314 tests still pass, but whether such a turn owes a card is the author's call. Please add a regression: aggregateBackgroundAgentResponses: true with a seeded group target, stubProactiveFetch failing only the first send (429), dispatch turn 1 ('Working.', status: 'completed', turnComplete: true, label: 'Worker one'), then dispatch turn 2's marker ('\n', status: 'cancelled', turnComplete: true, partial: true) and assert the second send's text still begins ## ✅ Agent · Worker one — removing the detach guard turns it into ## ⏹️ Agent · Worker one（部分）, i.e. red.
中文说明
新回合的 detach 以 text.trim().length > 0 为条件，因此一次「非空但 trim 后为空」的下发不会被当作新回合：它会落到下方的合并逻辑，覆盖上一个回合仍然保留、正在等待重试的聚合——包括其 status、label、completionPartial 以及待发的 delivery，连 refreshBackgroundResponseDelivery 随后重写的 chunk 0 表头也一起被改掉。
聚合键是 JSON.stringify([sessionId, context.taskId])，其中 taskId: meta.agentId，即按 agent 维度，所以同一个后台 agent 的每个回合共用一个键。回合 1 带文本完成，其首个主动推送分片失败（钉钉 429，正是重试机制要应对的突发），于是表项以 turnComplete: true、nextChunk === 0 的已捕获 delivery 和武装好的 30s 重试留在 map 中。随后回合 2 下发了一段「非空但 trim 后为空」的文本。这是可达的、并非假设：responseText 只在 if (!part.text) continue;（Session.ts:9816）之后累加，因此某轮迭代如果唯一的非思考文本是 '\n' 或 ' '，就会得到 responseText.length > 0 而 responseText.trim().length === 0；两条中继链路都按真值判断（text || / content.text ||），而这里的 detach 判断却做了 trim。于是回合 2 的下发合并进回合 1 的表项，出现两个错误结果：重试会把回合 1 的缓冲输出以回合 2 的 label 与 status 重新投出（把已完成的文本套上被取消回合的 ## ⏹️ Agent · …（部分），或把第一个 worker 的报告挂在另一个 worker 的名字下）；同时回合 2 自己的完成状态被同一表项消费掉——completionDelivered = true 使 owesTerminalBackgroundResponseCard 为假并删除表项——因此再也没有卡片报告回合 2 的真实状态。在飞的冲刷记账同样被破坏，因为 completionDelivered 是依据被改写后的 delivery.partial 计算的。
修复：让合并以「回合身份」而非文本为判据，使已完成表项上的任何后续下发都另起一条。等价做法是：当表项的 turnComplete 已为真时，拒绝覆盖其 status / label / completionPartial / delivery。
修复必须保住两点：一是 flushBackgroundResponseAggregation 只在表项仍是 map 的值或属于 detached 集合时才接受它（DingtalkAdapter.ts:2718-2722），而 owesTerminalBackgroundResponseCard 要求 aggregation.delivered === true && aggregation.completionDelivered !== true（:2899-2900）——这正是 DingtalkAdapter.test.ts:8667-8670 所钉住的仅表头完成卡，因此提前 detach 必须让 detached 表项自己的重试与完成义务继续运行，否则那张卡会消失；二是一个实测到的副作用，需要明确决策：去掉该条件后，若某回合唯一的下发就是空终止标记，它会先 detach、再落到 super.dispatchBackgroundResponse 的空文本提前返回，于是该回合不会产出自己的完成卡。314 项测试全绿，但这种回合是否应当出卡由作者决定。
验收：请补一条回归——aggregateBackgroundAgentResponses: true 并种入群聊目标，stubProactiveFetch 只让第一次发送失败（429），下发回合 1（'Working.'、status: 'completed'、turnComplete: true、label: 'Worker one'），再下发回合 2 的标记（'\n'、status: 'cancelled'、turnComplete: true、partial: true），断言第二次发送的正文仍以 ## ✅ Agent · Worker one 开头；移除 detach 判据后它会变成 ## ⏹️ Agent · Worker one（部分），即变红。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.23.0)
| parked.resolvers--; | ||
| if ( | ||
| this.pendingBackgroundResponseTerminals.get(key) === parked && | ||
| parked.resolvers === 0 && | ||
| !parked.retryTimer && | ||
| !parked.resolutionDropped | ||
| ) { |
There was a problem hiding this comment.
[Critical] R5-3: (fix-induced) [certifies-falsely] [new-surface] The round-5 fix for R5-3 propagates resolutionDropped onto an already-created aggregation and then clears it on the parked record — which is exactly what opens this finally's delete gate. When an overlapping resolver exits through a failure branch after another resolver already created the aggregation, that branch returns before the parked-terminal merge below, and this finally deletes the parked entry while it still holds the turn's turnComplete / status / label.
With aggregation on and named sessions, segment A suspends in resolveBackgroundResponseDelivery (it awaits namedSessions.resolvePresentation, which can contend on the owner lock, so the window is not a single microtask); the turn's empty-text terminal marker parks onto pending; segment B arrives and resolvers becomes 2. A resolves first and creates the aggregation, but its own merge is skipped because parked.resolvers is still 1 — so A only arms the ten-minute timer. B's resolution then fails: because the aggregation now exists, B takes the else branch, sets existing.resolutionDropped = true, clears parked.resolutionDropped = undefined, and returns before reaching the merge. This finally then finds every delete gate satisfied and removes the entry with turnComplete: true still inside it. The aggregation is left permanently open: the ten-minute bounded wait eventually delivers the parts as ## ⏹️ Agent · X（部分） using the segment's status: 'running' instead of the terminal's ✅ … completed, the completion card is never sent (owesTerminalBackgroundResponseCard requires turnComplete === true, which is never set), and because removeBackgroundResponseAggregation is only reached under retiring || turnComplete, the entry stays in the map re-arming a fresh ten-minute timer indefinitely — 40 minutes observed, still present.
Do not delete a pending entry that still holds an unapplied terminal. Either add parked.turnComplete !== true to this delete gate, or — better, so the entry cannot outlive the turn — extract the merge below into a helper and call it from both failure branches inside if (existing) { … } before the throw error; / return;, then kick the flush.
Witness:
probe (two gated resolvers; A resolves, the blank terminal parks while B is in flight, then B rejects)
BEFORE (PR code)
parked after terminal: {"resolvers":1,"turnComplete":true,"status":"completed","label":"Worker one",...}
after A resolves: agg.turnComplete=undefined parked.resolvers=1 pushes=0 <- merge skipped
after B REJECTS: pending.size=0 agg={"status":"running","parts":["a"],"resolutionDropped":true,"timeoutArmed":true} pushes=[]
after 1st bounded wait: pushes=["## ⏹️ Agent · Worker one（部分）\n\na"] mapStillHoldsEntry=true
after 3 more waits (40 min total): pushes=1 mapStillHoldsEntry=true
AFTER (apply the parked terminal to the already-verified aggregation in both give-up branches)
after B REJECTS: pending.size=0 agg={"turnComplete":true,"status":"completed",...}
pushes=["## ✅ Agent · Worker one（部分）\n\na"] mapStillHoldsEntry=false
Two measured caveats on the fix. The bail-out itself must stay — if (!delivery || this.router.getTarget(sessionId) !== delivery.target) { return; } (packages/channels/base/src/ChannelBase.ts:551-553) is the staleness rule that forbids delivering to an unverified target, so the fix may only move the terminal marker onto the already-verified aggregation and must never send B's unverified text. And an unconditional flush kick there turns marks an aggregation partial when an overlapping resolver fails (DingtalkAdapter.test.ts:8290) red — expected '## ⏹️ …（部分）' to be '## ✅ …（部分）', 307 passed | 1 failed — because it fires before that test's successful resolver applies its own merge, so apply the terminal without kicking a flush ahead of an in-flight successful resolver. Please add the mirror of keeps a parked terminal until every overlapping resolver exits (:8216) with the gates reversed: the first resolver resolves, the blank terminal marker is dispatched while the second gate is pending, then the second gate rejects; assert pushProactive receives a ## ✅ Agent · … card without advancing the ten-minute timer, and that advancing another ten minutes produces no further send. The existing :8216, :8290 and waits for every overlapping resolver before completing a turn (:8140, all resolvers succeed) all stay green with the defect present, so this ordering is the one that must go red.
中文说明
R5-3 的第 5 轮修复会把 resolutionDropped 传播到已创建的聚合上，然后清除 parked 记录上的该标志——而这恰恰打开了本 finally 的删除条件。当某个重叠 resolver 在另一个 resolver 已创建聚合之后从失败分支退出时，该分支会在下方的「停放终止标记合并」之前返回，于是这个 finally 会删除仍然持有本回合 turnComplete / status / label 的 parked 表项。
开启聚合与命名会话时：分段 A 挂起在 resolveBackgroundResponseDelivery（它会 await namedSessions.resolvePresentation，可能争用 owner 锁，因此窗口不止一个 microtask）；本回合的空文本终止标记停放到 pending 上；分段 B 到达，resolvers 变为 2。A 先解析成功并创建聚合，但它自己的合并被跳过（此时 parked.resolvers 仍为 1），所以 A 只武装了十分钟定时器。随后 B 的解析失败：由于聚合已存在，B 走 else 分支，置 existing.resolutionDropped = true、清除 parked.resolutionDropped = undefined，然后在到达合并逻辑之前返回。于是本 finally 发现所有删除条件都满足，把仍带着 turnComplete: true 的表项删掉。该聚合因此永久处于未终止状态：十分钟等待上限最终会以分段自身的 status: 'running' 把内容投成 ## ⏹️ Agent · X（部分），而不是终止标记带来的 ✅ … completed；完成卡永远不会发出（owesTerminalBackgroundResponseCard 要求 turnComplete === true，而它从未被置位）；并且由于 removeBackgroundResponseAggregation 只在 retiring || turnComplete 时才到达，该表项会留在 map 中不断重新武装新的十分钟定时器——实测 40 分钟后依然存在。
修复：不要删除仍持有未套用终止标记的 pending 表项。可以给该删除条件加上 parked.turnComplete !== true；更好的做法是把下方的合并逻辑抽成辅助函数，在两个失败分支的 if (existing) { … } 内、throw error; / return; 之前调用它，再触发冲刷。
关于修复的两点实测注意：一是这里的提前返回本身必须保留——if (!delivery || this.router.getTarget(sessionId) !== delivery.target) { return; }（packages/channels/base/src/ChannelBase.ts:551-553）是禁止向未校验目标投递的过期性规则，因此修复只能把终止标记移到已校验的聚合上，绝不能发送 B 的未校验文本；二是在该处无条件触发冲刷会让 marks an aggregation partial when an overlapping resolver fails（DingtalkAdapter.test.ts:8290）变红（expected '## ⏹️ …（部分）' to be '## ✅ …（部分）'，307 passed | 1 failed），因为它会抢在该用例中成功的 resolver 套用自身合并之前执行——所以只套用终止标记，不要抢在在飞的成功 resolver 之前冲刷。
验收：请补一条与 keeps a parked terminal until every overlapping resolver exits（:8216）门控相反的用例——第一个 resolver 成功，空终止标记在第二个门控仍挂起时下发，随后第二个门控拒绝；断言 pushProactive 在不前进十分钟定时器的情况下就收到 ## ✅ Agent · … 卡片，并且再前进十分钟不再产生新的发送。现有的 :8216、:8290 与 waits for every overlapping resolver before completing a turn（:8140，全部 resolver 成功）在该缺陷存在时都是绿的，因此只有这个顺序才是必须变红的那一条。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.23.0)
| pending.retryAttempts = (pending.retryAttempts ?? 0) + 1; | ||
| if (pending.retryAttempts >= BACKGROUND_RESPONSE_AGGREGATION_MAX_RETRIES) { | ||
| pending.resolutionDropped = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Critical] R4-4: (fix-induced) [certifies-falsely] [new-surface] The round-4 fix for R4-4 made an exhausted pendingBackgroundResponseTerminals entry survive until a later aggregation consumes its resolutionDropped flag — but both delete sites are gated on !resolutionDropped and no teardown path walks this map, so an entry whose resolution keeps failing is retained for the life of the process, and the next turn of the same [sessionId, taskId] key inherits its turnComplete / status / label.
With aggregation on and named-session ownership flapping, turn N's segment fails target resolution three times; the third failure sets resolutionDropped with no retry timer armed. Neither the dispatch finally nor the retry callback's .finally() can delete the entry any more, and drainBackgroundResponseAggregations iterates only the two aggregation collections — never this map — so the entry survives onSessionDied, onSessionRetiring and disconnect. That leaks one entry per exhausted key in a daemon that runs for weeks, in a file that caps every comparable collection at 1000. The wrong output follows when the same agent notifies again: the fresh-turn reset just below requires a live parked.retryTimer, which exhaustion cleared, so turn N+1's first segment reuses the stale record — its resolutionDropped transfers to the new aggregation, and its turnComplete / status / label are applied there too, overriding the label set two lines earlier. Turn N+1's very first segment is flushed at once as a card announcing a completed turn under turn N's label, while the rest of turn N+1 aggregates into a separate card. If turn N+1's first resolution fails instead, retryAttempts is already at the ceiling, so it gives up with zero retries and drops that segment outright.
Clear the turn-scoped state at give-up so a later turn cannot inherit it, and let retirement reap the entry:
if (pending.retryAttempts >= BACKGROUND_RESPONSE_AGGREGATION_MAX_RETRIES) {
pending.resolutionDropped = true;
pending.turnComplete = undefined;
pending.status = undefined;
pending.label = undefined;
pending.completionPartial = undefined;
return;
}plus a sessionId field on PendingBackgroundResponseTerminal (set at both creation sites) so drainBackgroundResponseAggregations(sessionId?) can also walk pendingBackgroundResponseTerminals, clearTimeout(entry.retryTimer) and delete the matching entries. The field clearing alone does not close the leak — the probe below still reported afterDisconnect=1 with it applied.
Witness:
probe (real adapter, fake timers; resolveBackgroundResponseDelivery rejecting 3x then resolving)
BEFORE (PR code)
after exhaustion: pending.size=1 entry=[{"resolvers":0,"retryAttempts":3,"turnComplete":true,
"status":"completed","label":"Turn one","completionPartial":false,"resolutionDropped":true}] pushes=[]
after turn-two FIRST segment: pushes=["## ✅ Agent · Turn one（部分）\n\nTurn two result."]
LEAK: pending.size afterExhaustion=1 afterOnSessionDied=1 afterOnSessionRetiring=1 afterDisconnect=1
AFTER (candidate fix)
after exhaustion: entry=[{"resolvers":0,"retryAttempts":3,"resolutionDropped":true}]
after turn-two FIRST segment: pushes=[]
FINAL: pushes=["## ✅ Agent · Turn two（部分）\n\nTurn two result."]
with the patch applied, the existing test below still passes (1 passed | 307 skipped)
The fix must not disturb retains resolution loss until the next segment can report it (DingtalkAdapter.test.ts:8443), which asserts '## ✅ Agent · Worker one（部分）\n\nFinal result.' after three resolution failures — resolutionDropped has to survive give-up until the next segment reports it, so only the turn-scoped fields may be cleared, not the entry, and the reaping must be scoped to retirement rather than to this give-up branch. Please add a regression beside that test: reject resolution three times, park a turnComplete: true marker labelled Turn one, advance 3 × 30s, then dispatch turn two's first segment labelled Turn two and assert pushProactive has not been called yet; after turn two's terminal, assert the single card is ## ✅ Agent · Turn two（部分）\n\nTurn two result. — removing the clearing must turn that red, and a second assertion that pendingBackgroundResponseTerminals is empty after onSessionDied('session-1') pins the drain half.
中文说明
R4-4 的第 4 轮修复让「解析重试耗尽」的 pendingBackgroundResponseTerminals 表项保留下来，直到后续聚合消费它的 resolutionDropped 标志——但两个删除点都以 !resolutionDropped 为条件，而任何清理路径都不会遍历这个 map，因此解析持续失败的表项会在进程的整个生命周期内被保留，并且同一 [sessionId, taskId] 键的下一个回合会继承它的 turnComplete / status / label。
开启聚合且命名会话归属抖动时：回合 N 的分段连续三次解析失败，第三次失败会置上 resolutionDropped 且不再武装重试定时器。此时 dispatch 的 finally 与重试回调的 .finally() 都无法删除该表项，而 drainBackgroundResponseAggregations 只遍历两个聚合集合、从不遍历这个 map，所以该表项在 onSessionDied、onSessionRetiring、disconnect 之后依然存在。对于运行数周的守护进程，这意味着每个耗尽的键泄漏一条表项——而本文件对所有可比集合都设了 1000 条上限。错误输出随后出现：同一 agent 再次通知时，紧随其后的「新回合重置」要求 parked.retryTimer 仍在，而耗尽恰恰清掉了它，于是回合 N+1 的首个分段复用了这条陈旧记录——它的 resolutionDropped 转移到新聚合上，turnComplete / status / label 也被套用过去，覆盖了两行之前刚设置的 label。回合 N+1 的第一个分段立刻被冲刷成一张以回合 N 的标签、宣告回合已完成的卡片，而回合 N+1 的其余内容聚合进另一张卡。如果回合 N+1 的首次解析反而失败，retryAttempts 已在阈值上，于是零次重试直接放弃，该分段被彻底丢弃。
修复：在放弃分支清掉回合级状态，使后续回合无法继承；并让会话回收能够清理该表项（给 PendingBackgroundResponseTerminal 增加 sessionId 字段，在 drainBackgroundResponseAggregations(sessionId?) 中一并遍历、clearTimeout 并删除）。仅清字段并不能解决泄漏——下方探针在应用该改动后仍报告 afterDisconnect=1。
约束：修复不得破坏 retains resolution loss until the next segment can report it（DingtalkAdapter.test.ts:8443），它断言三次解析失败后得到 '## ✅ Agent · Worker one（部分）\n\nFinal result.'——resolutionDropped 必须保留到下一个分段能报告它为止，因此只能清回合级字段、不能删表项，且清理必须限定在会话回收时机而非本放弃分支。
验收：请在该测试旁补一条回归——三次拒绝解析、停放一个 label 为 Turn one 的 turnComplete: true 标记、前进 3 × 30s，再发送 label 为 Turn two 的回合二首个分段并断言此时 pushProactive 尚未被调用；回合二的终止标记之后断言唯一卡片为 ## ✅ Agent · Turn two（部分）\n\nTurn two result.。移除上述清理逻辑后该断言必须变红；另加一条断言 onSessionDied('session-1') 之后 pendingBackgroundResponseTerminals 为空，用来钉住清理这一半。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.23.0)
4 participants
What this PR does
Background Agent responses in DingTalk are now delivered as soon as each non-empty response segment is available by default, and every message is headed with
Agent · <name>so concurrent work remains attributable. Users who prefer one result per notification turn can opt in withaggregateBackgroundAgentResponses: true; aggregated and partial results keep the same Agent attribution.The response metadata now carries a bounded display label and an explicit notification-turn boundary through both channel bridge paths. Aggregation keeps concurrent Agents separate, preserves already-produced text on interrupted turns, bounds incomplete turns, and retries transient delivery failures without changing the default immediate path.
This supersedes #10807.
Why it's needed
Always aggregating background Agent responses delays useful intermediate output and makes the new behavior impossible to avoid. Immediate labeled delivery is the safer default, while an explicit setting still supports users who want fewer DingTalk messages.
Reviewer Test Plan
How to verify
aggregateBackgroundAgentResponses, start two named background Agents, and confirm every non-empty response segment is sent immediately with the correctAgent · <name>heading.aggregateBackgroundAgentResponsestotrue, repeat the concurrent run, and confirm each Agent produces one independent labeled result after its notification turn completes.Agent · 后台任务heading.Automated coverage verifies default delivery, explicit aggregation, interleaved Agents, terminal and partial boundaries, legacy and malformed metadata, timeout flushing, session teardown, and retry behavior.
Evidence (Before & After)
Before: current main sends background notification responses without Agent attribution, while #10807 changes DingTalk to wait and aggregate by default. After: DingTalk immediately sends labeled Agent responses by default and aggregates only when explicitly enabled. No live DingTalk message was sent during local validation.
Tested on
Environment (optional)
macOS, Node.js v25.9.0, local build/typecheck/lint and package-scoped Vitest suites.
Risk & Scope
Linked Issues
Supersedes #10807.
中文说明
本 PR 做了什么
钉钉中的后台 Agent 响应现在默认在每个非空响应分段可用时立即发送，并且每条消息都以
Agent · <名称>开头，确保并发任务的输出可归属。希望每个通知回合只收到一条结果的用户可以通过aggregateBackgroundAgentResponses: true显式开启聚合；聚合结果和部分结果同样保留 Agent 标注。响应元数据现在会通过两条 Channel Bridge 链路传递长度受限的展示名称和明确的通知回合边界。聚合模式会隔离并发 Agent、在回合中断时保留已经生成的文本、为未完成回合设置等待上限，并对临时发送失败进行重试，同时不改变默认即时返回路径。
本 PR 替代 #10807。
为什么需要
强制聚合后台 Agent 响应会延迟有价值的中间输出，并且用户无法关闭这一行为。默认即时且带标注的返回更安全，同时显式设置仍能满足希望减少钉钉消息数量的用户。
Reviewer Test Plan
如何验证
aggregateBackgroundAgentResponses的钉钉 Channel，启动两个有不同名称的后台 Agent，确认每个非空响应分段都会立即发送，并带有正确的Agent · <名称>标题。aggregateBackgroundAgentResponses设置为true后重复并发运行，确认每个 Agent 都在自己的通知回合结束后产生一条独立且带标注的结果。Agent · 后台任务标题。自动化测试覆盖默认发送、显式聚合、并发 Agent 隔离、终止和部分结果边界、旧版及畸形元数据、超时刷新、会话清理和重试行为。
前后对比证据
改动前：当前 main 发送后台通知响应时没有 Agent 归属标注，而 #10807 会让钉钉默认等待并聚合。改动后：钉钉默认立即发送带 Agent 标注的响应，仅在显式开启时聚合。本地验证期间未通过真实钉钉机器人发送消息。
测试系统
环境（可选）
macOS、Node.js v25.9.0、本地 build/typecheck/lint 以及按 package 运行的 Vitest 测试。
风险与范围
关联事项
替代 #10807。