fix(channels): return only final ACP response text - #6615
Conversation
|
Thanks for the PR! (Re-run — author addressed prior feedback.) Template looks good ✓ Problem: The bug in #6602 is real and well-documented — Direction: Aligned. Channel response quality is a real user-facing problem, and the Size: All changes are in Approach: Scope concerns from the prior run are fully resolved. The A2A infrastructure and the unrelated autofix workflow test edits have been removed. This is now a focused, single-purpose PR — exactly what was requested. Moving on to code review. 🔍 中文说明感谢贡献！（Re-run — 作者已回应之前的反馈。） 模板完整 ✓ 问题： Issue #6602 中的 bug 真实且有文档—— 方向： 对齐。频道回复质量是真实的用户问题， 规模： 所有变更在 方案： 上次审查的范围问题已完全解决。A2A 基础设施和不相关的 autofix workflow 测试编辑已移除。现在是一个聚焦的单一目的 PR——正是之前建议的。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
096be4c to
987edc5
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Code ReviewThe implementation is clean and correct. The
I independently arrived at the same approach before reading the diff. Nothing missed, nothing extra. No critical issues, no AGENTS.md violations. TestingAll 530 tests pass across 5 test files. Build and typecheck succeed. New regression tests cover: tool calls (pending + auto-approved), plan updates as turn boundaries, permission requests as boundaries, block-streaming buffer clearing, cancel-state boundary suppression, adapter-specific buffer clearing (Feishu card state, QQ stream state). Tmux real-scenario testing: not feasible — channel ACP sessions require live DingTalk/Feishu/Telegram credentials. The unit tests comprehensively mock the multi-turn event flow and assert the exact behavior described in the PR. This is the appropriate testing level for channel infrastructure changes. 中文说明代码审查实现干净且正确。
我在阅读 diff 之前独立提出了相同方案。没有遗漏，没有多余。无关键问题，无 AGENTS.md 违规。 测试5 个测试文件共 530 个测试全部通过。Build 和 typecheck 均成功。 Tmux 真实场景测试： 不可行——频道 ACP session 需要真实的 DingTalk/Feishu/Telegram 凭据。单元测试全面 mock 了多轮事件流并断言了 PR 中描述的精确行为。对于频道基础设施变更，这是合适的测试层级。 — Qwen Code · qwen3.7-max |
|
This is a clean, well-scoped bug fix on re-run. The problem in #6602 is real and clearly reproduced — channel users get walls of intermediate narration text because The implementation introduces a Both scope concerns from the prior run are resolved — the A2A infrastructure and the autofix workflow test edits are gone. The maintainer's feedback about the autofix test file has been addressed. Approving. ✅ 中文说明Re-run 上这是一个干净、范围合理的 bug 修复。Issue #6602 中的问题真实且复现清晰——频道用户收到大量中间过程叙述文本，因为 实现引入了 上次审查的两个范围问题都已解决——A2A 基础设施和 autofix workflow 测试编辑已移除。Maintainer 关于 autofix 测试文件的反馈已被处理。 批准合入。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
987edc5 to
732bee8
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/channels/base/src/AcpBridge.ts:399, packages/channels/base/src/DaemonChannelBridge.ts:781 |
Zero logging when responseBoundary fires or chunks are cleared. When production responses come back empty or wrong, there's no log trail to diagnose whether the boundary fired, how many chunks were dropped, or whether it was suppressed by cancel state. |
Add process.stderr.write debug logging in emitResponseBoundary and adapter boundary handlers, consistent with existing logging patterns in these files. |
packages/channels/base/src/AcpBridge.ts:332, packages/channels/base/src/DaemonChannelBridge.ts:591 |
plan sessionUpdate emits responseBoundary unconditionally (no status guard unlike tool_call). If the agent's final action is todo_write preceded by response text, all text is cleared and prompt() returns "". |
Consider adding a guard — e.g., only emit boundary when plan entries actually change, or track whether text has been emitted since the last boundary and skip if so. |
— qwen3.7-max via Qwen Code /review
| this.activePromptControllers.delete(sessionId); | ||
| } | ||
|
|
||
| private emitResponseBoundary(sessionId: string): void { |
There was a problem hiding this comment.
[Critical] DaemonChannelBridge.handlePermissionRequest() does not call emitResponseBoundary(sessionId) before emitting permissionRequest, unlike AcpBridge.requestPermission() (line ~393) which does. In daemon-mode sessions (the production path for DingTalk, the channel named in issue (6602)), intermediate text chunks emitted before a permission prompt will NOT be cleared and will leak into the final channel response.
| private emitResponseBoundary(sessionId: string): void { | |
| private emitResponseBoundary(sessionId: string): void { | |
| this.emit('responseBoundary', sessionId); | |
| } |
Add this.emitResponseBoundary(sessionId); in handlePermissionRequest() before the this.emit('permissionRequest', ...) call, mirroring the AcpBridge implementation. Also add a corresponding daemon test mirroring the ACP permission boundary test.
— qwen3.7-max via Qwen Code /review
| releaseHeldChunks(); | ||
| } | ||
| }; | ||
| const onResponseBoundary = (sid: string) => { |
There was a problem hiding this comment.
[Critical] Adapter-level state is not cleared on response boundary. The boundary clears heldChunks here, but adapters with onResponseChunk overrides (QQBot's streamState.buffer, Feishu's cardState.accumulatedText) accumulate intermediate text in their own state. When onResponseComplete fires, QQBot uses state.buffer (all accumulated text including intermediate) instead of fullText (boundary-filtered final text). For non-block-streaming adapters (the default for QQBot and Feishu), users still receive "Let me search. Now I will read. Final answer." instead of just "Final answer." — the bridge-level fix is bypassed at the adapter layer.
Consider adding a protected onResponseBoundary() hook to ChannelBase that adapters can override to clear their own accumulated state, and invoke it from this handler. Alternatively, have onResponseComplete prefer fullText over state.buffer when a boundary was observed during the turn.
— qwen3.7-max via Qwen Code /review
| return; | ||
| } | ||
| heldChunks.length = 0; | ||
| }; |
There was a problem hiding this comment.
[Critical] onResponseBoundary unconditionally clears heldChunks without checking promptState.cancelPending. During the cancel-pending window, onChunk correctly holds chunks (guarded by if (!promptState.cancelPending) releaseHeldChunks()), but if a responseBoundary fires while cancel is in flight (e.g., the agent emits a pending tool_call before the cancel RPC settles), heldChunks is wiped. If the cancel then fails, releaseHeldChunks() replays an empty array — text that should have been delivered to the user is silently lost.
Apply the same cancelPending guard at both onResponseBoundary locations (line ~795 and ~3558):
| }; | |
| const onResponseBoundary = (sid: string) => { | |
| if (sid !== sessionId || promptState.cancelled || promptState.cancelPending) { | |
| return; | |
| } | |
| heldChunks.length = 0; | |
| }; |
When the cancel succeeds, chunks are discarded via cancelled anyway; when it fails, they're preserved for replay.
— qwen3.7-max via Qwen Code /review
| return; | ||
| } | ||
| heldChunks.length = 0; | ||
| streamer?.stop(); |
There was a problem hiding this comment.
[Critical] onResponseBoundary clears heldChunks at the ChannelBase level but does not notify adapters to reset their own text accumulators. In non-block-streaming mode, releaseHeldChunks() feeds adapter overrides via this.onResponseChunk():
- Feishu:
cardState.accumulatedText(FeishuAdapter.ts:905) - QQBot:
streamState.buffer(QQChannel.ts:494)
When the boundary fires, heldChunks is cleared but the adapter's accumulated intermediate text persists. The final response from bridge.prompt() is then delivered via onResponseComplete — the user sees both the intermediate card text and the final answer, which is the exact bug this PR aims to fix leaking through a different path.
In block-streaming mode this is not an issue: Feishu's onResponseChunk returns early (if (this.config.blockStreaming === 'on') return) and QQBot similarly skips.
Suggested fix: Add an adapter-level reset hook (e.g., a protected onResponseBoundary(sessionId) method on ChannelBase that adapters can override) and call it from the boundary handler. Feishu should reset cardState.accumulatedText; QQBot should reset streamState.buffer. This applies to both onResponseBoundary locations.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
scripts/tests/qwen-autofix-workflow.test.js:307 |
Unrelated autofix workflow test changes bundled into commit cf18ad4d5 ("fix(channels): align daemon response boundaries"). These assertions about concurrency group naming and issue routing have nothing to do with the responseBoundary event system. |
Move into a separate commit or PR with its own commit message. |
DaemonChannelBridge.ts:566 |
tool_call_update with status: 'pending' fires responseBoundary identically to tool_call. A tool_call_update with pending status is a status regression, not a new turn boundary — it could clear legitimate text produced between updates. |
Scope boundary emission to type === 'tool_call' only: if (type === 'tool_call' && event.status === 'pending'). |
AcpBridge.ts:315 |
AcpBridge handles only tool_call (not tool_call_update) while DaemonChannelBridge handles both. This pre-existing asymmetry is now more consequential: a missing tool_call_update means the text buffer is never cleared at that boundary. |
Add case 'tool_call_update': to AcpBridge's switch, mirroring DaemonChannelBridge. |
ChannelBase.ts:795 |
No test for the channel loop path's onResponseBoundary handler (only the block-streaming path at line ~3558 is tested). |
Add a test in the channel loop test section that emits responseBoundary and verifies heldChunks are cleared. |
AcpBridge.ts:400 |
Zero logging for responseBoundary emission and chunk clearing. When debugging empty or truncated channel responses, there's no trace of whether the boundary fired or how many chunks were discarded. |
Add process.stderr.write with session ID and discarded chunk count in emitResponseBoundary. |
— qwen3.7-max via Qwen Code /review
| status: (update['status'] as string) || 'pending', | ||
| rawInput: update['rawInput'] as Record<string, unknown> | undefined, | ||
| }; | ||
| if (event.status === 'pending') { |
There was a problem hiding this comment.
[Critical] Auto-approved tools bypass the response boundary. Session.ts:5113-5117 emits tool_call with status: 'in_progress' for auto-approved tools (YOLO mode, AUTO_EDIT, L3/L4 allowlist). Since the boundary only fires on status === 'pending', intermediate text is never cleared in YOLO/auto mode — the exact bug from #6602 persists.
The same gap exists in DaemonChannelBridge.ts:585.
| if (event.status === 'pending') { | |
| if (event.status === 'pending' || event.status === 'in_progress') { | |
| this.emitResponseBoundary(sessionId); | |
| } |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Please drop this PR's edit to
|
# Conflicts: # scripts/tests/qwen-autofix-workflow.test.js
| if (!inboundMsgId) return; | ||
| const cardState = this.cardSessions.get(inboundMsgId); | ||
| if (!cardState || cardState.stopped) return; | ||
| cardState.accumulatedText = ''; |
There was a problem hiding this comment.
[Critical] onResponseBoundary clears accumulatedText but does not clear pendingUpdateTimer. Every other card-reset path (onResponseComplete L1226, handleStop L1791, cleanup L1866) clears this timer. When the boundary fires while a throttled card update is pending (up to 1500ms), the timer callback reads the now-empty accumulatedText and calls updateCard with empty content — the streaming card flashes blank before the final answer arrives.
| cardState.accumulatedText = ''; | |
| protected override onResponseBoundary( | |
| _chatId: string, | |
| sessionId: string, | |
| ): void { | |
| if (this.config.blockStreaming === 'on') return; | |
| const inboundMsgId = this.sessionToInboundMsg.get(sessionId); | |
| if (!inboundMsgId) return; | |
| const cardState = this.cardSessions.get(inboundMsgId); | |
| if (!cardState || cardState.stopped) return; | |
| if (cardState.pendingUpdateTimer) { | |
| clearTimeout(cardState.pendingUpdateTimer); | |
| cardState.pendingUpdateTimer = undefined; | |
| } | |
| cardState.accumulatedText = ''; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in f41e881. The response boundary now cancels and clears the pending card update timer before clearing accumulated text. Verified with cd packages/channels/feishu && npx vitest run src/adapter.test.ts (69 passed) and npm run build.
| } | ||
| this.streamState.delete(sessionId); | ||
| this.pendingStreamDelete.delete(sessionId); | ||
| this.flushedSessions.delete(sessionId); |
There was a problem hiding this comment.
[Critical] onResponseBoundary omits flushingSessions cleanup. Compare with onSessionDied (L739) which correctly clears all four maps. When an idle-flush send is in flight at boundary time, the stale flushingSessions entry causes onResponseComplete (L717) to defer, and the old send's .finally() identity guard skips cleanup — flushingSessions becomes permanently stuck and the final response is never delivered.
| this.flushedSessions.delete(sessionId); | |
| protected override onResponseBoundary( | |
| _chatId: string, | |
| sessionId: string, | |
| ): void { | |
| const state = this.streamState.get(sessionId); | |
| if (state?.timer) { | |
| clearTimeout(state.timer); | |
| } | |
| this.streamState.delete(sessionId); | |
| this.flushingSessions.delete(sessionId); | |
| this.pendingStreamDelete.delete(sessionId); | |
| this.flushedSessions.delete(sessionId); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in f41e881. The response boundary now clears the in-flight flush marker with the rest of the QQ stream state. Verified with cd packages/channels/qqbot && npx vitest run src/stream.test.ts (43 passed) and npm run build.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. 引入 responseBoundary 事件统一处理中间文本丢弃，比后处理干净。迭代中已修复 auto-approved tool、adapter 层状态清理、cancel-pending race 等关键问题。测试覆盖充分。
Nit: 两处 onResponseBoundary handler（~L788, ~L3537）逻辑近似（block-streaming path 多 streamer?.stop()），后续可考虑抽 helper 减少重复，不阻塞。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| : undefined, | ||
| }; | ||
| if (event.status === 'pending' || event.status === 'in_progress') { | ||
| this.emitResponseBoundary(sessionId); |
There was a problem hiding this comment.
[Suggestion] Both tool_call and tool_call_update share this case block, so when a tool transitions as tool_call(status='pending') → tool_call_update(status='in_progress'), two boundaries fire. Any text chunk arriving between these events is silently discarded. AcpBridge does NOT handle tool_call_update, so this double-emission is unique to the daemon path.
Consider tracking which tool-call IDs have already emitted a boundary, or only emitting on pending status:
if (event.status === 'pending') {
this.emitResponseBoundary(sessionId);
}— qwen3.7-max via Qwen Code /review
| @@ -383,6 +396,10 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { | |||
| }); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] emitResponseBoundary and the clearChunks callback silently zero the chunks array with no log, counter, or diagnostic. When debugging boundary-related issues (e.g., empty responses), there is no way to distinguish "agent generated no text" from "boundaries discarded text" from "bug swallowed chunks" without attaching a debugger.
Consider adding a debug log:
private emitResponseBoundary(sessionId: string): void {
process.stderr.write(`[${this.constructor.name}] responseBoundary for session ${sessionId}\n`);
this.emit('responseBoundary', sessionId);
}— qwen3.7-max via Qwen Code /review
| @@ -220,6 +220,260 @@ describe('DaemonChannelBridge', () => { | |||
| bridge.stop(); | |||
There was a problem hiding this comment.
[Suggestion] AcpBridge has a test asserting boundary does NOT fire on completed status ("preserves text when tool calls are not pending"), but DaemonChannelBridge has its own independent handleSessionUpdate implementation with the same conditional logic. A copy-paste error or divergence in the daemon bridge's status check would go undetected.
Consider adding a test mirroring AcpBridge's "preserves text when tool calls are not pending" case, using a daemon session_update with sessionUpdate: 'tool_call' and status: 'completed'.
— qwen3.7-max via Qwen Code /review
| _chatId: string, | ||
| sessionId: string, | ||
| ): void { | ||
| if (this.config.blockStreaming === 'on') return; |
There was a problem hiding this comment.
[Suggestion] The blockStreaming === 'on' early return is correct — BlockStreamer owns delivery in that mode — but both existing boundary tests use the default config (blockStreaming off). If someone removes or inverts this guard, card text would be wiped mid-stream during block streaming, and no unit test would catch it.
Consider adding a test with { blockStreaming: 'on' } that asserts accumulatedText is unchanged after onResponseBoundary.
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
This PR changes channel ACP prompt collection so intermediate text from earlier tool-call turns is discarded before the final response is returned to channel adapters. It also treats plan updates and permission requests as turn boundaries, and propagates that boundary through the channel bridge so buffered block-streaming text is dropped before delivery.
Why it's needed
Channel integrations send the string returned by the ACP prompt as the completed response. When a prompt performs multiple tool-call rounds, the previous behavior accumulated every streamed text chunk across the whole loop, so group chat users could receive a long message containing both intermediate narration and the actual final answer. Fixes #6602.
Reviewer Test Plan
How to verify
Ask a channel-backed ACP session a question that requires multiple tool calls or permission approval. The delivered final message should contain only the final answer text, not earlier narration such as searching, reading, or permission status text. For TodoWrite-only intermediate rounds, any text before the plan update should also be excluded from the completed response.
Evidence (Before & After)
Before: the new regression tests would receive concatenated intermediate and final text, for example
Let me search. Now I will read. Final answer.. After:npx vitest run src/AcpBridge.test.tsandnpx vitest run src/ChannelBase.test.tsinpackages/channels/basepass and assert that only post-boundary final text is returned or delivered from buffered block streaming.Tested on
Environment (optional)
Local Node/npm workspace after
npm install.Risk & Scope
中文说明
这个 PR 做了什么
这个 PR 调整了频道 ACP prompt 的文本收集逻辑：当较早的 tool-call 轮次结束并进入后续轮次时，会丢弃这些中间文本，只把最终回复返回给频道适配器。同时把 plan 更新和 permission request 也视为轮次边界，并把这个边界事件传到频道层，用于清理尚未发送的 block streaming 缓冲。
为什么需要
频道集成会把 ACP prompt 返回的字符串作为完整回复发送出去。当一次 prompt 需要多轮工具调用时，旧逻辑会累积整个循环里的所有流式文本，因此群聊用户可能收到一条很长的消息，里面既有中间说明，也有真正的最终答案。Fixes #6602。
Reviewer 测试计划
如何验证
在使用 channel 的 ACP session 中询问一个需要多次工具调用或权限确认的问题。最终发送出去的消息应该只包含最终答案，不包含较早的搜索、读取、权限确认等状态说明。对于 TodoWrite-only 的中间轮次，plan 更新前的说明文字也不应该出现在完整回复里。
证据（Before & After）
修复前：新增回归测试会拿到中间文本和最终文本拼接后的结果，例如
Let me search. Now I will read. Final answer.。修复后：在packages/channels/base执行npx vitest run src/AcpBridge.test.ts和npx vitest run src/ChannelBase.test.ts通过，并断言返回或缓冲发送的文本只保留边界之后的最终内容。测试平台
环境（可选）
执行过
npm install的本地 Node/npm 工作区。风险与范围