fix(qqbot): streaming improvements — idle flush, remove splitText, replyMsgId TTL, markdown pipe - #5902
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Test coverage gaps: 55 tests cover api.ts, accounts.ts, sendMessage, and exported utilities well. However, ~750 lines of QQChannel.ts remain untested — including the inbound message pipeline (handleC2C, handleGroup, isDuplicate), WebSocket protocol state machine (handleGatewayMessage, sendIdentify, heartbeat), reconnect/recovery system, streaming buffer lifecycle (onResponseChunk/onResponseComplete/onToolCall), and state persistence/restore (including the backward-compat format migration in restoreQQState). These are the most complex and failure-prone paths in the PR.
blockStreaming interaction: When blockStreaming: "on" is configured (listed as supported in qqbot.md), chunks are dispatched to both QQChannel's streamBuffer (via onResponseChunk) and BlockStreamer, producing duplicated output. The PR description recommends blockStreaming: "off", but the code should either guard against this or the docs should explicitly mark "on" as unsupported.
— qwen3.7-max via Qwen Code /review
…diagnostics Critical fixes from review: - Per-session streamState Map prevents concurrent session buffer clobbering - Markdown fallback increments msg_seq (was reusing same seq as first attempt) - Token refresh retries indefinitely with 60s backoff (was giving up after 2 failures, = QwenLM#5411) - Close handler stores timer in this.reconnectTimer + logs skip reason (= QwenLM#5413) - fixRestoredSessions logs diagnostic when SessionRouter internals aren't found - READY state restore guarded by coldStart flag (skip on warm reconnect) - channelType changed to 'qq' to match docs and registry lookup Suggestions addressed: - flushQQState uses mode 0o600 - msgSeqMap evicted alongside replyMsgId in periodic cleanup - saveTimer/replyMsgIdCleanupTimer/seenCleanupTimer .unref()'d - Slash command log shows only command name, not arguments - READY log shows session count instead of IDs - resolveRoute logs on token refresh failure - reconnectWithRetry inner loop checks disposed after each sleep
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R4 incremental review (commits f5a850c + fce7df4): No new issues found.
The two fix commits are clean:
botOpenIdremoval is complete — no dangling references, fixes the duplicate-append-on-reconnect bug<@OPENID>format applied consistently across all 3 handlers (handleC2C, handleGroup, handleGroupAll), now aligned with the instructions
Build passed. 55/55 tests passed. tsc/eslint clean.
R3 Critical findings still outstanding (not re-posted):
handleGroupAlluses rawcontentinstead ofcleanTextgroupActiveMsgEnabledtracked but never enforced- No bot/self-message loop guard in
handleGroupAll
— qwen3.7-max via Qwen Code /review
Eric-GoodBoy-Tech
left a comment
Eric-GoodBoy-Tech
left a comment
There was a problem hiding this comment.
All 38 inline comments addressed and resolved. Summary of changes across two rounds:
Batch 1 — Bug fixes (17 items):
- #17: onToolCall now targets triggering session via
event.sessionId - #18: reconnectWithRetry increments
reconnectAttemptson exhausted gateway retries - #19: token refresh retry timer calls
.unref() - #20:
coldStart=falsemoved into.then()/.catch()after restore resolves - #21: restoreSessions
.catchnow logs error before calling onReady - #23: INVALID_SESSION handler sets
coldStart=truefor full state restore - #24: restoreQQState validates all Map entries (chatTypeMap, replyMsgId, msgSeqMap, groupActiveMsgEnabled)
- #27: groupActiveMsgEnabled guard added in sendMessage + handleGroupAll
- #28: bot self-check (
event.author.bot) in handleGroupAll - #30: streamState cleanup in handleGroupDelRobot cancels timer before delete
- #31: keywordTriggers filters empty strings
- #32:
<noreply>adds diagnostic log - #33: isDuplicate moved to top of handleGroupAll
- #34: dead
useMarkdownvariable removed
Batch 2 — System prompt & UX refinements (4 items):
- #35: handleGroup + handleC2C now emit
[atMention=true]prefix - #36: senderName sanitized (strip
[ ]) in all three handlers to prevent protocol marker spoofing - #37: redundant
[可选回复]prefix removed (superseded by[atMention=false]) - #38: C2C scoping guard added to system instructions: "以下规则仅适用于群聊消息"
Declined with comment (2 items):
- #22: streamState leak on cancel/error — ChannelBase has no onPromptEnd hook; documented in comment
- #26: raw content with
<@...>tags kept intentionally — model needs @mention syntax
TypeScript compiles clean, all 55 tests pass.
Eric-GoodBoy-Tech
left a comment
Eric-GoodBoy-Tech
left a comment
There was a problem hiding this comment.
Round 3 — 9 new inline comments addressed and resolved.
Round 2 (4 items): - QwenLM#35: add [atMention=true] prefix to handleGroup/handleC2C - QwenLM#36: sanitize senderName (strip [ ]) in all handlers - QwenLM#37: remove redundant [可选回复] prefix - QwenLM#38: add C2C scoping guard to system instructions Round 3 (9 items): - QwenLM#39: add GROUP_MESSAGE intent (bit 26) to types + IDENTIFY - QwenLM#40: fix msgSeqMap.delete key (msgId not groupId) - QwenLM#41: truncate token error body to prevent appSecret leak - QwenLM#42: add group_openid null guard to toggle handlers - QwenLM#44: remove dead hasMarkdownSyntax/hasLinkSyntax - QwenLM#45: validate gateway URL protocol (wss:/ws:) - QwenLM#46: add error logging to restore* catch blocks - QwenLM#47: add disposed check in connect() retry loop - QwenLM#43: already fixed in round 1 (isDuplicate at top)
Eric-GoodBoy-Tech
left a comment
Eric-GoodBoy-Tech
left a comment
There was a problem hiding this comment.
All 3 resolved — already addressed in previous commits:
- #48 (#39 duplicate):
sendIdentify()already includesIntent.GROUP_MESSAGE(bad2fa2). Other group events (ADD/DEL_ROBOT, MSG_REJECT/RECEIVE) are covered byGROUP_AT_MESSAGE(1<<25) per QQ Bot API docs — no separate intent bits needed. - #49 (#39 duplicate):
Intent.GROUP_MESSAGE(1<<26) already added to types.ts in bad2fa2. - #50 (#33 duplicate):
isDuplicatealready moved to top ofhandleGroupAllin round 1 (before chatTypeMap.set, policy, keyword matching).
GitHub may have been showing the reviewer an older diff — current head (bad2fa2) has all three fixes.
Eric-GoodBoy-Tech
left a comment
Eric-GoodBoy-Tech
left a comment
There was a problem hiding this comment.
Round 4 — 10 inline comments addressed:
Fixed (8):
- #51: replyMsgId only updated when isAtBot in handleGroupAll
- #52: senderName truncated to 64 chars (.slice(0, 64)) in all 3 handlers
- #53: heartbeatTimer now calls .unref()
- #55: token refresh retry capped at 10 attempts with FATAL log
- #56: chatTypeMap/groupActiveMsgEnabled evicted alongside replyMsgId TTL
- #57: saveQQState now writes to .tmp then renameSync for crash safety
- #58: send.test.ts TS2322/TS4111 — bracket notation + type cast
- #59: send.test.ts TS2749 — import type QQChannel as QQChannelClass
Already fixed (1):
- #54: tokenRefreshTimer .unref() + disposed guard already added in round 1
Already resolved (1):
- #60: hasMarkdownSyntax already removed in round 3
TypeScript compiles clean, 42 tests pass.
- QwenLM#51: only update replyMsgId when isAtBot (prevent non-@ overwrite) - QwenLM#52: truncate senderName to 64 chars (prompt injection defense) - QwenLM#53: heartbeatTimer .unref() (prevent process hang) - QwenLM#55: token retry max 10 attempts with FATAL log - QwenLM#56: evict chatTypeMap/groupActiveMsgEnabled alongside replyMsgId - QwenLM#57: saveQQState atomic write (tmp+rename for crash safety) - QwenLM#58: send.test.ts TS2322/TS4111 type fix - QwenLM#59: send.test.ts TS2749 type fix
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings on unchanged code (cannot be inline-commented):
[Critical] reconnectTimer missing .unref() in close handler (line 898): The close-handler's setTimeout for reconnectWithRetry() lacks .unref(), preventing Node.js process exit for up to 30s during shutdown. Regression — the fallback timer at line 1152 correctly calls .unref().
[Critical] flushQQState() lacks atomic write (line 562): Writes directly to qqStatePath without the tmp+rename pattern used by saveQQState(). A crash during disconnect corrupts the state file, losing all persisted routing state.
[Critical] Test coverage gaps: The PR removes ~326 net lines of tests but adds no tests for critical new behaviors: <noreply> suppression, groupActiveMsgEnabled blocking, replyMsgId TTL expiry, handleGroupAll (all policy branches), streaming idle-flush timer, and token refresh retry exhaustion.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R7 incremental review
This review focuses on the delta since R6 (a4abddaf9 → 35cedd922). Deterministic: tsc --build on @qwen-code/channel-qqbot is clean (no type errors).
R6 Critical findings — resolution check
| R6 finding | Status in R7 |
|---|---|
msgSeqMap not updated after plain-text fallback succeeds (R6 #3487745517) |
Fixed — QQChannel.ts:347 sets sentSeq when it differs from nextSeq |
event.author.bot accessed without null guard in handleGroupAll (R6 #3487745520) |
Fixed — QQChannel.ts:1463 adds the guard |
INVALID_SESSION re-IDENTIFY has no readyTimeout (R6 #3487745524) |
Partially fixed — new timeout is installed (line 1102) but see inline Suggestion below for a timer-leak edge case |
chatTypeMap.set ordering inconsistent with handleGroup (R6 #3487745527) |
Fixed — chatTypeMap.set now runs before guards at line 1445 |
disconnect() does not clear readyTimeout (R6 #3487745529) |
Fixed — QQChannel.ts:405 clears it |
| JSDoc says cascade hits 4 maps but code only hits 1 (R6 #3487745531) | Fixed — JSDoc updated at line 538 |
Intent enum missing GROUP_MESSAGE / sendIdentify only sends 2 intents (R6 #3487360051, #3487418155) |
Fixed — types.ts:21 adds GROUP_MESSAGE: 1 << 26; QQChannel.ts:1142 sends all 3 |
msgSeqMap.delete(groupId) uses wrong key type (R6 #3487360052) |
Fixed — code now looks up replyEntry.msgId first (line 1405) |
handleGroupMsgReject/handleGroupMsgReceive missing group_openid null guard (R6 #3487360056) |
Fixed — guards at lines 1423, 1432 |
fetchAccessToken leaks appSecret in error body (R6 #3487360054) |
Re-evaluated as non-issue — api.ts:39 only interpolates the response body (QQ's token endpoint does not echo the request); body.slice(0, 80) further limits exposure |
Outstanding Critical findings from R1–R6 (not addressed by R7, still open)
handleGroupmissinggroupActiveMsgEnabledguard (R6 #3487625073)- No bot/self-message loop guard in
handleGroupAll(R3 #3486984928) senderNamesanitization misses fullwidth brackets (R6 #3487447896)- TOCTOU race on
msgSeqMapacrossawait sendQQMessage()(R6 #3487447895) replyMsgId.set()before the empty-content check (R6 #3487447893)chatId = 'unknown'fallback causes cross-user collision (R6 #3487680979)groupAllPolicylacks runtime validation (R6 #3487680980)globalSessionsPathshared across channels whenstart.tsis the router owner (R6 #3487680984)
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R8 incremental review (commit 94c6baf4c)
Scope: 4-line change in QQChannel.ts INVALID_SESSION handler. No new findings on the incremental diff.
Resolved from R7
- [Suggestion] readyTimeout timer-leak (prior comment at line 1106): now correctly clears the prior timer with
clearTimeout(this.readyTimeout)+nullassignment before reassigning.readyTimeout: ReturnType<typeof setTimeout> | nullmakes the guard type-safe. Pattern matches every otherreadyTimeoutreassignment site in the file (lines 407, 911, 1009, 1073). ✅
Still outstanding from R1–R6 (8 Criticals, unchanged code)
These are on code not touched by this commit — listed for tracking, not re-flagged inline:
handleGroupmissinggroupActiveMsgEnabledgating- No self-message loop guard on outbound
- Fullwidth bracket
[/]accepted byhasLinkSyntax/isValidChatId - TOCTOU race on
msgSeqMapread-then-write replyMsgIdset before the "empty text" early-returnchatId='unknown'collision across chatsgroupAllPolicyparsed without validationglobalSessionsPathshared mutable state withstart.ts
Deterministic
tsc --buildon@qwen-code/channel-qqbot: clean (0 errors, 0 warnings).- CI (🧐 Qwen PR Review workflow): all pass (authorize SUCCESS; review-config / ack / delay / resolve / review-pr all SKIPPED/completed).
— qwen3.7-max via Qwen Code /review
|
@wenshao Thanks for R7/R8. Quick note on the 8 "outstanding" items — I believe most were already addressed in R5/R6:
If these look correct on your end, could we dismiss the stale |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R10: All 5 R9 findings addressed. Verified correct:
- ✅
saveQQState()aftermsgSeqMapupdate (debounced 500ms, no I/O concern) - ✅
blockStreamingguard inonResponseChunk(matches Feishu pattern) - ✅
state?.buffer ?? _fullTextfallback inonResponseComplete(safe — base class calls this only when blockStreaming is off via mutually exclusivestreamer.flush()vsonResponseComplete()paths) - ✅
renameSyncmock added in both test files - ✅ Test expectation updated for fullText fallback
Tests pass (59/59), typecheck clean, eslint clean. Downgraded from Approve to Comment: 1 CI check still pending (Test ubuntu-latest).
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R10 reviews the incremental diff (3 files, +7/-3) addressing R9 findings. All 5 fixes are correct and 59 tests pass.
[Suggestion] msgSeqMap persistence gap on error path (QQChannel.ts:339)
The R9 commit added saveQQState() on the success path (line 349) for msgSeqMap durability. The error-path rollback at line 338 (this.msgSeqMap.set(msgId, nextSeq - 1) on HTTP failure) also mutates msgSeqMap but does not call saveQQState(). If the process crashes before the next successful send, the rollback is lost and the old (higher) seq value is restored, potentially causing QQ API duplicate-seq rejection on restart. Consider adding if (msgId) this.saveQQState(); before the return at line 339 to make persistence symmetric across both paths.
|
@qwen-code /triage |
|
Thanks for the PR and the extensive iteration across R1–R19! Template looks good ✓ On direction: this is solidly aligned with qwen-code's channel architecture. QQ Bot is an officially supported channel, and the streaming/BlockStreamer mismatch is a real user-facing problem — long tool-call outputs get merged into unreadable walls of text. The group full-message feature ( On approach: the PR title says
The streaming core (idle-flush replacing BlockStreamer, 中文说明感谢贡献以及 R1–R19 的大量迭代！ 模板完整 ✓ 方向：与 qwen-code 的 channel 架构完全对齐。QQ Bot 是官方支持的 channel，BlockStreamer 与流式输出的不匹配是真实的用户痛点——长工具调用输出被合并成无法阅读的大段文字。群聊全量消息功能（ 方案：PR 标题是
流式核心（idle-flush 替代 BlockStreamer、移除 — Qwen Code · qwen3.7-max |
Code ReviewI read the full diff (5495 lines across 11 files). The core change is What I'd have done (independent proposal): Replace BlockStreamer with a per-session timer-based buffer, add TTL to replyMsgId entries, unify slash detection across the three message paths, and add a policy-gated Correctness — no blockers found:
Security — solid:
Minor observations (non-blocking):
Real-Scenario TestingQQ Bot requires QQ Open Platform credentials (appID + appSecret) for real-scenario testing — these aren't available in this CI environment. Instead, relying on the comprehensive verification already performed:
The existing verification is the strongest available evidence for this change class. 中文说明代码审查我阅读了完整 diff（11 个文件，5495 行）。核心变更在 我的独立方案：用按会话的计时器缓冲区替代 BlockStreamer，给 replyMsgId 加 TTL，统一三条消息路径的斜杠命令检测，加一个策略门控的 正确性——无阻塞项：
安全——扎实：
次要观察（不阻塞）：
真实场景测试QQ Bot 需要 QQ 开放平台凭证（appID + appSecret），本 CI 环境中不可用。依赖已有验证：
— Qwen Code · qwen3.7-max |
|
@qwen-code /resolve |
19 similar comments
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /resolve |
|
@qwen-code /triage |
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts, but could not push to Merge Conflict Resolution Summary — PR #5902Branches
Conflicted Files (6)1.
|
|
This PR has been superseded by the split PRs:
All code from this PR is now on main through the split PRs. This PR can be closed once #6206 lands. |
|
Qwen Code resolved the merge conflicts, but could not push to Merge Conflict Resolution Summary
Resolved filesOnly conflicted files were touched. No unrelated edits.
|
|
Update: PR #6206 has been closed and replaced by #6457 (clean squash, 1 commit).
|
5 participants
What this PR does
Refactors QQ Bot streaming behavior: replaces BlockStreamer coalescing with a 2-second idle flush, removes a self-imposed 2000-character limit, adds 5-minute TTL to passive reply tracking, and fixes markdown table detection.
Adds P1 group features: full-message handling with groupAllPolicy, slash command unification across all message paths, noreply mechanism for non-@ messages, and @mention support with default Markdown replies.
Why it's needed
BlockStreamer incompatible with QQ Bot. BlockStreamer coalesces text by paragraph boundary (
\n\nat minChars=400). QQ Bot cannot edit sent messages, so coalesced chunks arrive as a single undecipherable message — e.g., 15 short tool-call progress updates get merged into one.Self-imposed character limit.
splitText()capped messages at 2000 characters with no API basis. QQ Bot Open Platform imposes no content length limit on text or markdown messages.Expired replyMsgId. Passive replies require
msg_idwithin a 5-minute window. Old code never removed expired entries, causing API rejections.Tables sent as plain text.
hasMarkdownSyntaxdid not detect pipe tables, so pure tables (no other markdown markers) were sent asmsg_type=0and rendered as raw text in QQ.No group full-message support. QQ Bot API v2 supports
GROUP_MESSAGE_CREATEevents (since 2025-06-22) when the group owner enables full-message permission. Previously all non-@ messages were silently dropped.Slash commands only worked in one path. C2C and full-message paths lacked slash command detection, causing
/help,/new,/clearto be treated as plain text.Changes
Streaming & Reply Infrastructure
BlockStreamer+splitTextwithonResponseChunkidle-flush (2-second timer)replyMsgIdCleanupTimer(5-minute TTL) to prevent stale passive replieshasMarkdownSyntax— all replies default to markdown (msg_type=2), API rejection triggers plain-text fallbacksplitText(no API character limit exists)P1 Group Features
handleGroupAll()withgroupAllPolicy(log|keyword|all) andkeywordTriggersGROUP_ADD_ROBOT,GROUP_DEL_ROBOT,GROUP_MSG_REJECT,GROUP_MSG_RECEIVEgroupActiveMsgEnabledper-group with state persistencehandleGroup,handleGroupAll, andhandleC2Cmsg_type=2) for @mention supportopenid/member_openidinto message context prefixbotOpenIdto model context[可选回复]prefix for non-@ messages, model outputs{{skip}}to skipSecurity hardening
api.ts: validate gateway URL protocol (wss:only) vianew URL()parsingQQChannel.ts: usesanitizeSenderName()andsanitizeLogText()from@qwen-code/channel-basefor all user-controlled input in prompts and audit logsQQChannel.ts:sanitizePromptText()applied to user message content in all three handlers to prevent prompt injection via invisible charactersQQChannel.ts: bot messages tagged with[bot]prefix for model judgment instead of hard blockReviewer Test Plan
How to verify
cd packages/channels/qqbot && npx vitest run— all 158 tests pass.settings.jsonwithblockStreaming: "off".groupAllPolicy: "all", and send/newin a group (with and without @mention).Evidence (Before & After)
Before: Model outputs 10 lines of tool-call progress → BlockStreamer merges them → one massive message.
After: Each status line appears as a separate message. The 2-second idle timer ensures complete sentences are sent as units.
Before (slash commands):
/newin C2C or full-message paths → treated as plain text → sent to LLM.After:
/newcorrectly intercepted byparseCommand()in all three paths.Tested on
Risk & Scope
contentfield (documented as required for groups but API appears to tolerate its absence).blockStreamingconfig for QQ Bot should be set to "off" — existing users with "on" will see no change unless they update their config. ThesplitTextfunction is removed.groupAllPolicy,keywordTriggers,groups.*.requireMention,allowMention,chatTypes— all backward-compatible with sensible defaults.cron-msg-experimental(replacesexperimentalfrom earlier draft) — gates all cron/non-prompt textChunk handling;falseby defaultLinked Issues
Closes #5901
Refs #6094 — cron/blockStreaming interaction resolved,
cron-msg-experimentalconfig replacesexperimental