Skip to content

fix(web-shell): batch transcript dispatch to avoid tab-return freeze - #7012

Merged
wenshao merged 7 commits into
QwenLM:mainfrom
wenshao:fix/web-shell-transcript-batched-dispatch
Jul 17, 2026
Merged

fix(web-shell): batch transcript dispatch to avoid tab-return freeze#7012
wenshao merged 7 commits into
QwenLM:mainfrom
wenshao:fix/web-shell-transcript-batched-dispatch

Conversation

@wenshao

@wenshao wenshao commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

When a Web Shell tab is hidden and then restored, the SSE stream replays a burst of buffered transcript events. Previously each event was dispatched to the transcript store individually, and every dispatch copies and freezes the entire block array (O(blocks)). A burst of E events against a transcript of B blocks is therefore O(E×B) of synchronous main-thread work, which can freeze the tab for minutes or crash it on very long sessions.

This PR coalesces the live event stream into one dispatch per macrotask, so a burst collapses into a single O(B) reduction. It also caps the in-memory transcript window the client retains (the daemon stays the full source of truth), and skips the dev-only block freeze in production builds where it is pure overhead.

Why it's needed

Long-running Web Shell sessions became unusable after the tab was backgrounded: switching back triggered a multi-minute main-thread block (or a tab crash) while the buffered stream drained. The cost was quadratic in transcript size, so it got dramatically worse as sessions grew — matching the report of lag/crash with many sessions or one large session after switching the tab away and back.

Reviewer Test Plan

How to verify

  • Unit: a new provider test feeds a 100-chunk burst and asserts the resulting transcript contains every chunk in original order (no loss, no reordering) after a single coalesced dispatch. Suites pass: provider 152, webui daemon 287, SDK 1348, web-shell SplitView 36; typecheck, lint (changed files), and build are clean.
  • Manual: open a very long session, start or continue streaming, switch the browser tab away for a while, then return. The transcript should catch up promptly instead of freezing the tab for minutes.

Evidence (Before & After)

N/A — this is a main-thread performance fix; the effect (no multi-minute freeze on tab return) is not meaningfully capturable in screenshots. Verified via the unit burst test and the suites listed above. A live browser stress run on a large session is recommended as a final confirmation.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Unit tests via vitest; npm run build, npm run typecheck, npm run lint.

Risk & Scope

  • Main risk or tradeoff: transcript dispatch is deferred to a macrotask, so any code that reads the store synchronously right after the live loop must flush first. This is handled by explicit synchronous flushes at every control point (turn settle, replay_complete, epoch/ring reset, and teardown).
  • Not validated / out of scope: a live multi-session browser stress test; the daemon-side replay window is already bounded separately.
  • Breaking changes / migration notes: none. maxBlocks only caps the client's in-memory window; the daemon remains the authoritative full transcript.

Linked Issues

Reported internally; no tracking issue.

中文说明

本 PR 做了什么

当 Web Shell 标签页被隐藏后再切回时,SSE 流会重放一批缓冲的 transcript 事件。此前每个事件都单独 dispatch 到 transcript store,而每次 dispatch 都会拷贝并冻结整个 block 数组(O(blocks))。因此 E 个事件 × B 个 block 就是 O(E×B) 的同步主线程开销,在很长的会话里会让标签页卡顿数分钟甚至崩溃。

本 PR 把实时事件流合并为每个宏任务一次 dispatch,于是一次突发突发折叠为一次 O(B) 归约。同时为客户端保留的内存 transcript 窗口设置上限(daemon 仍是完整真源),并在生产构建里跳过仅开发态使用的 block 冻结(它在生产环境纯属额外开销)。

为什么需要

长时间的 Web Shell 会话在标签页被切到后台后会变得不可用:切回来时,随着缓冲流被排空,会出现数分钟的主线程阻塞(或标签页崩溃)。该开销随 transcript 大小呈二次方增长,因此会话越大越严重——正好对应「会话很多或单个大会话、切走再切回就卡顿/崩溃」的反馈。

评审者测试计划

如何验证

  • 单元测试:新增的 provider 测试喂入 100 个 chunk 的突发,断言合并为一次 dispatch 后,最终 transcript 完整包含每个 chunk 且保持原始顺序(无丢失、无乱序)。各测试套件通过:provider 152、webui daemon 287、SDK 1348、web-shell SplitView 36;typecheck、lint(仅改动文件)、build 均干净。
  • 手动:打开一个很长的会话,开始或继续流式输出,把浏览器标签页切走一段时间再切回。transcript 应当迅速追上,而不是让标签页卡顿数分钟。

证据(前后对比)

N/A——这是主线程性能修复,其效果(切回标签页不再卡顿数分钟)无法用截图有效表达。已通过单元突发测试和上述套件验证。建议再对大会话做一次真实浏览器压测作为最终确认。

测试环境

操作系统 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

运行环境(可选)

vitest 单元测试;npm run buildnpm run typechecknpm run lint

风险与范围

  • 主要风险/权衡:transcript dispatch 被推迟到宏任务,因此在实时循环之后任何同步读取 store 的代码都必须先 flush。已通过在每个控制点(turn 结算、replay_complete、epoch/ring 重置、卸载)显式同步 flush 来处理。
  • 未验证/超出范围:真实的多会话浏览器压测;daemon 侧重放窗口已另行做了上限约束。
  • 破坏性变更/迁移说明:无。maxBlocks 仅限制客户端的内存窗口;daemon 仍是完整 transcript 的权威来源。

关联 Issue

内部反馈;无跟踪 issue。

Dispatching each buffered SSE event individually makes a tab-return burst O(events x blocks) on the main thread (per-dispatch block-array copy + freeze), freezing very long sessions for minutes. Coalesce the live stream into one dispatch per macrotask, cap the client's in-memory transcript window, and skip the dev-only block freeze in production.
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: Real, well-documented performance issue. The O(E×B) main-thread freeze on tab-return SSE burst is traced to specific code paths (takeBlocksOwnership array copy + Object.freeze per dispatch in the live loop). Root cause analysis is thorough and the new 100-chunk burst unit test directly verifies the coalescing behavior. Not theoretical.

Direction: Aligned. Web Shell transcript performance is core to the product — long sessions becoming unusable after a tab switch is a genuine user-facing defect.

Size: 174 production lines (140 in DaemonSessionProvider.tsx, 16 in transcript.ts, 14 in sessions.ts, 4 in SplitView/WorkspaceSessionProvider) + 263 test lines + 209 design doc lines. No core paths touched. Well under any thresholds.

Approach: Focused and minimal. The batcher is local to the run() scope — no new module, no abstraction for its own sake. Macrotask vs microtask choice is well-justified (microtask wouldn't coalesce within the for await drain). Flush points are carefully placed at every control boundary. The maxBlocks cap and production freeze skip are clean secondary wins. Design doc is one of the most thorough I've seen — a nice signal.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:真实且有据可查的性能问题。标签页切回时 SSE 突发导致的 O(E×B) 主线程冻结,已追溯到具体代码路径(每次 dispatch 中 takeBlocksOwnership 数组拷贝 + Object.freeze)。新增的 100-chunk 突发单元测试直接验证了合并行为。不是理论性假设。

方向:对齐。Web Shell transcript 性能是产品核心体验——长会话在标签页切换后变得不可用是真实的用户级缺陷。

规模:174 行生产代码(DaemonSessionProvider.tsx 140 行、transcript.ts 16 行、sessions.ts 14 行、SplitView/WorkspaceSessionProvider 各 2 行)+ 263 行测试 + 209 行设计文档。未触及核心路径。远低于任何阈值。

方案:聚焦且精简。批处理器局限在 run() 作用域内——没有新建模块,也没有为抽象而抽象。选择宏任务而非微任务的理由充分(微任务在 for await 排空中无法合并)。每个控制边界都仔细放置了 flush 点。maxBlocks 上限和生产环境跳过冻结是干净的附带收益。设计文档非常详尽——很好的信号。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 32927e7ff5cc568ee959972d0d6e89ee01767bf8 · re-run with @qwen-code /triage

@wenshao
wenshao force-pushed the fix/web-shell-transcript-batched-dispatch branch from c2237a5 to 5866da8 Compare July 16, 2026 05:10
@github-actions

Copy link
Copy Markdown
Contributor

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)为单个提交。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 32927e7. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

sidebar-attention-dark before/after

split-view-restored-dark before/after

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal (from title + "Why"): buffer transcript events in the live SSE loop, flush in a single store.dispatch per macrotask to collapse an O(E×B) burst into O(B). Synchronous flush at control boundaries (turn terminals, replay_complete, prompt.cancelled, unmount, error). Optionally cap client-side block retention.

The PR matches this exactly, and goes further in the right ways:

  • Batcher is minimal and localenqueue/flushSync/dispatchNow/clearPending, all scoped to the run() closure. No new module, no abstraction for its own sake.
  • Macrotask choice is correctfor await drains buffered events back-to-back via microtasks, so queueMicrotask wouldn't coalesce. setTimeout(0) only fires once the generator blocks on a new network event, collapsing a whole burst. requestAnimationFrame was correctly rejected (doesn't fire in hidden tabs).
  • Every control point has the right flush/clear call: flushSync before turn terminals, dispatchNow for all assistant.done variants (turn_complete, turn_error, replay_complete, prompt.cancelled, stream_ended), clearPending before store.reset() on resync/ring-eviction, flushSync on loop exit and unmount.
  • Observer debug guard — the flush-before-read fix for activeAssistantBlockId is a subtle correctness issue that was caught by reviewer ytahdn and properly resolved: the buffer is flushed before the guard reads the committed store, scoped to the rare observer-debug-interleave path so steady streaming keeps batching.
  • Error resiliencerunTranscriptFlush wraps store.dispatch in try/catch, preventing an uncaught reducer throw from aborting error recovery or unmount cleanup.
  • Production freeze skipFREEZE_TRANSCRIPT_BLOCKS gated by process.env.NODE_ENV is clean; the typeof process guard protects unbundled browser consumers.
  • maxBlocks cap (50,000 vs 200,000 default) — well-documented rationale in the constant's JSDoc. Daemon stays authoritative; this only caps the client window.

No critical blockers found. No AGENTS.md violations.

Testing

This is a browser-side performance fix (main-thread freeze on tab-return from SSE burst). Terminal-based tmux testing cannot demonstrate the fix — the behavior change is dispatch coalescing, invisible in CLI output. CI and local test runs are the evidence.

Local test results (PR code applied to worktree)

DaemonSessionProvider tests (webui):

 ✓ src/daemon/session/DaemonSessionProvider.test.tsx (154 tests) 3245ms
 Test Files  1 passed (1)
      Tests  154 passed (154)
   Duration  4.38s

Includes 3 new tests:

  • coalesces a burst of streamed chunks into one complete ordered transcript — 100-chunk burst, asserts dispatchBatchSizes equals [100] (single coalesced dispatch, no per-event regression) and all chunks present in order.
  • flushes buffered transcript events on unmount instead of dropping them — asserts a still-buffered event lands in the store on unmount (fake timers prevent the scheduled flush from racing).
  • keeps an observer assistant burst in one block when a debug event interleaves — regression for the observer debug guard; asserts a single assistant block with both chunks, no debug block splitting it.

SDK tests:

 Test Files  29 passed (29)
      Tests  1356 passed (1356)

Typecheck: clean on both packages/webui and packages/sdk-typescript.

CI (GitHub Actions): all checks green — Test (ubuntu-latest) pass, web-shell E2E Smoke pass, Capture web-shell visuals pass.

Summary

Clean, focused performance fix. The implementation is correct, well-tested, and addresses every concern raised by the previous review round (ytahdn's observer-debug-guard finding, qwen-code-ci-bot's Criticals on unprotected dispatch and missing catch-block flush — all resolved). The design doc is unusually thorough and the test coverage pins the key invariants (coalescing, ordering, flush-not-drop on unmount, observer guard).

中文说明

代码审查

独立方案(仅根据标题和"为什么需要"):在实时 SSE 循环中缓冲 transcript 事件,以每个宏任务一次 store.dispatch 刷新,将 O(E×B) 突发折叠为 O(B)。在控制边界(turn 终止、replay_complete、prompt.cancelled、卸载、错误)同步刷新。可选地为客户端 block 保留设置上限。

PR 完全匹配此方案,并在正确的方向上更进一步:

  • 批处理器精简且局部enqueue/flushSync/dispatchNow/clearPending 全部局限在 run() 闭包中。没有新建模块,没有为抽象而抽象。
  • 宏任务选择正确for await 通过微任务逐个排空缓冲事件,因此 queueMicrotask 无法合并。setTimeout(0) 仅在生成器阻塞等待新网络事件时触发,将整个突发折叠为一次 dispatch。正确拒绝了 requestAnimationFrame(在隐藏标签页中不触发)。
  • 每个控制点都有正确的 flush/clear 调用: turn 终止前 flushSync,所有 assistant.done 变体使用 dispatchNow,resync/ring-eviction 的 store.reset()clearPending,循环退出和卸载时 flushSync
  • Observer debug 守卫 — 在读取 activeAssistantBlockId 之前先 flush 的修复是一个微妙的正确性问题,由审查者 ytahdn 发现并正确解决。
  • 错误恢复runTranscriptFlush 用 try/catch 包裹 store.dispatch,防止 reducer 异常中断错误恢复或卸载清理。
  • 生产环境跳过冻结FREEZE_TRANSCRIPT_BLOCKS 通过 process.env.NODE_ENV 门控;typeof process 保护未打包的浏览器消费者。
  • maxBlocks 上限(50,000 vs 默认 200,000)— 常量的 JSDoc 中有充分的理由。Daemon 仍是权威来源;这仅限制客户端窗口。

未发现阻塞性问题。未违反 AGENTS.md 规范。

测试

这是浏览器端的性能修复(标签页切回时 SSE 突发导致的主线程冻结)。基于终端的 tmux 测试无法展示此修复——行为变化是 dispatch 合并,在 CLI 输出中不可见。CI 和本地测试运行是证据。

本地测试结果(PR 代码应用到 worktree)

  • DaemonSessionProvider:154 个测试全部通过(含 3 个新增的突发/刷新/observer 测试)
  • SDK:1356 个测试全部通过(29 个测试文件)
  • Typecheck:webuisdk-typescript 均无错误
  • CI(GitHub Actions):所有检查绿色

总结

精简、聚焦的性能修复。实现正确,测试充分,解决了上一轮审查提出的所有问题(ytahdn 的 observer-debug-guard 发现,qwen-code-ci-bot 关于未保护 dispatch 和缺失 catch-block flush 的 Critical——全部已解决)。设计文档非常详尽,测试覆盖锁定了关键不变量(合并、排序、卸载时 flush 而非丢弃、observer 守卫)。

Qwen Code · qwen3.7-max

Reviewed at 32927e7ff5cc568ee959972d0d6e89ee01767bf8 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean performance fix, well-tested, every prior review concern resolved.

This is a textbook example of a focused performance PR done right. The problem is real and well-traced (O(E×B) main-thread freeze on tab-return SSE burst), the fix is minimal (batcher local to the run() scope, no new abstractions), and the test coverage pins the exact invariants that matter — coalescing into a single dispatch, event ordering preserved, flush-not-drop on unmount, observer debug guard.

Going back to my independent proposal from Stage 2: buffer events, flush per macrotask, sync at control boundaries. The PR matches this exactly. I don't see a simpler path — the batcher is already about as small as it can be while being correct. The secondary improvements (production freeze skip, maxBlocks cap) are well-bounded and well-justified.

Every concern from the prior review rounds has been addressed:

  • ytahdn's observer-debug-guard finding → flush before guard read, scoped to the rare path
  • Unprotected store.dispatch in catch block and unmount → try/catch in runTranscriptFlush
  • Bare store.dispatch for control events → all routed through dispatchTranscriptNow
  • Test assertion too loose → tightened to toEqual([CHUNK_COUNT])

154 provider tests pass locally (including 3 new ones), 1356 SDK tests pass, typechecks clean, CI green. No blocking issues found.

Approving. ✅

中文说明

信心度:4/5 — 干净的性能修复,测试充分,之前审查的所有问题均已解决。

这是一个聚焦的性能 PR 的典范。问题是真实且有据可查的(标签页切回时 SSE 突发的 O(E×B) 主线程冻结),修复精简(批处理器局限在 run() 作用域内,没有新建抽象),测试覆盖锁定了关键不变量——合并为单次 dispatch、事件顺序保持、卸载时 flush 而非丢弃、observer debug 守卫。

回到我在 Stage 2 的独立方案:缓冲事件、按宏任务刷新、在控制边界同步。PR 完全匹配此方案。我没有找到更简单的路径——批处理器在保持正确性的前提下已经尽可能精简了。附带改进(生产环境跳过冻结、maxBlocks 上限)有明确边界和充分理由。

之前审查轮次的所有问题均已解决:

  • ytahdn 的 observer-debug-guard 发现 → 在读取守卫前先 flush,限定在罕见路径
  • catch 块和卸载中未保护的 store.dispatchrunTranscriptFlush 中添加 try/catch
  • 控制事件的裸 store.dispatch → 全部改用 dispatchTranscriptNow
  • 测试断言过松 → 收紧为 toEqual([CHUNK_COUNT])

本地 154 个 provider 测试通过(含 3 个新增),1356 个 SDK 测试通过,typecheck 无错误,CI 绿色。未发现阻塞性问题。

已批准。✅

Qwen Code · qwen3.7-max

Reviewed at 32927e7ff5cc568ee959972d0d6e89ee01767bf8 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Review

Verdict: the core fix is correct and does what it claims — I verified the asymptotics and every ordering control point against the code, and ran the affected suites locally at the PR head. No blockers. One small teardown change is worth making before merge, plus a portability hardening and some design-doc drift to clean up.

Independently verified

  • The O(E×B) → O(B) claim is real. reduceDaemonTranscriptEvents clones block state once per dispatch, not per event: takeBlocksOwnership is guarded by the module-level ownedBlocks WeakMap, so the first mutation in a reducer pass copies and every later event in the same batch mutates the owned copy in place (transcript.ts ~1257–1267). A coalesced burst is exactly one block-array copy. Multi-event dispatch is also the long-exercised replay-injection path (store.dispatch(allUiEvents)), so batching live events rides proven reducer semantics rather than a new code path.
  • All in-loop ordering control points hold. settleActivePromptFromTurnEvent only acts on turn_complete/turn_error (it early-returns otherwise), so the turn-terminal flushTranscriptSync() guard covers both of its direct store.dispatch calls; replay_complete flushes before the awaitingResync snapshot read; the epoch-reset and ring-eviction paths clear the buffer before store.reset() and then rebuild from a fresh /load, so dropped pending events are recovered from the snapshot; loop exit flushes before the post-loop assistant.done dispatches; observer/cancel terminals go through dispatchTranscriptNow. The pre-loop dispatch sites (replay injection, load warnings, reconnect assistant.done) can never see a non-empty buffer because at least one macrotask elapses between a loop exit/throw and the next attach, which drains the 0 ms timer.
  • Hidden-tab behavior improves twice over. setTimeout is the right macrotask choice (rAF never fires in hidden tabs): background timer throttling just makes batches bigger while the SSE loop keeps draining via unthrottled microtasks, and the old behavior of running an O(B) reduce per event in a hidden tab goes away entirely.
  • Ran locally at the PR head (deps symlinked, vitest aliases resolve to the worktree source): provider suite 152/152 including the new burst test, web-shell SplitView 36/36, sdk transcript reducer tests, and eslint --max-warnings 0 on all changed files — clean. The Ubuntu Test job was still pending when I posted this.

Should fix before merge

1. Teardown should flush, not drop — the design doc already says so.
The effect cleanup calls clearPendingTranscriptEvents() (DaemonSessionProvider.tsx ~1600), but the design doc's flush-points list says "flushSync() on loop exit and provider unmount so no buffered events are lost." The distinction matters because DaemonSessionClient.iterateEvents advances lastSeenEventId as each event is yielded — before the batched dispatch runs. Any path that keeps the session across an effect re-run and resumes incrementally (keepSessionForNextEffect → the attachedExistingSession PATH A resume) would permanently skip the dropped events. I traced today's first-party flows and they all recover — controlled loads and createSession rebuild from a /load snapshot or reset the store — so this is a latent hazard plus a doc contradiction rather than a live bug. But flushing is strictly safer and free: the store's notify is queueMicrotask-deferred, so a flush during cleanup causes no synchronous setState; on unmount it's an orphaned dispatch, and on session switch the next run resets the store anyway. Suggest flushTranscriptSync() there (and if you keep the drop, update the design doc to match and say why).

2. First bare process.env access in the browser-reachable SDK graph.
FREEZE_TRANSCRIPT_BLOCKS = process.env.NODE_ENV !== 'production' (transcript.ts:127) is safe in every first-party path — Vite app builds statically replace the dotted NODE_ENV form, and Node/vitest have process. But daemon/ui sits exactly on the surface DaemonClient.ts documents as browser-hostile ("a literal process.env[...] would explode at module load on browser bundles" — the readTokenFromEnv comment), and Vite lib builds (vite.lib.config.ts, and webui's own lib build) intentionally preserve process.env.NODE_ENV in their output, so an embedder consuming those libs without a NODE_ENV-defining bundler crashes at import. There is currently no other bare process.env member access anywhere in the client-reachable graph — this would be the first. Suggest:

const FREEZE_TRANSCRIPT_BLOCKS =
  typeof process !== 'undefined' && process.env.NODE_ENV !== 'production';

App builds still fold this to false (the member expression is replaced syntactically, leaving typeof process !== 'undefined' && false), unbundled browsers get false instead of a ReferenceError, and Node keeps the env check. Known tradeoff: a Vite dev-server browser session would skip the freeze too (no process global there), while vitest/CI — where the mutation-discipline tests actually run — keep it. If you want browser-dev freezing as well, a try { … } catch { return false; } wrapper preserves it at the cost of uglier code; either is fine by me, the current form is the only one I'd avoid.

Design doc / test-plan drift (minor)

  • The doc says the flush is "requestAnimationFrame where available, setTimeout(0) fallback" — the implementation is setTimeout-only, which I think is the better choice (see hidden-tab note above); update the doc rather than the code.
  • The doc's verification plan promises a prod-vs-dev freeze unit test, a coalescing assertion, and an unmount-flush test; none are in the PR. In particular the new burst test asserts completeness and order but never counts store.dispatch calls, so a regression back to per-event dispatch would still pass it. A dispatch-count spy (e.g. assert one reducer pass for the 100-chunk burst) would pin the actual property this PR exists to provide.

Noted, no action needed

  • Dispatch sites outside the effect scope — actions.ts sendPrompt's optimistic appendLocalUserMessage and its abort/error assistant.done (~304–343) — can't flush the batcher and may now precede up to one macrotask (~0–4 ms) of already-received chunks. That's the same class as the pre-existing network-in-flight race, only marginally widened; text deltas merge into existing blocks by id, so the worst case is a rare cosmetic block-ordering blip. Worth remembering if a "split assistant bubble" report ever surfaces.
  • shouldGuardAssistant snapshot staleness is acknowledged in the doc and only affects debug-event filtering — agreed it's immaterial.
  • The 50 000 maxBlocks cap is the knob DEFAULT_MAX_BLOCKS explicitly invites callers to use; trim slices from the front and the trimmed-sentinel maps are themselves capped by maxBlocks, so no unbounded auxiliary state; the constant is module-stable so the useMemo store is never re-created. Both web-shell mount points (main provider and split panes) are covered.

🤖 Generated with Claude Code — Claude Fable 5

… browser

Address review feedback: teardown now flushes buffered transcript events instead of dropping them (the SSE client advances lastSeenEventId as events are yielded, so a dropped buffer would be skipped by a same-session incremental resume). Guard FREEZE_TRANSCRIPT_BLOCKS with typeof process so an unbundled browser consumer of the daemon/ui surface does not throw a ReferenceError. Add a dispatch-count assertion to the burst test and an unmount-flush regression test, and align the design doc (setTimeout-only flush, verification plan).

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one reproducible transcript-ordering regression in the new batching path. The existing provider suite passes, but a focused observer-burst regression test fails as described inline.

Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
@ytahdn

ytahdn commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Overall assessment for cc4d5893: the performance direction is sound, but the current implementation is not ready to merge yet.

Using a macrotask (setTimeout(0)) to coalesce an already-buffered async-iterator burst is appropriate and should reduce the dominant tab-return cost from O(events × blocks) to roughly one O(blocks) reduction per burst. Flushing at turn terminals, replay completion, resets, stream end, and teardown is also the right class of safeguard. The Web Shell-specific maxBlocks cap and production-only freeze removal are reasonable supporting optimizations, although the 50,000-block value would benefit from benchmark evidence because steady streaming can still copy a large block array per network chunk.

The remaining concern is architectural consistency: dispatch used to update the store synchronously, while this change creates two transcript states—the committed store and pendingTranscriptEvents. Correctness now depends on manually identifying every control path that reads the store or dispatches an ordering-sensitive event and flushing first. The reproduced shouldGuardAssistant regression is one missed read site and demonstrates that this invariant is currently too easy to violate.

Minimum path forward: make the observer/debug guard account for queued events and add the focused burst regression test. Preferably, encapsulate reads that require the effective state (committed + pending), then re-audit every getSnapshot(), reset(), and immediate dispatch control point. With that fixed and verified, the overall approach is acceptable.

…ursts in one block

Address ytahdn's PR QwenLM#7012 review: the batched-dispatch debug guard read the committed store's activeAssistantBlockId, which lags the pending buffer within a burst, so a debug event interleaved in an observer assistant burst was not filtered and split the block. Flush the buffer before the guard, scoped to observer-mode debug events (rare) so steady streaming keeps batching. Add a focused burst regression test, make the unmount-flush test deterministic with fake timers (it was timing-racy), and update the design doc.
qwen-code-ci-bot pushed a commit that referenced this pull request Jul 16, 2026
ytahdn
ytahdn previously approved these changes Jul 16, 2026

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the complete PR at 930d08876. The previously reported observer-burst ordering regression is fixed and covered by a focused regression test.

Verified locally:

  • DaemonSessionProvider: 154/154
  • SDK daemon UI reducer: 269/269
  • Web Shell: 50/50
  • SDK and WebUI typecheck

The batching, reset, replay-complete, stream-end, teardown, and observer-debug ordering paths are consistent. No remaining blocking findings. Approving; the Ubuntu CI job is still running and should remain a merge requirement.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reviewed: chunk 1, chunk 2, chunk 3 — no agent reported covering these; nobody read them.

— qwen3.7-max via Qwen Code /review

Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx Outdated
qwen-code-ci-bot pushed a commit that referenced this pull request Jul 16, 2026
qwen-code-ci-bot pushed a commit that referenced this pull request Jul 16, 2026

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reviewed: chunk 1, chunk 2, chunk 3 — no agent reported covering these; nobody read them.

— qwen3.7-max via Qwen Code /review

Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx Outdated
Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
The catch block at the end of the connection loop skipped the post-loop
flush, leaving buffered transcript events on a scheduled timer. The
retriable path resumes via Last-Event-ID without resetting the store,
and lastSeenEventId has already advanced past those events, so clearing
the buffer would drop them on the incremental delta-resume. Flush
instead.

Also route the restored-prompt settle and replay_complete control
dispatches through dispatchTranscriptNow so each is self-contained
(flush + dispatch) rather than relying on an earlier flush by timing,
and tighten the burst regression test from toContain(CHUNK_COUNT) to
toEqual([CHUNK_COUNT]) so a regression emitting redundant per-event
dispatches also fails.

Addresses the ci-bot review.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reviewed: Agent 0: Issue fidelity & root-cause ownership — no prompt was built for it (agent-prompt --role 0 never ran).

Not reviewed: Agent 1a: Line-by-line correctness — no prompt was built for it (agent-prompt --role 1a never ran).

Not reviewed: Agent 2: Security — no prompt was built for it (agent-prompt --role 2 never ran).

Not reviewed: Agent 3: Code quality — no prompt was built for it (agent-prompt --role 3 never ran).

Not reviewed: Agent 4: Performance & efficiency — no prompt was built for it (agent-prompt --role 4 never ran).

Not reviewed: Agent 5: Test coverage — no prompt was built for it (agent-prompt --role 5 never ran).

Not reviewed: Agent 6a: Undirected audit — attacker mindset — no prompt was built for it (agent-prompt --role 6a never ran).

Not reviewed: Agent 6b: Undirected audit — 3 AM oncall mindset — no prompt was built for it (agent-prompt --role 6b never ran).

Not reviewed: Agent 6c: Undirected audit — six-months-later maintainer — no prompt was built for it (agent-prompt --role 6c never ran).

Not reviewed: Agent 1b: Removed-behavior audit — no prompt was built for it (agent-prompt --role 1b never ran).

Not reviewed: Agent 1c: Cross-file tracer — no prompt was built for it (agent-prompt --role 1c never ran).

Not reviewed: Agent 7: Build & test verification — no prompt was built for it (agent-prompt --role 7 never ran).

Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped).

Not reviewed: verification — the review posts findings, but no verifier ran (Step 4 builds its prompt with agent-prompt --role verify; none was recorded, so the findings were not verified).

— qwen3.7-max via Qwen Code /review

Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
A reducer throw inside runTranscriptFlush escaped as an uncaught
setTimeout error on the macrotask path and, via flushTranscriptSync,
propagated out of the catch block (aborting lastSeenEventId bookkeeping,
reconnect, auth branching, terminal cleanup, and pendingSessionLoad
rejection) and out of the useEffect cleanup (leaving half-torn-down
state). Wrap the dispatch in try/catch and log it with the batch size so
the throw is surfaced without crashing the session or skipping teardown;
one guard fixes all three paths.

Also document the flush precondition on settleActivePromptFromTurnEvent,
which dispatches assistant.done directly and previously carried that
contract only as an inline comment at the call site.

Addresses the ci-bot review.
qwen-code-ci-bot pushed a commit that referenced this pull request Jul 16, 2026
wenshao added a commit that referenced this pull request Jul 16, 2026
… can act on it

A role with no recorded prompt proves one thing: the brief never reached an
agent. The roster check claimed more than that — "no prompt was built for it
(`agent-prompt --role 0` never ran)" — and on #7012 it said that about all
twelve dimensions of a review that had just posted two Criticals with line
numbers. The agents were in the same comment the gate was calling empty.

Both failures are real and neither is the other. An orchestrator that writes the
launch by hand gets an agent that runs, reads the diff and finds things, having
never seen the severity bar, the finding format or this project's rules — all of
which live in the brief it was never given. That is worth blocking on. It is not
"nobody looked", and a check may not report the reading it cannot see.

Three changes, one shape:

- The per-role text says the brief never reached an agent, and that the
  dimension was reviewed "if at all" from a prompt the run wrote for itself.
  It no longer speaks for the agent's existence.
- Every role briefless collapses to one line. It is one failure — the run did
  not use the prompt builder — and saying it twelve times buries the fact that
  explains all twelve.
- The public body drops the internal command. `agent-prompt --role 2` is not
  something a PR author can run; on #7012 fourteen lines of it were the whole
  CHANGES_REQUESTED while the findings sat inline below the fold. The call
  survives in check-coverage's stderr, where the orchestrator reads it, and the
  role number is already in each label.

check-coverage no longer leads with a count: the collapsed line covers the whole
roster, so "1 required brief" would undercount it by the size of the review.

Behaviour is unchanged — the gate fires on exactly the same runs and still caps
the verdict. Only the sentence changes, and only where it was overclaiming or
talking to the wrong reader.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/webui/src/daemon/session/DaemonSessionProvider.tsx
@wenshao

wenshao commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Code review @ 32927e7

Verdict: LGTM — the correctness-critical surfaces all check out. Two non-blocking hardening suggestions below.

Overview

The PR fixes a real quadratic hot spot: the live SSE loop dispatched once per daemon event, and every dispatch pays O(B) in the reducer (block-array copy + freeze), so a buffered burst of E events on tab-return cost O(E×B). The change coalesces transcript events into one store.dispatch per macrotask, caps the web-shell client transcript window at 50k blocks (main provider + split panes), and gates the dev-only Object.freeze(result.blocks) out of production builds. A dated design doc records the full audit trail, including post-review rounds.

What I verified (independently, at head)

  • The asymptotic claim is real. reduceDaemonTranscriptEvents clones, trims, and freezes once per dispatch and loops applyDaemonTranscriptEvent over the batch; takeBlocksOwnership (transcript.ts:1267) copy-on-writes at most once per pass via the ownedBlocks WeakMap. Batching N events genuinely collapses N block-array copies into one.
  • Flush discipline is complete. I mapped every store.dispatch / store.getSnapshot() / store.reset() site in the provider:
    • Every live-loop store read flushes first: the turn-terminal flush (:1174) exactly matches settleActivePromptFromTurnEvent's internal guard (it only acts on turn_complete/turn_error and dispatches assistant.done directly); the observer debug guard flushes before reading activeAssistantBlockId (:1222); replay_complete flushes before the awaitingResync read (:1258).
    • Every loop exit flushes: normal stream end (:1408, also covers all four break paths), the catch (:1476), and the effect cleanup (:1642, covering the catch's disposed/aborted early return).
    • Both store.reset() paths (epoch reset :1088, ring eviction :1339) clear pending rather than flush — equivalent to pre-PR behavior, since the reset wiped already-dispatched events anyway and the follow-up full /load (lastEventId=0) replays them.
    • The pre-loop bare dispatch sites (assistant.done reconnected :739, replay injection :822/:878, load warnings :1064) run with a provably empty buffer: every path into a new while iteration flushed on the previous exit.
  • "Flush, never drop" on the catch/unmount paths is load-bearing and correct: DaemonSessionClient advances lastSeenEventId as events are yielded (DaemonSessionClient.ts:739), before the batched dispatch runs, so dropped buffered events would be permanently skipped on a Last-Event-ID resume.
  • The passive-observer done-timer can't race the flush for content that matters: any loop iteration that enqueues generation-signal events (assistant.text.delta / thought.text.delta / tool.update, :2306) re-arms the 3s debounce after the enqueue in the same iteration, so the 0ms flush timer always fires first. An expired timer racing a non-signal-only batch would merely order assistant.done before status-type events, which is immaterial (see suggestion 2).
  • maxBlocks layering and coverage: SDK state default 1k → webui provider default 200k → web-shell 50k; both web-shell mounts (WorkspaceSessionProvider, SplitView panes) pass the constant, and they are the only DaemonSessionProvider mounts in web-shell. The daemon stays authoritative; trimming keeps the tool/permission sentinel maps capped.
  • Freeze gating is sound: takeBlocksOwnership copies before mutating regardless of the freeze, so production correctness doesn't depend on it; the typeof process guard matches the existing SDK idiom, and dev/CI (vitest, Vite dev) keep NODE_ENV !== 'production', so the safety net still fires where the mutation-discipline tests run.
  • Tests pin the load-bearing properties: the burst test asserts dispatchBatchSizes toEqual([100]) (catches both a per-event revert and redundant duplicate dispatches), the unmount test uses fake timers to prove the buffer flushes rather than drops, and the observer debug-interleave test pins the round-6 block-integrity fix. CI is green at head (ubuntu unit tests, web-shell E2E smoke, visuals capture).

Suggestions (non-blocking)

  1. Reducer-throw now drops a whole batch; consider a per-event fallback. (Concurs with the bot's outstanding inline comment on :398.) Pre-batching, the per-event try/catch in the loop (:1117) meant a reducer throw lost only the offending event — dispatch throws before state is reassigned, and the loop continued. Post-batching, runTranscriptFlush's catch (:391–:398) drops up to a full burst, and because lastSeenEventId has already advanced past those events, a later delta resume never re-delivers them. In the catch, retrying the batch one event at a time (skipping only throwers) confines the loss to the single bad event; the degenerate O(E×B) cost only occurs on an already-buggy path. While there: surfacing the failure via addNotice (like the existing daemon.event_malformed notice) rather than only console.error would restore user-visible parity with the per-event path, and a small test for the fallback would pin it.

  2. Document the cross-file passive-timer invariant. schedulePassiveAssistantDone's callback (timing.ts:74) reads the snapshot and dispatches assistant.done outside the batcher. It is safe today only because of the re-arm-after-enqueue ordering described above — an invariant that now spans two files and the definition of hasActiveGenerationSignal, with nothing stating it. A one-line comment at the observer branch (:1311) or on schedulePassiveAssistantDone would keep a future change (arming the timer somewhere else, adding a non-signal event type that creates assistant blocks, or shortening the delay) from silently reintroducing split assistant blocks.

Notes (no action needed)

  • Steady-state cost after the fix is one O(B) pass per network chunk with B ≤ 50k — the cap bounds the constant, batching fixes the multiplier; this matches the design doc's framing of B1 as ceiling-not-fix.
  • Pending-buffer growth between macrotasks is bounded by the SSE iterator's maxQueued waves, so there is no unbounded client-side accumulation.
  • The design doc's audit-trail format (rounds 1–8, including the post-review regressions and their fixes) made this review materially easier to verify — worth keeping as a pattern.
中文版(Chinese version)

代码评审 @ 32927e7

结论:LGTM —— 关键正确性面均已核实。以下两条为非阻塞的加固建议。

概述

本 PR 修复了一个真实的二次方热点:实时 SSE 循环此前每个 daemon 事件 dispatch 一次,而每次 dispatch 在 reducer 中都要付出 O(B) 开销(block 数组拷贝 + freeze),因此标签页切回时 E 个缓冲事件的突发代价是 O(E×B)。改动将 transcript 事件合并为每个宏任务一次 store.dispatch,把 web-shell 客户端 transcript 窗口上限设为 5 万个 block(主 provider + 分屏面板),并在生产构建中跳过仅开发态需要的 Object.freeze(result.blocks)。附带一份按日期归档的设计文档,完整记录了包括评审后各轮在内的审计过程。

我独立核实的内容(基于 head)

  • 渐近复杂度的说法属实。 reduceDaemonTranscriptEvents 每次 dispatch 只做一次 clone、trim、freeze,并对整批事件循环应用 applyDaemonTranscriptEventtakeBlocksOwnershiptranscript.ts:1267)借助 ownedBlocks WeakMap 每个归约过程最多写时复制一次。批处理确实把 N 次 block 数组拷贝折叠为一次。
  • flush 纪律是完备的。 我逐一核对了 provider 中所有 store.dispatch / store.getSnapshot() / store.reset() 调用点:
    • 实时循环内的每个 store 读取都先 flush:turn 终结 flush(:1174)与 settleActivePromptFromTurnEvent 的内部守卫完全对应(它只处理 turn_complete/turn_error 并直接 dispatch assistant.done);observer debug 守卫在读取 activeAssistantBlockId 前 flush(:1222);replay_complete 在读取 awaitingResync 前 flush(:1258)。
    • 每个循环出口都会 flush:正常流结束(:1408,同时覆盖全部四个 break 路径)、catch(:1476)、effect 清理(:1642,覆盖 catch 的 disposed/aborted 提前返回)。
    • 两处 store.reset()(epoch 重置 :1088、ring 淘汰 :1339清空而非 flush 挂起事件 —— 与 PR 前行为等价:reset 本来就会抹掉已 dispatch 的事件,随后的完整 /load(lastEventId=0)会重放它们。
    • 循环前的裸 dispatch 调用点(assistant.done reconnected :739、replay 注入 :822/:878、load 警告 :1064)可证明缓冲区为空时才执行:进入新一轮 while 的所有路径都在上一轮出口 flush 过。
  • catch/卸载路径「flush 而非丢弃」的理由成立且必要: DaemonSessionClient 在事件被 yield 时就推进 lastSeenEventIdDaemonSessionClient.ts:739),早于批处理 dispatch,因此若丢弃缓冲事件,Last-Event-ID 增量恢复将永久跳过它们。
  • passive-observer 完成定时器不会在关键内容上跑赢 flush: 任何 enqueue 了生成信号事件(assistant.text.delta / thought.text.delta / tool.update:2306)的循环迭代,都会在 enqueue 之后 重置 3 秒防抖,因此 0ms 的 flush 定时器总是先触发。过期定时器与纯非信号批次竞争时,只会让 assistant.done 排在 status 类事件之前,无实质影响(见建议 2)。
  • maxBlocks 分层与覆盖: SDK 状态默认 1 千 → webui provider 默认 20 万 → web-shell 5 万;web-shell 的两个挂载点(WorkspaceSessionProviderSplitView 面板)都传入该常量,且它们是 web-shell 中仅有的 DaemonSessionProvider 挂载点。daemon 仍是权威数据源;裁剪同时对 tool/permission 哨兵映射做了上限。
  • freeze 门控是稳妥的: takeBlocksOwnership 无论是否 freeze 都先拷贝再修改,生产正确性不依赖 freeze;typeof process 守卫符合 SDK 既有习惯用法,dev/CI(vitest、Vite dev)保持 NODE_ENV !== 'production',安全网在运行变异纪律测试的环境中依然生效。
  • 测试钉住了承重性质: 突发测试断言 dispatchBatchSizes toEqual([100])(既能捕获退回逐事件 dispatch,也能捕获冗余重复 dispatch);卸载测试用假定时器证明缓冲区是 flush 而非丢弃;observer debug 穿插测试钉住第 6 轮的 block 完整性修复。head 上 CI 全绿(ubuntu 单测、web-shell E2E 冒烟、视觉捕获)。

建议(非阻塞)

  1. reducer 抛错现在会丢掉整批事件;建议增加逐事件回退。(与 bot 在 :398 的未决行内评论一致。)批处理前,循环内逐事件的 try/catch:1117)意味着 reducer 抛错只丢失肇事的那一个事件 —— dispatchstate 重新赋值前抛出,循环继续。批处理后,runTranscriptFlush 的 catch(:391–:398)最多丢掉一整个突发,且由于 lastSeenEventId 已越过这些事件,后续增量恢复不会再投递它们。在 catch 中对该批事件逐个重试(只跳过抛错者)可把损失限制在单个坏事件;退化的 O(E×B) 开销只发生在本就有 bug 的路径上。顺带:用 addNotice(类似既有的 daemon.event_malformed 提示)而不只是 console.error 来暴露失败,可恢复与逐事件路径同等的用户可见性;再补一个小测试钉住该回退。

  2. 为跨文件的 passive 定时器不变量补文档。 schedulePassiveAssistantDone 的回调(timing.ts:74)在批处理器之外读取快照并 dispatch assistant.done。它如今的安全性完全依赖上文所述的「enqueue 之后重置防抖」顺序 —— 这一不变量横跨两个文件外加 hasActiveGenerationSignal 的定义,却没有任何地方声明。在 observer 分支(:1311)或 schedulePassiveAssistantDone 上加一行注释,可防止未来的改动(在别处启用该定时器、新增会创建 assistant block 的非信号事件类型、或把延迟缩短到 flush 窗口以内)悄悄重新引入 assistant block 被拆分的问题。

备注(无需处理)

  • 修复后的稳态开销是每个网络分片一次 O(B) 归约、B ≤ 5 万 —— 上限约束常数,批处理修复倍数;与设计文档把 B1 定位为「天花板而非修复」的表述一致。
  • 宏任务之间挂起缓冲的增长受 SSE 迭代器 maxQueued 波次约束,客户端不会无界累积。
  • 设计文档的审计轨迹格式(第 1–8 轮,含评审后回归及其修复)让本次评审的核实工作省力很多 —— 值得保留为惯例。

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Approve ✅

Reviewed the full diff at 32927e7 against main in a clean worktree. This is a well-scoped main-thread performance fix — root cause, design, and the three fixes all hold up. All prior review threads (ci-bot + ytahdn) are resolved, and I re-verified each against the current commit.

Independently verified

  • packages/webuiDaemonSessionProvider.test.tsx: 154 passed
  • packages/sdk-typescript — daemon UI reducer: 269 passed
  • packages/web-shellSplitView.test.tsx: 36 passed
  • typecheck (webui + sdk): clean
  • eslint (all changed files): clean

Correctness checks that hold

  • Coalescing property: the macrotask (setTimeout 0) flush is required — a microtask flush would fire between every for await event and never coalesce. The burst test pins it with expect(dispatchBatchSizes).toEqual([CHUNK_COUNT]), so a per-event regression fails rather than silently passing.
  • "Every store read in the live loop is flush-preceded": audited all store.* sites. The two in-loop getSnapshot() reads (debug guard, awaitingResync) are each preceded by a flushTranscriptSync(); all pre-loop dispatches (setup/replay) run on an empty buffer.
  • Debug guard: faithfully restores pre-batching semantics — debug is filtered only when the committed store has an active assistant block, and the scoped flush commits prior-iteration chunks first.
  • No silent loss: buffered events are always either flushed or intentionally clearPendingTranscriptEvents()'d before store.reset(). The catch path flushes (not clears) because the retriable resume advances lastSeenEventId; unmount flushes for the same reason.
  • Throw containment: runTranscriptFlush swallows a reducer throw, so neither the timer path nor the catch/unmount flushTranscriptSync() can cascade into error-recovery/teardown.
  • settleActivePromptFromTurnEvent callers both satisfy the flush precondition (the in-loop caller is flushed just above; the replay-setup caller runs on an empty buffer).
  • passive-assistant-done timer (setTimeout 80/3000) always fires after the batched flush (setTimeout 0) by timer ordering — even under background throttling — so assistant.done never lands ahead of buffered content.

Non-blocking observations

  • Fix B2 (dev-only freeze): skipping the freeze in production means a latent "external consumer mutates the COW-shared blocks array in place" bug would corrupt silently rather than throw; the dev/CI safety net remains. Documented, reasonable tradeoff.
  • The reducer-throw console.error logs eventCount but not payloads — limits post-mortem detail but avoids writing session content to logs; acceptable.
  • dispatchTranscriptNow's control store.dispatch is unguarded, but this matches pre-PR behavior and sits inside the loop's try/catch — not a regression.

Nice work — the design doc's audit trail (rounds 1–8) makes the reasoning fully traceable. The only thing not covered by unit tests is a live large-session tab-away/return stress run, which the PR already flags as recommended final confirmation.

@wenshao

wenshao commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增量 review 通过(930d088..32927e7,2 commits):

  • catch 块的 flushTranscriptSync 正确补回了 try 块内被 throw 跳过的 post-loop flush,选择 flush 而非 clear 的理由(retriable path 不 reset store)注释到位
  • restored-prompt / replay_complete 两条路径改用 dispatchTranscriptNow 保证了 buffered transcript 在 assistant.done 之前的正确顺序
  • runTranscriptFlush 的 try/catch 堵住了 reducer throw 的三个逃逸路径(setTimeout uncaught、catch 块 error recovery、useEffect cleanup),一个 guard 修三处
  • 测试从 toContain 收紧到 toEqual 能检测冗余 dispatch regression

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 17, 2026
Merged via the queue into QwenLM:main with commit c56ae42 Jul 17, 2026
267 of 272 checks passed
@wenshao
wenshao deleted the fix/web-shell-transcript-batched-dispatch branch July 17, 2026 01:05
wenshao added a commit that referenced this pull request Jul 17, 2026
…its FIX

Two findings from the bot review of the previous commit.

The shellContextEnv suite isolated QWEN_CODE_SESSION_ID and QWEN_CODE_CLI but
not QWEN_CODE_PROJECT_DIR — the third variable the CLI exports to every shell,
and the one this suite's own per-session tests assign without cleanup.
Reproduced: run the suite with it set, as any `npm test` from inside a qwen
session does, and exactly the two `.toEqual()` exact-match tests fail on a key
the test never set. Same isolation, same shape, and it retires the in-file
leak too.

The remediation channel covered blind agents and the Step 4/5 gaps and stopped
there: missing briefs, rewritten launches, unread briefs and never-opened
diffs still reached the body with no FIX line beside them. A body disclosure
with no repair command is how #7012's orchestrator got to "the agents clearly
did their job" — the whole reason the channel exists. Each category now pushes
one remediation line (missing briefs point at `--roster`; the relaunch-shaped
ones say relaunch with the same printed prompt), and a test pins the pair for
a roster gap: the body says "brief never reached an agent" with no command in
it, and the remediation names the roster call.
wenshao added a commit that referenced this pull request Jul 17, 2026
Two sentences, same class, both this branch's own thesis applied to itself.

A chunk agent that ran on a hand-written prompt while its chunk was never
built landed in the body as "no prompt was built for it (`agent-prompt` never
ran for this chunk)" — an internal command on the author-facing surface, one
line per chunk on a 3B replay of the #7012 shape. The label now says what
happened in the author's register (ran on a prompt the run wrote itself; the
brief never reached it); the rebuild command already rides the
rewritten-launches remediation line on stderr.

And the Step 4/5 `not-built` texts still said "no auditor ran" / "no verifier
ran" — the one residual of the overclaim this branch exists to retire.
`not-built` is decided before the transcripts are consulted: a run that
skipped the builder and hand-wrote the launch leaves no brief on disk whose
open could be looked for, so such an auditor is invisible to the check, and
"no auditor ran" claims sight it does not have. Both texts now use the roster
wording: what a missing record proves (no agent was launched with a prompt
this skill builds), then what it costs ("ran, if at all, without the method
its brief carries"). The Delivery docstring records why.

Tests pin the new sentences positively and negatively; the register pin
(no `agent-prompt`/`--chunk` in a body label) guards the first one.
wenshao added a commit to wenshao/qwen-code that referenced this pull request Jul 18, 2026
…ne call (QwenLM#7033)

* fix(review): name a rewritten launch as itself, and leave nothing to hand-assemble

Dogfooded on a real 3A review of a live PR, and the run talked its way past the gate:

  compose-review printed: Verdict: Comment — an Approve was NOT available: a
  dimension nobody reviewed

  the run's next thought: "the compose-review flagged reverse audit as unreviewed
  (transcript visibility issue — the reverse audit did run substantively with two
  dry rounds). Let me proceed."

  the run then reported, and saved: Verdict: Approve

The gap was right and its wording was wrong, and the wording is what let the run
dismiss it. Two auditors HAD run — 16 and 23 tool calls each — and both HAD opened
their brief. What had actually happened is that the orchestrator skipped `--findings`
and hand-wrote their launches, keeping only the brief pointer, so no agent was
launched with the prompt the CLI built. The gap said "no agent was launched with it
that opened its brief", which is false as written, and "a transcript visibility
issue" is what a reader concludes from a message that does not describe what
happened.

So the floor tells the four shapes apart instead of collapsing them into one
boolean, and each says what happened and what to do:

  not-built     — the step was skipped; `agent-prompt --role <r>` never ran
  not-launched  — the prompt was built and nothing was launched with it
  rewritten     — an agent ran and opened its brief, but no agent got the built
                  prompt: the launch was written by hand instead of pasted
  brief-unread  — an agent got the built prompt and never opened the brief

`rewritten` is the one that just happened, and it is now un-dismissable: it concedes
the agent ran and read its brief, and names the orchestrator's own edit as the defect.

And the path that produced it is gone: `--findings` is now REQUIRED for a role that
takes findings. There is no bare-block-plus-hand-assembly path left — the command
refuses, and prints one block to paste. An early reverse-audit round with nothing
confirmed yet passes an empty file, which the command renders as "Nothing is
confirmed yet".

SKILL: Step 4/5 say `--findings` is required. Step 6 gains the second half of the
lesson — you may not overrule the line compose-review gives you; a cap you can
explain is still a cap, and the fix is to make the step verifiable and re-run, not to
keep the verdict you preferred. Step 8's report interpolates the verdict out of the
composed JSON (`jq -r .event`) instead of typing it, because the terminal is prose
and the archive is forever.

* fix(review): call the CLI that is running, not whatever `qwen` PATH finds

Reported from a real session: `npm run dev:daemon`, `/review 6998 --comment` in the
web shell, and the run died on

  Missing required argument: chunk

with a help screen for a command that has no `--role` at all. The daemon was running
the checkout — the skill it loaded is the current one, and says `--role 0` — but the
skill shells out to `qwen review agent-prompt …`, and `qwen` on that machine is
`/usr/bin/qwen` → a v0.19.10 global install whose `agent-prompt` predates QwenLM#6892
entirely. The skill and the CLI it was talking to were different programs.

The skill assumed `qwen` on PATH is the build running it. That holds for a single
install and breaks for exactly the people most likely to run a dev daemon. It is also
invisible when it breaks: the error names an argument, not a version.

So the entry is passed down instead of rediscovered. `scripts/cli-entry.js` is the
executable entry and the one thing that knows its own path, so it publishes it as
`QWEN_CODE_CLI` (`||=`, so the relaunch into dist/cli.js keeps pointing callers back
at the wrapper with the shebang, not at itself). `daemon-dev.js` sets it too — the dev
daemon is started as `node scripts/dev.js` and never passes through the wrapper, which
is why this bit there first. `getShellContextEnvVars` passes it to every shell
subprocess, beside the session and project-dir vars that are already handed down for
the same reason. The skill's 23 command sites now read `"${QWEN_CODE_CLI:-qwen}"
review …`; the fallback keeps hosts that do not export it on the old behaviour.

PATH was the other candidate and was rejected: prepending a shim dir means writing an
executable at spawn time and overriding `PATH` in an env that
`normalizePathEnvForWindows` has already normalised — a `Path`/`PATH` collision on
Windows in exchange for saving one variable.

The env var is isolated in `shellContextEnv.test.ts` the way the session id already
is: the CLI now exports it to every shell it spawns, so `npm test` run from inside a
qwen session inherits it, and the exact-equality assertion would have failed on a
variable the test never set. Verified by running the suite with it set.

* fix(review): point the dev daemon's CLI at the source it is running, not dist

Verifying the previous commit on a real `npm run dev:daemon` caught it doing a
smaller version of the bug it fixes. The daemon runs the TypeScript **source**
through tsx; `cli-entry.js` runs `dist/cli.js`. Pointing QWEN_CODE_CLI there traded
"the subprocess is a whole major version behind" for "the subprocess is however
stale the last build was" — measured on the box that reported this, dist was **105
source files** behind the daemon. Same bug, smaller hat.

`scripts/dev.js` is the entry that runs what the daemon itself runs, so the dev
daemon points there. It gains a shebang and the exec bit, which is what lets a
caller invoke it as `"${QWEN_CODE_CLI}" review …` without knowing it needs node —
the same shape `cli-entry.js` already has for the published path.

Verified end to end on a headless box: started the dev daemon, read
/proc/<pid>/environ (QWEN_CODE_CLI=<repo>/scripts/dev.js, -rwxr-xr-x), and ran the
command that started this whole thread. Before: `Missing required argument: chunk`.
After: `agent-prompt: --role 0 needs a plan with prNumber and ownerRepo` — the role
is understood, and the complaint is about the fixture, which is the correct answer.

* fix(review): say what a missing brief proves, once, to the reader who can act on it

A role with no recorded prompt proves one thing: the brief never reached an
agent. The roster check claimed more than that — "no prompt was built for it
(`agent-prompt --role 0` never ran)" — and on QwenLM#7012 it said that about all
twelve dimensions of a review that had just posted two Criticals with line
numbers. The agents were in the same comment the gate was calling empty.

Both failures are real and neither is the other. An orchestrator that writes the
launch by hand gets an agent that runs, reads the diff and finds things, having
never seen the severity bar, the finding format or this project's rules — all of
which live in the brief it was never given. That is worth blocking on. It is not
"nobody looked", and a check may not report the reading it cannot see.

Three changes, one shape:

- The per-role text says the brief never reached an agent, and that the
  dimension was reviewed "if at all" from a prompt the run wrote for itself.
  It no longer speaks for the agent's existence.
- Every role briefless collapses to one line. It is one failure — the run did
  not use the prompt builder — and saying it twelve times buries the fact that
  explains all twelve.
- The public body drops the internal command. `agent-prompt --role 2` is not
  something a PR author can run; on QwenLM#7012 fourteen lines of it were the whole
  CHANGES_REQUESTED while the findings sat inline below the fold. The call
  survives in check-coverage's stderr, where the orchestrator reads it, and the
  role number is already in each label.

check-coverage no longer leads with a count: the collapsed line covers the whole
roster, so "1 required brief" would undercount it by the size of the review.

Behaviour is unchanged — the gate fires on exactly the same runs and still caps
the verdict. Only the sentence changes, and only where it was overclaiming or
talking to the wrong reader.

* fix(review): name the directory the missing briefs were missing from

"The prompt builder never ran" and "the prompt builder ran against a different
--plan" arrive at this check as the same thing — an absent file — and they are
fixed differently. Nothing in the error told them apart.

The record directory hangs off the plan path as given, so a relative --plan
resolves against the caller's cwd, and the skill runs Steps 2-6 from inside the
worktree it just created. Two cwds, one relative path, two directories. Proven
locally: the same `--plan .qwen/tmp/p.json` from a repo root and from a worktree
under it yields two record dirs.

That is not a reason to resolve the path differently — resolving a relative path
against the cwd is what a relative path means, and the mismatch mostly fails
loudly, because the plan is not in the worktree either and the read errors. It
is a reason to print where it looked. One line, on stderr, where the
orchestrator reads it; the PR author gets no path to a temp directory.

* feat(review): build the whole roster in one call, because compliance decays per call

The launch prompts are already small — a role line, the brief pointer, the diff
reads — and it did not save the run that stopped building them. Dogfooded on one
PR, the same environment went from a clean review to "no prompt was built for
any of twelve roles" over three reviews in a day. The per-agent form asks the
orchestrator for ~30 build-then-launch round trips on a large review, and that
is a compliance cost paid per agent, per review, forever; what decays under
repetition eventually decayed.

`agent-prompt --roster` builds every prompt the plan requires — chunk agents,
dimension agents, invariants — in one call: one labelled block per agent, each
recorded under the key `check-coverage` will look it up by. The list is
`requiredAgents(plan)`, the same list the coverage gate reads, so what gets
built is exactly what gets checked; a key the two derive differently is refused
at build time rather than surfacing later as "brief never reached an agent" on
a compliant run.

The blocks are separated by lines that are visibly not prompt text, and a block
copied lazily — separator included — still passes the add-only delivery check.
That is load-bearing: if honest-but-sloppy copying read as a rewrite, the gate
would punish exactly the behaviour this call exists to buy.

The per-agent forms stay, for rebuilding a single prompt after Step 3D names a
gap. Step 4/5 verify and reverse-audit are untouched: they are built per round,
with the findings folded in.

SKILL.md's Step 3A and 3B now ask for the roster once instead of one call per
agent, and check-coverage's missing-brief error names the one-call fix first.

* fix(review): close the review's five consistency gaps in the CLI-pinning story

Review feedback on this PR found five places where the fix stopped short of its
own thesis. All five, addressed:

1. Four copyable SKILL.md commands had missed the QWEN_CODE_CLI sweep —
   `pr-context` (lightweight mode), `cleanup` (cache hit), `capture-local
   --file` (file-path reviews), `agent-prompt --whole-diff` (Agent 8). On a
   skewed host those modes died exactly the way the motivating run did. All
   four now carry the prefix; the remaining bare mentions are prose.

2. check-coverage's own stderr recommended recovery with a bare `qwen` — the
   message is the interface the orchestrator acts on, and on a skewed host the
   recommended recovery reproduced the skew. All four recommendation sites now
   print the prefixed form, and the rebuild hint covers `--chunk <id>`, which a
   missing chunk agent needs and `--role` cannot express.

3. Ambient inheritance could silently re-point an entry at another session's
   CLI. A dev daemon started from inside another qwen session's shell — the
   usual dogfooding flow — inherited that session's QWEN_CODE_CLI through
   `??`/`||=` and called the OUTER build: the same skew, one level up, and
   silent. Every entry now stamps itself unconditionally; nested sessions each
   call their own build. The `||=` comment in cli-entry.js also claimed a
   relaunch hazard that does not exist (the relaunch child runs dist/cli.js and
   never re-executes the wrapper) — the comment now states the real reason.

4. The third dogfooding entry point was still unpinned: `npm run dev` and
   `npm start` published nothing, so a /review from a plain dev TUI fell back
   to PATH. `scripts/dev.js` now stamps the variable in the env it spawns with
   — which also covers the daemon, since daemon-dev launches serve through it,
   and the daemon's own deferring copy is gone (one writer, not two).
   `scripts/start.js` does the same and gains the shebang and exec bit that
   make it callable as the entry it now names.

5. `"${VAR:-fallback}"` is POSIX parameter expansion, which cmd.exe passes
   through literally and PowerShell rejects. The skill was already POSIX-bound
   (Step 0 pipes through `tee`); the requirement is now total, and SKILL.md
   says so where the variable is introduced: on Windows, run the review from
   git-bash.

The unconditional stamp is pinned by a test that inherits a foreign
QWEN_CODE_CLI and asserts the spawned child gets this checkout's dev.js;
flipping the assignment back to `??` turns exactly that test red.

* fix(review): finish the two-register split, and pin the last unpinned entry

Round-2 review feedback: five more places where this PR's own rules were not
yet applied to itself.

The Agent 7 brief handed its subagent a bare `qwen`. Its two fenced command
blocks (`build-test`, `test-efficacy`) are the one call site where a SUBAGENT
shells out to the review CLI — reachable by neither the SKILL.md sweep nor the
stderr hints. Its shell gets QWEN_CODE_CLI exactly as the orchestrator's does,
so the standard prefix works verbatim; without it, an old PATH global likely
lacks these subcommands entirely, wedging the agent between its mandate (no
hand-run builds) and a command that does not exist. A test now rejects any
line-initial bare `qwen review` in that brief.

The Step 4/5 gap texts and the blind-agent line carried remediation commands
into the posted body — the register §4 stripped from missingRoles, surviving
in the sibling paths, and partly ADDED by this PR (the rewritten texts). Each
gap is now two sentences for two readers: `gap` (author-facing, no internal
commands, rendered under `Not reviewed:`) and `fix` (orchestrator-facing,
printed by compose-review to stderr as `FIX:` lines, carried on the result as
`remediation`). The four-shape precision is intact — it moved channels, not
content — and tests pin both directions: the body may not contain
`agent-prompt`/`--findings`, and the remediation must.

Pinning start.js exposed a stdout contamination: check-build-status.js printed
"Checking build status..." to stdout ahead of every child, and start.js is now
an entry whose stdout callers consume — `review parse-args --stdin | tee`
would write a plan file whose first line is not JSON. The checker's status
lines go to stderr with its warnings; `./scripts/start.js --version` now emits
the version alone.

Also from review: the all-briefless hint no longer points at role labels the
collapsed line does not carry, and start.js's stamp gets the same test dev.js
has — inherit a foreign QWEN_CODE_CLI, assert the spawned child gets this
checkout's entry.

* fix(review): isolate the env var this PR exports, and give every gap its FIX

Two findings from the bot review of the previous commit.

The shellContextEnv suite isolated QWEN_CODE_SESSION_ID and QWEN_CODE_CLI but
not QWEN_CODE_PROJECT_DIR — the third variable the CLI exports to every shell,
and the one this suite's own per-session tests assign without cleanup.
Reproduced: run the suite with it set, as any `npm test` from inside a qwen
session does, and exactly the two `.toEqual()` exact-match tests fail on a key
the test never set. Same isolation, same shape, and it retires the in-file
leak too.

The remediation channel covered blind agents and the Step 4/5 gaps and stopped
there: missing briefs, rewritten launches, unread briefs and never-opened
diffs still reached the body with no FIX line beside them. A body disclosure
with no repair command is how QwenLM#7012's orchestrator got to "the agents clearly
did their job" — the whole reason the channel exists. Each category now pushes
one remediation line (missing briefs point at `--roster`; the relaunch-shaped
ones say relaunch with the same printed prompt), and a test pins the pair for
a roster gap: the body says "brief never reached an agent" with no command in
it, and the remediation names the roster call.

* fix(review): retire the last two overclaims the round-3 review found

Two sentences, same class, both this branch's own thesis applied to itself.

A chunk agent that ran on a hand-written prompt while its chunk was never
built landed in the body as "no prompt was built for it (`agent-prompt` never
ran for this chunk)" — an internal command on the author-facing surface, one
line per chunk on a 3B replay of the QwenLM#7012 shape. The label now says what
happened in the author's register (ran on a prompt the run wrote itself; the
brief never reached it); the rebuild command already rides the
rewritten-launches remediation line on stderr.

And the Step 4/5 `not-built` texts still said "no auditor ran" / "no verifier
ran" — the one residual of the overclaim this branch exists to retire.
`not-built` is decided before the transcripts are consulted: a run that
skipped the builder and hand-wrote the launch leaves no brief on disk whose
open could be looked for, so such an auditor is invisible to the check, and
"no auditor ran" claims sight it does not have. Both texts now use the roster
wording: what a missing record proves (no agent was launched with a prompt
this skill builds), then what it costs ("ran, if at all, without the method
its brief carries"). The Delivery docstring records why.

Tests pin the new sentences positively and negatively; the register pin
(no `agent-prompt`/`--chunk` in a body label) guards the first one.

* test(review): make the every-gap-has-a-FIX claim true, and pin the partial stderr shape

Round-5 review caught a test whose title outran its body: "every coverage gap
… has a FIX" exercised only the missing-roles path, so dropping the
remediation push for unread briefs — or rewritten launches, or never-opened
diffs — failed nothing. That is the exact disclosure-without-repair state the
channel exists to prevent, asserted by a test that could not see it.

The title now claims what the test covers, and a sibling test covers the rest:
one plan, three defects — a chunk agent on a hand-written prompt, one that
never opened its brief, one that never opened the diff — asserting each
category's FIX line and that none of the three drags a command into the body.
Between the blind-agent test, the missing-roles test and this one, every
category that discloses is now asserted to repair; mutation-checked by
deleting each push in turn, one red test each.

Also from the review: the missing-briefs stderr had handler coverage only for
the all-briefless collapse. The partial shape — one role missing, the rest
briefed — reached stderr through no test, so a formatting regression there
(a broken join, a lost --roster hint, a garbled Looked-in path) would ship
unseen. A second handler test pins it: the per-role detail, the rebuild
hints, and the record-dir line, with the collapse text asserted absent.

* fix(review): close the round-5 findings — entry contracts, gap reach, repair loops

A GPT-5 review pass filed twenty-eight findings against this branch. Nineteen
were real and are fixed here; two were refuted with evidence (the scripts test
suite IS in CI: `test:ci` runs `npm run test:scripts`); the rest are recorded
follow-ups of documented floor designs.

Entry contracts. The standalone package launches through a shim that carries
the bundled Node and announces itself via QWEN_CODE_LAUNCHER_PATH — stamping
cli-entry.js there handed subprocesses a `#!/usr/bin/env node` script on hosts
that may have no system Node; the shim is now preferred, with a test. The
variable also predates this branch with a second meaning: desktop tooling sets
it to a vendored dist/cli.js — a module path, no shebang — which a POSIX shell
would run as a shell script; getShellContextEnvVars now drops a shebang-less
script (and only a script: a native binary needs none), restoring the bare
`qwen` fallback for those hosts. And both dev launchers read a signal-killed
child (`code === null`) as exit 0 — a killed gate command reported green; both
now re-raise the signal, with close(null, 'SIGKILL') regressions. The
production entry's stamp gets the test only the dev entries had.

Gap reach. `not-launched` said the pass "did not run" — but a hand-written
launch that never opened the brief lands in that shape too, so it now uses the
certification language the other shapes got. The roster check judged only the
FIRST transcript matching a built prompt, so a failed attempt masked the
compliant relaunch that the remediation itself prescribes — all matches are
consulted now. An agent flagged rewritten is no longer also flagged unopened
(contradictory repairs for one agent), and the all-briefless collapse no
longer coexists with one "none was built" line per chunk transcript.

Repair loops. Every rebuild command the run prints is now executable as
written — plan, selector, and `--rules` included, because a rebuild without
the rules file writes a rules-free brief that every delivery check still
passes; the verify variant stops inviting the empty findings file that is only
legitimate for a reverse-audit round. check-coverage prints exact selectors
beside the human labels. Idle agents and unread chunks get FIX lines too, and
a handler test pins the boundary: every FIX on stderr, before the verdict,
never in the JSON. SKILL.md Step 6 now says what FIX lines are for: one
bounded repair round, recompose, then the cap stands.

Roster integrity. The output is self-checking against the 30 000-character
shell truncation the skill itself documents — numbered blocks, an
end-of-roster line, and SKILL.md redirects it to a file read back paged. A
PR-controlled filename can no longer forge a block boundary: control
characters flatten to spaces in the label and the launch prompt, and a test
pins the separator count.

The jq interpolation in the report template is gone — the verdict line is
copied from Step 6's output, not recomputed by a binary the host may not have.
The findings read-error no longer advises omitting a flag another guard
requires.

* fix(review): filter by overwriting, not omitting — the spread carries what the record drops

The shebang filter fixed the wrong layer. It omitted QWEN_CODE_CLI from the
record getShellContextEnvVars returns — but every spawn site composes the
child env as `{...process.env, ...vars}`, so a key omitted from the additive
record arrives anyway, inherited through the spread. On exactly the hosts the
filter was written for (desktop tooling setting the variable to a shebang-less
vendored dist/cli.js), the value leaked through and every
`"${QWEN_CODE_CLI:-qwen}"` in the skill died on exit 126 — where before this
branch those hosts ran bare `qwen` and worked.

The fix is the pattern this same function already documents for the
agent/prompt IDs: write an EMPTY string, which overwrites the inherited value
through the spread, and which the consumer's `:-` expansion treats exactly
like unset. The test comment that justified omission — "an empty string would
shadow the fallback" — was true only of the colon-less `${VAR-qwen}` form and
is corrected where it stood, so the reasoning that produced the bug does not
outlive it.

The tests now assert on the channel the bug lived in: composing
`{...process.env, ...getShellContextEnvVars()}` and reading the child env —
for the shebang-less case, the unreadable-path case, and the pass-through
case. Reverting the overwrite to an omission turns exactly the two filter
tests red. Verified end-to-end: with the desktop shape in the parent env, a
child shell resolves `"${QWEN_CODE_CLI:-qwen}"` to the PATH `qwen` again.

Also from the same review: the two adjacent `missingReceipts` blocks in
compose-review are one block now (disclosure and repair cannot drift apart),
and the `Exact selectors:` line says a rebuild of an already-built role is
idempotent, so the over-prescription cannot make an operator hesitate.

* fix(review): reunite roleLabel with the doc comment the selectorOf insertion orphaned

The insertion left roleLabel's one-line JSDoc stranded above selectorOf,
stacked on top of the new function's own — a maintainer chasing a wrong-label
bug would have edited the rebuild-flags function. Each doc sits on its
function again.

* fix(review): close the round-9 findings — convergence, injectivity, and the claims a record can carry

Fourteen findings from a GPT-5 review of the previous head; twelve fixed here,
one was already fixed in the commit the review missed, one re-recorded as the
standing roster-design follow-up.

Repair loops now converge. Coverage accumulated every historical failed
transcript, so the relaunch its own FIX line prescribes ADDED a transcript
while the failed one kept its flag — ok stayed false, the same FIX printed
forever. A failed attempt is now superseded by a compliant attempt at the same
target (same chunk served verbatim with the diff opened; same built prompt
delivered to an agent that opened its brief), and a rewritten agent is not
also told to relaunch the prompt that was the defect.

One transcript, one credit. Pasting the whole roster output to a single agent
produced one transcript that verbatim-contains every block, matched every
requirement independently, and certified an N-agent fan-out with one reader
(reproduced upstream: roster 8, agents 1, ok true). Requirements now claim
distinct transcripts; the paste-all run fails with a sentence that names the
mistake.

Records claim only what they prove. "Its brief never reached an agent" said
more than a missing record can see (the builder may have run against another
--plan spelling); it now reads "no record shows its brief reaching an agent".
The rewritten texts claimed the brief's method never arrived — but that shape
is DETECTED by the brief being opened; they now state exactly that, and that
the launch was not the built one. A zero-byte record (a torn write) no longer
counts as built anywhere: one predicate serves the collapse, the roster loop
and the chunk lookup.

Entries the shell can actually run. The shebang filter now also requires the
execute bit (a 0644 script passes the header check and dies on EACCES), and
cli-entry consumes QWEN_CODE_LAUNCHER_PATH at stamp time — the serve/mcp fast
path never reached the branch that deleted it, so a standalone daemon leaked
the outer shim into every child, where a different checkout would republish it
as its own entry.

Inputs a PR cannot weaponize, commands an operator can run. The invariant
brief interpolated the raw PR-controlled filename into the file the agent is
told is the whole of its instructions — display sinks now flatten control
characters and the functional read argument is JSON-quoted. Agent 7 no longer
receives the review rules its own workflow forbids it (SKILL.md: deterministic
commands, not code review). The verifier refuses an empty findings file — a
vacuous pass that cleared the delivery floor while ruling on nothing — while
the early reverse-audit round keeps it. FIX lines carry the run's real plan
path instead of a `<plan>` placeholder that pastes as a shell redirection, the
roster truncation hint names --file and --rules, and composed.json persists
the exact verdictLine so the archived report copies rather than reconstructs
it — event and cappedBy alone cannot express a presubmit downgrade.

Every new behaviour is pinned: convergence, paste-all refusal, and the
zero-byte collapse are mutation-checked (disabling each turns exactly its test
red); the exec-bit, brief-injection, launcher-consumption and verdictLine
contracts each carry a direct test. 1 236 tests across the affected suites.

* fix(review): close the three paths the round-11 review found still open

The Step 4/5 FIX lines still carried a literal `--plan <plan>`. Round 9
substituted the real path into compose-review's own remediation strings and
check-coverage's hints, and left the one builder both Step 4/5 gaps flow
through — `rebuildFix` — untouched: its output reached stderr through
verificationGaps with the placeholder intact, and a literal `<plan>` pasted
into a POSIX shell parses as input redirection, so the one repair round Step 6
prescribes could never run there. The push sites now substitute the plan path
verificationGaps was handed, and the test that pins the fix text asserts no
literal `<plan>` survives anywhere in the remediation.

A lightweight cross-repo review can now be REQUIRED to run Agent 0. plan-diff
takes `--pr <n> --repo <owner/repo>` — passed only after pr-context succeeds,
so the pair's presence doubles as the context-availability signal — and
writes the identity into the plan; the roster requires role 0 wherever the
full identity is present, not only in worktree mode (fetch-pr always writes
both fields, so PR-worktree behavior is unchanged). Half an identity is
refused: a roster demanding an agent nobody can brief would wedge the run.
SKILL.md's lightweight capture block carries the flags and the
when-not-to-pass-them rule.

And the path-inertness boundary is one function with a wider net: `inertPath`
now flattens every control character (a terminal escape in a filename must
not reach a terminal), the separator glyph, and the backtick — which could
close the Markdown code span the path is rendered inside and let the tail of
a PR-controlled filename run as markup in the brief the agent treats as
authoritative. The roster label and launch-prompt sites that had their own
narrower regexes now share it. The injection test's hostile filename gained a
backtick and an ESC sequence, and asserts the rendered heading carries
exactly the span's own backtick pair and no control bytes, while the
JSON-quoted functional read argument still round-trips the raw path.

Each fix is mutation-checked: reverting the substitution, re-gating the
roster on worktree mode, and narrowing inertPath each turn exactly one test
red.

* fix(review): bind the receipt to what was delivered, and match what actually assigns

Three review-integrity holes from the round-12 review, each with a
reproduction, each fixed at the layer the reproduction named.

The verify receipt could be satisfied by a partial delivery. The record was
deliberately the findings-free launch block, so one key could serve every
shard by the add-only rule — and that same rule let a caller build with a real
findings file, launch the agent with only the recorded tail, and clear the
gate while no verifier ever saw a finding. The record is now the EXACT printed
prompt, findings folded in, keyed per findings-content digest
(`verify--<sha>`, `reverse-audit--chunk-N--<sha>`); the delivery side collects
the whole key family with the documented floor of one. Tail-only delivery
matches nothing; each shard verifies against its own list; shard records no
longer share a key, so none clobbers another.

The injective roster matching was greedy, and greedy rejects valid
assignments. With transcript T1 containing blocks A+B and T2 containing only
A, first-come claiming took T1 for A and reported B missing — a compliant
repair permanently capped by transcript filename order. The claim set is now a
maximum bipartite matching (Kuhn's augmenting paths), seeded on the edges
where the transcript also opened the requirement's brief and extended over all
verbatim edges, so a requirement reports missing only when no injective
completion exists at all.

A rules-free rebuild could silently strip the brief. The launch prompt only
points at the brief, so rebuilding a rules-bearing role without --rules left
the recorded launch byte-identical while the project rules vanished from the
one file the agent treats as authoritative — every delivery check kept
passing. writeBrief now refuses the downgrade at the single choke point both
build paths pass through, with the escape hatch named (delete the record dir
to start over deliberately).

All three are mutation-checked: regressing the record to findings-free, the
matching to greedy, or disabling the downgrade guard each turns its own test
red. 704 review tests green.

* docs(review): let the docs and comments claim only what the new record design does

The round-13 review caught the drift this branch's own thesis forbids: two
SKILL.md sentences still described the findings-free record the previous
commit retired — an orchestrator reasoning from them would conclude a
findings-less delivery still matches, precisely the bypass that commit closed.
Both now state the new contract: the record is the exact printed block, keyed
per findings digest, and a launch that drops the list matches no record.

And the matching comment claimed more than Kuhn guarantees: phase-2
augmentation can displace an opened match onto an unopened edge to enlarge the
matching, so an unread flag describes the assignment, not an impossibility.
The comment now says so, and why cardinality is the right thing to maximize.

* docs(review): finish retiring the findings-free record from every sentence that described it

Round 15 found the three survivors round 13 missed — all in code, not
SKILL.md: the findingsSection docstring (all three of its clauses false since
the digest-key commit), the findings field doc ('Printed, not recorded'), and
the --findings --help text, which told an operator the exact opposite of what
the command now does. Each now states the new contract: the findings are part
of the recorded prompt, keyed per digest, and a launch that drops them matches
no record.

Also from the same review: the plan-path substitution uses a function
replacer, so a path containing $& or $` cannot be misrendered as a
replacement pattern. Practically unreachable for .qwen/tmp paths; closed
because it costs four characters.

* docs(review): the actually-last sentence describing the findings-free record

Round 16 counted one survivor of the sweep the previous commit's title
claimed complete: the acceptsFindings jsdoc in agent-briefs.ts, present-tense,
whose '(see runAgentPrompt)' pointed at a function whose own comment says the
opposite. It now states the digest-key contract like its siblings, and a
whole-tree grep for present-tense descriptions of the retired design comes
back empty.

* test(review): pin the idle and missing-chunk FIX lines to the remediation channel

Round-18 review: the two remediation pushes added for the every-gap-has-a-FIX
rule had no test of their own — deleting either failed nothing, leaving a body
disclosure whose repair could silently vanish, the exact state the channel
exists to prevent. The idle-plan test now asserts the relaunch FIX; the
blind-plan test, whose chunks nobody reads, now asserts the chunks-nobody-read
FIX beside the blind one. Both mutation-checked: deleting each push turns
exactly one test red.

* fix(review): quote the plan path in every printed repair, and test the executable shebang-less shape

Round-21 review, three items. The plan path is now single-quoted at all seven
sites that print it into a repair command — a workspace path containing a
space split the copy-pasted FIX at the space, exactly the operator moment the
lines exist for; the earlier uniformity deferral ends here, uniformly.
PlanDiffResult declares prNumber/ownerRepo so a refactor away from the
conditional spread cannot silently drop the fields the roster's Agent-0
requirement reads. And the filter gains the test its primary target deserved:
an EXECUTABLE shebang-less .js (the desktop vendored bundle shape) is rejected
by the header read itself — the existing 0644 fixture never reached that
branch, so a regression in the byte read would have passed every test.

* fix(review): shell-quote the plan path properly — an apostrophe is not rarer than a space

Round-22 review: the bare '…' wrap from the previous commit closed at an
embedded apostrophe, so ~/Documents/John's Projects broke where it had worked
unquoted — one breakage class traded for another instead of both closed. A
shared shellQuotePath (the same '\'' dance as utils/standalone-update.ts)
now serves all six repair-printing sites, and a test drives verificationGaps
from a plan under an apostrophe directory, asserting the escaped form and
rejecting the naive wrap.

* fix(review): quote the --file selector, un-dead the spawn guard, test the half-identity

Round-24/25 reviews, four small items. selectorOf now shell-quotes the --file
path — the same copy-paste contract the --plan quoting just earned, on the one
selector that carries a path. RULES_MARKER moves above writeBrief's JSDoc,
which it had been silently stealing. The check-build-status test's reject
guard was dead (execFile always delivers string stdout, so an ENOENT resolved
and the empty-stdout assertion passed on a script that never ran) — it now
rejects on spawn-level errors, which carry string codes, while non-zero exits
still resolve. And the roster's ownerRepo guard gets the independent test it
never had: a plan with prNumber but no ownerRepo requires no Agent 0, since
the brief builder cannot serve half an identity.
yiliang114 added a commit that referenced this pull request Aug 15, 2026
- 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
yiliang114 added a commit that referenced this pull request Aug 17, 2026
…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.
euntaek-hong pushed a commit to wrongbutworks/qwen-code that referenced this pull request Aug 19, 2026
…dechannel (QwenLM#9202)

* fix(sdk): route unrecognized diagnostics onto a bounded transcript sidechannel

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.

* fix(sdk): align browser bundle budget

* fix(sdk): close the sidechannel review round (QwenLM#8823)

- 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 QwenLM#7012
  flush-before-guard keeps a discriminating stimulus

* fix(sdk): address round-2 sidechannel review for QwenLM#8823

- 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

* fix(webui): avoid flushing sidechannel diagnostics

* fix(sdk): preserve diagnostics across rewind

* fix(webui): dedupe sidechannel history records

* fix(webui): align the paging sidechannel test with the normalizer keys

The paging test added in e6b40e5 failed deterministically (webui
suite red, CI Test job red) for two reasons:

1. The fixtures stamped only _meta['qwen.session.recordId'], but the
   SDK normalizer's extractSourceRecordIds reads
   _meta.qwenTranscript.sourceRecordIds — no sidechannel entry ever
   carried sourceRecordIds, so the dedupe assertion could not pass and
   the new displayedRecordIds loop was never exercised by a passing
   test. Stamp BOTH keys, matching production replay frames
   (acp-bridge buildUpdateMeta) and the sibling dedupe test.
2. Cap arithmetic: LIMIT-1 live entries + 2 fresh history entries =
   LIMIT+1, so the newest-wins slice evicted record-old-1 which the
   test asserted present. Emit LIMIT-2 live events so the post-merge
   total lands exactly on the cap.

Also correct the post-merge index assertions: history entries come
first (old-1, old-2), then the deduped-once live overlap, then the
first live mystery event. Suite 506/506, eslint + prettier clean.

* fix(sdk): raise diagnostic sidechannel bundle budget

* fix(sdk): raise the daemon browser bundle budget to 198KB and pin the 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.

* fix(sdk): reset the user pointer on sidechanneled diagnostics, share 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 QwenLM#7012/QwenLM#8823 guard pair classifies every debug event
against one source instead of three hand-written copies.

* fix(ci): prevent bite harness SIGPIPE

---------

Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants