feat: support fork from any conversation - #8817
Conversation
Add durable response checkpoints so Web Shell sessions can branch from eligible completed Assistant turns without mutating the source history. - Record and validate checkpoints behind serialized topology fences - Preserve historical anchors through replay, daemon, SDK, and UI layers - Publish bounded forks with crash-safe ownership and referenced backups - Serialize prompt, rewind, branch, automatic turn, and close mutations - Cover stale anchors, replay pagination, cleanup, and pending UI states Note: Responses recorded before this change remain non-branchable. # Conflicts: # packages/acp-bridge/src/bridge.ts # packages/acp-bridge/src/bridgeTypes.ts # packages/cli/src/acp-integration/acpAgent.test.ts # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/serve/routes/session.ts # packages/cli/src/serve/server.test.ts # packages/core/src/services/chatRecordingService.ts # packages/core/src/services/sessionService.test.ts # packages/core/src/services/sessionService.ts # packages/sdk-typescript/src/daemon/DaemonClient.ts # packages/web-shell/client/components/MessageItem.tsx # packages/web-shell/client/components/MessageList.tsx
Keep Assistant-response branching intact across the daemon stack after rebases, including history serialization and persisted-session ownership. - Forward durable checkpoint IDs through Bridge, SDK, and UI layers - Serialize live history mutations and retain valid nested branch anchors - Preserve persisted branches during generation cleanup - Add cross-layer regression tests for replay and stale checkpoints
Keep the PR review report as a local ignored backup instead of shipping it with the feature branch. - Remove the generated PR comment evaluation from tracked files - Preserve the report under the ignored analyze directory
Historical branch requests could outlive the client timeout during an active turn, and interactive forks lacked the recorder's cross-process writer-lease barrier. - Hide Assistant Branch actions while a turn is active - Run interactive fork creation inside the recorder write barrier - Use the concrete checkpoint recorder contract in Session - Document committed-session ownership and implemented design status
Build branch catalogs during the frozen index scan so the first history page no longer reopens and materializes the complete active chain. - Retain a compact projection for shared branch-point resolution - Correlate live branch anchors with the completed prompt and final reply - Complete recorder mocks required by the concrete Session contract - Update the reviewed design with performance and correlation invariants
…tId guard, stale toast (QwenLM#8274)
…tId guard, stale toast (QwenLM#8274)
…n, UUID validation, dead code (QwenLM#8274)
) Add focused tests requested in PR review: - branch catalog resolves checkpoints that fall on a later page - accept a parallel tool batch closed within a single turn - exercise the linkSync->copyFileSync fork backup fallback success path - prove a remapped checkpoint stays usable via a nested fork - isolate each branch-point validation conjunct across bridge and SDK
- Make the directory-fsync durability test platform-aware (skip on win32), since fsyncDirectoryBestEffort swallows the injected error on Windows and the rejection path is non-Windows by design. - Reject atRecordId on the side-task fork path instead of silently discarding it, so the API surface no longer implies acceptance. - Correct the design doc: name the real promptQueue FIFO (not the nonexistent historyMutationQueue) and describe filtered checkpoint boundaries as remapped to the nearest retained predecessor, not unconditionally null. - Add focused tests: branch-point assistantRecordUuid mismatch rejection, and insight-block branchRecordId anchoring (insight-only block must not anchor onto the previous reply).
…nversation # Conflicts: # packages/cli/src/acp-integration/acpAgent.ts
…t shapes (QwenLM#8274) - Filter null/non-object part elements in the shared branch resolver so a transcript containing null parts no longer makes forkSession throw a TypeError for every checkpoint. - Tag tool calls carried in from the pre-boundary prefix so a dangling call left by a crashed turn no longer permanently disables checkpoint recording; only calls issued inside the turn must close. - Merge duplicate-uuid records first-wins for identity fields in the transcript reader, matching the byUuid index and fork aggregation, so the reader never advertises a branch marker the fork path must reject.
…nversation # Conflicts: # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/acp-integration/session/history-replay-page.test.ts
…nversation # Conflicts: # packages/cli/src/acp-integration/session/history-replay-page.test.ts
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; this diff has OS-sensitive paths (fsync win32 branch, backup path validation) verified on Linux only.
Not explored to full depth (tool budget reached): chunk 35: none — all checks I planned completed within budget.; chunk 16: I did not run the four new vitest cases dynamically (node_modules is not installed in this worktree; I relied on static cross-checking against the implementatio…; chunk 16: dynamic execution of the four new vitest cases ( server.test.ts -t 'POST /session/:id/branch' ) — not run; node_modules is not installed in this worktree, so …; chunk 34: none — I did not attempt a standalone tsc run for the optional-chain narrowing in assistantDoneFromTurnEvent ( data['branchPoint'] after isRecord(data?.['…; chunk 5: could not run the tests (no node_modules in the worktree; full monorepo npm install exceeds budget) — static verification only. Also did not fully page throug…, and 6 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; this diff has OS-sensitive paths (fsync win32 branch, backup path validation) verified on Linux only。
未探索到全部深度（达到工具调用预算）：chunk 35：none — all checks I planned completed within budget.；chunk 16：I did not run the four new vitest cases dynamically (node_modules is not installed in this worktree; I relied on static cross-checking against the implementatio…；chunk 16：dynamic execution of the four new vitest cases ( server.test.ts -t 'POST /session/:id/branch' ) — not run; node_modules is not installed in this worktree, so …；chunk 34：none — I did not attempt a standalone tsc run for the optional-chain narrowing in assistantDoneFromTurnEvent ( data['branchPoint'] after isRecord(data?.['…；chunk 5：could not run the tests (no node_modules in the worktree; full monorepo npm install exceeds budget) — static verification only. Also did not fully page throug…，另有 6 条。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const record = activeChain[index]!; | ||
| pendingCalls.push(...functionCalls(record)); | ||
| for (const response of functionResponses(record)) { | ||
| closeToolCall(pendingCalls, response); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The boundary scan hand-inlines the exact body of updatePendingBranchToolCalls (same file, ~lines 186-193) instead of calling it; the writer-side tracking (chatRecordingService.updateActiveBranch) uses the helper. — Failure scenario: a future change to close semantics (closeToolCall ordering, carried-call handling) applied to one site but not the other makes writer and reader disagree about which calls were pending at a boundary → checkpoints get written that resolveBranchPoints later rejects, silently removing branch points from the UI with no error.
| const record = activeChain[index]!; | |
| pendingCalls.push(...functionCalls(record)); | |
| for (const response of functionResponses(record)) { | |
| closeToolCall(pendingCalls, response); | |
| } | |
| const record = activeChain[index]!; | |
| updatePendingBranchToolCalls(pendingCalls, record); |
中文说明
边界扫描把 updatePendingBranchToolCalls（同文件约 186-193 行）的函数体逐行内联，而不是直接调用它；写入侧的跟踪（chatRecordingService.updateActiveBranch）使用的是该 helper。失败场景：未来对关闭语义（closeToolCall 顺序、carried-call 处理）的修改只应用到其中一处时，写入方与读取方对"边界处哪些调用处于 pending"的判断就会分歧 → checkpoint 被写入后又被 resolveBranchPoints 拒绝，Branch 入口从 UI 上静默消失且无任何报错。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| }); | ||
| }); | ||
|
|
||
| describe('recordNotificationStrict', () => { |
There was a problem hiding this comment.
[Suggestion] The deleted test 'flushes before reading the canonical active transcript' was the only pin of readActiveTranscriptChain's flush-before-load ordering; the method body is unchanged (relocated to chatRecordingService.ts:1207) and still consumed in production (goal-runtime.ts:633/802/1191, config.ts:7372) — and the final commit also deleted the rewritten version, leaving zero references in this file. — Failure scenario: a future refactor that drops or reorders await this.flush() → goal-runtime rebase rebuilds goal evidence from a stale transcript prefix (missing the latest unflushed turn) and nothing turns red. Suggested fix: restore an ordering test (spy flush, mock getSessionService().loadSession, assert order ['flush', 'load']).
中文说明
被删除的测试 'flushes before reading the canonical active transcript' 是 readActiveTranscriptChain "先 flush 再 load" 顺序的唯一固定；该方法本体未变（仅移动到 chatRecordingService.ts:1207），且仍有生产消费者（goal-runtime.ts:633/802/1191、config.ts:7372）——并且最后一个提交连重写版也删掉了，本文件中已无任何引用。失败场景：未来某次重构删除或调整 await this.flush() 的顺序 → goal-runtime rebase 会基于过期的转录前缀（缺少最新未落盘回合）重建 goal 证据，而没有任何测试变红。建议修复：恢复顺序测试（spy flush，mock getSessionService().loadSession，断言顺序 ['flush', 'load']）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| await Promise.all([ | ||
| fs.promises.mkdir(chatsDir, { recursive: true }), | ||
| fs.promises.mkdir(backupRoot, { recursive: true, mode: 0o700 }), | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] The test 'uses asynchronous filesystem APIs for fork publication and backup staging' (spied 19 sync fs APIs) was added by 0259297e3d and deleted by the final commit 07f354f284; nothing now pins that the daemon-side fork commit path avoids synchronous fs calls — an explicit requirement of design §14.3 and a §18.4 verification item. — Failure scenario: a follow-up reintroducing a sync call into the staging/commit path (an easy accident, e.g. an existsSync/mkdirSync 'simplification') blocks the daemon event loop for the duration of the write — every SSE stream and RPC stalls — and passes CI silently. Suggested fix: restore the spy-based assertion, scoped to the commit path if the original spies were brittle.
中文说明
测试 'uses asynchronous filesystem APIs for fork publication and backup staging'（spy 了 19 个同步 fs API）由 0259297e3d 添加，又被最终提交 07f354f284 删除；现在没有任何测试固定 "daemon 侧 fork 提交路径不使用同步 fs 调用"——这是设计 §14.3 的明确要求与 §18.4 的验证项。失败场景：后续修改在 staging/commit 路径中重新引入同步调用（很容易发生，例如 existsSync/mkdirSync 式的"简化"）会在写入期间阻塞 daemon 事件循环——所有 SSE 流与 RPC 停摆——且 CI 静默通过。建议修复：恢复基于 spy 的断言；如果原 spy 过于脆弱，可把断言范围限定在提交路径。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| describePOSIX('qwen serve — historical Assistant response branch', () => { | ||
| it('creates, opens, and continues a branch through the real daemon', async () => { |
There was a problem hiding this comment.
[Suggestion] This file sits outside every npm workspace, so no workspace test command collects it — and the one CI job that owns it, Integration Tests (CLI, No Sandbox), is skipped again at this commit (it was skipped in rounds 1 and 2 as well). — Failure scenario: if the integration job is skipped at merge, the only end-to-end coverage of branchSession through a real daemon (create → prompts → branchSession({atRecordId}) → load → continue) runs nowhere, and a regression in checkpoint resolution or transcript forking reaches merge gated only by unit-level tests. Suggested fix: confirm the integration job covering test:integration:cli:sandbox:none runs and passes for this PR before merge; no code change needed if it does.
中文说明
该文件不属于任何 npm workspace，因此没有任何 workspace 测试命令会收集它——而唯一负责它的 CI 任务 Integration Tests (CLI, No Sandbox) 在本提交上再次被跳过（第 1、2 轮同样被跳过）。失败场景：如果合并时该集成任务仍被跳过，branchSession 经由真实 daemon 的唯一端到端覆盖（创建 → prompt → branchSession({atRecordId}) → 加载 → 续写）将无处运行，checkpoint 解析或转录 fork 的退化会在仅有单测把关的情况下进入合并。建议修复：合并前确认覆盖 test:integration:cli:sandbox:none 的集成任务已运行并通过；若已通过则无需代码改动。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| } catch (error) { | ||
| if (error instanceof BranchPointInvalidError) { | ||
| throw new RequestError(-32009, error.message, { | ||
| errorKind: 'branch_point_invalid', | ||
| recordId: error.recordId, | ||
| }); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-6: still stands at this commit — the branch handler's catch translates only BranchPointInvalidError; the admission errors newly introduced by this diff's gate lambda — assertCanStartTurn()'s 'Session is closing' / 'Session history mutation is in progress' RequestError.invalidParams — carry no errorKind and surface across the bridge/HTTP boundary as generic 500s. — Failure scenario: a branch racing a close or another mutation reaches the gate and its rejection propagates raw: sendBridgeError has no kind to map, so the client sees a 500 with no structured errorKind and cannot tell a retryable race from a crash (same unmapped-error family as the session_busy Critical on bridge.ts). Suggested fix: give the gate/admission errors an errorKind (e.g. session_busy / session_closing) and map them here or in branchSession alongside the session_busy fix.
中文说明
R2-6：在本提交仍然存在——branch 处理器的 catch 只转换 BranchPointInvalidError；本 diff 的门控 lambda 新引入的准入错误——assertCanStartTurn() 的 'Session is closing' / 'Session history mutation is in progress' RequestError.invalidParams——不带 errorKind，越过 bridge/HTTP 边界后表现为通用 500。失败场景：与 close 或其他变更竞争的 branch 到达门控时，其拒绝原样传播：sendBridgeError 无 kind 可映射，客户端看到没有结构化 errorKind 的 500，无法区分可重试的竞争与崩溃（与 bridge.ts 上 session_busy Critical 同属未映射错误家族）。建议修复：给门控/准入错误加上 errorKind（如 session_busy / session_closing），并在此处或 branchSession 中与 session_busy 修复一并映射。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| 'releases side-task admission when restore failure is %s', | ||
| async (restoreFails) => { |
There was a problem hiding this comment.
[Suggestion] R2-8: still stands at this commit — the restore-failure case of the side-task admission test never asserts the sessionClose cleanup call the bridge makes for the committed-but-unrestorable session (bridge.ts:7478 issues SERVE_CONTROL_EXT_METHODS.sessionClose); only the admission release is pinned. — Failure scenario: if a future refactor drops or reorders that cleanup, the orphaned runtime entry leaks for the connection's lifetime and no test turns red. Suggested fix: in the restoreFails case, assert the sessionClose extMethod call for the committed session id.
中文说明
R2-8：在本提交仍然存在——side-task 准入测试的 restore 失败用例从未断言 bridge 对"已提交但无法恢复"会话发起的 sessionClose 清理调用（bridge.ts:7478 发出 SERVE_CONTROL_EXT_METHODS.sessionClose）；目前只固定了准入释放。失败场景：未来重构若删除或调整该清理，孤儿运行时条目会在连接的整个生命周期内泄漏，且没有测试变红。建议修复：在 restoreFails 用例中断言针对已提交会话 id 的 sessionClose extMethod 调用。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (cleanup === 'detach') { | ||
| await vi.waitFor(() => | ||
| expect(bridge.detached).toContainEqual({ |
There was a problem hiding this comment.
[Suggestion] R2-11: still stands at this commit — the fork-race cleanup's requireZeroAttaches: true kill guard, the only protection against killing a branch another client attached to, is not pinned by this test, because FakeBridge.killSession(sessionId: string) (line 219) drops the options argument entirely. — Failure scenario: if dispatch regresses to killSession without requireZeroAttaches (or the bridge changes the option's meaning), the cleanup could kill a session a second client just attached to, and the test still passes. Suggested fix: capture the options in FakeBridge.killSession(sessionId, options) and assert requireZeroAttaches: true in the kill branch.
中文说明
R2-11：在本提交仍然存在——fork 竞争清理中的 requireZeroAttaches: true kill 守卫（防止杀掉已被其他客户端 attach 的分支的唯一保护）并未被本测试固定，因为 FakeBridge.killSession(sessionId: string)（219 行）完全丢弃了 options 参数。失败场景：如果 dispatch 退化为不带 requireZeroAttaches 的 killSession（或 bridge 改变了该选项的含义），清理可能杀掉第二个客户端刚刚 attach 的会话，而测试仍然通过。建议修复：让 FakeBridge.killSession(sessionId, options) 捕获 options，并在 kill 分支断言 requireZeroAttaches: true。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| code: -32602, | ||
| data: { errorKind: 'session_busy' }, | ||
| }); | ||
| expect(lastSessionMock?.rewindToTurn).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R2-15: still stands at this commit — the rewind busy-rejection test asserts the rejection and that rewindToTurn was not called, but never asserts beginHistoryMutation was NOT called by the rejected rewind (it was already called once by the gate-holding branch), leaving the fail-fast-before-gate-acquisition ordering unpinned. — Failure scenario: if a refactor moves the isIdle() fail-fast behind runExclusiveHistoryMutation acquisition, a busy rewind would acquire and hold the gate before rejecting — client-visible behavior is unchanged, the test stays green, and branch/close operations queue behind a gate held for nothing.
| expect(lastSessionMock?.rewindToTurn).not.toHaveBeenCalled(); | |
| expect(lastSessionMock?.rewindToTurn).not.toHaveBeenCalled(); | |
| expect(lastSessionMock?.beginHistoryMutation).toHaveBeenCalledTimes(1); |
中文说明
R2-15：在本提交仍然存在——rewind 忙拒绝测试断言了拒绝本身与 rewindToTurn 未被调用，但从未断言被拒绝的 rewind 没有调用 beginHistoryMutation（此时它已被持有 gate 的 branch 调用过一次），导致"先快速失败、后获取 gate"的顺序未被固定。失败场景：如果重构把 isIdle() 快速失败移到 runExclusiveHistoryMutation 获取之后，忙的 rewind 会先获取并持有 gate 再拒绝——客户端可见行为不变、测试保持绿色，而 branch/close 操作会排在一个毫无意义持有的 gate 后面。
— qwen3.8-max via Qwen Code /review (v0.21.9)
…conversation # Conflicts: # packages/acp-bridge/src/bridge.ts # packages/sdk-typescript/scripts/build.js # packages/web-shell/client/App.tsx # packages/webui/src/daemon/session/actions.test.ts # packages/webui/src/daemon/session/actions.ts
…conversation # Conflicts: # packages/acp-bridge/src/bridge.test.ts
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at 3b12f83. Verified all seven prior-round Criticals are fixed at this head, and audited the core fork path end to end.
Architecture: completed turns write branch_checkpoint system records into the JSONL (chatRecordingService.recordBranchCheckpointTransaction, end_turn only, under a topology fence with a cursor staleness guard). Forks resolve valid checkpoints via branch-points.resolveBranchPoints (tool-call closure + single visible terminal assistant record), and SessionService.forkSession materializes the new session.
Verified invariants:
- Fork truncation/lineage: atRecordId is validated against the source active chain and bounded at the first occurrence of the checkpoint uuid; only the active parentUuid branch is copied (rewind-abandoned branches can't resurrect); the fork is relinked as a clean linear chain with per-record forkedFrom, parent_session/session_source stripped, checkpoint boundaries remapped to retained predecessors, file-history snapshots remapped so /rewind works in the branch, and the written transcript is re-validated post-build. Source transcript is read-only throughout — no corruption path.
- Durability: staged tmp file (wx + fsync) → hardlink commit with EEXIST handling → chatsDir fsync; published backup dir is rolled back if the transcript commit fails; target-collision pre-checks in place.
- Concurrency: branch/rewind fail fast on Session.isTurnIdle() (covers interactive, cron, and notification turns; deliberately narrower than isIdle), re-checked inside a per-session history-mutation gate via assertCanStartTurn/beginHistoryMutation, and the fork itself runs under the recorder write barrier. cancel() now normalizes sessionId before aborting queued prompt controllers (fixes the raw-vs-normalized key mismatch).
- API contract: branch_point_invalid → 409, session_busy → 409 + Retry-After (SessionBusyError now mapped in bridge.branchSession and error-response), invalid_rewind_target → 400. SDK transcript reducer attaches branchRecordId via promptId-scoped findFinalVisibleAssistantForPrompt gated on reason==='end_turn', immune to synthetic assistant.done dispatches. web-shell dedupes in-flight branches at App level (pendingBranchRequestsRef keyed by session+record), survives virtualizer unmounts; stale-checkpoint recovery reload is guarded against session switches. Integration test loads the branch before prompting and pins source immutability + truncation.
CI at head: Test (ubuntu, Node 22) green; windows/macos/integration skips match repo-wide PR behavior (same skips on other PRs). One red job, SDK Java ubuntu-latest / Java 11 — DaemonSessionClientTest.blockedTerminalContinuationsApplyBoundedPublicationBackpressure fails with a detach-outcome ambiguity; this PR touches no Java code, unrelated PRs fail different Java jobs in the same window, and the Real daemon E2E job is green — treating as flaky/infra, not diff-related.
Remaining nits (P2/P3, non-blocking, most already tracked in prior rounds): branch-handler catch translates only BranchPointInvalidError so a raced assertCanStartTurn rejection surfaces without errorKind; backupRoot directory entry is never fsynced; fork-race cleanup (requireZeroAttaches kill guard) and the post-publication rollback path lack direct test pins; the branch-point UUID regex exists as three independent copies across bridge/SDK/webui.
…conversation # Conflicts: # packages/web-shell/client/App.test.tsx
…conversation # Conflicts: # packages/web-shell/client/components/MessageItem.tsx # packages/web-shell/client/components/MessageList.dom.test.tsx
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-review at a30d619 after the prior approval at 3b12f83 was dismissed by a push.
What changed since 3b12f83: two merges of upstream/main only (#8990 webui same-session refresh race fixes, #8955 prompt-admission ownership hardening, #9000 docs, #8951/#8976 desktop follow-ups + OSS release mirror, #8973 web-shell compact tool activity). Zero fork-feature commits; the diff touches no fork/checkpoint/session-storage/daemon-fork files, so the merge was clean against this PR's code and the previously verified state carries over byte-for-byte.
Invariants still hold at this head (files unchanged from the approved head): source session remains read-only, tmp+fsync+hardlink atomic commit, the three-layer concurrency gate, and the 409 branch_point_invalid/session_busy error contract.
Merge surface checked for fork interaction:
- acp-bridge (#8955): adds an
eventEpochguard to same-session refresh replay, rejecting refresh application when the session instance changed underneath — complementary to fork correctness. - webui DaemonSessionProvider (#8990): transition-cleanup consolidated into
cleanupTransitionArtifactswithpreserveInFlightCapture, plus a controlled-retry that re-fires a deferred refresh once source-bound operations drain. The webui fork entry rides on this path; the change closes the old refresh-race gap rather than opening a new one.
Carry-over nits (non-blocking, unchanged from prior review): missing errorKind on gate-race refusal, backupRoot dir not fsynced, fork-race cleanup/post-publish rollback paths lack direct tests, branch-point UUID regex duplicated across three packages.
CI at a30d619: green — 25 success, 0 failures; standard skips (macos/windows Node 22.x, CLI no-sandbox integration, coverage/automation jobs); web-shell E2E smoke passed.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3 (round 3 reported findings; all were verified, but no further audit round ran).
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; OS-sensitive paths (fsync win32 branch, backup symlink fallback) verified on Linux only.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; chunk 28: none — all checks I opened were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — every check I opened was closed before the ceiling., and 6 more.
中文说明
已审查。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
未审查：reverse audit — did not converge within the reverse-audit round cap of 3 (round 3 reported findings; all were verified, but no further audit round ran)。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; OS-sensitive paths (fsync win32 branch, backup symlink fallback) verified on Linux only。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；chunk 28：none — all checks I opened were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — every check I opened was closed before the ceiling.，另有 6 条。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| Before an ordinary branch is queued behind that boundary, the Agent checks | ||
| `sourceSession.isIdle()` and returns `session_busy` immediately when an | ||
| interactive, cron, or notification turn is active. This is not a replacement |
There was a problem hiding this comment.
[Suggestion] Design doc §12.2 names the wrong predicate: it says the Agent checks sourceSession.isIdle(), but the implementation's branch/rewind fail-fast checks sourceSession.isTurnIdle() (acpAgent.ts:11197 branch, acpAgent.ts:10902 rewind). isIdle() = isTurnIdle() && collectActiveWorkHolds().length === 0 (Session.ts:2415-2418); the only agent-side isIdle() consumer is the unrelated settings-reload path (acpAgent.ts:11579). The PR's own tests pin the weaker predicate (isIdle=false with non-turn work → branch/rewind proceeds). — Failure scenario: a maintainer doing future race analysis reasons with the documented guard and assumes branch/rewind fail fast while a background-agent work hold is active (no active turn), when in fact the mutation proceeds; conversely, 'fixing' the code to match the doc would start rejecting branches during pending background tasks even though proceeding is safe and test-pinned. §8.3 also still says "The prompt holds the Agent history mutation lock for this entire interval", contradicting §12.2 and the implementation. — Suggested fix: change §12.2 to sourceSession.isTurnIdle() (optionally noting that active work holds deliberately do not block branching), and fix §8.3's prompt-holds-the-lock sentence to match the implementation.
中文说明
设计文档 §12.2 指定了错误的谓词：文档说 Agent 检查 sourceSession.isIdle()，但 branch/rewind 快速失败实际检查的是 sourceSession.isTurnIdle()（branch 在 acpAgent.ts:11197，rewind 在 acpAgent.ts:10902）。isIdle() = isTurnIdle() && collectActiveWorkHolds().length === 0（Session.ts:2415-2418）；agent 侧唯一的 isIdle() 使用者是无关的 settings-reload 路径（acpAgent.ts:11579）。本 PR 自己的测试固定的是较弱的谓词（isIdle=false 且仅有非回合工作 → branch/rewind 继续执行）。— 失败场景：维护者未来做竞态分析时会按文档中的守卫推理，误以为后台 agent 工作持有活动期间 branch/rewind 会快速失败，而实际变更会继续执行；反之，把代码"修正"为与文档一致，会在后台任务挂起时拒绝 branch——而继续执行本是安全的且已有测试固定。§8.3 中"prompt 在整个区间持有 Agent 历史变更锁"的说法同样与 §12.2 和实现矛盾。— 建议：把 §12.2 改为 sourceSession.isTurnIdle()（可注明工作持有刻意不阻塞分支），并修正 §8.3 的相应表述。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const branchBeforeText = JSON.stringify(branchBeforeContinue.events); | ||
| expect(branchBeforeText).toContain('historical branch turn one'); |
There was a problem hiding this comment.
[Suggestion] The only E2E test of the historical-branch feature pins user-prompt text exclusively; the anchor turn's assistant response is never asserted in the branched transcript. The fake model answers every turn with the constant 'fake response complete' (line 248), and that string is asserted nowhere in the test. — Failure scenario: an anchor-boundary regression that keeps the user record but drops the anchor's assistant record (or a replay/load-layer regression corrupting assistant content) passes every assertion: toContain('historical branch turn one') matches the user prompt, and the not.toContain(two/three) checks are unaffected — the regression this test exists to catch ships green. — Suggested fix: also assert expect(branchBeforeText).toContain('fake response complete') so the anchor turn's assistant record is pinned.
中文说明
历史分支功能的唯一 E2E 测试只固定了用户 prompt 文本；分支后 transcript 中从未断言锚点回合的 assistant 响应。假模型每个回合都回复常量 'fake response complete'（第 248 行），该字符串在测试中没有任何断言。— 失败场景：保留 user 记录但丢失锚点 assistant 记录的边界回归（或破坏 assistant 内容的回放/加载层回归）能通过所有断言：toContain('historical branch turn one') 匹配的是 user prompt，not.toContain(two/three) 也不受影响——本测试要捕获的回归会绿着上线。— 建议：补充 expect(branchBeforeText).toContain('fake response complete')，固定锚点回合的 assistant 记录。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(JSON.stringify(branchAfterContinue.events)).toContain( | ||
| 'continue the historical branch', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] After continuing the branch, the test never re-asserts the branch's own pre-branch prefix — it re-asserts all three turns on the source side but only the new prompt on the branch side. This is the only end-to-end coverage of branch continuation (the web-shell smoke E2E uses a mock daemon). — Failure scenario: a regression that truncates or re-initializes the fork's transcript on load/continue passes green: the new prompt is still recorded, and the source-side assertions are unaffected — the forked conversation silently loses everything before the branch point. — Suggested fix: in the branchAfterContinue block also assert .toContain('historical branch turn one') and .not.toContain('historical branch turn two').
中文说明
在分支上继续对话后，测试没有重新断言分支自身的前缀——源会话侧重新断言了全部三个回合，但分支侧只检查新 prompt。这是分支续写唯一的端到端覆盖（web-shell smoke E2E 使用 mock daemon）。— 失败场景：load/continue 时截断或重置 fork transcript 的回归会绿着通过：新 prompt 仍被记录，源侧断言不受影响——fork 出的会话悄悄丢失分支点之前的全部内容。— 建议：在 branchAfterContinue 断言块中补充 .toContain('historical branch turn one') 与 .not.toContain('historical branch turn two')。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(handle.agent.extMethodCalls).toContainEqual( | ||
| expect.objectContaining({ | ||
| method: SERVE_CONTROL_EXT_METHODS.sessionBranch, | ||
| params: expect.objectContaining({ | ||
| atRecordId: '11111111-1111-4111-8111-111111111111', | ||
| }), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The new mutation tests pin outgoing branch params (atRecordId here, sessionId in the overlap test) but no test anywhere asserts the outgoing rewind params — the bridge's forwarding of promptId/rewindFiles in rewindSession (bridge.ts:10136-10144) is unpinned. All sessionRewind extMethod references in this file stub canned responses that never inspect params; integration-tests contain no rewind E2E. — Failure scenario: a refactor dropping or renaming promptId: req.promptId ships green; the agent handler resolves the rewind target from params['promptId'] or params['targetTurnIndex'], so every production rewind then fails with invalidParams or resolves the wrong turn — visible only to users. — Suggested fix: in the serialization test (which already runs a rewind leg), add the symmetric assertion on the outgoing sessionRewind params.
中文说明
新的变更测试固定了 branch 出站参数（此处 atRecordId，overlap 测试中 sessionId），但没有任何测试断言 rewind 出站参数——bridge 在 rewindSession 中对 promptId/rewindFiles 的转发（bridge.ts:10136-10144）未被固定。本文件所有 sessionRewind extMethod 引用都使用不检查参数的固定响应；integration-tests 中也没有 rewind E2E。— 失败场景：删除或重命名 promptId: req.promptId 的重构会绿着上线；agent 处理器从 params['promptId'] 或 params['targetTurnIndex'] 解析 rewind 目标，生产环境中所有 rewind 都会以 invalidParams 失败或解析到错误回合——只有用户可见。— 建议：在已执行 rewind 的序列化测试中，对 sessionRewind 出站参数补充对称断言。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| it.each([false, true])( | ||
| 'releases side-task admission when restore failure is %s', | ||
| async (restoreFails) => { |
There was a problem hiding this comment.
[Suggestion] The diff converts the fork-RPC-failure admission-release test into 'does not acquire live admission when persisted branch creation fails', and the side-task path that now owns admission has release tests only for restore success/failure — none for the case where the sessionSideTask extMethod itself rejects. Grep of all seven sessionSideTask sites confirms none throws from that method while observing admission. — Failure scenario: release on fork failure works today solely via the shared finally in branchSession (~line 8224). A refactor relocating releaseAdmissionOnce() — next to the successful restoreSession call or into only the restore-error catch — leaves every existing test green, but each agent-rejected side-task fork dispatch then leaks one fresh-session admission slot until daemon restart. — Suggested fix: add a test where extMethodImpl throws for sessionSideTask; assert createSideTaskSession rejects, admission was acquired once (operation 'branch'), and its release() was called once.
中文说明
本 diff 把 fork-RPC 失败时的准入释放测试改为"持久化分支创建失败时不获取实时准入"，而现在持有准入的 side-task 路径只有 restore 成功/失败的释放测试——没有 sessionSideTask extMethod 本身被拒绝的用例。对全部 7 处 sessionSideTask 引用的 grep 确认没有一处在观察准入期间从该方法抛出。— 失败场景：目前 fork 失败时的释放仅靠 branchSession 的共享 finally（约 8224 行）。若把 releaseAdmissionOnce() 挪到成功的 restoreSession 调用旁、或只放进 restore-error catch，现有测试仍全绿，但每次被 agent 拒绝的 side-task fork 都会泄漏一个新会话准入槽，直到 daemon 重启。— 建议：新增测试：extMethodImpl 在 sessionSideTask 上抛出；断言 createSideTaskSession 拒绝、准入只获取一次（operation 为 'branch'）且其 release() 被调用一次。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| describe('isStaleBranchPointError', () => { | ||
| it('accepts the daemon stale-branch contract', () => { |
There was a problem hiding this comment.
[Suggestion] The stale-branch recovery contract is only tested with hand-fabricated errors; no test drives DaemonClient.branchSession through a non-2xx response and asserts a DaemonHttpError with parsed body emerges. The branchSession describe contains only a 201-success test and a deadline-abort test. Both recovery consumers key off the predicate from a real rejection (webui actions.ts:1644, web-shell App.tsx:7545), and the web-shell test mocks the predicate itself away (App.test.tsx:505). Probe-verified: replacing the failOnError throw with a plain Error keeps the entire SDK suite green (32 files / 1525 tests). — Failure scenario: a future change altering branchSession's error path (replacing throw await this.failOnError(res, ...) with a plain throw, or rewrapping without the parsed body) makes isStaleBranchPointError return false for every real 409 branch_point_invalid from the daemon: users clicking Branch on a stale checkpoint get a raw error instead of the transcript re-sync, with every existing test green. — Suggested fix: add one test to the branchSession describe: recordingFetch(() => jsonResponse(409, { code: 'branch_point_invalid' })); await expect(client.branchSession('source-1')).rejects.toSatisfy(isStaleBranchPointError).
中文说明
过期分支恢复契约只用手造错误测试；没有测试驱动 DaemonClient.branchSession 经历非 2xx 响应并断言产生带解析 body 的 DaemonHttpError。branchSession describe 只有 201 成功测试与 deadline 中止测试。两个恢复消费者都依赖真实拒绝上的谓词（webui actions.ts:1644、web-shell App.tsx:7545），而 web-shell 测试把谓词本身 mock 掉了（App.test.tsx:505）。探针验证：把 failOnError 抛出替换为普通 Error 后整个 SDK 套件全绿（32 个文件 / 1525 个测试）。— 失败场景：未来改变 branchSession 错误路径的修改（把 throw await this.failOnError(res, ...) 换成普通抛出，或重新包装但丢失解析后的 body）会使 isStaleBranchPointError 对 daemon 返回的每个真实 409 branch_point_invalid 返回 false：用户在过期 checkpoint 上点击 Branch 时得到原始错误而不是 transcript 重新同步，且所有现有测试保持绿色。— 建议：在 branchSession describe 中新增一个测试：recordingFetch(() => jsonResponse(409, { code: 'branch_point_invalid' }))；await expect(client.branchSession('source-1')).rejects.toSatisfy(isStaleBranchPointError)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(onToast).not.toHaveBeenCalledWith( | ||
| 'error', | ||
| 'This response is no longer on the active history path, and the transcript could not be refreshed. Please retry.', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The stale-recovery failure arm (branch.staleRefreshFailed — recovery reload rejects with a non-abort error while still on the source session; implementation ternary t(refreshed ? 'branch.stale' : 'branch.staleRefreshFailed') at App.tsx:7569) has no positive test; the string only appears in this negative assertion inside the superseded-reload scenario, which returns before any toast regardless of refreshed's value. Probe-verified: setting refreshed = true unconditionally keeps all 14 branch/stale/checkpoint App tests green, while a temp test with a failing non-abort reload flips. — Failure scenario: a future edit setting refreshed = true unconditionally in the catch (App.tsx:7561-7563) leaves the recovery reload genuinely failing (daemon unreachable mid-recovery) while the user is told "The transcript has been refreshed.", trusts the still-stale transcript view, and never gets the "Please retry" prompt. — Suggested fix: add a test where branchSession rejects with the 409 branch_point_invalid error, reloadSession rejects with a non-abort error (e.g. new Error('load failed')), the session is not switched, and assert the failure toast fires.
中文说明
过期恢复失败分支（branch.staleRefreshFailed——恢复 reload 在仍处于源会话时以非 abort 错误拒绝；实现为 App.tsx:7569 的三元 t(refreshed ? 'branch.stale' : 'branch.staleRefreshFailed')）没有正向测试；该字符串只出现在被取代 reload 场景中的这个反向断言里，而该场景在任何 toast 之前就返回，refreshed 取值无关紧要。探针验证：无条件设置 refreshed = true 后 14 个 branch/stale/checkpoint App 测试全绿，而带非 abort 失败 reload 的临时测试会翻转。— 失败场景：未来在 catch 中无条件设置 refreshed = true 的修改（App.tsx:7561-7563）会让恢复 reload 真实失败（恢复过程中 daemon 不可达），用户却看到"The transcript has been refreshed."，继续信任仍旧过期的 transcript 视图，且永远得不到"Please retry"提示。— 建议：新增测试：branchSession 以 409 branch_point_invalid 错误拒绝，reloadSession 以非 abort 错误（如 new Error('load failed')）拒绝，会话未切换，断言失败 toast 触发。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| ...(textBlock.branchRecordId | ||
| ? { branchRecordId: textBlock.branchRecordId } | ||
| : {}), |
There was a problem hiding this comment.
[Suggestion] The append path — the path that merges consecutive assistant text blocks into one rendered message — has no test for branchRecordId propagation. All three new tests in transcriptToMessages.test.ts exercise single-block turns (push path) or the insight path. Probe-verified: deleting the conditional spread on this append branch drops the Branch anchor for turns delivered as multiple consecutive assistant blocks while all 124 existing tests pass. — Failure scenario: if this append branch's conditional spread is later deleted or broken, turns whose final checkpointed text block is merged into a preceding assistant block silently lose their Branch anchor — showAssistantBranch (MessageList.tsx:4449-4453) becomes false and the user can no longer branch from that turn — while the entire new test block still passes. The replay path makes this realistic: history-replay-page.ts documents that a checkpointed record can replay as several chunks. — Suggested fix: add a case to 'Assistant branch anchors' feeding two consecutive assistant text blocks where only the second carries branchRecordId, asserting the single merged message (content: 'first part' + 'second part') holds the checkpoint.
中文说明
append 路径——把连续 assistant 文本块合并为一条渲染消息的路径——没有 branchRecordId 传播的测试。transcriptToMessages.test.ts 的三个新测试全部覆盖单块回合（push 路径）或 insight 路径。探针验证：删除该 append 分支的条件展开后，以多个连续 assistant 块交付的回合会丢失 Branch 锚点，而现有 124 个测试全部通过。— 失败场景：若该 append 分支的条件展开日后被删除或破坏，最终带 checkpoint 的文本块被合并进前一个 assistant 块的回合会悄悄丢失 Branch 锚点——showAssistantBranch（MessageList.tsx:4449-4453）变为 false，用户无法再从该回合分支——而整个新测试块仍然通过。回放路径使该场景真实存在：history-replay-page.ts 记录了带 checkpoint 的记录可能以多个 chunk 回放。— 建议：在 'Assistant branch anchors' 中新增用例：输入两个连续 assistant 文本块且只有第二个携带 branchRecordId，断言合并后的单条消息（content: 'first part' + 'second part'）持有该 checkpoint。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| click(c.querySelector('[data-testid="branch-anchored"]')!); | ||
| expect(onBranchSession).toHaveBeenCalledWith('checkpoint-1'); |
There was a problem hiding this comment.
[Suggestion] The real MessageItem boundBranchSession wiring (() => onBranchSession(branchRecordId), MessageItem.tsx:69-75 — the only code that delivers the checkpoint id from a message to the host) is pinned by no unit test: this file mocks MessageItem and the mock re-implements the binding; MessageItem.dom.test.tsx mocks AssistantMessage with an assistant-branch button no test ever clicks (and whose onClick drops the argument); AssistantMessage.test.tsx injects handlers directly; no test render of <MessageItem passes onBranchSession/branchRecordId. — Failure scenario: refactoring MessageItem.tsx's binding to pass onBranchSession through unbound (or with undefined) leaves every unit suite green; in production, clicking Branch on a historical turn would send no atRecordId and silently fork from the newest record instead of the anchored checkpoint. Today only the smoke E2E's request-body assertion (toEqual({ atRecordId: branchRecordId })) would catch it. — Suggested fix: add a unit test exercising the real binding — e.g. in MessageItem.dom.test.tsx, render the real MessageItem with a branchRecordId and assert the mocked AssistantMessage receives a handler that, when invoked via the already-scaffolded assistant-branch button, calls the host handler with the record id.
中文说明
真实的 MessageItem boundBranchSession 接线（() => onBranchSession(branchRecordId)，MessageItem.tsx:69-75——唯一把 checkpoint id 从消息传递给宿主的代码）没有任何单元测试固定：本文件 mock 了 MessageItem 且 mock 自己重新实现了该绑定；MessageItem.dom.test.tsx mock 的 AssistantMessage 带有一个没有任何测试点击过的 assistant-branch 按钮（其 onClick 丢弃参数）；AssistantMessage.test.tsx 直接注入处理器；没有任何渲染 <MessageItem 的测试传入 onBranchSession/branchRecordId。— 失败场景：把 MessageItem.tsx 的绑定重构为直接透传 onBranchSession（或传 undefined）会让所有单元测试套件保持绿色；生产环境中在历史回合上点击 Branch 将不发送 atRecordId，悄悄从最新记录而不是锚定的 checkpoint fork。目前只有 smoke E2E 的请求体断言（toEqual({ atRecordId: branchRecordId })）能捕获。— 建议：新增单元测试覆盖真实绑定——例如在 MessageItem.dom.test.tsx 中用 branchRecordId 渲染真实 MessageItem，断言 mock 的 AssistantMessage 收到的处理器在通过已搭建的 assistant-branch 按钮调用时，以记录 id 调用宿主处理器。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const switchStarted = | ||
| isCurrentLogicalSession(session) && | ||
| pendingSessionLoadIdRef.current === loadGeneration; |
There was a problem hiding this comment.
[Suggestion] The loadGeneration conjunct is only ever the deciding factor for a same-session reload started during an in-flight branch, and no test exercises that path. The new 'does not let a late branch result supersede newer navigation' test drives the newer navigation via loadSession('session-b') — a cross-session load; startLegacySessionSwitch clears sessionRef.current for a cross-session target (actions.ts:419), so isCurrentLogicalSession(session) is already false and the first conjunct forces switchStarted:false regardless of the generation check. reloadSession skips requireStableSession() for non-memory replay (actions.ts:877), permitting a concurrent same-session reload. Probe-verified: dropping the conjunct ships green (actions 73 tests + provider branch tests), while a same-session reload probe flips from switchStarted:false to true. — Failure scenario: dropping && pendingSessionLoadIdRef.current === loadGeneration means a stale branch result calls startSessionSwitch(branchId, 'load'), whose startPendingSessionLoad rejects the in-flight reload with "Session load superseded by a newer request" — aborting the newer reload and switching the user to the branch. — Suggested fix: add a case where a reloadSession() (same sessionId) starts after branchSession() is issued but before the branch deferred resolves; assert switchStarted is false and startSessionSwitch/beginCrossSessionTransition is not invoked — making the generation comparison, not isCurrentLogicalSession, the load-bearing assertion.
中文说明
loadGeneration 合取项只有在 branch 进行中启动的同会话 reload 场景才是决定因素，而没有任何测试覆盖该路径。新的 'does not let a late branch result supersede newer navigation' 测试通过 loadSession('session-b') 驱动新导航——这是跨会话加载；startLegacySessionSwitch 对跨会话目标会清除 sessionRef.current（actions.ts:419），因此 isCurrentLogicalSession(session) 已为 false，无论 generation 检查如何，第一个合取项都会强制 switchStarted:false。reloadSession 对非 memory 回放跳过 requireStableSession()（actions.ts:877），允许并发的同会话 reload。探针验证：删除该合取项后测试全绿（73 个 actions 测试 + provider branch 测试），而同会话 reload 探针会从 switchStarted:false 翻转为 true。— 失败场景：删除 && pendingSessionLoadIdRef.current === loadGeneration 后，过期的 branch 结果会调用 startSessionSwitch(branchId, 'load')，其 startPendingSessionLoad 以"Session load superseded by a newer request"拒绝进行中的 reload——中止较新的 reload 并把用户切换到分支。— 建议：新增用例：在 branchSession() 发出之后、branch 的 deferred resolve 之前启动 reloadSession()（同一 sessionId）；断言 switchStarted 为 false 且 startSessionSwitch/beginCrossSessionTransition 未被调用——让 generation 比较而非 isCurrentLogicalSession 成为承重断言。
— qwen3.8-max via Qwen Code /review (v0.21.10)
…conversation # Conflicts: # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/acp-integration/session/Session.test.ts # packages/cli/src/acp-integration/session/Session.ts # packages/core/src/services/chatRecordingService.ts
|
@qwen-code-ci-bot Please take a look again |
|
Thanks for the update. The current head still has content conflicts with the latest
Please sync the branch with the latest |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
I found one blocking compatibility regression at the reviewed head 7e1fdab19948: the existing v1 branchSession contract now returns a persisted-only identity instead of a restored/attached session, even when the new optional historical checkpoint is omitted. The inline Critical explains the source and runtime break and compatible remediation options. I also rechecked every previously unresolved Critical against the current head; those earlier defects are fixed.
…conversation # Conflicts: # packages/sdk-typescript/scripts/build.js # packages/web-shell/client/e2e/utils/mockDaemon.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; OS-sensitive paths (fsync win32 branch, O_NOFOLLOW/symlink handling, backup path validation) verified on Linux only.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — I finished within budget and did not leave a check unfinished.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget., and 17 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; OS-sensitive paths (fsync win32 branch, O_NOFOLLOW/symlink handling, backup path validation) verified on Linux only。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — I finished within budget and did not leave a check unfinished.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.，另有 17 条。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Keep existing latest-state branch callers source- and wire-compatible while retaining the persisted-only behavior for historical checkpoint branches. - Restore no-anchor branches before returning their live client identity - Add a separate typed result for persisted historical branch requests - Clean up restored attachments on stale navigation and disconnect races - Cover immediate continuation and historical persistence independently
Prevent branch creation and automatic Goal turns from racing session teardown or interactive history mutations. - Reject branch admission while a conditional close is authorized - Serialize Goal continuations behind the history mutation gate - Limit branch checkpoints to interactive prompts - Add regressions for close and Goal scheduling races
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; OS-sensitive paths (fsync win32 branch, O_NOFOLLOW handling) verified on Linux only.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": (none — all checks I opened were completed within budget); "You are review agent reverse-audit — Reverse audit agent…": none — the full chunk was read un-truncated in one pass and all follow-up source verifications completed within budget.; chunk 6: none — all checks I intended completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": I did not trace where resolveCompletion() fires relative to the previous turn's checkpoint transaction (cursor-acquisition-vs-active-fence overlap in cli/Sess…; "本 PR（QwenLM/qwen-code #8817）实现从任意已完成 Assistant…": full npm run typecheck /build across packages not run (would independently confirm no residual signature mismatches; static walk found none)., and 9 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; OS-sensitive paths (fsync win32 branch, O_NOFOLLOW handling) verified on Linux only。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：(none — all checks I opened were completed within budget)；"You are review agent reverse-audit — Reverse audit agent…"：none — the full chunk was read un-truncated in one pass and all follow-up source verifications completed within budget.；chunk 6：none — all checks I intended completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：I did not trace where resolveCompletion() fires relative to the previous turn's checkpoint transaction (cursor-acquisition-vs-active-fence overlap in cli/Sess…；"本 PR（QwenLM/qwen-code #8817）实现从任意已完成 Assistant…"：full npm run typecheck /build across packages not run (would independently confirm no residual signature mismatches; static walk found none).，另有 9 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| async branchSession( | ||
| sessionId: string, | ||
| req: DaemonBranchSessionRequest, | ||
| clientId?: string, | ||
| ): Promise<DaemonBranchSessionResult>; |
There was a problem hiding this comment.
[Suggestion] The third branchSession overload (req: DaemonBranchSessionRequest) is unreachable: because HistoricalBranchSessionRequest extends BranchSessionRequest, a union-typed request is assignable to the second overload's req?: BranchSessionRequest, so TypeScript always resolves overload 2 and the call is statically typed Promise<DaemonBranchedSession>. — Failure scenario: a caller holding a DaemonBranchSessionRequest passes atRecordId → overload 2 matches → the result is statically DaemonBranchedSession (which declares the required state field), but the daemon answers a historical branch with the persisted-only shape (no state), so branch.state.models typechecks yet throws TypeError: Cannot read properties of undefined at runtime. No current caller passes the union, so nothing breaks today — but the overload exists precisely for such callers and can never be selected. The empty suggestion below removes the unreachable overload. — Concrete cost: latent runtime TypeError for the first union-typed caller; dead API surface in the public SDK.
| async branchSession( | |
| sessionId: string, | |
| req: DaemonBranchSessionRequest, | |
| clientId?: string, | |
| ): Promise<DaemonBranchSessionResult>; |
中文说明
第三个 branchSession 重载（req: DaemonBranchSessionRequest）不可达：由于 HistoricalBranchSessionRequest extends BranchSessionRequest，联合类型的请求可以赋值给第二个重载的 req?: BranchSessionRequest，TypeScript 总是解析到重载 2，调用被静态标注为 Promise<DaemonBranchedSession>。失败场景：持有 DaemonBranchSessionRequest 的调用方传入 atRecordId 时命中重载 2，返回类型被静态固定为 DaemonBranchedSession（声明了必填的 state 字段），而 daemon 对历史分支返回仅持久化的结构（无 state），于是 branch.state.models 能通过类型检查、却在运行时抛出 TypeError: Cannot read properties of undefined。当前没有调用方传入该联合类型（因此目前不会触发），但这个重载正是为这类调用方存在的，且永远不会被选中。下方的空 suggestion 代码块可一键删除该不可达重载。具体代价：为未来第一个使用联合类型的调用方埋下运行时 TypeError；公共 SDK 中存在死 API 面。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (!transcriptReloadSupported) { | ||
| pushToast('error', t('branch.staleUnsupported')); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] Of the branches of the new 409 stale-checkpoint recovery in branchCurrentSession, the capability-gated unsupported branch (!transcriptReloadSupported → toast branch.staleUnsupported, no reload) has no test; the reload/success, switched-away, and superseded-reload branches are all covered in App.test.tsx, but nothing references branch.staleUnsupported. — Failure scenario: against a daemon that does not advertise session_transcript_pagination, a stale checkpoint must toast branch.staleUnsupported and skip the reload; if the gate regresses (check removed or capability derivation changed), the App calls reloadSession on a non-paginable session and shows the wrong recovery toast, and no test turns red. Suggested fix: add an App test with mockConnection.capabilities.features lacking session_transcript_pagination, reject branchSession with a 409 branch_point_invalid DaemonHttpError, and assert the toast text and that reloadSession is not called.
中文说明
branchCurrentSession 中新的 409 过期 checkpoint 恢复逻辑的各分支里，能力开关控制的不支持分支（!transcriptReloadSupported → 提示 branch.staleUnsupported、不执行重载）没有任何测试；重载成功、已切换会话、重载被取代等分支在 App.test.tsx 中均有覆盖，但没有任何测试引用 branch.staleUnsupported。失败场景：当 daemon 未声明 session_transcript_pagination 能力时，过期 checkpoint 应提示 branch.staleUnsupported 且跳过重载；若该判断退化（判断被移除或能力推导方式改变），App 会对不可分页的会话调用 reloadSession 并显示错误的恢复提示，且没有测试会变红。建议修复：新增一个 App 测试，令 mockConnection.capabilities.features 缺少 session_transcript_pagination，让 branchSession 以 409 branch_point_invalid 的 DaemonHttpError 失败，断言提示文案且 reloadSession 未被调用。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (!res.writable) { | ||
| void releaseLiveBranch(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The branch route's !res.writable cleanup path (releaseLiveBranch: detachClient for attached results, killSession({ requireZeroAttaches: true }) for unattached) is pinned by no test; the new server.test.ts suite covers the happy path and the generation-close contract but not response-delivery failure after commit, while the sibling ACP path changed the same way (dispatch.ts conn.destroyed cleanup) does get it.each detach/kill tests. — Failure scenario: if a future refactor inverts or drops the detach-vs-kill selection on response-delivery failure after commit, no test in server.test.ts turns red. Note: the repo carries an explicit precedent (server.test.ts:12280) skipping route-level disconnect tests because supertest + Node http close-event timing is flaky in CI — if that convention is the reason here too, a one-line pointer comment at this branch would keep the gap deliberate and documented.
中文说明
branch 路由的 !res.writable 清理路径（releaseLiveBranch：attached 结果走 detachClient，未 attached 走 killSession({ requireZeroAttaches: true })）没有任何测试固定；新的 server.test.ts 套件覆盖了正常路径与 generation 关闭契约，但没有覆盖提交后响应投递失败的场景，而以相同方式修改的 ACP 兄弟路径（dispatch.ts 的 conn.destroyed 清理）却有 it.each detach/kill 测试。失败场景：若未来重构在“提交后响应投递失败”场景下弄反或移除了 detach/kill 的选择，server.test.ts 中没有任何测试会变红。注意：仓库中有明确先例（server.test.ts:12280）因 supertest + Node http close 事件时序在 CI 中不稳定而跳过路由级断连测试——如果此处缺口也是出于该约定，建议在此分支加一行注释指明，让这一取舍保持有意且可追溯。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…conversation # Conflicts: # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/acp-integration/session/history-replay-page.test.ts # packages/core/src/services/chatRecordingService.ts # packages/core/src/services/session-transcript-reader.ts # packages/core/src/services/sessionService.ts
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review: my blocking Critical is fixed — clearing my CHANGES_REQUESTED
I verified at the current head 134925e22e. My earlier block was a single Critical — the v1 branchSession contract regression — and commit 00ce3166 ("preserve v1 branch session contract") resolves both halves of it. There are no unresolved Criticals on the PR at this head (the 83 open threads are all [Suggestion]).
The Critical, checked against the code as it now stands → fixed by this diff
My finding was that session_branch stayed at v1 but returned persisted-only {sessionId, displayName, forkedFrom} and left forks unattached even when atRecordId was omitted — so an existing v1 client reading clientId and immediately prompting would break (source-level TS2339 + runtime "fork absent from byId"). The fix addresses both:
- Type/source:
DaemonBranchedSessionagainextends DaemonRestoredSession, DaemonPersistedBranchedSession(andBridgeBranchedSession extends BridgeRestoredSession), soclientIdis back. Overloads route a no-anchorbranchSession(...)toDaemonBranchedSessionand anatRecordIdrequest toDaemonPersistedBranchedSession, withBranchSessionRequestno longer carryingatRecordId. - Runtime/wire: in
bridge.ts,const restoreBranch = isSideTask || req.atRecordId === undefinednow gates the reserve-and-restore path (wasisSideTask) and the early persisted-only return (if (!restoreBranch)). So a latest-state branch is restored/attached before returning a live session with a validclientId, while only historical (atRecordId) branches stay persisted-only.dispatch.tsdrops its separateloadSessionand the route adds detach-vs-kill cleanup on stale navigation / disconnect.
Witness (executed at head 134925e22e, scratch tree of the PR head)
sdk-typescripttsc --noEmit: clean (0 errors) — the TS2339 onbranched.clientIdis gone; the overloads compile.packages/sdk-typescriptDaemonClient.test.ts: 334/334 (includes the fix's new "immediate continuation vs historical persistence" coverage).packages/acp-bridgebridge.test.ts: 612/612 (branch/restore contract).packages/webuiactions.test.ts: 76/76 (branch action switch/skip).packages/corebranch-points+chatRecordingService+session-transcript-reader: 220/220.packages/clitransport.test.ts+server.test.ts: 1240/1242. The two failures (GET /capabilities"drops cached Voice capability", "advertises browser automation MCP…") are not from this PR — this PR changes zero lines ofserver.ts, both tests exist verbatim at the merge base, and they depend on a Voice runtime / CDP browser adapter absent in my sandbox. The historical-branch atomic flow itself is exercised by the real-daemon E2E, which is green in CI.
Scope of this approval (so it isn't read as more than it is)
This is a large core-surface feature (transcript topology, session concurrency, replay, transport, persistence, UI). I re-derived that my specific blocking finding is resolved and confirmed no Critical remains at the current head across the surfaces I ran above; I did not exhaustively re-audit every concurrency/atomicity path in all 62 files. The broad sign-off on those modules belongs with the maintainers who own packages/core/src/services/** and the daemon routes — this approval clears the contract-regression block I raised, it doesn't substitute for that.
Non-blocking Suggestions worth the author's attention (not gating)
- Triplicated branch-point UUID validation. The same regex is independently redeclared in three packages —
CHAT_RECORD_UUID_RE(acp-bridge/src/bridge.ts:1858), inlinerecordUuidPattern(sdk-typescript/.../DaemonClient.ts:6504), andRECORD_UUID_PATTERN(webui/.../DaemonSessionProvider.tsx:516). Three copies of a validation rule that must agree on what a valid checkpoint id is tend to drift; a single shared constant would be safer (this is the bot's R1-17). - The bot's remaining
[Suggestion]set (uncapped in-memory active-chain mirror R1-8, doc drift on the 409 body and the removedsession_branchedevent, several missing-coverage pins) is worth a pass, but none of it blocks per the repo's "a missing test for changed behavior is a Suggestion" rule.
Net: the reason I requested changes is gone, so I'm removing my block.
|
Qwen Code review did not complete successfully. Run review failed. See workflow logs for details. A transient error is retried automatically; if you are seeing this, retry with |
…conversation # Conflicts: # packages/acp-bridge/src/bridge.ts # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/serve/routes/session.ts
All the critical comments have been resolved
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-reviewed head df26a9af (delta since a30d6196).
Delta composition: 5 merges of upstream/main (resolving the content conflicts @LaZzyMan flagged) plus two fork-specific commits:
00ce316694— restores the v1 branch-session contract: abranchSessioncall withoutatRecordIdnow restores the latest-state branch inside the bridge and returns the full restored session (DaemonBranchedSession extends DaemonRestoredSessionagain); calls withatRecordIdstay persisted-only historical branches. Dispatch/route/SDK/webui call sites and tests updated accordingly.a3b7c27728— fixes R5-1: bothbranchSessionadmission gates now useisClosingOrAuthorizingClose(entry)(pre-queue and post-dispatch recheck), with regression tests for queued-branch-vs-close races. Also serializes the Goal continuation drain behindrunExclusiveAutomaticHistoryMutationand skips branch checkpoints for scheduled goal turns.
Invariants re-verified at head: three-layer admission gating with the full close predicate; session_busy → SessionBusyError → 409+Retry-After mapping; narrow isTurnIdle() fail-fast for branch/rewind; normalized session-id lookups; checkpoint transaction keeps the topology fence, flush, and "transcript changed" guard via appendRecordStrict; REST fork route releases a live branch when the response is aborted; web-shell keeps the pending-branch request dedupe. No regression to source-session immutability or lineage handling observed in the conflict resolutions.
Feedback status: doudouOUC's v1-contract Critical and qwen-code-ci-bot's R5-1 are both addressed; the R1 items were already addressed at 7e1fdab199.
CI at df26a9af: green — Qwen Code CI, SDK Java, Serve A/B, web-shell visuals, Live Host, and Security Checks all succeeded (the cancelled suites are the duplicate runs from the double push; only the bot's own review run is still in progress).
7 participants
What this PR does
Previously, session branching used the latest active session state and could not reliably target an earlier Assistant response. Treating a visible message as a branch point is unsafe because tool calls, cancellations, metadata records, transcript pagination, rewinds, and concurrent transcript mutations can make the displayed response different from the authoritative active history.
Durable checkpoints provide a single source of truth shared by recording, replay, UI presentation, and Core validation. This lets users explore an alternative direction from an earlier answer without modifying the original session or exposing a partially created branch.
Why it's needed
Previously, session branching used the latest active session state and could not reliably target an earlier Assistant response. Treating a visible message as a branch point is unsafe because tool calls, cancellations, metadata records, transcript pagination, rewinds, and concurrent transcript mutations can make the displayed response different from the authoritative active history.
Durable checkpoints provide a single source of truth shared by recording, replay, UI presentation, and Core validation. This lets users explore an alternative direction from an earlier answer without modifying the original session or exposing a partially created branch.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: Branching represented the latest active session state. Historical Assistant responses did not have an authoritative, durable branch point.
After: Eligible completed Assistant responses carry a recorded and replayable checkpoint. Selecting Branch creates a new session ending at the chosen turn, while stale checkpoints are rejected and trigger a transcript refresh.
Tested on
Environment (optional)
Risk & Scope
Linked Issues
Refs #8271
中文说明
Why it's needed
此前，会话分支只能使用源会话的最新活动状态，无法可靠地定位到更早的一条 Assistant 回复。直接把界面上可见的消息作为分支点并不安全，因为工具调用、取消、元数据记录、分页回放、会话回退和并发 transcript 写入都可能导致界面显示内容与权威活动历史不一致。
持久化 checkpoint 为录制、回放、界面展示和 Core 校验提供了统一的事实来源。用户因此可以从较早的回答探索另一条对话路径，同时不修改原始会话，也不会看到尚未完整创建的分支会话。
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before： 分支操作只能表示会话当前最新的活动状态。历史 Assistant 回复没有权威且持久化的分支点。
After： 符合条件的已完成 Assistant 回复带有可录制、可回放的 checkpoint。选择 Branch 后，新会话会准确截止于所选回合；过期 checkpoint 会被拒绝，并触发 transcript 刷新。
Tested on
Environment (optional)
Risk & Scope
Linked Issues
Refs #8271