fix(sdk): route unrecognized diagnostics onto a bounded transcript sidechannel - #9202
Merged
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.
Collaborator
Collaborator
|
Re-run on the current head — the prior pass (2026-08-15) predates the last several pushes. Template: all required sections present ✓ Problem: observed bug, not theory — #8823 was filed by a different user (labeled Direction: aligned — a correctness fix in the daemon-UI transcript layer. The sidechannel follows the established pattern ( Size: ~253 production lines (119 in the transcript reducer, 68 in types, 43 in the webui provider, barrels + budget bump), ~698 lines of tests, 20 lines of docs. Under the 500-line bar — no size-based escalation. Approach: the scope is right. The reducer routing alone would not be enough — the webui provider drops Risk: no elevated risk signals — none of the revert-correlated paths are touched. Moving on to code review. 🔍 中文说明在当前 head 上重新运行——上一次审查（2026-08-15）早于最近几次推送。 模板： 必填章节齐全 ✓ 问题： 是已观测到的 bug，不是理论问题——#8823 由另一位用户提交（标签 方向： 对齐——daemon UI transcript 层的正确性修复。sidechannel 遵循既有模式（ 规模： 约 253 行生产代码（transcript reducer 119 行、types 68 行、webui provider 43 行，其余为 barrel 导出与预算调整），约 698 行测试，20 行文档。低于 500 行门槛，无需因规模升级。 方案： 范围合理。仅改 reducer 路由是不够的——webui provider 会在 burst 守卫周围丢弃 风险： 无升级风险信号——未触及与 revert 相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent baseline before reading the diff: route the normalizer-classified What stands out:
Non-blocking nits:
No critical blockers found. Test evidence — PR's own CI on the reviewed head (via API, PR code not executed here)All checks on the reviewed commit are settled; every real job is green:
The one cancelled Sandboxed verification would settle the remaining gap: 中文说明代码审查读 diff 前的独立基线：把 normalizer 归类的 亮点：
非阻塞小问题：
未发现关键阻塞项。 测试证据（审查 commit 上 PR 自身的 CI，经 API 获取；本审查未执行 PR 代码）审查 commit 上所有 check 已结束，真实 job 全部绿色（表格见英文部分）：ubuntu 单测、Windows/ubuntu Desktop Shell、web-shell E2E Smoke、web-shell 视觉采集、依赖 CVE 审计、TruffleHog 密钥扫描均通过；macOS/Windows 单测与 CLI 集成测试在本仓库同源 run 中被跳过。唯一 cancelled 的 沙箱验证可以补上剩余缺口： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — a textbook correctness fix: real bug with a linked report, the minimal routing change, and tests that would fail if the fix were reverted. Reservations are procedural, not about the code: the branch currently conflicts with Stepping back: the approach matches my independent proposal exactly, and it goes further in the right directions — a shared runtime classification so both routing guards can't drift apart, correlation fields preserved for developer tooling, and a bounded cap so long sessions stay inspectable without growing unbounded. Every hunk earns its place; the webui guard narrowing and the history-paging merge are load-bearing for the fix reaching users, not scope creep. Six months from now this reads like the reference implementation for the forward-compat principle it documents. The only thing a human should second is the intentional rewind behavior (sidechannel survives Procedural flag: the PR is currently Approving on the code. ✅ 中文说明置信度：4/5 —— 教科书式的正确性修复：真实 bug 且有关联报告、最小化的路由改动、以及"回滚修复即失败"的测试。保留意见是流程性的，与代码无关：分支当前与 整体看：方案与我的独立设想完全一致，还在正确的方向上走得更远——共享运行时常量分类让两处路由守卫不会漂移、为开发者工具保留关联字段、有界上限让长会话可查且不无限增长。每个改动块都有存在价值；webui 守卫收窄与历史分页合并是修复触达用户的必要环节，不是范围蔓延。六个月后读起来就是它所记录的前向兼容原则的参考实现。唯一值得人工复核的是 rewind 时 sidechannel 刻意保留的行为，但这已有文档和测试钉死。 流程提示：PR 当前与 基于代码，批准。✅ — 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
previously approved these changes
Aug 15, 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.
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
reviewed
Aug 15, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
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
Collaborator
Author
|
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. |
Collaborator
Author
|
Closeout for this pass: Changed:
Verified:
Pending:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
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; 1560 tests pass — this review observed 1575, 20169, 494, 3609, 498 passed.
— qwen3.8-max via Qwen Code /review (v0.21.12)
Collaborator
Author
|
Closeout from resolve-pr-comments: Changed: no code. Restored the PR body Chinese details section from the current template. Pending: existing review/CI snapshot only. |
- export the sidechannel API through the daemon barrel (selectUnrecognizedDiagnostics, UNRECOGNIZED_DIAGNOSTICS_LIMIT, DAEMON_UI_UNRECOGNIZED_DIAGNOSTIC_REASONS + types) and pin the reachability in daemon-public-surface.test.ts - restore the MAX_TEXT_BLOCK_LENGTH cap on sidechannel text, mirroring truncateText exactly (suffix fits within the cap) - ship the unrecognized reason subset as a runtime const array and route by membership, so a new reason cannot fall through to appendStatusBlock - copy the correlation fields createBase stamps (promptId, sourceRecordIds, branchRecordId, originatorClientId) onto sidechannel entries; drop the dead source/data switches - un-fuse the budget-history comment chain in scripts/build.js - update docs/developers/daemon-ui for the split routing - tests: full entry shape, text cap, block-path debugReason counterpart, and a webui malformed_payload interleave sibling so the #7012 flush-before-guard keeps a discriminating stimulus
Collaborator
Author
|
Closeout — review-round fixes (b2a83e0) Changed
Verified
Intentionally not changed
Pending
— patrol auto-closeout (resolve-pr-comments) |
qwen-code-ci-bot
requested changes
Aug 15, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
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.
中文说明
Test Plan（非阻断）：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)
- build.js: bump daemon browser bundle budget 191KB -> 192KB (195,591 bytes measured > 195,584 cap; build failed at head) - webui: narrow the observer-mode debug guard so unrecognized_* diagnostics reach the reducer sidechannel; only block-path debug events are dropped - webui: merge history-store unrecognizedDiagnostics in applyTranscriptHistory so paged-back sessions keep diagnostics - transcript: extract truncateTextAtLimit shared by the block and sidechannel truncation paths - transcript: reset unrecognizedDiagnostics on rewind alongside the sibling per-turn state resets - types: rename DaemonUnrecognizedDiagnostic.receivedAt to clientReceivedAt (matches the sibling block projection) - tests: reason-prefix conformance pin, rewind reset, narrowed guard, history pagination merge
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.
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; 1560 tests pass — this review observed 1582, 500, 20166, 494, 3609 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：test/unit/daemonUi.test.ts — no such file or directory; test/daemon-ui-transcript.test.ts — no such file or directory; 1560 tests pass — this review observed 1582, 500, 20166, 494, 3609 passed。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Collaborator
Author
|
Closeout summary: latest head already merged main and pinned sidechannel pagination; I confirmed the conflict is cleared and SDK build passes on that resolution. Resolved the pagination test thread. I did not expand the PR into a public SDK guard export or a CI/typecheck-scope change for erased type-only exports. |
Collaborator
Author
|
Closeout for this patrol: merged latest main, resolved the sidechannel pagination test gap, and pushed 8dbabf5. Verified with the focused DaemonSessionProvider test, eslint on the touched files, and diff-check. Remaining threads are intentionally left for maintainer/API-scope decisions; CI is still pending. |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Collaborator
Author
|
Closeout from resolve-pr-comments: no code change. Resolved the remaining review suggestions as out of scope for this PR: exporting a new SDK guard would expand the public surface, and making type-only surface tests load-bearing requires a CI typecheck-scope change. Verified: active review threads now 0. Pending: automatic review-pr is still running. |
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.
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.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：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)
Collaborator
Author
|
Merged latest main and resolved the one SDK build-budget conflict. Changed: kept this PR's unrecognized-diagnostic sidechannel budget note and main's newer 196KB daemon browser bundle budget. Verified: conflict markers absent, git diff --check passed, and node --check passed for packages/sdk-typescript/scripts/build.js. Pending: CI/review on 53a5a88. |
Collaborator
Author
|
Follow-up on the post-merge red CI. Cause: the exact-head build failed because the daemon SDK browser bundle was 201585 bytes while the merged budget was 196KB. Post coverage and visuals failed downstream from the same build step. Changed: raised the budget to 197KB, scoped to this sidechannel PR after merging current main. Verified: node --check and git diff --check passed. Local workspace build was attempted but blocked by local esbuild package resolution; the CI failure itself showed the generated bundle size. Pending: CI/review on 6b5a533. |
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.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-1 routing membership predicate duplicated across the SDK/webui boundary — already reported (comment 3792307918)
- stale 190→191 / 191→192 budget ledger entries in scripts/build.js — already reported (comment 3789733135)
- R4-4 applyTranscriptHistory newest-wins eviction untested — already reported (comment 3790160498)
- R5-2 selectUnrecognizedDiagnostics pinned only by a typeof check — already reported (comment 3792307924)
- sidechannel array not frozen under FREEZE_TRANSCRIPT_BLOCKS — already reported (comment 3788840467)
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; 1560 tests pass — this review observed 1603, 531, 21077, 494, 3723 passed.
中文说明
已审查——无阻断问题。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：test/unit/daemonUi.test.ts — no such file or directory; test/daemon-ui-transcript.test.ts — no such file or directory; 1560 tests pass — this review observed 1603, 531, 21077, 494, 3723 passed。
— qwen3.8-max via Qwen Code /review (v0.21.13)
# Conflicts: # packages/sdk-typescript/scripts/build.js
Collaborator
Author
|
Closeout update: merged latest main to resolve the conflict and pushed 1e6f922. Conflict was limited to the SDK bundle-budget comment; kept the 197KB cap and combined both current reasons. Verified conflict markers are gone, node --check passes for the build script, and git diff --check passes. Post-push state is mergeable with CI/review queued or running. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Reviewed diff-only — the PR’s existing discussion could not be fetched, so this is not an approval and not a no-blockers claim. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 applyTranscriptHistory merge-eviction test gap — already reported at packages/webui/src/daemon/session/DaemonSessionProvider.tsx:338 (comment 3792307902)
Unresolved, please confirm:
- [Critical] packages/sdk-typescript/scripts/build.js (comment 3789936164, commit b2a83e0, outdated anchor): existing '[Critical]' budget blocker — cannot tell: PR discussion context could not be fetched (GitHub 404 on the reviews endpoint), full body …
- [Critical] packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx:4337 (comment 3792724757, current head): existing '[Critical]' paging-test blocker — cannot tell: PR discussion context could not be fetched (GitHub 404 on the reviews endpoin…
Not reviewed: issue-fidelity — pr-context unavailable (GitHub 404 on the reviews endpoint); Agent 0 skipped per context-unavailable rules.
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 6a": verifying whether daemon live-journal truncation can split a single record's update burst, and whether transcript pagination serves records merged by compaction….
Not reviewed: the linked-issue fidelity pass — its prompt was built, but no agent on record was launched with it.
Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies.
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.
中文说明
仅完成部分审查，审查缺口已披露。 仅审查了 diff——无法获取 PR 已有的讨论，因此这不构成批准，也不构成"无阻断问题"的结论。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 2 条（原文未翻译，列表见上方英文部分）。
未审查：issue-fidelity — pr-context unavailable (GitHub 404 on the reviews endpoint); Agent 0 skipped per context-unavailable rules。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6a"：verifying whether daemon live-journal truncation can split a single record's update burst, and whether transcript pagination serves records merged by compaction…。
未审查：关联 issue 一致性检查——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
未审查：验证与反向审计——两者都运行并打开了各自的 brief，但都不是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，发布的发现不能算作已验证，agent 实际被要求做的也不是本 skill 所认证的内容。
Test Plan（非阻断）：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.13)
… diagnostics selector - The sidechannel routing + selector cost ~1037 B over the 197KB cap (bundle measured 201893 B), failing the browser-bundle size gate; bump MAX_DAEMON_BROWSER_BUNDLE_BYTES to 198 * 1024. - Fold the rebase-residue 190→191→192 KB ledger entries into the accurate 190→195→196→197→198 lineage so the next bump has one canonical history. - Add a behavioral pin for selectUnrecognizedDiagnostics: it must return the routed sidechannel itself (toBe), discriminating a `return []` or shallow-copy regression that the typeof-only surface test cannot see; flip-verified.
Collaborator
Author
|
Closeout for the round findings, pushed in 6150a4e:
SDK transcript + public-surface suites green (39/39); changed files lint-clean. All four threads resolved. |
qwen-code-ci-bot
requested changes
Aug 17, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
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:
- R1-4 selectUnrecognizedDiagnostics typeof-only pin / no call site — already reported (comment 3797430922)
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none — the walk completed; candidates dropped for lack of a concretely reachable trigger were: cap-eviction dedupe hole in paging, chat-record projection diagno….
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; 1560 tests pass — this review observed 1610, 531, 21239, 494, 3723 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
docs/developers/daemon-ui/README.md:393 — [review] new doc guarantees malformed_* diagnostics stay in the transcript, but the observer assistant-guard this PR re-narrows still drops them silently (no block, no sidechannel entry, no log)packages/sdk-typescript/src/daemon/ui/transcript.ts:1336 — [probe] sidechannel path skips the onTruncation report — offline projection returns complete: true with no transcript_text_truncated diagnostic for lossy >100KB unrecognized payload…
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：none — the walk completed; candidates dropped for lack of a concretely reachable trigger were: cap-eviction dedupe hole in paging, chat-record projection diagno…。
Test Plan（非阻断）：test/unit/daemonUi.test.ts — no such file or directory; test/daemon-ui-transcript.test.ts — no such file or directory; 1560 tests pass — this review observed 1610, 531, 21239, 494, 3723 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…the routing predicate appendUnrecognizedDiagnostic left activeUserBlockId untouched while the replaced appendStatusBlock path reset it for every non-user block; a later mergeable user.text.delta with no promptId stamp (e.g. a peer client's $ <cmd> echo) then appended onto the earlier user block across the diagnostic, collapsing two user turns into one and skewing rewindTranscriptToUserTurn's kind==='user' turn indexing. Keep the reset (assistant/thought pointers stay untouched, the point of the sidechannel); witness test flip-verified red without the one-line reset. Also export isUnrecognizedDiagnosticReason from types.ts next to DAEMON_UI_UNRECOGNIZED_DIAGNOSTIC_REASONS and call it at all three routing-guard sites (reducer, provider flush condition, provider drop filter) so the #7012/#8823 guard pair classifies every debug event against one source instead of three hand-written copies.
Collaborator
Author
|
Bot-review round closeout (pushed 99c945d):
Verified: sdk transcript/daemonUi 325/325, webui provider 219/219, sdk + webui typecheck, prettier + eslint clean on touched files. Both threads resolved. |
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.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- newest-wins cap-eviction branch of applyTranscriptHistory is mutation-uncovered (paging test lands exactly on the cap) — already reported as R4-4 (comment 3790160498)
- offline chat-record projection discards the routed sidechannel — already reported (comment 3788840468)
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; 1560 tests pass — this review observed 1611, 531, 21239, 494, 3723 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/sdk-typescript/src/daemon/ui/transcript.ts:1784 — [probe] sidechannel truncation skips the onTruncation completeness report — offline projection returns complete: true for lossy >100KB unrecognized payloads (continues round-2 defer…
中文说明
已审查——无阻断问题。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：test/unit/daemonUi.test.ts — no such file or directory; test/daemon-ui-transcript.test.ts — no such file or directory; 1560 tests pass — this review observed 1611, 531, 21239, 494, 3723 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Collaborator
Author
|
@qwen-code /review --timeout=360 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- offline chat-record projection discards the routed sidechannel — already reported (comment 3788840468)
- newest-wins cap-eviction branch of applyTranscriptHistory is mutation-uncovered (paging test lands exactly on the cap) — already reported as R4-4 (comment 3790160498)
Not explored to full depth (tool budget reached): "agent 4": none — no check was cut short..
Test Plan (not a blocker): 1560 tests pass — this review observed 1611, 531, 21236, 494, 3723 passed.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx:3381 — [review] re-anchored #7012 test comments describe the removed filtering mechanism
中文说明
已审查。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 4"：none — no check was cut short.。
Test Plan（非阻断）：1560 tests pass — this review observed 1611, 531, 21236, 494, 3723 passed。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/9202/merge is unavailable — resolve conflicts and re-run. 中文 — 判定：
|
qwen-code-ci-bot
previously approved these changes
Aug 19, 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. ✅
doudouOUC
reviewed
Aug 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Independent review — verified against HEAD 99c945d9
Reviewed the full diff plus the end-to-end wiring of the new sidechannel. The implementation itself is clean and I found no correctness defects. One mechanical blocker (merge conflict) keeps me from approving, plus one unrelated nit.
What I verified
- Not a dead switch.
unrecognizedDiagnosticsis populated in the reducer, exposed through a selector (transcript.ts), and actually consumed by the webui provider — history and live entries are merged inapplyTranscriptHistory. Read sites exist outside the reducer, so the new state field is genuinely wired. - Bounded on both paths. The reducer caps at
UNRECOGNIZED_DIAGNOSTICS_LIMIT = 50on append, and the history merge re-applies.slice(-UNRECOGNIZED_DIAGNOSTICS_LIMIT)— so concatenating a capped history page with a capped live array cannot yield 100 entries. Per-entry text keeps the previousMAX_TEXT_BLOCK_LENGTHcap via the extractedtruncateTextAtLimit, which the block path now shares. - Reference-stability contract preserved.
cloneTranscriptStateforwards the array by reference and the reducer replaces it wholesale rather than mutating in place, matching the existinglastFollowupSuggestionconvention that lets subscribers skip re-renders. - The
activeUserBlockIdreset is deliberately retained with a rationale for why dropping it would let a later un-stamped mergeableuser.text.deltacollapse two turns and skewrewindTranscriptToUserTurn. Keeping the streaming assistant/thought pointer untouched is exactly the point of the sidechannel, and the doc comment explains why renderer-side filtering could not have fixed either symptom. - History-page dedup covers the new channel —
materializeTranscriptHistoryseedsdisplayedRecordIdsfrom diagnosticsourceRecordIdsas well as block ones, so a re-fetched page can't double-count. - 0 unresolved review threads; CI green (15/15 pass).
🔴 Blocker: merge conflict (single file)
mergeable: CONFLICTING / mergeStateStatus: DIRTY. I resolved the scope with a read-only merge-tree against current main: every substantive file auto-merges cleanly; the only conflict is packages/sdk-typescript/scripts/build.js, and it is purely a comment-block collision — see the inline note for the exact resolution and the one thing that needs re-measuring afterwards.
🟡 Nit (unrelated to this PR's intent)
The diff flips packages/sdk-typescript/scripts/build.js from mode 100755 to 100644, dropping its executable bit. Functionally harmless — package.json invokes it as node scripts/build.js, and there is no shebang — but it's an incidental permission change in a PR about diagnostics routing, so it's worth restoring 100755 to keep the diff to its subject.
Once rebased (and the budget re-measured), this is good to go from my side.
…-merge # Conflicts: # packages/sdk-typescript/scripts/build.js
yiliang114
enabled auto-merge
August 19, 2026 14:00
doudouOUC
approved these changes
Aug 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Re-review after rebase — verified against HEAD 02194093
Verdict: APPROVE. My previous blocker is resolved and I found no new defects.
Previous blocker — fixed
The merge conflict is gone (mergeable: MERGEABLE). The PR was rebased onto d96f264d, and the build.js conflict was resolved in exactly the right shape: main's fuller rationale is kept and this PR's 197KB → 198KB sidechannel line is appended on top; the // surface after merging current main. placeholder that would have overwritten a still-applicable rationale is gone.
The re-measure concern is answered by CI rather than by assumption: build.js throws Browser daemon SDK bundle is N bytes; expected <= … when the emitted bundle exceeds the constant, so a green build on the rebased base is direct evidence that 198KB still fits after #9405's bundle additions. Details in the resolved thread.
Carried forward from my first pass (all still hold at this HEAD)
unrecognizedDiagnosticsis genuinely wired — reducer → selector → webui provider, with read sites outside the reducer, so it is not a dead field.- Bounded on both paths: capped at
UNRECOGNIZED_DIAGNOSTICS_LIMIT = 50on append, and the history merge re-applies.slice(-UNRECOGNIZED_DIAGNOSTICS_LIMIT), so a capped history page concatenated with a capped live array cannot double the cap. Per-entry text keeps the priorMAX_TEXT_BLOCK_LENGTHbound via the sharedtruncateTextAtLimit. - Reference-stability contract preserved in
cloneTranscriptState; the reducer replaces the array wholesale instead of mutating in place. - The
activeUserBlockIdreset is deliberately retained, so a later un-stamped mergeableuser.text.deltacan't collapse two turns and skewrewindTranscriptToUserTurn— while the streaming assistant/thought pointer stays untouched, which is the point of the sidechannel. - History-page dedup seeds
displayedRecordIdsfrom diagnosticsourceRecordIdsas well as block ones, so a re-fetched page can't double-render.
Notes (non-blocking, no action required to merge)
- The mode change is still present.
packages/sdk-typescript/scripts/build.jsremains100755 → 100644in the diff. Functionally inert —package.jsonruns it asnode scripts/build.jsand there is no shebang — so this is cosmetic diff noise rather than a defect. Worth restoring if you touch the branch again, not worth a round on its own. - The one added file outside the PR's subject is legitimate.
scripts/tests/qwen-autofix-workflow.test.jsgains acat > /dev/nullstdin drain in a stub script; that follows an existing convention in the same file (two instances already onmain), so it reads as a stub-hygiene fix rather than unrelated scope creep.
CI is green apart from review-pr, which is the bot's own review job.
wenshao
approved these changes
Aug 19, 2026
Collaborator
|
Released in v0.21.15. |
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
4 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
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.packages/sdk-typescript/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,packages/sdk-typescript/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
中文说明
本 PR 做了什么
把 normalizer 归类出来的
unrecognized_event/unrecognized_session_update诊断放到 transcript state 新增的有界unrecognizedDiagnosticssidechannel，而不是追加到blocks[]的debugblock。sidechannel 最多保留 50 条（保留最新），并通过selectUnrecognizedDiagnostics导出；malformed_payload诊断和客户端主动派发的 debug 事件仍然按原语义渲染为 transcript block。为什么需要
修复 #8823 的两个可见正确性问题：未识别诊断会 finalize 正在 streaming 的 assistant block，导致后续 usage 丢失；隐藏诊断还会占用
maxBlocks预算，挤掉真实 user/assistant block。渲染端过滤发生在状态变更之后，无法阻止这些问题。Reviewer 测试计划
用 issue 中的 reducer 事件序列验证：diagnostic 不再打断 usage；
maxBlocks: 2下不会驱逐真实 block；malformed_payload和客户端 debug 事件保持原行为；sidechannel 有界且保留最新。还更新了 SDK daemon UI 既有 pin。验证包含 SDK tests、tsc --noEmit和 package build。风险与范围
主要取舍是未识别诊断不再出现在只遍历
blocks[]的 renderer 中；需要它们的 adapter 可以读取 selector。Web Shell UI 暴露 sidechannel 不在本 PR 范围内。变更是 additive，没有 breaking change。关联 Issue
Fixes #8823