feat(serve): add POST /session/:id/branch for session forking#4812
Conversation
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR @doudouOUC!
Template: several required sections from the PR template are missing:
## Why it's needed— motivation / problem being solved### How to verify,### Evidence (Before & After),### Tested on(the test plan checklist is a good start but needs the sub-sections)## Risk & Scope— main risk, out-of-scope items, breaking changes## Linked Issues—Ref: #4514 T3.1is inline but should use the proper section withCloses #NorRef #N<details>中文说明</details>— bilingual translation block
Please update the PR body to match the template. The content you have is useful — it just needs to be in the right shape so reviewers can find what they need quickly.
On direction: this addresses T3.1 from #4514 (session forking over HTTP), rated ⭐⭐⭐ importance. Claude Code ships /branch natively (confirmed in their CHANGELOG with fixes for worktree/background-session branching), so session branching is clearly within the product domain. Bringing it to the daemon HTTP surface is a logical step for SDK/API consumers who want to fork conversations programmatically. Direction looks aligned.
On approach: +326/-53 across 13 files feels proportionate. The architecture — HTTP route → ACP extMethod → JSONL fork → resume semantics → event emission → SDK surface — follows the existing patterns established by loadSession/resume. Extracting computeUniqueBranchTitle from CLI-only useBranchCommand.ts to @qwen-code/qwen-code-core for reuse is the right call. One minor note: the PR removes contextual comments in sendBridgeError referencing #4282 — those comments explain why McpServerRestartFailedError maps to 502, which is the kind of non-obvious rationale worth keeping.
Please fix the template sections, then we'll continue with code review. 🔧
中文说明
感谢 @doudouOUC 的 PR!
模板: PR 正文缺少模板中的多个必填章节:## Why it's needed(动机)、### How to verify / ### Evidence / ### Tested on(测试计划子章节)、## Risk & Scope(风险与范围)、## Linked Issues(关联 Issue)、以及中文翻译块。请按模板格式补充。
方向: 对应 #4514 T3.1(HTTP 会话分叉),⭐⭐⭐ 重要性。Claude Code 已有 /branch 功能(CHANGELOG 中有相关修复记录),将会话分叉暴露到 daemon HTTP 层对 SDK/API 消费者是合理的。方向对齐。
方案: +326/-53,13 个文件,规模合理。架构遵循已有的 loadSession/resume 模式。将 computeUniqueBranchTitle 从 CLI 提取到 core 是正确的复用决策。小建议:sendBridgeError 中删除的 #4282 注释解释了 502 状态码的原因,建议保留。
请补充模板章节后继续代码审查。🔧
— Qwen Code · qwen3.7-max
📋 Review SummaryThis PR implements session forking functionality via a new 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalType errors must be fixed before merging:
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
There was a problem hiding this comment.
Pull request overview
This PR adds first-class “session forking” support to the qwen serve daemon by introducing a new HTTP endpoint (POST /session/:id/branch) backed by an ACP control method, and then surfaces the feature in the TypeScript SDK (typed response + typed SSE event + capability tag). It also extracts the branch-title collision logic into @qwen-code/qwen-code-core for reuse by both the CLI and daemon/agent path.
Changes:
- Add
POST /session/:id/branchserve route, bridge implementation, ACP extMethod handler, andsession_branchcapability. - Add SDK support:
DaemonClient.branchSession(), response/request types, and a typedsession_branchedevent. - Move
computeUniqueBranchTitlefrom CLI hook into coresessionServiceutilities.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-typescript/src/daemon/types.ts | Add request/response types for branching (BranchSessionRequest, DaemonBranchedSession). |
| packages/sdk-typescript/src/daemon/index.ts | Re-export new branch-related types/events from the SDK entrypoint. |
| packages/sdk-typescript/src/daemon/events.ts | Add new known SSE event type session_branched, reducer state, and runtime guards. |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Add branchSession() HTTP client method for /session/:id/branch. |
| packages/core/src/services/sessionService.ts | Export computeUniqueBranchTitle for shared branch-title uniqueness logic. |
| packages/cli/src/ui/hooks/useBranchCommand.ts | Switch CLI /branch command to use core computeUniqueBranchTitle. |
| packages/cli/src/serve/server.ts | Add HTTP route POST /session/:id/branch and map BranchWhilePromptActiveError to 409. |
| packages/cli/src/serve/httpAcpBridge.ts | Implement bridge-side branchSession() including extMethod call, resume, and event fan-out. |
| packages/cli/src/serve/capabilities.ts | Register new serve capability tag session_branch. |
| packages/cli/src/acp-integration/acpAgent.ts | Implement ACP control method qwen/control/session/branch (flush, fork, compute title, rename). |
| packages/acp-bridge/src/status.ts | Add extMethod constant sessionBranch. |
| packages/acp-bridge/src/bridgeTypes.ts | Add bridge-level request/response types + HttpAcpBridge.branchSession() signature. |
| packages/acp-bridge/src/bridgeErrors.ts | Add typed error BranchWhilePromptActiveError for 409 mapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
left a comment
There was a problem hiding this comment.
[Critical] useBranchCommand.ts:11 — SessionService type import is unused after extracting computeUniqueBranchTitle to core. Remove it to fix TS6133 and the eslint @typescript-eslint/no-unused-vars error.
(Posting as body because the import line is not in the PR diff hunks.)
[Suggestion] No test coverage for ~326 lines of new branching functionality. Key untested areas:
POST /session/:id/branchroute (success, 409 active prompt, 404 not found, session limit)computeUniqueBranchTitleinpackages/core(collision bumping, timestamp fallback)isSessionBranchedDatatype guard andreduceDaemonSessionEventsession_branched caseDaemonClient.branchSessionSDK method
— qwen3.7-max via Qwen Code /review
Review response summary (e7d1fd3)
🤖 Generated with Qwen Code |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
cdd1f5b to
a1d9db7
Compare
wenshao
left a comment
There was a problem hiding this comment.
[Critical] npm run build fails — SDK browser bundle size exceeds limit.
Error: Browser daemon SDK bundle is 108702 bytes; expected <= 108544
The new types added to packages/sdk-typescript/src/daemon/ (DaemonSessionBranchedData, DaemonBranchedSession, DaemonSessionBranchedEvent, BranchSessionRequest, session_branched event, lastBranch view state) push the browser bundle 158 bytes over the 106 KB budget in scripts/build.js:23 (MAX_DAEMON_BROWSER_BUNDLE_BYTES = 106 * 1024).
Fix: bump the limit to 107 * 1024 (or higher) in packages/sdk-typescript/scripts/build.js, or reduce the type surface exported from the daemon SDK barrel.
— qwen3.7-max via Qwen Code /review
|
Fixed in 7f1076c — bumped |
Local Verification Report — PR #4812Tested on: macOS Darwin 25.4.0 (Apple Silicon) Test Results Summary
server.test.ts Failure Analysis (15 total)3 PR-caused failures — capability registry tests:
Root cause: PR adds Fix: Add 12 pre-existing / flaky failures —
ConclusionNot merge-ready — 3 capability registry tests need updating to include All other checks pass: acp-bridge (376 tests), typecheck, lint, and full build chain including SDK. Verified locally by wenshao |
Review response (69c3651)
|
wenshao
left a comment
There was a problem hiding this comment.
R6 APPROVE at 69c3651 — No new findings. The latest commit correctly fixes the empty baseName issue. Build passes, all related tests pass (80 events + 53 sessionService). All previously raised Critical/Suggestion issues from R1-R5 have been either fixed or discussed and acknowledged as design trade-offs. — claude-opus-4-6 via Qwen Code /review
Local Verification ReportBranch: Unit Tests
Test failure analysis — all 11 failures are pre-existing on
0 new test failures introduced by this PR ✅ Note: Initial bridge test run had 4 collection-time failures ( ESLintFiles linted: TypeScript Type Check
No typecheck errors in any of the 14 PR-changed files. Whitespace CheckMinor: trailing blank line at end of Summary
All verification checks pass (no regressions from PR). Minor whitespace nit in Verified locally by wenshao |
777ad9e to
2b3589e
Compare
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Zero test coverage for all new code paths
This PR adds ~300 lines of new logic across 7 files with no tests. The following paths are entirely uncovered:
bridge.branchSession(bridge.ts:2638-2730) — 5 code branchesPOST /session/:id/branch(server.ts:1422-1462) — HTTP routesessionBranchext method handler (acpAgent.ts:3035-3098) — agent-sidecomputeUniqueBranchTitle(sessionService.ts:1367-1384)promptActivestate transitions andhasActivePromptsemantic changeBranchWhilePromptActiveError→ 409 mapping
Existing test files (bridge.test.ts, server.test.ts, sessionService.test.ts) already have patterns for analogous features. Any of these paths can silently regress without detection.
[Critical] session_branch missing from test assertion
capabilities.ts adds session_branch to SERVE_CAPABILITY_REGISTRY, but server.test.ts's EXPECTED_STAGE1_FEATURES / EXPECTED_REGISTERED_FEATURES assertion was not updated. CI will fail.
[Suggestion] computeUniqueBranchTitle duplicated in useBranchCommand.ts
packages/cli/src/ui/hooks/useBranchCommand.ts:71 has a pre-existing local copy of computeUniqueBranchTitle that is identical to the newly exported one in packages/core/src/services/sessionService.ts:1367. The cli copy should be replaced with an import from @qwen-code/qwen-code-core to avoid future drift.
— qwen3.7-max via Qwen Code /review
Review Round 5 — Action SummaryCommit:
|
Review Round 6 — Action SummaryCommit:
|
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
PR #4812 Verification ReportReviewer: wenshao 1. Build & Type Check
2. Unit Tests (
|
| Failure | Root Cause | PR-related? |
|---|---|---|
returns session hooks status from the bridge |
Test bug in this PR — route GET /session/:id/hooks calls bridge.getSessionHooksStatus(), but GET /workspace/hooks calls workspace.getWorkspaceHooksStatus() on the DaemonWorkspaceService. After the #4563 refactor that extracted DaemonWorkspaceService, the workspace hooks test should inject a fake workspace service, not rely on the fake bridge. The workspace hooks test and session hooks test both return 404 because the route handler calls the workspace service (not the bridge), but the test only provides a fake bridge. |
|
10× runQwenServe timeout errors |
Pre-existing hook timeout issue (10s afterEach cleanup) |
❌ No |
4× qwen serve: createServeApp default fsFactory warnings |
Pre-existing test infrastructure | ❌ No |
Recommendation: Fix the session hooks test by providing a fake DaemonWorkspaceService with getSessionHooksStatus, or wire the fake bridge's getSessionHooksStatus through the workspace service constructor.
3. Integration Tests (Real Daemon, tmux)
Started daemon: node packages/cli/dist/index.js serve --port 3100
| # | Test Case | Expected | Actual | Result |
|---|---|---|---|---|
| 1 | GET /capabilities includes session_branch |
session_branch in features array |
Present | ✅ |
| 2 | POST /session — create session |
200 with sessionId | 200 | ✅ |
| 3 | Send prompt to session | 202 with promptId | 202 | ✅ |
| 4 | POST /session/:id/branch with {"name":"experiment"} |
201 with new sessionId + title | 201, title=experiment (Branch), forkedFrom correct |
✅ |
| 5 | Send prompt to branched session | 202 | 202 | ✅ |
| 6 | Send prompt to source session after branch | 202 | 202 | ✅ |
| 7 | Branch non-existent session | 404 | 404 with error message | ✅ |
| 8 | Branch during active prompt (409 test) | 409 or queued wait | 201 — branch chains onto promptQueue, serializes correctly |
✅ (by design) |
| 9 | Branch without name field |
Derive from parent title | 201, title=9b74bd93 (Branch) (from session ID prefix) |
✅ |
| 10 | Branch with 250-char name | Truncated to 200 | 201, title length 209 (200 + (Branch)) |
✅ |
| 11 | SSE session_branched event |
Event emitted on source session's SSE | Event received: sourceSessionId, newSessionId, displayName all correct |
✅ |
| 12 | Session list includes branches | All branched sessions visible | 8 sessions listed, all branches visible | ✅ |
| 13 | Branch with empty body | Graceful fallback | 201, title=9b74bd93 (Branch 2) (collision avoidance) |
✅ |
4. Title Collision Detection
Verified incremental suffix works:
- First branch:
experiment (Branch) - Second branch with same base:
conflict-test (Branch)→conflict-test (Branch 2) - No-name branch:
9b74bd93 (Branch)→9b74bd93 (Branch 2)(collision avoided)
5. Code Review Notes
Design observations:
branchSessioncorrectly chains ontopromptQueuefor FIFO serialization — thepromptActivecheck is a safety belt, not primary guardBranchWhilePromptActiveError→ 409 cannot be triggered via HTTP (queue prevents it), but serves as internal invariant protectionwriteFileSyncinforkSessionis acknowledged as a known limitation (PR description)- Event fan-out broadcasts
session_branchedto all sessions (source + peers), which is correct
Minor issues:
- Test bug (blocking): The
returns session hooks status from the bridgeandreturns workspace hooks status from the bridgetests are wired incorrectly after theDaemonWorkspaceServiceextraction — the fake bridge's hooks methods are never called because the route uses the workspace service object - Non-blocking: Some formatting-only changes in
acpAgent.ts(line breaks in ternary expressions) could have been a separate commit to keep the diff focused
6. Summary
| Category | Verdict |
|---|---|
| Feature correctness (happy path) | ✅ |
| Error handling (404, empty body) | ✅ |
| SSE event broadcasting | ✅ |
| Title collision avoidance | ✅ |
| Source session isolation | ✅ |
| Capabilities advertising | ✅ |
| SDK type exports | ✅ |
| Unit test (new tests) | |
| Security (input sanitization) | ✅ |
Overall: LGTM with minor fix needed — the hooks test wiring issue should be fixed before merge. All runtime behavior is correct and matches the PR description.
Verified by wenshao
…T3.1) Adds a dedicated HTTP route that forks a live session's JSONL transcript and loads the fork via resume semantics (no history replay). Remote clients can now programmatically branch sessions without the interactive dialog the CLI /branch command requires. Key design decisions: - Uses resume (not load) to avoid flooding SSE with full history replay - Source session must be idle (409 if prompt active via `promptActive` flag) - ACP extMethod pattern for the fork operation (flush + forkSession + title) - Validates originator via resolveTrustedClientId before event emission - Cross-client events on source bus + workspace-wide fan-out - Extracts computeUniqueBranchTitle to core for reuse 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
- Fix #1: Add detachClient branch for attached sessions in !res.writable cleanup (mirrors restoreSessionHandler pattern) - Fix #3: Move resolveTrustedClientId validation before restoreSession to prevent orphaned live sessions if client ID becomes invalid - Fix #2: Clean up orphan JSONL in acpAgent when post-fork title operations fail (removeSession on catch) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…ridge shim Without this re-export, server.ts fails to compile because it imports from './acpSessionBridge.js' which did not forward the new error class. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Prevents unbounded name input from exceeding SESSION_TITLE_MAX_LENGTH after computeUniqueBranchTitle appends the " (Branch N)" suffix. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…anch)" The regex stripping "(Branch N)" suffix could produce an empty string when the title itself was just "(Branch)". Now falls back to sessionId prefix in that case. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…ing blank line Rebase onto latest daemon_mode_b_main which added session_rewind and SessionBusyError features. Keep both rewind and branch additions. Fix trailing blank line in sessionService.ts (wenshao nit).
- Serialize branch with promptQueue to close TOCTOU race - Wrap sessionBranch ext method with runWithAcpRuntimeOutputDir - Guard promptActive against sync exceptions before .finally() - Add best-effort orphan JSONL cleanup on restore failure - Strip control characters from branch name parameter - Replace duplicated computeUniqueBranchTitle with core import - Add session_branch to capability test assertion arrays
… drop dead forkedFrom field - Chain branchSession onto entry.promptQueue (same pattern as sendPrompt) to prevent concurrent prompt dispatch during the fork window - Log cleanup errors in bridge catch block and acpAgent removeSession instead of silently swallowing - Remove dead forkedFrom field from agent return value (bridge constructs its own forkedFrom object, never reads the agent's)
41bf68e to
ec273cf
Compare
Local Verification ReportBranch: TypeScript Compilation (
|
| Package | PR Branch | Base Branch | Delta | Notes |
|---|---|---|---|---|
| core | 0 errors | 0 errors | 0 | ✅ Clean |
| cli | 207 errors | 201 errors | +6 | See below |
| acp-bridge | 2 errors | 2 errors | 0 | ✅ Pre-existing (stale dist/) |
| sdk-typescript | 1 error | 1 error | 0 | ✅ Pre-existing (mcpTimeouts import) |
CLI +6 errors breakdown (all stale dist/ or new catch blocks, not real regressions):
- 2×
computeUniqueBranchTitlenot exported from@qwen-code/qwen-code-core— function moved tocore/sessionService.ts, stale localdist/ - 1×
sessionBranchnot on ext method type — new ext method, staledist/ - 1×
BranchWhilePromptActiveErrornot exported from@qwen-code/acp-bridge/bridgeErrors— new error class, staledist/ - 2×
erris of type 'unknown' in newserver.tscatch blocks (lines 3777, 3779) — matches existing pattern on base branch
Test Results
| Test Suite | Result | Details |
|---|---|---|
| acp-bridge vitest | ✅ 376 passed | 8 test files, all green (includes bridge.test.ts 211 tests) |
| core sessionService vitest | ✅ 53 passed | Covers computeUniqueBranchTitle moved from CLI |
| cli server.test.ts | ❌ Failed | Pre-existing @qwen-code/acp-bridge/mcpTimeouts import resolution failure — same failure on base branch |
Conclusion
- No new TSC regressions: All 6 extra CLI errors are due to stale local
dist/artifacts (resolved by CI's fresh build) or follow existing patterns - All relevant tests pass: Bridge tests (376) and sessionService tests (53) all green
- server.test.ts failure is pre-existing on
daemon_mode_b_mainbase (verified via identical error on base branch checkout) - Core package compiles cleanly with 0 errors
✅ Recommendation: Safe to merge
… title length limit - Replace manual for-of loop with broadcastWorkspaceEvent helper for session_branched fan-out (adds per-session try/catch) - Truncate baseName in computeUniqueBranchTitle to ensure final title stays within SESSION_TITLE_MAX_LENGTH after suffix append
Review Round 8 — Action SummaryCommit:
|
chiga0
left a comment
There was a problem hiding this comment.
Review Summary — PR #4812
PR: feat(serve): add POST /session/:id/branch for session forking
Author: @doudouOUC | +396/-63 | 15 files | 9 commits
HEAD: e792da85bb | Base: daemon_mode_b_main
Independent Review
All 15 files reviewed across 4 layers:
| Layer | Key Files | Verification |
|---|---|---|
| HTTP Route (server.ts) | Input sanitization (ctrl chars + 200 cap), 409 on BranchWhilePromptActiveError, best-effort cleanup on !writable | Correct |
| Bridge (bridge.ts) | branchSession serializes via promptQueue, checks promptActive + session cap, restore cleanup on failure, broadcastWorkspaceEvent for fan-out |
Correct |
| Agent (acpAgent.ts) | sessionBranch ext method: validates sessionId, flushes recording, runWithAcpRuntimeOutputDir, orphan JSONL cleanup on title failure |
Correct |
| SDK (types/events/DaemonClient) | BranchSessionRequest, DaemonBranchedSession, session_branched event with type guard + reducer + lastBranch view state |
Correct |
wenshao Audit
9 review rounds — all Critical findings addressed:
- TOCTOU race on
promptActive→ serialized throughpromptQueue - Missing
runWithAcpRuntimeOutputDir→ wrapped - Orphan JSONL on restore failure → best-effort cleanup
- Session cap bypass → includes
inFlightSpawns+inFlightRestores promptActivestuck on sync exception → try/catch guard addedhasActivePromptsemantic shift → now uses explicitpromptActivebooleanbroadcastWorkspaceEvent→ adopted (replaces manual for-of loop)- Title length enforcement →
computeUniqueBranchTitletruncates baseName for suffix room
Verdict: APPROVED — 0 findings.
Reviewed at HEAD e792da85bb.
Already have 2 approved. 3ks.
What this PR does
Adds a dedicated
POST /session/:id/branchHTTP route to the daemon that forks a live session's JSONL transcript and loads the fork via resume semantics (no history replay). Remote clients (web shell, IDE extensions, SDK consumers) can now programmatically branch sessions without the interactive dialog the CLI/branchcommand requires.Why it's needed
The CLI
/branchcommand is dialog-based (type: 'dialog') and cannot be invoked via the slash-command passthrough path (POST /session/:id/prompt). SDK/API consumers need a typed HTTP endpoint to fork conversations programmatically — this is T3.1 from #4514.Reviewer Test Plan
How to verify
qwen serve --port 3100POST /session/:id/branchwith{"name":"experiment"}→ expect 201 with new sessionId + titlesession_branchinGET /capabilitiesEvidence (Before & After)
N/A — new feature, no prior behavior to compare.
Tested on
Risk & Scope
forkSessionuses synchronous I/O (writeFileSync) which blocks the event loop briefly for large sessions. Acceptable for now; worker-thread optimization deferred.afterMessageIdparameter (fork from a specific point), multi-daemon coordination.Linked Issues
Ref #4514 (T3.1)
中文说明
本 PR 做了什么
为 daemon 添加专用的
POST /session/:id/branchHTTP 路由,用于 fork 活跃 session 的 JSONL 转录文件,并通过 resume 语义加载 fork(不重放历史)。远程客户端(web shell、IDE 扩展、SDK 消费者)现在可以编程式地分叉会话,无需 CLI/branch命令的交互式对话框。为什么需要
CLI 的
/branch命令是对话框式的(type: 'dialog'),无法通过 slash-command 透传路径(POST /session/:id/prompt)调用。SDK/API 消费者需要一个带类型的 HTTP 端点来编程式地 fork 会话 —— 这是 #4514 的 T3.1。风险与范围
forkSession使用同步 I/O(writeFileSync),对大 session 会短暂阻塞事件循环。目前可接受,worker 线程优化延后。afterMessageId参数(从特定点 fork)、多 daemon 跨主机协调。关联 Issue
Ref #4514 (T3.1)