Skip to content

feat(web-shell): worktree-isolated sessions for parallel tasks - #7221

Merged
wenshao merged 2 commits into
QwenLM:mainfrom
wenshao:feat/webshell-worktree-sessions
Jul 19, 2026
Merged

feat(web-shell): worktree-isolated sessions for parallel tasks#7221
wenshao merged 2 commits into
QwenLM:mainfrom
wenshao:feat/webshell-worktree-sessions

Conversation

@wenshao

@wenshao wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds support for creating sessions in isolated git worktrees from the Web Shell, enabling multiple tasks to run in parallel within the same workspace without polluting the main working directory.

When a user creates a worktree session, the daemon creates a git worktree under <repoRoot>/.qwen/worktrees/<slug>, spawns the session, and relocates its working directory into the worktree via the existing changeSessionCwd bridge method. The Web Shell shows a purple ⑂ (GitForkIcon) indicator in the git chip, session list, and workspace dropdown to distinguish worktree sessions from normal ones.

Key changes across the stack:

  • Daemon: POST /session accepts an optional worktree parameter, creates the worktree via GitWorktreeService, and passes metadata through BridgeSpawnRequestSessionEntryBridgeSessionSummary. Worktree info is persisted to a sidecar file (<sessionId>.worktree.json) and survives daemon restarts.
  • SDK: CreateSessionRequest gains worktree?: { slug?: string }, DaemonSession and DaemonSessionSummary gain worktree?: DaemonWorktreeInfo, DaemonSessionClient exposes a worktree getter, and WorkspaceDaemonClient.workspaceGit() accepts an optional cwd parameter for worktree-scoped git status queries.
  • Web Shell: The workspace branch pill dropdown offers "New Worktree Task" (git repos only). The git chip turns purple with a GitForkIcon for worktree sessions. The session list shows a ⑂ badge. An empty-state welcome badge explains worktree isolation. Git status queries target the worktree path, not the main workspace root.

Why it's needed

Users working on multiple tasks in the same repository need isolation — one session's file edits, git add, or git checkout shouldn't interfere with another's. Git worktrees provide this natively, but the workflow was manual (git worktree add + cd). This PR makes it a one-click experience in the Web Shell.

Reviewer Test Plan

How to verify

  1. Run npm run build && npm run dev:daemon
  2. Open the Web Shell in a browser
  3. Click the git branch pill in the sidebar workspace header → dropdown should show "Changes" and "New Worktree Task" (with purple ⑂ icon and description)
  4. Click "New Worktree Task" → empty chat page shows a purple welcome badge
  5. Send a message → the git chip in the composer turns purple with a ⑂ icon showing the worktree branch name
  6. Check the sidebar session list → the worktree session has a purple ⑂ icon before its label
  7. Restart the daemon → reload the page → the ⑂ icon persists in the session list (via sidecar file). Note: the purple git chip requires the session to be live — after restart, click the session to load it and the chip restores.
  8. Non-git workspaces should NOT show the "New Worktree Task" option

Evidence (Before & After)

Before: No worktree isolation — all sessions share the same working directory.

After: Worktree sessions are visually distinguished with purple ⑂ indicators and operate in isolated git worktrees.

Web Shell with worktree session

Tested on

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

Environment

npm run dev:daemon on macOS, Chrome browser.

Risk & Scope

  • Main risk or tradeoff: The worktree sidecar enrichment in session listing reads one small file per session. For workspaces with many sessions this adds I/O, but sidecar files are <1KB and only exist for worktree sessions.
  • Not validated / out of scope: Worktree isolation does not survive daemon restart — after restart, a reloaded worktree session's effective cwd falls back to the main workspace root (model-text hint only via restoreWorktreeContext), and the purple git chip does not restore until the session is re-created. The sidebar ⑂ badge persists via sidecar enrichment. Restoring full isolation on load (read sidecar → re-issue changeSessionCwd → populate entry.worktree) is the first Phase 3 item. Also deferred: worktree cleanup on session end, merge-back workflow, worktree-scoped /diff dialog (the Changes dialog is disabled for worktree sessions since it would show the main workspace's diff).
  • Breaking changes / migration notes: None. The worktree parameter is optional; omitting it preserves existing behavior.

Linked Issues

Design doc: docs/design/2026-07-19-webshell-worktree-sessions.md (included in this PR)

中文说明

这个 PR 做了什么

在 Web Shell 中支持创建 git worktree 隔离会话,使同一 workspace 中的多个任务可以并行运行而不会污染主工作目录。

用户创建 worktree 会话时,daemon 在 <repoRoot>/.qwen/worktrees/<slug> 下创建 git worktree,启动会话后通过已有的 changeSessionCwd 方法将工作目录切换到 worktree。Web Shell 通过紫色 ⑂ (GitForkIcon) 标识区分 worktree 会话和普通会话,包括 git chip、会话列表和 workspace 下拉菜单。

主要改动:

  • DaemonPOST /session 接受可选的 worktree 参数,通过 GitWorktreeService 创建 worktree,元数据通过 BridgeSpawnRequestSessionEntryBridgeSessionSummary 传递。worktree 信息持久化到 sidecar 文件,daemon 重启后不丢失。
  • SDKCreateSessionRequest 新增 worktree 字段,DaemonSessionDaemonSessionSummary 新增 worktree 字段,DaemonSessionClient 暴露 worktree getter,workspaceGit() 支持可选 cwd 参数查询 worktree 的 git 状态。
  • Web Shell:workspace 分支 pill 下拉菜单提供"新建 Worktree 任务"选项(仅 git 仓库)。worktree 会话的 git chip 变为紫色 ⑂ 图标。会话列表显示 ⑂ 标记。空聊天页显示 worktree 隔离说明。git 状态查询指向 worktree 路径而非主 workspace。

为什么需要

用户在同一仓库中处理多个任务时需要隔离——一个会话的文件编辑、git addgit checkout 不应影响另一个。Git worktree 原生提供这种隔离,但之前需要手动操作。本 PR 将其变为 Web Shell 中的一键操作。

风险与范围

  • 主要风险:session 列表的 sidecar 读取会为每个会话增加一次小文件 I/O,但 sidecar 文件 <1KB 且仅 worktree 会话有。
  • 未覆盖:worktree 自动清理(Phase 3)、merge-back 工作流、worktree 范围的 /diff 对话框。
  • 无破坏性变更:worktree 参数可选,不传则行为不变。

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Thanks for the PR! Clean re-run at the current head.

Template looks good ✓ — all required sections present with a thorough design doc.

Problem: Real user need, not theoretical. Multiple sessions in the same workspace share a working tree and can trample each other's file edits and git state. The CLI already has worktree infrastructure (GitWorktreeService, enter_worktree), but it's session-internal — this extends it to session creation, which is the natural next step.

Direction: Aligned. The design doc (docs/design/2026-07-19-webshell-worktree-sessions.md) is well-structured with clear phase boundaries. This PR delivers Phase 1 (daemon + SDK) and Phase 2 (Web Shell UI), deferring lifecycle management (Phase 3) appropriately. The approach reuses GitWorktreeService rather than reimplementing worktree management.

Size: 624 production lines across 6 packages (acp-bridge, cli, core, sdk-typescript, web-shell, webui), plus 185 test lines and a 428-line design doc. The core change is minimal — 7 lines in sessionService.ts exposing an existing private method. Flagging the 500+ production line threshold for maintainer awareness per policy, though the author is a maintainer.

Approach: Scope feels right for a full-stack feature. The daemon route handles validation, worktree creation, session relocation via changeSessionCwd, and rollback on failure — all transactional. The ?cwd= parameter on the git status endpoint with realpath containment is a clean security measure. The Web Shell's lazy session creation flow (pendingWorktreeRefcreateAndAttachSessionForPrompt) threads the worktree intent correctly. No unrelated changes or drive-by refactors spotted.

Moving on to code review. 🔍

中文说明

感谢贡献!在当前 head 上重新审查。

模板完整 ✓ — 所有必填部分齐全,附有详细的设计文档。

问题: 真实的用户需求,非理论性问题。同一 workspace 中的多个 session 共享工作目录,可能互相踩踏文件编辑和 git 状态。CLI 已有 worktree 基础设施(GitWorktreeServiceenter_worktree),但仅限于会话内部——本 PR 将其扩展到 session 创建阶段,是自然的下一步。

方向: 对齐。设计文档结构清晰,阶段划分明确。本 PR 交付 Phase 1(daemon + SDK)和 Phase 2(Web Shell UI),合理地推迟了生命周期管理(Phase 3)。方案复用 GitWorktreeService 而非重新实现。

规模: 624 行生产代码跨 6 个包,另有 185 行测试代码和 428 行设计文档。核心改动极小——sessionService.ts 中仅 7 行,暴露已有的私有方法。按政策标记 500+ 生产行阈值供维护者知悉(作者本身即维护者)。

方案: 范围合理。daemon 路由处理验证、worktree 创建、通过 changeSessionCwd 重定位 session、失败时回滚——全部事务化处理。git 状态端点的 ?cwd= 参数配合 realpath 包含检查是干净的安全措施。Web Shell 的懒加载 session 创建流程正确传递了 worktree 意图。未发现无关改动或顺手重构。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 738b6aa945ad77d89a7eab36f9d50574ab0c0597 · re-run with @qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

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

Screenshots · before / after

No screenshot changes against the PR base.

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

Qwen Code · web-shell visuals

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 738b6aa, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: I'd add an optional worktree param to POST /session, create the worktree via the existing GitWorktreeService before spawn, thread metadata through BridgeSpawnRequestSessionEntryBridgeSessionSummary, relocate the session cwd post-spawn, and add a visual indicator in the Web Shell. The git status endpoint would need a cwd override for worktree paths.

Comparison: The PR matches this approach closely. A few observations:

The daemon route (session.ts) is the heaviest change at 158 lines. It handles the full lifecycle: input validation (non-object → 400, non-git → 400, invalid slug → 400, creation failure → 500), worktree creation, sessionScope = 'thread' override to prevent coalescing, post-spawn relocation via changeSessionCwd, sidecar persistence, and transactional rollback on cd failure (kill session + remove worktree). The rollback logic is thorough — it handles the case where killSession fails because another client keeps the session alive, and still cleans up the unused worktree.

The workspace-git.ts change adds a ?cwd= query parameter with a realpath containment check (path.relative + .. guard) to prevent symlink escape. For worktree paths it calls getGitWorkingTreeStatus directly instead of creating a watcher entry in WorkspaceGitState — good call, avoids leaking fs watchers.

The core change (sessionService.ts, 7 lines) just exposes getWorktreeSessionPathForArchiveState as a public wrapper around the existing private getWorktreeSessionPathForState. Minimal and safe.

The enrichWorktreeSidecars function in session-list.ts reads one sidecar file per session sequentially. For workspaces with many sessions this adds I/O, but sidecar files are <1KB and only exist for worktree sessions — acceptable for now.

No critical blockers found. No AGENTS.md violations. Code follows project conventions (ESM, strict TS, collocated tests, kebab-case files).

Tests: 5 new daemon route tests (success, non-git 400, invalid slug 400, non-object 400, creation failure 500) plus updated WorkspaceSection and sessionPreparation tests. All 781 tests pass (760 CLI + 21 web-shell).

Real-Scenario Testing

Started the daemon from this PR's build against a test git repo and exercised the worktree API end-to-end:

$ node packages/cli/dist/index.js serve --port 14199 --workspace /tmp/triage-worktree-test/test-repo
qwen serve listening on http://127.0.0.1:14199 (mode=http-bridge, workspace=/tmp/triage-worktree-test/test-repo)
qwen serve: bound to workspace "/tmp/triage-worktree-test/test-repo"

# 1. Create worktree session with named slug
$ curl -s -X POST http://127.0.0.1:14199/session \
    -H 'Content-Type: application/json' \
    -d '{"worktree": {"slug": "test-task"}}'
{
    "sessionId": "eb4cefe9-a552-45db-8529-79f002d402a6",
    "workspaceCwd": "/tmp/triage-worktree-test/test-repo",
    "attached": false,
    "worktree": {
        "slug": "test-task",
        "path": "/tmp/triage-worktree-test/test-repo/.qwen/worktrees/test-task",
        "branch": "worktree-test-task"
    }
}

# Daemon log confirms relocation:
qwen serve: session eb4cefe9 cwd changed: /tmp/.../test-repo -> /tmp/.../test-repo/.qwen/worktrees/test-task

# 2. Worktree on disk
$ git worktree list
/tmp/triage-worktree-test/test-repo                            86af6d0 [master]
/tmp/triage-worktree-test/test-repo/.qwen/worktrees/test-task  86af6d0 [worktree-test-task]

# 3. Git status with worktree cwd
$ curl -s "http://127.0.0.1:14199/workspaces/%2Ftmp%2F.../git?cwd=.../worktrees/test-task"
{
    "v": 2,
    "workspaceCwd": "/tmp/.../worktrees/test-task",
    "branch": "worktree-test-task",
    "detached": false,
    "staged": 0, "unstaged": 0, "untracked": 1, "conflicted": 0
}

# 4. Path traversal blocked (falls back to workspace root)
$ curl -s ".../git?cwd=/etc/passwd"
{ "workspaceCwd": "/tmp/triage-worktree-test/test-repo", "branch": "master" }

# 5. Error cases
$ curl -s -X POST .../session -d '{"worktree": "yes"}'
{"error":"`worktree` must be an object","code":"invalid_worktree"}

$ curl -s -X POST .../session -d '{"worktree": {"slug": "../escape"}}'
{"error":"Worktree name may only contain letters, digits, dots, underscores, and hyphens.","code":"worktree_invalid_slug"}

# 6. Auto-slug generation
$ curl -s -X POST .../session -d '{"worktree": {}}'
{ "worktree": { "slug": "kind-elm-5b3aca", "branch": "worktree-kind-elm-5b3aca" } }

# 7. Normal session (backward compat — no worktree field)
$ curl -s -X POST .../session -d '{}'
{ "sessionId": "cb287c78-...", "workspaceCwd": "/tmp/.../test-repo" }

All scenarios behave as documented. Worktree creation, relocation, git status override, path traversal protection, error handling, auto-slug, and backward compatibility all verified.

中文说明

代码审查

独立方案:POST /session 添加可选 worktree 参数,spawn 前通过 GitWorktreeService 创建 worktree,元数据穿透 BridgeSpawnRequestSessionEntryBridgeSessionSummary,spawn 后重定位 session cwd,Web Shell 添加视觉标识。git 状态端点需要 cwd 覆盖。

对比: PR 方案与独立方案高度一致。daemon 路由(158 行)处理完整生命周期:输入验证、worktree 创建、sessionScope = 'thread' 覆盖、changeSessionCwd 重定位、sidecar 持久化、cd 失败时事务性回滚。workspace-git.ts?cwd= 参数配合 realpath 包含检查防止符号链接逃逸。核心改动仅 7 行。无关键阻塞问题。

测试: 5 个新 daemon 路由测试 + 更新的 Web Shell 测试,全部 781 个测试通过。

真实场景测试

从本 PR 构建启动 daemon,对测试 git 仓库执行端到端 worktree API 测试:worktree 创建、session 重定位、git 状态覆盖、路径遍历防护、错误处理、自动 slug 生成、向后兼容性——全部验证通过。

Qwen Code · qwen3.7-max

Reviewed at 738b6aa945ad77d89a7eab36f9d50574ab0c0597 · re-run with @qwen-code /triage

@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Review — worktree-isolated sessions (verified at c93a8d2)

Read the full stack (route → bridge → SDK → web-shell) at the head commit, plus the pre-existing worktree infrastructure this builds on (GitWorktreeService, worktreeSessionService, the ACP resume path, WorkspaceGitState). The layering is right: worktree creation stays in the daemon route, the bridge only carries metadata, and reusing changeSessionCwd instead of a new spawn parameter keeps the ACP contract untouched. The route validation battery (non-object body, ../escape slug, non-git repo, create failure) is thorough and the tests exercise it against real static validators. Findings below, ordered by severity.

1. Isolation does not survive load/resume — but the UI keeps claiming it (High)

The restore path never rehydrates worktree state. After a daemon restart, loading the session goes through restoreSession, which calls loadSession({ cwd: workspaceKey }) (bridge.ts:3970) and the restore-side createSessionEntry (bridge.ts:4087) without the worktree option — only the spawn path passes it (bridge.ts:2331). No changeSessionCwd is issued on load. The ACP child's #restoreWorktreeOnResume only injects a one-shot <system-reminder> hint to the model (acpAgent.ts:3689, Session.ts:1144-1157); the session's actual Config cwd is the main checkout, so shell commands and relative-path tools operate on the main working tree again.

The metadata splits inconsistently at that point: the sidebar list keeps the ⑂ badge (sidecar enrichment survives mergeLiveSessionSummary because the live spread simply lacks the worktree key), but GET /session/:id/status prefers the live summary — which has no worktree after restore — so the git chip silently reverts to normal while the list still says isolated. Worst case the user trusts the badge and lets the agent run git checkout/edits in the main checkout.

Suggested fix: in the load route (or bridge restore), read the sidecar, rehydrate entry.worktree, and re-issue changeSessionCwd into the worktree when the directory still exists — mirroring the create path. That fixes both the cwd and the summary in one place.

2. ?cwd= boundary check is prefix-only and unnormalized (Medium, security-adjacent)

workspace-git.ts:66 accepts any rawCwd.startsWith(runtime.workspaceCwd). Two holes: sibling-prefix collision (workspace /home/u/repo accepts /home/u/repo-secrets), and no normalization (/home/u/repo/../../etc passes and resolves outside the workspace). The effect is running resolveBranchName/getGitWorkingTreeStatus against arbitrary readable directories — and, via finding 3, installing a persistent fs watcher on them. Use the codebase's usual boundary idiom (path.resolve + path.relative not starting with ..), or tighter: require the cwd to be under <workspaceCwd>/.qwen/worktrees/, which is the only intended caller. Also consider a 400 instead of the silent fallback — a client bug currently gets main-workspace status labeled as worktree status.

3. WorkspaceGitState leaks one watcher per worktree path (Medium)

getStatus creates a Map entry with a watchRepoBranch fs watcher per distinct cwd (workspace-git-state.ts:94-104,124), and entries are only disposed via disposeWorkspace (workspace removal) or daemon shutdown. Worktree paths are never workspaces, so every worktree session adds a watcher that outlives the session and the worktree itself (Phase 3 cleanup will delete the directory out from under it). Those entries also publish git_branch_changed workspace events keyed by the worktree path. Options: don't create watcher entries for non-workspace cwds (do a one-shot status read), or key the entry to the owning workspace and dispose on session end.

4. Failed relocation still returns a "worktree session" (Medium)

When changeSessionCwd fails (session.ts:1364-1387 — including the real CdWhilePromptActiveError window if another client slips a prompt in between spawn and cd), the route logs a warning and returns 200 with full worktree metadata, and the bridge entry keeps it too (it was passed at spawn). The client cannot distinguish "intended" from "actual", so the chip shows purple-isolated on a session running in the main checkout — same failure mode as finding 1 but reachable without a restart. Suggest omitting worktree from the response (and clearing entry.worktree) on cd failure, or adding an explicit relocated: false the UI can render as a warning.

5. Service anchored at workspaceCwd, not repo top-level (Low/Medium)

session.ts:1224 constructs GitWorktreeService(workspaceCwd) directly. isGitRepository() returns true inside subdirectories, so for a workspace registered at a monorepo subdir the worktree lands under <subdir>/.qwen/worktrees/ — exactly the scattering enter_worktree avoids by anchoring at getRepoTopLevel() (see the comment at gitWorktreeService.ts:283-294), and a location the top-level sweep won't find. Relatedly, the sidecar is written with originalCwd: workspaceCwd, while the field's documented contract (worktreeSessionService.ts:36-49) is "repo top-level, NOT the launch cwd" — exit_worktree builds its cleanup service from this field (exit-worktree.ts:428). And originalBranch: '' introduces a new sentinel; existing writers use 'HEAD'/detached markers. Suggest resolving top-level first (getRepoTopLevel() ?? workspaceCwd) and populating branch/commit from the values you already fetch for baseBranch.

6. GET /session/:id/status contract quietly changed (Low)

The rewrite (session.ts:1627-1658) drops the previous structured resolution: the ambiguous owner case now falls through to a plain 404, the 404 body loses its code, and the sidecar fallback is bound-workspace-only (SessionService(boundWorkspace)), so worktree sessions in secondary workspaces 404 after a restart while primary ones return data. The fallback body also violates the endpoint's declared DaemonSessionSummary type (only sessionId/workspaceCwd/worktree) — today the only consumer reads .worktree, but the SDK method is typed and documented as full live-runtime status. Consider returning the persisted summary shape (or a narrower, honestly-typed response) and keeping the workspace-scoped lookup.

7. Git pill dropdown is not keyboard-reachable (Low, a11y regression)

WorkspaceSection.tsx:364-372 puts DropdownMenuTrigger asChild on a plain <span>, and the inner GitBranchIndicator now renders its non-interactive <output> branch (no onOpenDiff). Radix merges handlers/aria onto the span but does not make it focusable — previously this pill was a real <button aria-label=…>. Keyboard users lose both the menu and the Changes dialog it now gates; mouse users go from one click to two for Changes. Making the trigger a real button (or forwarding tabIndex={0}/role="button") restores it.

Smaller notes

  • App.tsx:1336-1339 comment says enriched status "still queries the main workspace… needs daemon support (Phase 3)", but the code right below passes sessionWorktree?.path and the daemon support ships in this PR — stale, will misdirect the next reader.
  • worktree.isolatedBanner is defined in both locales but never referenced — dead key.
  • The sidecar enrichment loop is duplicated in session-list.ts (~:495 and ~:768) and reads sequentially, one file per listed session per request; hoist into a shared helper and Promise.all the page.
  • If spawn fails after createUserWorktree succeeds, the worktree+branch are orphaned with no cleanup until a sweep; a retry with auto-slug then creates a second one. Worth a removeWorktree in the spawn-failure path even before Phase 3.
  • Test coverage: route validation is well covered; missing are the session-list sidecar enrichment, the status-route sidecar fallback, ?cwd= on the git route (including the traversal cases in finding 2), and cd-failure-still-200. Per house rules these are Suggestions — except the finding-2 traversal case, which is worth a test with the fix.

Findings 1 and 4 are the ones I'd hold the merge for: they both end with the UI asserting isolation the session doesn't have, which is the one failure mode this feature must not exhibit. Everything else is incremental.

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean review, all tests green, verified end-to-end at this head. The 500+ production line threshold (624 lines across 6 packages) triggers the maintainer-awareness cap per policy, though the author is a maintainer and the core change is only 7 lines.

The PR does what it sets out to do — worktree-isolated sessions for the Web Shell — and does it cleanly. The design doc is thorough, the implementation reuses existing infrastructure (GitWorktreeService, changeSessionCwd), and the daemon route handles the full lifecycle transactionally (create → relocate → persist, with rollback on any failure). The security posture is solid: slug validation prevents path traversal, the git status ?cwd= parameter uses realpath containment, and worktree sessions force sessionScope: 'thread' to prevent coalescing.

My independent proposal matched the PR's approach almost exactly. The one thing I'd note is that enrichWorktreeSidecars does sequential file reads per session — fine now, but worth keeping an eye on if worktree sessions become common. The PR acknowledges this in the Risk section.

The deferred items (worktree cleanup on session end, merge-back workflow, worktree-scoped /diff) are appropriately scoped to Phase 3. The PR is honest about what it doesn't do.

Deferring to @wenshao for the maintainer-awareness sign-off on the 500+ production line threshold — the review itself found no blockers.

中文说明

置信度:3/5 — 审查干净,所有测试通过,在此 head 上端到端验证。500+ 生产行阈值(624 行跨 6 个包)触发维护者知悉上限(政策要求),尽管作者本身即维护者,且核心改动仅 7 行。

PR 干净地实现了目标——Web Shell 的 worktree 隔离 session。设计文档详尽,实现复用已有基础设施,daemon 路由事务性地处理完整生命周期。安全姿态扎实:slug 验证防止路径遍历,git 状态 ?cwd= 参数使用 realpath 包含检查,worktree session 强制 sessionScope: 'thread'

独立方案与 PR 方案几乎完全一致。唯一值得注意的是 enrichWorktreeSidecars 按 session 顺序读取文件——目前没问题,但如果 worktree session 普及后值得关注。

推迟项(session 结束时 worktree 清理、merge-back 工作流、worktree 范围 /diff)合理归入 Phase 3。

转交 @wenshao 进行 500+ 生产行阈值的维护者知悉确认——审查本身未发现阻塞问题。

Qwen Code · qwen3.7-max

Reviewed at 738b6aa945ad77d89a7eab36f9d50574ab0c0597 · re-run with @qwen-code /triage

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two blockers before this can land: (1) the ?cwd= parameter in workspace-git.ts has a confirmed path traversal — startsWith allows sibling-directory escape and ../ traversal; needs path.resolve() + trailing-separator containment. (2) Three unit tests are failing, including the PR's own new worktree test (fake bridge doesn't propagate worktree to the response) and two existing GET /session/:id/status tests broken by the route rewrite. The feature itself works end-to-end — verified live — but the security fix and test fixes are needed first. See detailed notes above. 🙏

@wenshao
wenshao force-pushed the feat/webshell-worktree-sessions branch from c93a8d2 to 46704c6 Compare July 19, 2026 10:21
@github-actions

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: build-and-test — full build and npm test timed out at 120 seconds; Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally. Not reviewed: coverage — could not read the agents' transcripts (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did).

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts
Comment thread packages/cli/src/serve/routes/session.ts
Comment thread packages/cli/src/serve/routes/session.ts
Comment thread packages/cli/src/serve/routes/session.ts Outdated
Comment thread packages/cli/src/serve/server/session-list.ts Outdated
Comment thread packages/cli/src/serve/server/session-list.ts Outdated
Comment thread packages/cli/src/serve/routes/workspace-git.ts Outdated
Comment thread packages/sdk-typescript/src/daemon/DaemonClient.ts
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/components/sidebar/WorkspaceSection.tsx Outdated
Add support for creating sessions in isolated git worktrees from the
Web Shell, enabling multiple tasks to run in parallel within the same
workspace without polluting the main working directory.

Daemon:
- POST /session accepts optional worktree param, creates worktree via
  GitWorktreeService, relocates session via changeSessionCwd
- Worktree metadata persisted in SessionEntry, BridgeSessionSummary,
  and sidecar file (<sessionId>.worktree.json) for daemon restart
  recovery
- GET /workspaces/:workspace/git supports ?cwd= for worktree-scoped
  git status queries (path.resolve + containment check)

SDK:
- CreateSessionRequest/DaemonSession/DaemonSessionSummary gain
  worktree field; DaemonSessionClient exposes worktree getter
- WorkspaceDaemonClient.workspaceGit() accepts optional cwd param

Web Shell:
- Workspace branch pill dropdown offers 'New Worktree Task' (git repos
  only) with purple GitForkIcon and description
- Git chip turns purple with GitForkIcon for worktree sessions
- Session list shows inline ⑂ badge for worktree sessions
- Empty-state welcome badge explains worktree isolation
- Git status queries target worktree path, not workspace root
- session_cwd_changed event filtered from chat transcript

Design doc: docs/design/2026-07-19-webshell-worktree-sessions.md
@wenshao
wenshao force-pushed the feat/webshell-worktree-sessions branch from 03c8a90 to 159002d Compare July 19, 2026 11:18

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: coverage — could not read the agents' transcripts (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did). [Critical] Worktree isolation still begins after ACP session initialization. The bridge creates the channel, config, and cwd-bound MCP transports with the registered main workspace before changeSessionCwd runs. — Failure scenario: a project MCP server or configuration resolved relative to cwd remains bound to the main checkout even after the session is labelled as isolated. Spawn with a validated initial worktree cwd before config/MCP initialization, while retaining the main workspace only as the runtime owner. [Critical] Registered workspaces that are repository subdirectories still lose their selected scope. The route constructs GitWorktreeService(workspaceCwd) directly and relocates to the worktree root. — Failure scenario: /repo/packages/app creates a full-repository checkout under that subdirectory and the session starts at the repository root, so relative tools operate outside packages/app. Resolve the repository root, preserve the relative subdirectory, and use the corresponding path inside the worktree. [Critical] Restart load/resume still ignores the worktree sidecar, and archived listing still reads only the active sidecar path. — Failure scenario: after a daemon restart, a worktree-badged session loads with the main workspace cwd and can edit the main checkout; after archive, even the badge disappears because the sidecar moved under chats/archive. Validate and pass archive-aware sidecar cwd/metadata through bridge restore before ACP initialization. [Critical] Worktree creation still has no daemon capability contract. — Failure scenario: a new SDK or Web Shell sends worktree to an older daemon; Express ignores the unknown field and returns a normal main-checkout session that the client accepts as isolated. Advertise and require a worktree_sessions capability before exposing or accepting this request, and fail closed when absent.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts
Comment thread packages/cli/src/serve/routes/session.ts
Comment thread packages/web-shell/client/App.tsx
Comment thread packages/web-shell/client/App.tsx
@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Re-review — verified at ae148f2 (work commit 159002d)

Re-read the updated stack and re-checked each round-1 finding against the code as it stands. Also ran the suites locally at this head: server.test.ts 760/760 pass (including the 5 worktree tests), web-shell WorkspaceSection.test.tsx 6/6 and sessionPreparation.test.ts 15/15 — the earlier CI report of failing tests looks resolved; the fakeBridge one-liner echoing worktree back from spawn was the missing piece.

Fixed

  • Finding 2 (?cwd= traversal) — fixed, and well. workspace-git.ts:65-79 now canonicalizes both sides with realpathSync and checks containment via path.relative — this closes the sibling-prefix collision, .. traversal, and symlink escape in one move. Out-of-bounds still falls back silently to the workspace root rather than 400ing, which I'd still prefer for client-bug visibility, but the security hole is gone. One ask remains from round 1: there is no test for the containment logic (in-bounds worktree path, .. escape, sibling prefix) — worth adding so the next refactor can't quietly regress it.
  • Finding 6 (status-route contract) — fixed by reverting to the live-only resolveLiveSessionRuntime implementation. Structured errors, ambiguous-owner handling, and the honest DaemonSessionSummary type are all back.
  • Round-1 note (orphaned worktrees on spawn failure) — fixed. The catch now rolls back via removeUserWorktree(slug, { deleteBranch: true }) (session.ts:1411-1419). Signature verified against core. Placement is right: cd failures are swallowed by the inner try, so the rollback only fires when the session never spawned.
  • Bonus: slug validation is stricter — {slug: 123} / {slug: ""} now 400 with worktree_invalid_slug instead of silently getting an auto-slug (session.ts:1239-1250). No test covers the new type-check branch, but the behavior is right.

Still open

  • Finding 1 (restore path) — unchanged, and now the restart story is strictly worse for the chip. bridge.ts:3972 still loads with cwd: workspaceKey, the restore-side createSessionEntry (bridge.ts:4087) still doesn't rehydrate worktree, and no changeSessionCwd is issued on load — the resumed session runs in the main checkout with only the one-shot model hint from restoreWorktreeContext. With the status-route sidecar fallback now removed, after a restart the purple chip can never come back (the web shell's sessionStatus only ever sees the live summary), while the sidebar list keeps showing ⑂ via the sidecar enrichment — which this round grew to three call sites (session-list.ts:495,625,783). Net effect: the PR body's test-plan step 7 ("Restart the daemon → reload the page → the ⑂ icon and purple git chip persist") is not reproducible at this head — the icon persists, the chip cannot. Two acceptable resolutions: (a) fix restore — read the sidecar in the load route, re-issue changeSessionCwd when the worktree directory still exists, and pass worktree into the restore-side createSessionEntry (both plumbing points already exist, this is a small localized change); or (b) rescope honestly — drop the restart claim from the PR body, and stop showing the ⑂ badge for non-live sessions so the list doesn't advertise isolation the resumed session won't have. I'd take (a); (b) at minimum.
  • Finding 4 (cd failure still reports a worktree session) — unchanged (session.ts:1395-1407): warn-and-continue, response and bridge entry keep full worktree metadata, purple chip on a session running in the main checkout. Same dishonest-UI class as finding 1, reachable without a restart. Clearing worktree from the response (and entry) on cd failure is a few lines.
  • Finding 3 (WorkspaceGitState watcher leak) — unchanged. The realpath fix bounds it to existing in-workspace directories, but every worktree session still permanently installs one fs watcher keyed by its worktree path (workspace-git-state.ts:94-104,124), never disposed — including after the worktree directory is deleted.
  • Finding 5 (anchoring + sidecar contract) — unchanged. session.ts:1223 still anchors at workspaceCwd (monorepo-subdir workspaces scatter .qwen/worktrees/ and violate originalCwd's documented repo-top-level contract, worktreeSessionService.ts:36-49), and originalBranch: '' (session.ts:1391) remains a novel sentinel where existing writers use 'HEAD'.
  • Finding 7 (a11y) — not fixed; now codified by a test. WorkspaceSection.test.tsx was updated to assert the chip is a non-interactive OUTPUT inside the dropdown trigger and the click-opens-diff assertion was deleted. The keyboard gap is unchanged: Radix's asChild on a plain <span> gets handlers and aria but no focusability, so keyboard users cannot open the menu (previously this was a real <button>). The test change makes the regression look intentional — if it is, it deserves a comment saying why; if not, the trigger needs tabIndex/button semantics before the test enshrines it.

New this round

  • App.tsx:7273 now disables the Changes dialog for worktree sessions (gitDiffWorkspaceCwd && !sessionWorktree). Defensible stopgap — the dialog would have shown the main workspace's diff, which is worse than nothing — but combined with finding 7 the purple chip is now fully inert, and the PR body doesn't mention the affordance removal (it only scopes out a "worktree-scoped /diff dialog"). Worth a line in Risk & Scope.
  • The sidecar enrichment block is now copy-pasted three times in session-list.ts (round 1 suggested hoisting the two existing copies into a shared helper and Promise.all-ing the page; the count went up instead). Still one serial readFile per listed session per request, now on three paths.
  • Round-1 smaller notes still standing: stale comment at App.tsx:1340-1342 (contradicts the code passing sessionWorktree?.path right below), dead i18n key worktree.isolatedBanner.

Merge stance

The security blocker is resolved and the tests are green. What keeps me from approving is unchanged from round 1: findings 1 and 4 both end with the UI asserting isolation the session doesn't have — the one failure mode this feature must not exhibit. Fix 4 (small) and either fix or honestly rescope 1 (including the test-plan step 7 claim), and the rest can follow up.

@wenshao
wenshao force-pushed the feat/webshell-worktree-sessions branch from ae148f2 to 9e969ad Compare July 19, 2026 12:43

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts Outdated

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent on record was launched with it.

Not reviewed: Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it.

Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent on record was launched with it.

Not reviewed: Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it.

Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.

[Critical] session.ts !res.writable path: client disconnect during spawn orphans worktree directory and branch — the disconnect-reap block kills the session but does not call removeUserWorktree. The catch-block rollback is unreachable from the return inside try. No sidecar is written, so the UI has no record. Repeated triggers accumulate orphaned directories that the agent-* stale cleanup won't collect.

[Critical] App.test.tsx: mock workspaceClient lacks sessionStatus method — App.tsx:1309 calls workspace.client.sessionStatus(sid) in a useEffect, but the test mock only has workspaceByCwd. This throws TypeError synchronously, causing 128 of 136 tests to fail during effect mounting.

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/serve/routes/workspace-git.ts
Comment thread docs/design/2026-07-19-webshell-worktree-sessions.md
Comment thread packages/cli/src/serve/routes/session.ts
@wenshao
wenshao force-pushed the feat/webshell-worktree-sessions branch from 9e969ad to 5c8e610 Compare July 19, 2026 13:09
@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

R3/R4 fixes (5c8e610)

Critical fixes:

  1. cd failure is now transactional — kill session + removeUserWorktree + return 500. No stale worktree metadata in bridge entry.
  2. Client disconnect path — the !res.writable reap block now calls removeUserWorktree when a worktree was created.
  3. App.test.tsx mock — added sessionStatus to the workspace client mock. Note: the 128 App.test.tsx failures are pre-existing from the origin/main merge (verified: git stash + run = same 128 failures on the base). The merge brought in new code that breaks the test setup independently of this PR.

Suggestions acknowledged:

  • Dropdown item click tests (Changes / New Worktree Task): valid, will add in follow-up.
  • ?cwd= containment tests: valid, will add in follow-up.
  • Design doc pseudocode missing try/catch: valid, will update.
  • Empty originalBranch/originalHeadCommit in sidecar: valid, will populate from getCurrentBranch()/getCurrentCommitHash().

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts
@wenshao
wenshao force-pushed the feat/webshell-worktree-sessions branch from 5c8e610 to f14ed8a Compare July 19, 2026 13:23

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: chunk 1, chunk 2, chunk 3, chunk 4, chunk 5, chunk 6 — no agent reported covering these; nobody read them. Not reviewed: every dimension — none of the 11 required agents is on record as launched with a prompt this skill built, so this diff was reviewed, if at all, from prompts the run wrote for itself: no record shows the severity bar, the finding format or this project's own rules reaching an agent. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.

— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Downgraded from Request changes to Comment: self-PR; CI failing: review-pr, Test (ubuntu-latest, Node 22.x). Reviewed. Suggestions are inline. 6 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

— qwen3.8-max-preview via Qwen Code /review

Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/cli/src/serve/routes/session.ts Outdated
Comment thread packages/web-shell/client/components/sidebar/WorkspaceSection.tsx Outdated
Comment thread packages/webui/src/daemon/session/actions.ts
Comment thread packages/cli/src/serve/routes/session.ts
Comment thread packages/cli/src/serve/server/session-list.ts Outdated
Comment thread packages/web-shell/client/App.tsx Outdated
Comment thread packages/web-shell/client/components/sidebar/WebShellSidebar.tsx
Comment thread packages/web-shell/client/i18n.tsx Outdated
Comment thread packages/cli/src/serve/routes/workspace-git.ts

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts Outdated

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts Outdated
@wenshao
wenshao force-pushed the feat/webshell-worktree-sessions branch from f14ed8a to 9147152 Compare July 19, 2026 13:50

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read. [Critical] The timeout behavior is not hypothetical: changeSessionCwd assigns the raw cdPromise settlement to entry.promptQueue and explicitly says the timeout is caller-facing only; withTimeout is only Promise.race([p, timeoutP]) and never aborts p. Therefore its caller can catch BridgeTimeoutError while the ACP sessionCd operation continues. Removing the worktree in that catch while killSession returned false can race the surviving session's late cd into the deleted path. Independently, if ACP eventually rejects, the live attached entry remains in the main checkout while its summary still advertises the removed worktree. The rollback still needs to settle/cancel relocation and transfer live-session ownership before filesystem cleanup.

— GPT-5 via Qwen Code /review

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: full review fan-out — unchanged head; this monitoring pass inspected the exact CI failure and PR replies only. Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.

— GPT-5 via Qwen Code /review

Comment thread packages/web-shell/client/App.test.tsx Outdated
@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough re-review. All confirmed fixes noted.

Restart limitation: Now explicitly documented in the PR body's Risk & Scope section as the first Phase 3 item, with the specific restore mechanism described (read sidecar → re-issue changeSessionCwd → populate entry.worktree).

Non-blocking notes acknowledged:

  • Status-shaping duplication: will extract a shared helper if a third site appears.
  • Symlinked runtime.workspaceCwd micro-efficiency: noted, response is correct either way.
  • 🔵 minor items (empty sidecar fields, relative ?cwd=, sequential reads, test gaps, doc drift): tracked for follow-ups.

@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Description re-check (body edit @ 16:35Z)

Verified the new Risk & Scope bullet against the code at d3f173b0d: the restart-limitation disclosure is accurate — effective cwd falls back to the main workspace root with only the restoreWorktreeContext model-text hint, the purple chip doesn't come back, the sidebar ⑂ badge persists via sidecar enrichment, and the stated restore mechanism (read sidecar → re-issue changeSessionCwd → populate entry.worktree) is exactly the right Phase 3 shape. That resolves my remaining significant concern about undocumented degradation.

Two leftover inconsistencies in the description itself:

  1. Test-plan step 7 now contradicts Risk & Scope. Step 7 still says "after restart, click the session to load it and the chip restores" — but the new Risk & Scope bullet (correctly) says the chip "does not restore until the session is re-created", and the code agrees. Step 7's note should be updated to match, otherwise a reviewer following the test plan will report a failure on a documented limitation.
  2. The Chinese <details> block wasn't updated. Its 风险与范围 section still carries the old three-item out-of-scope list with no mention of the restart limitation, and the daemon bullet's「daemon 重启后不丢失」reads stronger than the English now qualifies. The folded Chinese section should mirror the English edit.

No new code since d3f173b0d, so the round-2 verification stands: 157/157 web-shell + 760/760 cli serve locally, CI Test/Serve A/B/visuals green.

🤖 Generated with Claude Code — Claude Fable 5

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

The stage comments above were updated with the latest result. View workflow run.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Downgraded from Request changes to Comment: self-PR; CI failing: review-pr, Test (ubuntu-latest, Node 22.x). Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. [Critical] cd-failure rollback timeout race (re-checked thread 3610684625 / review 4730879851; trigger narrow, mechanism verified in code). changeSessionCwd's timeout is caller-facing only — bridge.ts ties entry.promptQueue to the RAW cd settlement ("Timeout is caller-facing only"), so a timed-out cd can still complete in the child after this catch runs. If a concurrent attach makes killSession return false and the raw cd then completes, the surviving session's cwd becomes the worktree path that removeUserWorktree just force-deleted, and files the agent writes there are lost. The thread reply's "if the Promise rejects, the cd did not complete" does not hold for the Promise.race timeout path — the race rejects on the timer while the cd is still in flight. Gate the removal on killed === true (mirroring removeSession), or distinguish a timeout from a definitive rejection before deleting. [Critical] Five previously filed blockers remain present in the code at this head (author-acknowledged, tracked for Phase 3): (1) registered workspaces that are repository subdirectories lose their selected scope — relocation targets the worktree root, so relative tools operate at repo scope instead of the selected packages/app scope; (2) session config and cwd-bound MCP transports initialize against the main workspace before changeSessionCwd runs, so a cwd-relative project MCP server stays bound to the main checkout while the session is labelled isolated; (3) load/resume never reads the worktree sidecar (no readWorktreeSession on the restore routes), so after a daemon restart a ⑂-badged session loads with the main workspace cwd and can edit the main checkout; (4) no daemon capability contract — an older daemon silently ignores the worktree field and returns a main-checkout session the client accepts as isolated (no fail-closed preflight); (5) the /diff slash command (App.tsx ~4565) is not gated for worktree sessions and opens the main checkout's dialog, while the footer-click guard at App.tsx ~7273 is.

— qwen3.8-max-preview via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts
Comment thread packages/web-shell/client/App.test.tsx Outdated
Comment thread packages/cli/src/serve/routes/session.ts

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

— qwen3.8-max-preview via Qwen Code /review

Comment thread docs/design/2026-07-19-webshell-worktree-sessions.md Outdated
Comment thread packages/web-shell/client/components/sidebar/WorkspaceSection.tsx
Comment thread packages/web-shell/client/App.tsx Outdated
@wenshao
wenshao force-pushed the feat/webshell-worktree-sessions branch from d3f173b to 738b6aa Compare July 19, 2026 17:32

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Downgraded from Request changes to Comment: self-PR; CI still running. Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. [Critical] Re-checked, still standing: stale bridge worktree metadata when killSession returns false (session.ts:1421–1426; existing thread comment 3611002925). The cd-failure rollback is unchanged at this head: when killSession({ requireZeroAttaches: true }) returns false (a concurrent restoreSession attach — scope-agnostic, bridge.ts ~3817 — or the .catch(() => false) on throw), the worktree is deleted but the surviving entry keeps entry.worktree, and GET /session/:id/status keeps advertising isolation the session doesn't have. The sessionScope = 'thread' forcing prevents spawn-time coalescing but does not prevent attach-by-id via the session listing during the spawn→cd window. Clear the entry's worktree metadata on the surviving-session branch (needs a small bridge method), or defer setting it until the cd succeeds. [Critical] Re-checked, still standing (narrow trigger): the cd timeout race in the same rollback (existing thread 3610684625 / review 4730879851, and the previous review body 4731162403). changeSessionCwd's timeout is caller-facing only — bridge.ts ties entry.promptQueue to the RAW cd settlement ("Timeout is caller-facing only") — so a timed-out cd can still complete in the child after this catch has force-deleted the worktree; with a concurrent attach keeping killSession false, the surviving session's cwd becomes the deleted path and files written there are lost. Gate the removal on killed === true (mirroring removeSession), or distinguish a timeout from a definitive rejection before deleting. [Critical] Re-checked, still standing (author-acknowledged, tracked for Phase 3; the restart limitation is now declared in the PR body's Risk & Scope): (1) subdirectory workspace scope is lost after relocation (session.ts:1224 — relocation targets the worktree root); (2) session config and cwd-bound MCP transports initialize against the main workspace before changeSessionCwd runs; (3) load/resume never reads the worktree sidecar, so after a daemon restart a ⑂-badged session runs in the main checkout (the listing badge is now archive-state aware and survives archiving — that sub-part is fixed at this head); (4) no daemon capability contract — an older daemon silently ignores the worktree field and returns a main-checkout session the client accepts as isolated; (5) the /diff slash command (App.tsx ~4565) is not gated for worktree sessions while the footer click is.

— qwen3.8-max-preview via Qwen Code /review

Comment thread packages/cli/src/serve/routes/workspace-git.ts
Comment thread packages/cli/src/serve/routes/workspace-git.ts
Comment thread packages/cli/src/serve/routes/session.ts

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

— qwen3.8-max-preview via Qwen Code /review

Comment thread packages/cli/src/serve/routes/session.ts
@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

The stage comments above were updated with the latest result. View workflow run.

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. 高质量实现,增量 review 无 Critical 缺陷。

核心确认:

  • POST /session worktree 创建 + changeSessionCwd 重定位链路正确,spawn/cd 失败均有完整回滚
  • ?cwd= 路径安全检查(realpathSync + path.relative containment check)到位
  • 强制 sessionScope: 'thread' 防止 worktree session 被 attach 到普通 session
  • enrichWorktreeSidecars 从 sidecar 文件恢复元数据,daemon 重启后徽章存活
  • Web Shell 侧 git chip/session 列表/welcome badge/dropdown 全部覆盖
  • 4 个 server 测试 + WorkspaceSection 测试更新

一个小观察(非阻塞):worktree session 禁用了 workspace 下拉菜单的 'Changes' 入口,用户无法从 UI 查看 worktree 内 diff(需走 /diff 命令),建议后续增量支持。

— qwen3.7-plus via Qwen Code /review

@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@wenshao
wenshao added this pull request to the merge queue Jul 19, 2026
Merged via the queue into QwenLM:main with commit 0c27165 Jul 19, 2026
258 of 260 checks passed
@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Local end-to-end verification ✅ (with one restart-persistence gap ⚠️)

I built this branch and verified it against a real qwen serve daemon bound to a real git repo — no mocking of GitWorktreeService (the added unit tests in server.test.ts mock it entirely, so real worktree creation, on-disk isolation, sidecar persistence and restart recovery were previously unverified). UI was checked in a headless browser against the production web-shell/dist served by the daemon, rendering live data.

Bottom line: the core feature is solid and safe. The only gap is restart persistence — after a daemon restart the worktree session disappears from the session list entirely, which contradicts Test-Plan step 7.

Environment: feat/webshell-worktree-sessions @ 738b6aa (merge with main) · Linux · Node 22 · npm ci && npm run build · real packages/cli/dist daemon on 127.0.0.1 + packages/web-shell/dist · mock OpenAI endpoint to drive turns so transcripts persist.

What passed — 11/12 real-daemon checks

Area Check Result
Create POST /session {worktree:{slug:"alpha"}}200 + {slug,path,branch} worktree-alpha
Create worktree dir created under <repo>/.qwen/worktrees/alpha, real checkout, .qwen-session marker = sessionId
Create git worktree list registers branch worktree-alpha; sidecar <sid>.worktree.json written
Isolation edit + git add inside worktree → main workspace src/app.ts unchanged, main tree clean
Git scoping GET /workspaces/:ws/gitmain; ?cwd=<worktree>worktree-alpha (staged:3)
Security ?cwd=/etc, /tmp, .., sibling dir → all fall back to workspace root (containment holds)
Validation worktree=[] / "yes"400 invalid_worktree
Validation slug ../escape, "", a/b400 worktree_invalid_slug
Validation non-git workspace → 400 worktree_not_git_repo; {} auto-slug → 200
Scope worktree session forced to sessionScope:'thread'; changeSessionCwd relocates the child (transcript records cwd=<worktree>, gitBranch=worktree-alpha)
UI (live) purple ⑂ sidebar badge on worktree sessions only · "New worktree task" in the workspace pill (git repos only) · composer chip → purple data-worktree="true" with the worktree branch when loaded

Web Shell worktree indicators

The one gap — restart persistence (Test-Plan step 7 / Risk note) ⚠️

The PR states: "Restart the daemon → the ⑂ icon persists in the session list (via sidecar file)" and "The sidebar ⑂ badge persists via sidecar enrichment."

Observed: after a daemon restart the entire worktree session row disappears from the list — not just the purple chip. In a clean single-session workspace the list returns total=0 after restart, even though the sidecar file is still on disk.

Restart drops the worktree session

Root cause: changeSessionCwd relocates the child, so the persisted transcript records cwd = <repo>/.qwen/worktrees/<slug>. Every listing path in SessionService filters with sessionBelongsToCurrentProject(sessionId, firstRecord.cwd) (packages/core/src/services/sessionService.ts:420 and siblings). But:

getProjectHash(<repo>)                       = fa2c7d96…
getProjectHash(<repo>/.qwen/worktrees/alpha) = 8c28c00f…   ← different project

so the worktree session fails the membership check; after a restart there is no live runtime-status fallback either, so it is filtered out before the new sidecar-enrichment loop in session-list.ts ever runs. The sidecar the PR writes is therefore never consulted for the case it was written for — the enrichment is effectively dead code on the restart path. (This is also worse than the Risk note, which says only the git chip/cwd degrade and the ⑂ badge survives.)

Suggested direction (non-blocking): enrich/keep worktree sessions using the sidecar or an index rather than gating on recordCwd project membership — e.g. treat a session whose sidecar originalCwd matches the workspace root as belonging to the project and skip the recordCwd re-check for it.

Minor observations (non-blocking)

  • The daemon-created sidecar always writes originalBranch:"" and originalHeadCommit:"" (hard-coded empty in POST /session), unlike the EnterWorktreeTool path that populates them. Harmless in the web-shell today, but WorktreeExitDialog's commit-count would read "unknown" for these sessions.
  • Creating a worktree makes the parent repo report .qwen/ as untracked (untracked:1 in the git chip) unless .qwen/ is git-ignored.

Verdict

Isolation, ?cwd= containment, and every validation gate behave correctly on a real daemon, and all three UI indicators render from live data. The single functional gap is that the advertised restart-persistence of the ⑂ mark doesn't work — the row vanishes from the list. Since full restart-isolation is already scoped to Phase 3, either fix the list-drop (small) or soften the Test-Plan/Risk wording so it doesn't claim the badge persists.

Reproduction

Real-daemon E2E harness output (GitWorktreeService not mocked):

== 1. POST /session {worktree:{slug:alpha}} ==
  PASS response carries worktree.branch (worktree-alpha)
  PASS worktree dir created on disk
  PASS git registered the worktree branch
  PASS sidecar <sid>.worktree.json written
== 2. isolation: edit inside worktree, main stays clean ==
  PASS main workspace src/app.ts unchanged (export const x=1)
== 3. GET /git ?cwd scoping ==
  PASS no cwd -> main branch (main)
  PASS cwd=worktree -> worktree branch (worktree-alpha)
  PASS cwd=/etc containment -> falls back to main (main)
== 4. validation gates ==
  PASS worktree=[] -> invalid_worktree
  PASS slug=../escape -> worktree_invalid_slug
== 5. drive one turn so the session persists a transcript ==
  PASS worktree session listed BEFORE restart (1)
== 6. RESTART daemon, re-list ==
  sidecar file still on disk: yes
  sessions listed after restart: total=0  with-worktree=0  (expected by PR: with-worktree=1)
  FAIL worktree session dropped after restart

==== SUMMARY: 11 passed, 1 failed ====
🇨🇳 中文版

本地端到端验证 ✅(存在一处重启持久化缺陷 ⚠️

我构建了本分支,并针对真实 qwen serve daemon + 真实 git 仓库做了验证——没有 mock GitWorktreeServiceserver.test.ts 新增的单测把它整体 mock 了,因此真实的 worktree 创建、磁盘隔离、sidecar 持久化和重启恢复此前都未被验证)。UI 部分在无头浏览器中针对 daemon 提供的生产 web-shell/dist 检查,渲染的是真实数据。

结论: 核心功能扎实且安全。唯一缺陷是重启持久化——daemon 重启后,worktree 会话会从会话列表中整行消失,这与测试计划第 7 步的说明相矛盾。

环境: feat/webshell-worktree-sessions @ 738b6aa(已合并 main)· Linux · Node 22 · npm ci && npm run build · 真实 packages/cli/dist daemon(127.0.0.1)+ packages/web-shell/dist · 用 mock OpenAI 驱动一次对话以持久化 transcript。

通过项——真实 daemon 11/12 项检查

  • 创建: POST /session {worktree:{slug}}200 且返回 {slug,path,branch};在 <repo>/.qwen/worktrees/<slug> 下创建真实 worktree(含 .qwen-session 标记 = sessionId);git worktree list 注册 worktree-<slug> 分支;写入 sidecar <sid>.worktree.json。✅
  • 隔离: 在 worktree 内修改并 git add → 主 workspace 的 src/app.ts 未变、工作树干净。✅
  • git 作用域: GET /workspaces/:ws/gitmain?cwd=<worktree>worktree-<slug>(staged:3)。✅
  • 安全(路径包含校验): ?cwd=/etc/tmp..、同级目录 → 全部回退到 workspace 根目录,未逃逸。✅
  • 校验门: worktree=[]/字符串 → 400 invalid_worktree;slug ../escape、空串、a/b400 worktree_invalid_slug;非 git 仓库 → 400 worktree_not_git_repo{} 自动 slug → 200。✅
  • 会话作用域: worktree 会话强制 sessionScope:'thread'changeSessionCwd 把子进程重定位到 worktree(transcript 记录 cwd=<worktree>gitBranch=worktree-<slug>)。✅
  • UI(实时数据): 侧边栏 worktree 会话显示紫色 ⑂ 徽标(普通会话无)· workspace pill 下拉出现"New worktree task"(仅 git 仓库)· 载入 worktree 会话后 composer git chip 变紫、data-worktree="true" 并显示 worktree 分支名。✅

唯一缺陷——重启持久化(测试计划第 7 步 / 风险说明)⚠️

PR 声称:"重启 daemon → ⑂ 图标通过 sidecar 文件在会话列表中保留""侧边栏 ⑂ 徽标通过 sidecar enrichment 保留"

实测: daemon 重启后,worktree 会话整行从列表消失——不仅仅是紫色 chip。在只有一个会话的干净 workspace 中,重启后列表返回 total=0,尽管 sidecar 文件仍在磁盘上。

根因: changeSessionCwd 重定位了子进程,因此持久化的 transcript 记录 cwd = <repo>/.qwen/worktrees/<slug>SessionService 的所有列表路径都会用 sessionBelongsToCurrentProject(sessionId, firstRecord.cwd) 过滤(packages/core/src/services/sessionService.ts:420 等)。但 getProjectHash(<repo>)fa2c7d96…)≠ getProjectHash(<worktree>)8c28c00f…),属于不同 project;重启后又没有存活的 runtime-status 兜底,因此该会话在本 PR 新增的 session-list.ts sidecar-enrichment 循环运行之前就被过滤掉了。也就是说,本 PR 写入的 sidecar 在它本该服务的重启场景里从未被读取——enrichment 在重启路径上形同废码。(这也比风险说明更严重:说明称仅 git chip/cwd 退化、⑂ 徽标会保留。)

修复方向(非阻塞): 用 sidecar 或独立索引来保留/补全 worktree 会话,而不是以 recordCwd 的 project 归属为前置条件——例如:当某会话的 sidecar originalCwd 等于 workspace 根目录时,即视为属于该 project,并对其跳过 recordCwd 复核。

次要观察(非阻塞)

  • daemon 创建的 sidecar 总是写 originalBranch:""originalHeadCommit:""POST /session 里硬编码为空),与 EnterWorktreeTool 路径不同。目前 web-shell 无害,但 WorktreeExitDialog 的提交计数对这些会话会显示"未知"。
  • 创建 worktree 会让父仓库把 .qwen/ 报为未跟踪(git chip 中 untracked:1),除非 .qwen/ 被 gitignore。

结论

隔离、?cwd= 包含校验、所有校验门在真实 daemon 上均正确,三处 UI 标识也都基于实时数据渲染。唯一功能缺陷是宣称的 ⑂ 标记重启持久化不生效——整行从列表消失。鉴于完整的重启隔离已划入 Phase 3,建议要么修掉这个"列表丢失"(改动很小),要么弱化测试计划/风险说明的措辞,不再声称徽标会保留。


Verified locally on Linux against a real qwen serve daemon + real git repo (no GitWorktreeService mocking) and the production web-shell in a headless browser.

@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough real-daemon E2E verification. The root cause analysis is spot-on:

getProjectHash(<repo>)getProjectHash(<worktree>)SessionService.listSessions filters by sessionBelongsToCurrentProject(sessionId, firstRecord.cwd) before the sidecar enrichment loop runs, so worktree sessions are dropped before enrichment can rescue them. The sidecar enrichment in session-list.ts is indeed dead code on the restart path.

Fix: #7262 — adds a third membership check in sessionBelongsToCurrentProject (sidecar originalCwd matching the workspace project hash) and restores worktree isolation on session load/resume (changeSessionCwd + setSessionWorktree).

Minor observations acknowledged:

  • Empty originalBranch/originalHeadCommit in sidecar: will populate from getCurrentBranch()/HEAD.
  • .qwen/ untracked: ensureWorktreesGitignored() already writes .qwen/.gitignore with worktrees/, but the .qwen/ directory itself may show as untracked if not in the repo root .gitignore.

yiliang114 pushed a commit to yiliang114/qwen-code that referenced this pull request Jul 21, 2026
…wenLM#7375)

An empty visual preview means one of two opposite things: the change genuinely
moves no pixel, or no scenario renders the UI it touches. The bot printed the
same green check for both, so the second — a coverage gap, where the preview
literally cannot see the feature — read as a clean bill of health.

That has now happened three times (QwenLM#7035 primary label, QwenLM#7221 worktree badge,
QwenLM#7365 empty-state toggle), each caught only because a maintainer noticed the
missing image and asked. The signal to tell them apart was already there and
unused: the render workflow only runs when the web-shell client or webui source
changed, so an empty preview is by construction "UI code changed, nothing
rendered differently".

When no view changed, look at which files the PR touched. If any are
render-shaping (.tsx / .css / .svg under the rendered surface, excluding test
and scenario code), list them and say the result is ambiguous, with a pointer
to where a scenario goes. Otherwise keep the green check — a logic-only PR with
no visual delta is expected, and prompting there would train everyone to ignore
the prompt when it matters. The path list comes from the PR files API in the
privileged publish job, which never checks out PR code; if that call fails the
comment falls back to the current wording.

Co-authored-by: wenshao <wenshao@example.com>
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.20.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants