feat(web-shell): support daemon session branching - #5613
Conversation
ef0fba1 to
b2e65be
Compare
b2e65be to
f6ef089
Compare
| 'Cannot fork while a response or tool call is in progress', | ||
| ); | ||
| } | ||
| return entry.promptQueue.then(async () => { |
There was a problem hiding this comment.
[Suggestion] launchSessionForkAgent chains onto entry.promptQueue.then(...) but never writes back entry.promptQueue = result.then(...), unlike sendPrompt (line 3065) and branchSession (line 3445) which both do entry.promptQueue = result.then(() => undefined, () => undefined).
This means a sendPrompt arriving while the fork's extMethod is in-flight won't serialize behind it — both can execute concurrently against the same agent channel. Today the fork's server-side handler is a quick fire-and-forget subagent launch so this has no practical impact, but if the fork ever gains state-mutating side effects or a longer-running RPC, the gap widens into a real race.
| return entry.promptQueue.then(async () => { | |
| const forkResult = entry.promptQueue.then(async () => { |
…and after the closing }); of the .then(), add:
entry.promptQueue = forkResult.then(() => undefined, () => undefined);
return forkResult;— qwen3.7-max via Qwen Code /review
| 'Branch session timed out', | ||
| ); | ||
| persistStableClientId(result.clientId, result.sessionId); | ||
| void startSessionSwitch(result.sessionId, 'load').catch( |
There was a problem hiding this comment.
[Suggestion] Two issues with this fire-and-forget startSessionSwitch:
-
Double error notice on timeout.
startSessionSwitch's internal timeout handler already callsdispatchActionError("Load session failed: Session load timed out"), marks the error with_alreadyDispatched: true, then rejects. This.catch()handler callsdispatchActionErroragain ("Branch session failed: Session load timed out") without checking_alreadyDispatched. The user sees two duplicate notices. -
Error paths untested. The new test only covers the happy path where
startSessionSwitchsucceeds. Neither the abort-error branch (silently swallowed) nor the non-abort-error branch (dispatches notice) is exercised.
For (1), guard the .catch callback:
| void startSessionSwitch(result.sessionId, 'load').catch( | |
| void startSessionSwitch(result.sessionId, 'load').catch( | |
| (switchError: unknown) => { | |
| if (isAbortError(switchError)) return; | |
| if ((switchError as Record<string, unknown>)?._alreadyDispatched) return; | |
| dispatchActionError( | |
| addNotice, | |
| 'Branch session failed', | |
| switchError, | |
| 'branch_session', | |
| ); | |
| }, | |
| ); |
— qwen3.7-max via Qwen Code /review
|
|
||
| const result = { | ||
| sessionId: entry.sessionId, | ||
| description: response.description ?? trimmed.slice(0, 60), |
There was a problem hiding this comment.
[Suggestion] The fallback description trimmed.slice(0, 60) doesn't collapse internal whitespace or append an ellipsis, unlike the agent-side collapseForkDirective(directive, 60) which does both. If this fallback ever triggers, the web-shell toast would show uncollapsed whitespace (e.g., "review \t this\nbranch") with a hard cutoff instead of "review this branch…".
Consider importing or replicating the collapse helper:
| description: response.description ?? trimmed.slice(0, 60), | |
| description: response.description ?? collapseForkDirective(trimmed, 60), |
— qwen3.7-max via Qwen Code /review
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical recheck on current head: the prior branch-client leak is resolved. The branch action now forwards the current client id, persists the returned branch client id before switching, and avoids orphaning an anonymous restored client. I did not find a new critical blocker in the daemon session branching path; remaining review notes are below this automation’s critical bar.
|
Thanks for the PR, @ytahdn! Template looks good ✓ — all required sections present including bilingual description, reviewer test plan, and risk assessment. On direction: this is well-aligned. Web Shell should have parity with the CLI for session management primitives ( On approach: the scope is large (1286 additions across 30 files) but it's a full-stack feature — bridge → SDK → Web Shell → webui provider — so most of the surface area is justified. The changes are layered well:
Two minor observations before code review:
CI is green across the board: Lint ✅, Tests on macOS/Ubuntu/Windows ✅, CodeQL ✅. Moving on to code review. 🔍 中文说明感谢贡献，@ytahdn！ 模板完整 ✓ — 所有必填章节齐全，包括双语说明、reviewer 测试计划和风险评估。 方向：高度对齐。Web Shell 应该在会话管理能力（ 方案：规模较大（30 个文件，+1286 行），但这是一个全栈特性——bridge → SDK → Web Shell → webui provider——大部分改动范围是合理的：
两个小观察：
CI 全绿：Lint ✅，Tests macOS/Ubuntu/Windows ✅，CodeQL ✅。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
🧪 Stage 2 — Code Review & Test ResultsReviewer: Qwen Code Triage Agent Code Review Summary
No critical blockers found. The code reuses existing patterns (restoreSession, SessionBusyError, reconstructHistory, background agent launching) rather than inventing new abstractions. Test Results (tmux session
|
| Job | Result |
|---|---|
| Test (macos-latest, Node 22.x) | ✅ pass (24m) |
| Test (ubuntu-latest, Node 22.x) | ✅ pass (18m) |
| Test (windows-latest, Node 22.x) | ✅ pass (29m) |
| Lint | ✅ pass (6m) |
| CodeQL | ✅ pass (8m) |
| Classify PR | ✅ pass |
All platforms green. No CI failures.
🇨🇳 中文摘要
代码审查摘要
| 方面 | 结论 |
|---|---|
| 架构 | ✅ 分层清晰：acp-bridge → sdk-typescript → web-shell → webui，每层只添加必要逻辑，无抽象泄漏 |
| 会话分支（core） | ✅ forkSession 正确调用 reconstructHistory() 仅复制 rewind 后的活跃分支，新测试已覆盖 |
| Fork agent（acp-bridge） | ✅ launchSessionForkAgent 采用双重 SessionBusyError 检查模式（队列前+队列内），包含超时包装和诊断日志 |
| Fork agent（CLI） | ✅ sessionForkAgent ext-method 分支正确处理模型检查、历史检查、工具获取、使用 FORK_SUBAGENT_TYPE 构建 agent、失败状态检测 |
| Transcript 回溯（SDK） | ✅ rewindTranscriptToUserTurn 遍历用户轮次并有回退逻辑；truncateTranscriptBeforeBlock 干净地重建索引 |
| Web-shell 集成 | ✅ /branch 和 /fork 命令分发 action、显示 toast 通知、触发后台任务刷新 |
| 国际化 | ✅ branch 和 fork 流程均有 EN/ZH 字符串（success, failed, started, notStarted） |
| 测试覆盖 | ✅ 每层均有新测试：server 路由 (4)、session service fork (7)、acpAgent fork (1)、daemonUI normalizer (241 全量)、transcript adapter (93 全量) |
未发现关键阻塞问题。 代码复用了现有模式（restoreSession, SessionBusyError, reconstructHistory, 后台 agent 启动），未引入新抽象。
测试结果汇总
所有 5 个测试套件全部通过：
- Fork endpoint: 4 passed
- Daemon UI normalizer: 241 passed
- Transcript adapter: 93 passed
- Session service fork: 7 passed
- ACP agent fork: 1 passed
CI 状态
macOS / Ubuntu / Windows 三平台全部通过，Lint 和 CodeQL 也通过，无 CI 失败。
🏁 Stage 3 — Reflection & VerdictVerdict: ✅ Approve ReflectionThis PR delivers a full-stack daemon session branching feature ( What works well:
Minor observations (non-blocking, author may consider):
SummaryWell-structured full-stack feature with clean reuse of existing patterns ( Recommended action: merge after maintainer sign-off. 🇨🇳 中文总结反思本 PR 实现了完整的守护进程会话分支功能（ 亮点：
非阻塞建议：
结论结构良好的全栈功能，干净地复用了现有模式，每层都有测试覆盖，CI 全绿，无阻塞问题。 建议操作：维护者确认后合并。 |
Verification — daemon session branching (
|
| Suite | Tests |
|---|---|
cli server.test.ts — POST /session/:id/fork |
4 ✅ |
acp-bridge bridge.test.ts |
296 ✅ |
cli acpAgent.test.ts |
128 ✅ |
core sessionService.test.ts (incl. forks only the active branch after rewind) |
65 ✅ |
sdk daemonUi.test.ts |
241 ✅ |
web-shell transcriptToMessages.test.ts |
93 ✅ |
webui DaemonSessionProvider.test.tsx |
78 ✅ |
Real daemon E2E (booted qwen serve + mock LLM)
POST /session/:id/fork
- ✅
202{sessionId, description:"review the current code", launched:true}. - ✅ The background agent really launched:
GET /session/:id/tasksshowskind:agent,label:"general-purpose: review the current code",isBackgrounded:true,status:completed,prompt:"review the current code"; the daemon log showslaunchSessionForkAgent requested → completed launched=true → 202(the new bridge diagnostics); the mock LLM received the subagent's calls. - ✅
400 missing_directivefor missing / whitespace-only directive. - ✅
400 invalid_client_idwhenX-Qwen-Client-Idisn't the session's registered client.
POST /session/:id/branch
- ✅
201with a new sessionId +forkedFrom:{sessionId, displayName}. - ✅ Copy correctness (parsed the branched JSONL): all 10 records re-stamped with the new sessionId, 0 records leak the old top-level sessionId, 8 carry the
forkedFromaudit referencing the source, theparentUuidchain is linear, and a unique conversation marker was copied into the branch. - ✅ Rewind safety (this revision's fix — copy only the active branch): the dedicated unit test
forks only the active branch after rewindpasses — a rewind record that abandons later turns is excluded from the copy, so a fork can't resurrect a rewound branch.
Evidence (key)
/fork — 202 + a real background task:
POST /session/<id>/fork (X-Qwen-Client-Id: <registered>) {"directive":"review the current code"}
-> HTTP 202 {"sessionId":"…","description":"review the current code","launched":true}
GET /session/<id>/tasks ->
{ kind:"agent", label:"general-purpose: review the current code",
isBackgrounded:true, status:"completed", prompt:"review the current code" }
daemon.log: launchSessionForkAgent requested → completed launched=true → status=202
/branch — 201 + correct copy:
-> HTTP 201 forkedFrom={"sessionId":"0bf79fc9…","displayName":"0bf79fc9"}
branched JSONL: top-level sessionId==NEW 10/10 ; ==OLD 0 ; forkedFrom→OLD 8 ; parentUuid linear: true ; marker copied: true
/fork error paths:
no directive -> 400 {code:"missing_directive"}
" " directive -> 400 {code:"missing_directive"}
unregistered client-> 400 {code:"invalid_client_id"}
Notes
- Verified on Linux (the PR marks macOS ✅, Linux/Windows
⚠️ ). The daemon ran against a mock OpenAI endpoint; the/forkbackground subagent genuinely executed against it. - The truncated parent-history fork note is added to the in-memory client history (covered by
acpAgent.test.ts); it isn't flushed to the on-disk chats JSONL at read time — expected persistence timing, not an issue.
No issues found end-to-end — /fork launches a real background agent and /branch copies the active conversation into an independent session with the correct sessionId rewrite, forkedFrom audit, and rewind safety. LGTM. 🚀
中文版（合并参考）
验证 —— daemon 会话分支（/branch）+ 后台 fork（/fork）✅
我用两种方式验证：跑了所有受影响层的单测，并且启动了真实的 qwen serve daemon（对接一个 mock OpenAI 接口），端到端走通 POST /session/:id/fork 和 POST /session/:id/branch 这两条 HTTP 路径 —— 也就是 Reviewer Test Plan 里的 daemon-API 路径。基于 HEAD 37fcd607，在 Linux 上 npm ci 构建。
单测（Linux 全绿）
| 测试套件 | 用例数 |
|---|---|
cli server.test.ts —— POST /session/:id/fork |
4 ✅ |
acp-bridge bridge.test.ts |
296 ✅ |
cli acpAgent.test.ts |
128 ✅ |
core sessionService.test.ts（含 forks only the active branch after rewind） |
65 ✅ |
sdk daemonUi.test.ts |
241 ✅ |
web-shell transcriptToMessages.test.ts |
93 ✅ |
webui DaemonSessionProvider.test.tsx |
78 ✅ |
真实 daemon 端到端（启动 qwen serve + mock LLM）
POST /session/:id/fork
- ✅
202{sessionId, description:"review the current code", launched:true}。 - ✅ 后台 agent 确实被启动：
GET /session/:id/tasks显示kind:agent、label:"general-purpose: review the current code"、isBackgrounded:true、status:completed、prompt:"review the current code"；daemon 日志显示launchSessionForkAgent requested → completed launched=true → 202（新增的 bridge 诊断日志）；mock LLM 收到了子代理的调用。 - ✅ directive 缺失/全空白 →
400 missing_directive。 - ✅
X-Qwen-Client-Id不是该 session 注册的 client →400 invalid_client_id。
POST /session/:id/branch
- ✅
201，返回新 sessionId +forkedFrom:{sessionId, displayName}。 - ✅ 复制正确性（解析分支出来的 JSONL）：全部 10 条记录的顶层 sessionId 都改写为新 id，0 条泄漏旧的顶层 sessionId，8 条带有指向源会话的
forkedFrom审计字段，parentUuid链是线性的，且唯一对话标记被复制进了分支。 - ✅ rewind 安全性（本轮修复 —— 只复制 active branch）：专门的单测
forks only the active branch after rewind通过 —— 一条 rewind 记录所废弃的后续 turn 不会被复制，所以 fork 不会复活被 rewind 掉的分支。
关键证据
/fork —— 202 + 真实后台任务：
POST /session/<id>/fork (X-Qwen-Client-Id: <registered>) {"directive":"review the current code"}
-> HTTP 202 {"sessionId":"…","description":"review the current code","launched":true}
GET /session/<id>/tasks ->
{ kind:"agent", label:"general-purpose: review the current code",
isBackgrounded:true, status:"completed", prompt:"review the current code" }
daemon.log: launchSessionForkAgent requested → completed launched=true → status=202
/branch —— 201 + 正确复制：
-> HTTP 201 forkedFrom={"sessionId":"0bf79fc9…","displayName":"0bf79fc9"}
分支 JSONL：顶层 sessionId==新 10/10 ；==旧 0 ；forkedFrom→旧 8 ；parentUuid 线性: true ；标记已复制: true
/fork 错误路径：
缺 directive -> 400 {code:"missing_directive"}
" " directive -> 400 {code:"missing_directive"}
未注册的 client -> 400 {code:"invalid_client_id"}
说明
- 在 Linux 上验证（PR 标注 macOS ✅、Linux/Windows
⚠️ ）。daemon 对接 mock OpenAI 接口运行；/fork的后台子代理是真实对接它执行的。 - 父会话里那条截断后的 fork 备注是写进内存中的 client history（由
acpAgent.test.ts覆盖），读取时还没落盘到 chats JSONL —— 这是正常的持久化时序，不是问题。
端到端未发现问题 —— /fork 会启动真实后台 agent，/branch 会把 active 对话复制成一个独立会话，且 sessionId 改写、forkedFrom 审计、rewind 安全性都正确。LGTM 🚀
4 participants
What this PR does
This PR adds daemon-backed session branching support for Web Shell. It exposes session branch and background fork operations through the daemon bridge, TypeScript daemon client, and Web Shell command handling so
/branchcan copy the current conversation into a new session and/forkcan launch a background agent from the current conversation.It also teaches the daemon UI normalizer and transcript adapter how to represent branch/fork-related notifications, background-task messages, and rewind events consistently in Web Shell. The Web Shell command list and localized copy now distinguish
/branchas copying the current conversation and/forkas starting a background agent.The review fixes in this revision improve
/forkerror handling and follow-up behavior: background launch failures are not double-reported,launched: falseno longer shows a success toast, the parent session only records a short truncated fork note instead of the full directive, branch copy text no longer says "fork", bridge-side diagnostics make fork launch failures easier to inspect, and out-of-range daemon rewind events now fall back to the last user turn instead of silently doing nothing.Why it's needed
Web Shell needs the same session workflow primitives that users expect from the CLI: quickly copy a conversation into a separate session for exploration, or send a directive to a background agent without blocking the active chat. Without daemon-level support these commands cannot work reliably from the browser client, and the transcript stream cannot show the resulting session and background notifications in a structured way.
The additional review fixes reduce confusing UX around failed fork launches and prevent a long user-controlled
/forkdirective from being copied verbatim into the parent session history.Reviewer Test Plan
How to verify
Start Web Shell against this branch, open an existing session that has conversation history, run
/branch optional name, and confirm a copied session is created and selected without adding the slash command itself as a user message. Then run/fork summarize the current investigation, confirm a background task is launched, and confirm fork-related status appears through the normal background-task UI instead of as an ordinary assistant response. For the daemon API path, POST to/session/:id/forkwith a validX-Qwen-Client-Idand confirm the response is202with the launched background agent metadata.Local command verification completed:
cd packages/cli && npx vitest run src/serve/server.test.ts -t "POST /session/:id/fork"passed;cd packages/sdk-typescript && npx vitest run test/unit/daemonUi.test.tspassed;cd packages/web-shell && npx vitest run client/adapters/transcriptToMessages.test.tspassed;npm run lintpassed;npm run buildpassed with existing warning-only output from the vscode companion package and existing Vite chunk-size/Browserslist warnings.Evidence (Before & After)
Before: Web Shell did not have daemon-backed slash command flows for copying the current session or launching a background fork agent, and branch/fork notifications were not normalized for Web Shell transcript rendering.
After: Web Shell can dispatch
/branchand/forkthrough the daemon-backed client path, the daemon stream can normalize branch/fork/rewind events, and failed fork launches surface as local errors or warnings rather than misleading success messages.Tested on
Environment (optional)
Local macOS development checkout, Node.js/npm workspace commands.
Risk & Scope
/forkdirective length limiting and shared helper extraction are intentionally out of scope because matching that behavior cleanly would require a broader lower-level change.Linked Issues
N/A
中文说明
What this PR does
这个 PR 为 Web Shell 增加基于 daemon 的会话分支能力。它把 session branch 和 background fork 操作接入 daemon bridge、TypeScript daemon client 和 Web Shell 命令处理，让
/branch可以把当前对话复制到新会话，让/fork可以基于当前对话启动后台智能体。它也让 daemon UI normalizer 和 transcript adapter 能够一致地表示 branch/fork 相关通知、后台任务消息和 rewind 事件。Web Shell 的命令列表和国际化文案现在会区分
/branch是复制当前对话，而/fork是启动后台智能体。本轮 review 修复也改善了
/fork的错误处理和后续行为：后台启动失败不会重复弹错，launched: false不再展示成功 toast，父会话只记录一条截断后的 fork 备注而不是完整 directive，branch 的复制文案不再写成 fork，bridge 侧增加诊断日志方便排查 fork 启动失败，daemon rewind 事件越界时会回退到最后一个 user turn，而不是静默无操作。Why it's needed
Web Shell 需要和 CLI 一致的会话工作流能力：快速把当前对话复制到独立会话继续探索，或者把一个指令交给后台智能体执行而不阻塞当前聊天。如果没有 daemon 层支持，这些命令无法从浏览器客户端稳定工作，transcript stream 也无法结构化展示对应的会话和后台任务通知。
额外的 review 修复减少了 fork 启动失败时的误导性 UX，也避免把很长、由用户控制的
/forkdirective 原样写入父会话历史。Reviewer Test Plan
How to verify
基于这个分支启动 Web Shell，打开一个已有对话历史的 session，执行
/branch optional name，确认会创建并切换到复制出的新会话，并且斜杠命令本身不会作为 user message 写入。然后执行/fork summarize the current investigation，确认后台任务被启动，并且 fork 相关状态通过正常的后台任务 UI 展示，而不是作为普通 assistant response 展示。daemon API 路径可以用有效的X-Qwen-Client-IdPOST/session/:id/fork，确认响应为202并返回后台 agent metadata。本地命令验证已完成：
cd packages/cli && npx vitest run src/serve/server.test.ts -t "POST /session/:id/fork"通过；cd packages/sdk-typescript && npx vitest run test/unit/daemonUi.test.ts通过；cd packages/web-shell && npx vitest run client/adapters/transcriptToMessages.test.ts通过；npm run lint通过；npm run build通过，仅有 vscode companion 包里已有的 warning-only 输出，以及现有的 Vite chunk-size/Browserslist warnings。Evidence (Before & After)
Before：Web Shell 没有基于 daemon 的当前会话复制和后台 fork agent 斜杠命令链路，branch/fork 通知也没有面向 Web Shell transcript 渲染做规范化。
After：Web Shell 可以通过 daemon-backed client path 分发
/branch和/fork，daemon stream 可以规范化 branch/fork/rewind 事件，fork 启动失败会以本地错误或 warning 展示，而不是误报成功。Tested on
Environment (optional)
本地 macOS 开发 checkout，使用 Node.js/npm workspace 命令验证。
Risk & Scope
/forkdirective 长度限制和共享 helper 提取刻意不放在本期，因为要干净对齐需要更底层的改动。Linked Issues
N/A