fix(channels/qqbot): key the cron textChunk guard on active prompts, not streamState - #9971
Merged
Merged
Conversation
…not streamState handleCronTextChunk used streamState.has(sessionId) to tell prompt-response chunks from cron/non-prompt chunks. That discriminator fails in both directions (#6094 items 1 and 2): - With blockStreaming: 'on', onResponseChunk early-returns and never populates streamState, so during an active cron flow every prompt-response chunk leaked into cronBuffer and was re-sent by the 2s idle flush on top of the BlockStreamer delivery (duplicate messages). - A residual streamState entry from a finished turn whose flush has not settled (e.g. cancelled/errored prompt) kept the guard true, silently dropping all subsequent cron textChunks for that session. Track sessions with a prompt turn in flight in activePromptSessions, maintained by the onPromptStart/onPromptEnd hooks. ChannelBase always pairs the two per turn (onPromptEnd runs in the prompt path's finally, even on error/cancel), and the marker is independent of streaming config, so it is a reliable discriminator. The set is also cleared in disconnect() and onSessionDied alongside the existing streaming-state cleanup. Items 3 and 4 from the issue are already addressed on main (plain-text fallback for no-msgId sends; setBridge re-attaches _cronTextHandler). Items 5 and 6 remain open as separate low-priority work.
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献！
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewNo blockers found. My independent take before reading the diff: What I verified in the code:
Testing evidence — the PR's own CI (fetched via API; nothing executed here)The unit suite is still running on the reviewed commit — one fetch, no polling; the finalize job updates the table below when CI settles. macOS/Windows unit jobs are skipped by design; security checks all passed. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the one claim the diff can't: 中文说明代码审查未发现阻塞问题。 读 diff 前的独立判断： 代码中核实的内容：
测试证据 —— 来自 PR 自身的 CI（通过 API 获取；此处未执行任何代码）单测套件在被审查的提交上仍在运行——只拉取一次、不轮询；finalize 任务会在 CI 落定后更新下方表格。macOS/Windows 单测任务按设计跳过；安全检查全部通过。 沙箱化验证可以落定 diff 无法证明的那一点： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — tight, correctly-scoped fix: the problem is real and confirmed in code, the discriminator swap is the minimal solution, and the invariant it relies on (prompt-start/end pairing) holds at every call site. Reflection:
Verdict: approve. The unit suite is still running on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度：5/5 —— 紧凑、范围正确的修复：问题真实存在且已在代码中确认，替换判别器是最小解法，其依赖的不变量（prompt start/end 配对）在所有调用点都成立。 反思：
结论：批准。单测套件仍在被审查的提交上运行，因此批准推迟到该提交的 CI 全绿之后。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Aug 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Comment on lines
+227
to
+229
| * reliable because ChannelBase always brackets a prompt turn with | ||
| * onPromptStart and onPromptEnd (onPromptEnd runs in the prompt path's | ||
| * finally, even on error/cancel), independent of streaming config. |
Collaborator
There was a problem hiding this comment.
[Suggestion] The "always brackets" claim here has a real hole: a turn whose bridge.prompt() never settles never runs the finally that calls onPromptEnd — inbound prompts have no timeout (only loop prompts do), so the wedge can be permanent, keeping the session marked until /clear, onSessionDied, or disconnect(). ChannelBase documents this state itself ("its finally may settle long after — or never", ChannelBase.ts:300). Failure shape: an inbound prompt for session S wedges permanently and nobody /clears it; an external cron flow later emits textChunks for S → this guard returns on every chunk → scheduled messages for S are silently dropped, with no log line, until disconnect/sessionDied. Under the old guard with blockStreaming:'on' the same chunks were delivered, so this is a narrowed corner the rationale doesn't argue (it defends error/cancel settlement, but not never-settlement).
Suggested change
| * reliable because ChannelBase always brackets a prompt turn with | |
| * onPromptStart and onPromptEnd (onPromptEnd runs in the prompt path's | |
| * finally, even on error/cancel), independent of streaming config. | |
| * reliable because ChannelBase brackets prompt turns with | |
| * onPromptStart and onPromptEnd (onPromptEnd runs in the prompt path's | |
| * finally, even on error/cancel), independent of streaming config — | |
| * except a turn whose bridge.prompt() never settles: it keeps the | |
| * session marked until `/clear`, `onSessionDied`, or disconnect. |
中文说明
这里的 "总是成对调用（always brackets）" 声明有一个真实的漏洞：如果某个回合的 bridge.prompt() 永远不结束，那么调用 onPromptEnd 的 finally 就永远不会执行 —— 入站 prompt 没有超时（只有 loop prompt 有），因此这种卡死可能是永久的，session 会一直被标记，直到 /clear、onSessionDied 或 disconnect()。ChannelBase 自己也记录了这种状态（"其 finally 可能很久之后才落定 —— 或者永远不落定"，ChannelBase.ts:300）。失败场景：session S 的一个入站 prompt 永久卡死且没有人 /clear 它；之后外部 cron 流对 S 发出 textChunk → 该守卫对每个块都直接返回 → S 的定时消息被静默丢弃且没有任何日志，直到 disconnect/sessionDied。在旧守卫 + blockStreaming:'on' 下，同样的块是会被投递的，因此这是收窄了一个注释中的理由并未论证的角落（理由论证了出错/取消时的落定，但没有论证永不落定的情况）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // streamState (#6094): streamState is empty under blockStreaming:'on' | ||
| // (prompt chunks would be duplicated) and can linger after a turn | ||
| // ends (cron chunks would be silently dropped). | ||
| if (this.activePromptSessions.has(sessionId)) return; |
Collaborator
There was a problem hiding this comment.
[Suggestion] No test pins the per-session keying of this guard: the mutant if (this.activePromptSessions.size > 0) return; — which blocks ALL cron capture while any prompt turn is active, not just the marked session's — passes the entire suite (verified by running it: 298 tests pass; the only failure was a purpose-built probe). Every test expecting cron capture has an empty marker set, and every test with a marker triggers chunks only for the marked session. The discriminator's core isolation property — session B's scheduled output still delivers while session A has an active prompt turn — is therefore unguarded, and a one-token mutant ships green. Suggested test (helper names per this file's conventions):
it('does not block cron capture for other sessions while a prompt is active', () => {
const ch = makeChannel();
ch.onPromptStart('test-chat', 'sess-A');
(ch as unknown as { _inCronFlow: number })._inCronFlow = 1;
triggerTextChunk(ch, 'sess-B', 'cron for b');
flushSetImmediate();
const cronBuffer = (ch as unknown as {
cronBuffer: Map<string, { buffer: string }>;
}).cronBuffer;
expect(cronBuffer.get('sess-B')?.buffer).toBe('cron for b');
expect(cronBuffer.has('sess-A')).toBe(false);
});中文说明
没有任何测试钉住该守卫的"按 session 区分"这一关键性质：变异体 if (this.activePromptSessions.size > 0) return;（只要存在任何活跃 prompt 回合就阻止所有 cron 捕获，而不仅仅是被标记 session 的块）能通过整个测试套件（已实际运行验证：298 个测试通过，唯一失败的是一个专门构造的探针测试）。所有期望 cron 被捕获的测试里标记集合都是空的，而所有设置了标记的测试都只对被标记的 session 触发块。因此该判别器的核心隔离性质 —— session A 有活跃 prompt 回合时，session B 的定时输出仍能正常投递 —— 目前没有任何测试保护，一个单 token 的变异体就能全绿通过。建议补充测试（辅助函数名遵循本文件现有约定，见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Comment on lines
991
to
+992
| this.flushedSessions.clear(); | ||
| this.activePromptSessions.clear(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This new disconnect-time activePromptSessions.clear() has no test gating it — a measured deletion mutant (removing just this line) leaves the entire suite green, per the test-efficacy probe (harness validated: the sibling deletion mutants in onPromptEnd and onSessionDied were both killed, so the kit discriminates). If a future edit drops this line it ships with a green suite: a session mid-prompt when disconnect() runs stays permanently marked active across the restart (start() resets disposed on the same instance), and handleCronTextChunk then silently drops every subsequent cron textChunk for that session until that session's next prompt turn completes — reintroducing exactly the #6094 item-2 silent cron drop, via the disconnect path. Suggested regression test (fits the existing disconnect cron cleanup block):
it('clears active-prompt markers on disconnect', () => {
const ch = makeChannel();
ch.onPromptStart('test-chat', 'sess-dc');
ch.disconnect();
expect(
(ch as unknown as { activePromptSessions: Set<string> })
.activePromptSessions.size,
).toBe(0);
});中文说明
这行新增的 disconnect 时 activePromptSessions.clear() 没有任何测试保护 —— 按测试有效性探针的实测结果，仅删除这一行的变异体能让整个套件保持全绿（探针 harness 已验证有效：onPromptEnd 和 onSessionDied 中对应的删除变异体都被杀死，说明该工具具备区分能力）。如果未来某次编辑删掉了这行，它会在测试全绿的情况下合入：disconnect() 发生时正处于 prompt 中的 session，其标记会在重启后永久保留（start() 会在同一实例上重置 disposed），此后 handleCronTextChunk 会静默丢弃该 session 的所有后续 cron textChunk，直到该 session 的下一个 prompt 回合完成 —— 这正是 #6094 第 2 条的静默丢消息问题，只不过经由 disconnect 路径复现。建议的回归测试（可放入现有 disconnect cron cleanup 块，见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // --------------------------------------------------------------------------- | ||
| // prompt/cron discriminator (issue #6094) | ||
| // --------------------------------------------------------------------------- | ||
| describe('prompt/cron textChunk discriminator (#6094)', () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] This comment is about the PR description, not this code line (anchored here because this describe block is the diff's only reference to #6094): the PR body's "Partially fixes #6094" contains GitHub's closing keyword fixes #6094, so merging this PR will auto-close the umbrella issue #6094 even though items 5 and 6 remain open — as the PR itself declares ("items 5 and 6 stay open"). GitHub resolves closing keywords anywhere in the PR body, and "Partially fixes" still matches. Same-repo precedent: PR #9631 ("Partially fixes #9487") auto-closed #9487 on merge (closed 2 seconds after the merge, state_reason: completed, close actor = the merging user). Items 5 (botOpenId instruction timing) and 6 (token-refresh connect() retry) have no other tracking issue, so they would be silently orphaned at merge. Suggestion: reword the Linked Issues section to a non-closing reference (e.g. "Part of #6094 — addresses items 1 and 2; items 5 and 6 tracked separately"), or split items 5/6 into their own issues before merge.
中文说明
本条评论针对的是 PR 描述，而不是这行代码（锚点选在这里，是因为这个 describe 块是 diff 中唯一引用 #6094 的位置）：PR 正文中的 "Partially fixes #6094" 包含 GitHub 的关闭关键字 fixes #6094，因此合入本 PR 会自动关闭伞 issue #6094 —— 尽管第 5、6 条仍然开放（PR 自己也声明了 "items 5 and 6 stay open"）。GitHub 会解析 PR 正文任意位置的关闭关键字，"Partially fixes" 同样会命中。同仓库先例：PR #9631（"Partially fixes #9487"）在合入时自动关闭了 #9487（合入后 2 秒即关闭，state_reason: completed，关闭者为合入人）。第 5 条（botOpenId 指令时机）和第 6 条（token 刷新后 connect() 无重试）没有其他跟踪 issue，合入后会被静默遗弃。建议：把 Linked Issues 部分改写为非关闭式引用（例如 "Part of #6094 — addresses items 1 and 2; items 5 and 6 tracked separately"），或在合入前把第 5/6 条拆成独立 issue。
— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
enabled auto-merge
August 25, 2026 07:34
chiga0
approved these changes
Aug 25, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings.
Scope: Standard tier — 2 files, 241 lines diff.
Checked:
Root cause (streamState as discriminator fails in both directions — #6094)
- Item 1:
blockStreaming:on→onResponseChunkearly-returns without populatingstreamState→streamState.has(sessionId)is always false → prompt-response chunks leak intocronBufferand get re-sent by the 2s idle flush. - Item 2: Lingering
streamStateentry from a finished turn whose flush has not settled →streamState.has()stays true → all subsequent cron chunks for that session are silently dropped.
Fix correctness
activePromptSessionsis aSet<string>maintained byonPromptStart(add) andonPromptEnd(delete). Both are idempotent onSet, so double-add/double-delete are safe.- Cleanup is present in both exit paths:
disconnect()(full clear) andonSessionDied()(per-session delete).
ChannelBase guarantee verified (packages/channels/base/src/ChannelBase.ts):
- Line 1749:
onPromptEndcalled insidefinallyof the main prompt loop. - Line 2025: same in the webhook prompt path.
- Lines 2927 + clearEvicted guard:
/cleareviction also callsonPromptEndand prevents the deferredfinallyfrom calling it again.
Test efficacy (mental mutation):
- Revert to
streamState.has(): Item 1 test fails becausestreamStateis empty under blockStreaming → chunk IS captured (assertioncronBuffer.has() === falseflips). Item 2 test fails because lingeringstreamStateblocks the cron chunk (assertioncronBuffer.get()?.buffer === "cron text"flips). Both mutations are caught. ✓
makeChannel(configOverrides?) signature extension: spread is safe — blockStreaming: "on" is a known config key; the new item 1 regression test exercises it.
Mock fix (protected onSessionDied): Previously missing from the mock ChannelBase, which would cause the onSessionDied test to throw when QQChannel called super.onSessionDied(). Addition is correct.
CI: Test (ubuntu-latest, Node 22.x) = ✅ SUCCESS. Test (macos/windows) SKIPPED — not a concern for pure Set operations with no platform-sensitive code paths. precheck-pr SKIPPED (fork PR security model).
Cross-check: Prior review (2026-08-25T03:51) raised 4 suggestions — all non-blocking: (S1) hung bridge.prompt() never settling → theoretical; /clear eviction + onSessionDied cover the practical paths. (S2) no multi-session test for per-session keying. (S3) disconnect() clear not directly tested. (S4) PR body wording. None are correctness blockers.
Reviewed with AI assistance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Replaces the cron textChunk discriminator in the QQ Bot channel.
handleCronTextChunkusedstreamState.has(sessionId)to decide whether an incomingtextChunkbelongs to an in-progress prompt (and must be left to the prompt path) or to a cron/non-prompt flow (and must be captured into the cron buffer). This PR tracks sessions with an active prompt turn in a dedicatedactivePromptSessionsset, maintained by theonPromptStart/onPromptEndhooks, and keys the cron guard on that set instead. The marker is cleared alongside the existing streaming-state cleanup indisconnect()andonSessionDied.Why it's needed
streamState.has(sessionId)is not a reliable prompt/cron discriminator, and it fails in both directions (issue #6094, items 1 and 2):blockStreaming: 'on'—onResponseChunkearly-returns under block streaming and never populatesstreamState. During an active cron flow, prompt-response chunks therefore pass thestreamState.has()guard, land incronBuffer, and get re-sent by the 2s idle flush on top of the BlockStreamer delivery.streamStateentry from a finished turn whose flush has not settled (e.g. a cancelled/errored prompt mid-flush) keeps the guard true, silently discarding all subsequent cron textChunks for that session.activePromptSessionsis reliable because ChannelBase always brackets a prompt turn withonPromptStartandonPromptEnd—onPromptEndruns in the prompt path'sfinally, even on error/cancel — and the marker is independent of streaming configuration.Reviewer Test Plan
How to verify
Unit-level reproduction of both failure modes, driven through QQChannel's real hooks (
onPromptStart/onResponseChunk/onPromptEnd,runCronFlow/_inCronFlow, bridgetextChunklistener):blockStreaming: 'on', active prompt turn (onPromptStart), concurrent cron flow → emit atextChunkfor the prompt's session. Before: chunk is captured intocronBufferand re-sent (duplicate). After: chunk is skipped; no cron send.onPromptEndwhile the streamState flush has not settled) → cron flow emits atextChunkfor the same session. Before: silently dropped. After: buffered and delivered.All three repro tests fail on
mainand pass with this PR (verified by running the suite with and without the QQChannel.ts change):Also run:
tsc --buildfor the package (clean),eslint+prettier --checkon the changed files (clean).Note: the cron path is gated behind the opt-in
cron-msg-experimentalconfig flag, so the default (stable) path is untouched.Evidence (Before & After)
N/A — channel-adapter logic, no user-visible UI. Test evidence above; failing tests before the fix:
Tested on
Environment (optional)
Unit tests only (
vitest runinpackages/channels/qqbot) + packagetsc --build.Risk & Scope
onPromptStart→onPromptEnd) instead of only the streamState lifetime — a slightly wider window in which cron chunks for the same session are dropped during a prompt. This matches the existing intent (never mix cron output into an active prompt) and is the same semantics the non-blockStreaming path always had. The pre-existing streamState-isolation unit test was rewritten to drive the new discriminator.send.test.ts;setBridgere-attaches_cronTextHandler). Items 5 (botOpenId instruction timing) and 6 (token-refreshconnect()retry) remain open as separate low-priority work. The author's broader cron-session refactor proposals (separate cron sessionIds / textChunkType discriminator) are design discussions, not part of this fix.cron-msg-experimentalflag.Linked Issues
Partially fixes #6094 (items 1 and 2). Items 3 and 4 were verified as already addressed on main; items 5 and 6 stay open.
中文说明
这个 PR 做了什么
替换 QQ Bot 频道中 cron textChunk 的判别器。
handleCronTextChunk原先用streamState.has(sessionId)来判断收到的textChunk是属于进行中的 prompt（应交给 prompt 路径处理），还是属于 cron/非 prompt 流（应捕获进 cron 缓冲区）。本 PR 改为用一个专门的activePromptSessions集合来跟踪有活跃 prompt 回合的 session，由onPromptStart/onPromptEnd钩子维护，并把 cron 守卫改为基于该集合判断。该标记在disconnect()和onSessionDied中与现有流式状态清理一起清除。为什么需要
streamState.has(sessionId)不是可靠的 prompt/cron 判别器，且两个方向都会出错（issue #6094 的第 1、2 条）：blockStreaming: 'on'时消息重复 —— 块流式下onResponseChunk提前返回，从不填充streamState。在 cron 流活跃期间，prompt 响应块因此能通过streamState.has()守卫，落入cronBuffer，被 2 秒空闲 flush 再次发送，造成在 BlockStreamer 投递之外的重复消息。streamState条目（例如被取消/出错的 prompt 的 flush 尚未落定）会让守卫一直为真，导致该 session 后续所有 cron textChunk 被静默丢弃。activePromptSessions是可靠的，因为 ChannelBase 总是成对调用onPromptStart和onPromptEnd——onPromptEnd在 prompt 路径的finally中执行，即使出错/取消也会执行 —— 且该标记与流式配置无关。审阅者测试方案
如何验证
通过 QQChannel 的真实钩子（
onPromptStart/onResponseChunk/onPromptEnd、runCronFlow/_inCronFlow、bridge 的textChunk监听器）对两种失败模式做单测级复现：blockStreaming: 'on'、活跃 prompt 回合（onPromptStart）、并发 cron 流 → 对该 prompt 的 session 发出textChunk。修复前：块被捕获进cronBuffer并被重发（重复）。修复后：块被跳过，无 cron 发送。onPromptEnd时 streamState 的 flush 尚未落定）→ cron 流对同一 session 发出textChunk。修复前：被静默丢弃。修复后：被缓冲并投递。三个复现测试在
main上失败、在本 PR 上通过（已通过"应用/还原 QQChannel.ts 改动"两种方式分别验证）：另外：该包
tsc --build（通过）、对改动文件跑eslint+prettier --check（通过）。注意：cron 路径在可选配置
cron-msg-experimental开关之后，默认（稳定）路径不受影响。证据（修复前后）
N/A —— 频道适配器逻辑，无用户可见 UI。测试证据见上；修复前失败的测试：
测试环境
环境（可选）
仅单元测试（
packages/channels/qqbot下vitest run）+ 该包tsc --build。风险与范围
onPromptStart→onPromptEnd）"，即在 prompt 进行中丢弃同 session cron 块的窗口略变宽。这与既有意图一致（不把 cron 输出混入活跃 prompt），也与非块流式路径一直以来的语义相同。原有的 streamState 隔离单测已改写为驱动新判别器。send.test.ts有覆盖；setBridge会重新挂载_cronTextHandler）。第 5 条（botOpenId 指令注入时机）和第 6 条（token 刷新后connect()无重试）仍作为独立的低优先级事项开放。作者提出的更宏观的 cron session 重构方向（独立 cron sessionId / textChunkType 判别器）属于设计讨论，不在本修复范围。cron-msg-experimental之下。关联 Issue
Partially fixes #6094（第 1、2 条）。第 3、4 条已确认在 main 上解决；第 5、6 条保持开放。