fix(sdk): route unrecognized diagnostics onto a bounded transcript sidechannel - #9202
fix(sdk): route unrecognized diagnostics onto a bounded transcript sidechannel#9202yiliang114 wants to merge 3 commits into
Conversation
…dechannel Normalizer-classified unrecognized_event / unrecognized_session_update debug events no longer enter transcript blocks[]: they are mirrored onto a capped unrecognizedDiagnostics sidechannel instead. This stops them from finalizing a streaming assistant/thought block (which dropped a following assistant.usage frame) and from consuming the maxBlocks budget (which let repeated noise evict real conversation content). malformed_payload diagnostics and client-dispatched debug events keep their existing block semantics.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template: all required sections present ✓ Problem: real and observed — #8823 ships a concrete reproduction against Direction: aligned. Forward-compat noise must not corrupt real transcript state — that's a correctness bug, not cosmetics. The fix also follows the reducer's existing sidechannel pattern ( Size: no core-infrastructure paths touched ( Approach: scope feels right. The tempting smaller fix — passing Risk: no high-risk path matches; no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献! 模板:必填章节齐全 ✓ 问题:真实且已观测 —— #8823 提供了基于 方向:对齐。前向兼容噪音不应破坏真实的 transcript 状态——这是正确性 bug,不是外观问题。修复方式也沿用了 reducer 既有的 sidechannel 模式( 规模:未触及核心基础设施路径( 方案:范围合理。看似更小的修法——给 风险:未命中高风险路径,无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline before reading the diff: both symptoms (usage orphaned by What I verified beyond the diff itself:
No blockers, no convention violations. The five new reducer tests replay the issue's exact event sequences and pin both preservation cases ( Testing evidence (the PR's own CI — this review ran no PR code)The Linux unit suite on the reviewed commit was still in flight at review time; per the no-poll rule that's reported as-is, and the table below updates once CI settles. The macOS/Windows test jobs are merge-queue-only by workflow design, so their Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The central claim here is behavioural — usage survives an interleaved unrecognized diagnostic, and hidden diagnostics can't evict conversation blocks. It rests on mock-free reducer-level regression tests replaying the issue's sequences, which is the right shape of evidence, but green CI alone doesn't prove those tests fail without the change. Sandboxed verification would settle that: 中文说明代码审查读 diff 前的独立方案:两个症状(usage 被 diff 之外的核验:
无阻塞问题,无规范违规。五个新 reducer 测试回放 issue 的原始事件序列,并钉住两个保留语义的场景( 测试证据(来自 PR 自身 CI——本审查未运行任何 PR 代码)审查时评审提交上的 Linux 单元测试套件仍在运行;按不轮询规则如实上报,CI 结束后下表会自动更新。macOS/Windows 测试作业按工作流设计仅在合并队列运行,PR 上显示 (CI 表格见上) 核心声明是行为性的——usage 能在交错的未识别诊断中幸存、隐藏诊断不能驱逐会话块。证据形态正确:无 mock 的 reducer 级回归测试回放 issue 序列;但仅凭绿色 CI 无法证明这些测试在缺少改动时会失败。沙箱验证可以定论: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal fix for a real correctness bug; the only nit is the PR body missing the template's Chinese summary section. Stepping back: this is what a good SDK bugfix looks like. The problem was observed and reproducible — #8823 provides exact event sequences — and the fix routes the two offending classifications around One flag, non-blocking: Approval is deferred until CI lands green on 中文说明信心度:4/5 —— 对一个真实正确性 bug 的干净、最小修复;唯一的小问题是 PR 正文缺少模板要求的中文摘要部分。 整体回顾:这是一个好的 SDK bugfix 应有的样子。问题已观测且可复现——#8823 给出了精确的事件序列——修复把两种肇事分类完全绕开 一个非阻塞提醒: 批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
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
left a comment
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
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): test/unit/daemonUi.test.ts — no such file or directory; test/daemon-ui-transcript.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.21.12)
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Test Plan (not a blocker): test/unit/daemonUi.test.ts — no such file or directory; test/daemon-ui-transcript.test.ts — no such file or directory.
— deepseek-v4-flash via Qwen Code /review (v0.21.11)
# Conflicts: # packages/sdk-typescript/scripts/build.js
|
Closeout: merged latest main to clear the conflict and resolved the current stale/follow-up review threads. Verified conflict-file Prettier and staged diff check. Intentionally not changed: SDK API-shape/docs/offline-projection expansions. |
|
Closeout for this pass: Changed:
Verified:
Pending:
|
What this PR does
Routes normalizer-classified
unrecognized_event/unrecognized_session_updatediagnostics onto a new boundedunrecognizedDiagnosticssidechannel on the transcript state, instead of appending them toblocks[]asdebugblocks. The sidechannel is capped at 50 entries (newest kept) and is exported viaselectUnrecognizedDiagnostics, alongside the existing sidechannel fields (toolProgress,approvalMode,lastFollowupSuggestion).malformed_payloaddiagnostics and client-dispatched debug events (e.g. Web Shell's model-switch summary) keep rendering as transcript blocks exactly as before.Why it's needed
Fixes the two user-visible correctness problems from #8823, both verified on main:
appendStatusBlock, whose defaultclearActiveTextfinalized the streaming assistant block. A followingassistant.usageframe then found no active block and was silently dropped.maxBlocksbudget, so a burst of forward-compat noise could evict real user/assistant blocks. Renderer-side filtering (like the Web Shell adapter) runs strictly after these mutations, so it cannot prevent either symptom.Reviewer Test Plan
How to verify
Reproduced with reducer-level regression tests using the exact event sequences from the issue:
assistant.text.delta -> unrecognized diagnostic -> assistant.usagepreserves the active assistant block and folds the usage onto it; the diagnostic lands on the sidechannel with its classification intact.maxBlocks: 2, repeated hidden diagnostics do not evict the user/assistant blocks.malformed_payloaddebug events and client-dispatched debug events (nodebugReason) still produce transcript blocks and finalize the active text — existing semantics pinned.UNRECOGNIZED_DIAGNOSTICS_LIMIT, keeping the newest entries.test/unit/daemonUi.test.tswas updated to assert the new routing (emptyblocks[], classification preserved on the sidechannel entry).The browser daemon SDK bundle grows ~150 bytes, so
MAX_DAEMON_BROWSER_BUNDLE_BYTESis bumped 189KB -> 190KB following the existing budget-bump convention inscripts/build.js.Evidence (Before & After)
Before: on
main, the sequences above leave the assistant block without itsusage, and amaxBlocks: 2transcript plus two unrecognized diagnostics contains only the two debug blocks (red tests). After: the transcript keeps['user', 'assistant']with usage folded, and the diagnostics are inspectable viastate.unrecognizedDiagnostics(green tests,test/daemon-ui-transcript.test.ts).Tested on
Environment (optional)
Unit tests only (
vitest), plustsc --noEmitand the package build.Risk & Scope
blocks[]. Web Shell already filters them out; adapters that want them can readselectUnrecognizedDiagnostics.unrecognizedDiagnosticsis additive, and block semantics formalformed_payloaddiagnostics and client-dispatched debug events are unchanged.Linked Issues
Fixes #8823