fix(web-shell): isolate worktree session execution - #8068
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
8cf1164 to
2f4ef7c
Compare
|
Thanks for the PR! Template looks good ✓ Problem: this is an observed bug, not theoretical hardening. The description carries a concrete before/after reproduction — two concurrent worktree sessions whose direct shell commands and implicit stdio MCP processes both land in the base checkout, overwriting the same Direction: aligned. Concurrent Web Shell worktree sessions sharing the base workspace cwd is a correctness bug in the daemon's multi-session model — built-in file tools already isolate per worktree, but direct shell and implicit MCP did not. This closes that gap. No CHANGELOG reference needed; the area is core to serve/worktree. Size: touches core paths ( Approach: the scope feels right, and it matches what I'd propose independently — track a per-session effective cwd and serialize direct shell behind in-flight cd changes; stamp the session cwd onto implicit stdio MCP servers centrally (so it joins the pool fingerprint and a relocation rebinds them); defer MCP discovery for a fresh worktree session until its trusted relocation; reconcile MCP on directory change. Consolidating six inline Risk: elevated — Stage 1e matched high-risk paths Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是已观测到的 bug，不是理论性加固。描述里给出了具体的 before/after 复现——两个并发 worktree session 的 direct shell 和隐式 stdio MCP 进程都落在主 checkout，覆盖同一个 方向：对齐。并发 Web Shell worktree session 共享主 workspace cwd 是 daemon 多 session 模型中的正确性 bug——内置 file tool 已经按 worktree 隔离，但 direct shell 和隐式 MCP 没有。本 PR 补上了这个缺口。 规模：触及核心路径（ 方案：范围合理，与我独立想到的方案一致——为每个 session 跟踪有效 cwd，并把 direct shell 串行化到正在进行的 cd 之后；集中式地把 session cwd 打到隐式 stdio MCP server 上（使其进入 pool fingerprint，relocation 时重新绑定）；新建 worktree session 在可信 relocation 完成前延迟 MCP discovery；目录变更时 reconcile MCP。把六处内联的 风险：升级——Stage 1e 命中高风险路径 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
E2E Test Report — concurrent worktree session isolationResult: PASS Tested the rebased PR branch at The daemon created two Session isolation observations
The mock MCP configuration did not provide Base workspace invariantsThe two worktree refs moved independently while Automated checksAdditional checks:
Full CLI typecheck is not reported as passing because the branch has pre-existing missing channel module and Ink type errors unrelated to this PR. Cleanup
|
Code reviewI wrote my independent proposal from the title + "Why it's needed" before reading the diff: per-session effective cwd with direct shell serialized behind in-flight cd changes, central cwd-stamping on implicit stdio MCP servers so a relocation rebinds the pool, worktree discovery deferral until trusted relocation, and MCP reconcile on directory change. The PR lands essentially that design, and the consolidation of six inline No critical blockers. The load-bearing logic reads correct:
Two non-blocking observations:
sequenceDiagram
participant P1 as Bridge
participant P2 as QwenAgent
participant P3 as Config
participant P4 as McpClientManager
P1->>P2: newSession, deferMcpDiscovery true
P2->>P3: initialize, skipMcpDiscovery true
Note over P3: no MCP process spawned in base checkout
P1->>P2: sessionCd after worktree relocation
P2->>P3: relocateWorkingDirectory, newDir
P3->>P3: stamp implicit stdio cwd with newDir
P3->>P4: discoverAllMcpToolsIncremental
P4-->>P2: mcpRefreshError if reconcile fails
P2-->>P1: warnings surfaced to caller
Files changed (8 of 14 shown)
TestingCI signal for the reviewed commit (fetched via the API; I do not run PR code):
The ubuntu unit suite and the web-shell E2E smoke are green; macOS/Windows tests and the no-sandbox integration suite are skipped (conditional on this trigger), not failed — no red checks, so no log excerpt to quote. The The author's before/after table (two concurrent daemon sessions isolated per worktree, base HEAD unmoved, MCP spawn cwd per worktree) is the author's own manual macOS run — attributed as their claim, not independently re-run here. Sandboxed verification would settle the remaining gap: 中文说明代码审查：我在读 diff 前先独立写了方案——按 session 跟踪有效 cwd、direct shell 串行化到在途 cd 之后、集中式给隐式 stdio MCP server 打 cwd 以便 relocation 重新绑定 pool、worktree 延迟 discovery、目录变更时 reconcile MCP。PR 基本就是这个设计，且把六处内联 无关键阻塞。承重逻辑正确：cwd 打标的谓词正确排除了 remote/tcp/sdk/显式 cwd 且不再原地修改入参；direct shell 的 abort 竞态健全（失败的 cd 不会让 shell 抛错，而是在上一个 cwd 运行，被中止的命令立即返回）； 两点非阻塞观察：(1) 测试：通过 API 读取本 commit 的 CI——ubuntu 单测与 web-shell E2E smoke 通过；macOS/Windows 与无沙箱集成测试为 skipped（该触发下条件性跳过）而非失败，无红色检查。作者的 before/after 并发隔离表是其本人在 macOS 上的手动运行结果，作为作者声明引用，未在此独立复现。沙箱验证可补上剩余缺口： — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — solid, well-tested fix that matches my independent design and ships green CI; one point off because the headline concurrent-isolation behaviour rests partly on the author's manual macOS run, and the PR sits on the high-risk Stepping back: this solves a problem users actually hit — concurrent worktree sessions clobbering the base checkout — and the reproduction is concrete, not hypothetical. The code is straightforward rather than clever: a per-session effective cwd, a promise tail to keep shell from overtaking cd, and a single source of truth for the cwd-stamped server map. Every change in the diff is in service of the stated goal; the only thing approaching scope creep is the shortening of a few pre-existing comments during the My independent proposal and the PR converged, so I'm not approving for lack of a better alternative — I'm approving because the design is the right one and the execution is clean. The two reservations I'd leave for the maintainer are genuine but non-blocking: confirm that single-session cds are serialized upstream so the un-chained Approving, pinned to the reviewed commit. ✅ 中文说明置信度 4/5——扎实、测试充分的修复，与我独立的设计一致，CI 全绿。扣 1 分是因为"并发隔离"这一核心行为部分依赖作者的手动 macOS 运行，且 PR 落在高风险的 mcp-client / acp-integration 路径上，沙箱 回顾：这解决的是用户真实会遇到的问题——并发 worktree session 覆盖主 checkout——复现具体而非假设。代码直白而非炫技：按 session 的有效 cwd、一条防止 shell 超车 cd 的 promise 尾、以及 cwd 打标后 server map 的唯一真源。diff 中每处改动都服务于既定目标；唯一接近范围蔓延的是抽取 我的独立方案与 PR 收敛，所以不是因为"没有更好方案"才通过——而是因为设计正确、执行干净。留给维护者的两点保留是真实但非阻塞的：确认单 session 的 cd 在上游已串行化，使未链式衔接的 已批准，固定在被审查的 commit 上。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
|
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）为单个提交。 |
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. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
CI failure diagnosed and fixed in |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action neededNo code changes were made this round. The feedback newer than the last evaluation contains no actionable items:
The two issue-level comments are informational only:
Nothing requires a response or a code change. The PR stands as-is. 中文说明Autofix 审查轮次 —— 无需处理本轮未做任何代码改动。自上次评估以来的新反馈中没有任何可处理的项目：
两条 issue 级别的评论仅为信息性内容：
无需任何回复或代码改动。PR 保持现状。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：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
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3679387279]
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：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
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedBoth inline findings from the automated reviewer are addressed in code. [rc:3679531702] Shell command could park forever on a hung cwd change — FIXED
Fix: race the cwd queue against the abort signal, then return the aborted [rc:3679531697] No test covered the abort-after-cwd-wait path — FIXEDAdded Conflict notes
Verification
中文说明已处理的审查反馈自动化审查者的两条行内发现均已在代码中处理。 [rc:3679531702] shell 命令可能永远卡在一个挂起的 cwd 切换上 — 已修复
修复：让 cwd 队列与中止 signal 做竞争，当 signal 已中止时返回 aborted 哨兵值。 [rc:3679531697] 没有测试覆盖「等待 cwd 切换后被中止」这条路径 — 已修复新增测试 冲突说明
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 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
Extract McpClientManager.getEffectiveMcpServers() as the single source of truth for the populateMcpServerCommand recipe, replacing six duplicated call sites that this PR otherwise had to edit in lockstep. Also cover relocateWorkingDirectory's combined memory + MCP refresh failure path so both errors stay surfaced.
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8068Both inline suggestions from the automated reviewer were addressed in code. No Feedback points
Conflict notesNone — the branch was not in conflict with Verification
Integration tests after 中文说明Autofix 审查轮次 — PR #8068自动审查器提出的两条内联建议均已在代码中处理。无需解决冲突（ 反馈要点
冲突说明无——该分支与 验证
本轮未在 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Review:
|
Independent local verification — Linux, real daemon, before/after buildsI rebuilt this PR from source and drove the real bundled Verdict: the fix is real and reproduces end-to-end — the before arm loses data exactly as described. But the
1. Setup2. Confirmed: the fix works
The MCP evidence is the process's own startup record, so it proves the first spawn already happened in the worktree — the deferral works, there is no start-in-base-then-relocate window. Both sessions closed 204; base checkout left with only 3. 🔴 Blocking — the effective-config
|
| Step | BEFORE 1643a6c9ae |
AFTER 1b4aefc819 |
|---|---|---|
1. qwen mcp approve cwdprobe |
stores hash 37637fa790db… |
stores hash 37637fa790db… |
| 2. daemon boots a session — server really spawns and connects | GET /workspace/mcp → approved ✅ |
→ approvalState: "pending", status: "warning" 🔴 |
| 3. operator clicks Approve in the Web Shell | hash unchanged | hash rewritten to 3803e8b53e46… (cwd-bound) 🔴 |
4. qwen mcp list |
Disconnected (i.e. approved) |
Pending approval 🔴 |
| 5. restart daemon, boot a new session | new MCP process spawned ✅ | no process spawned — server silently gated off 🔴 |
Two distinct user-visible failures:
- False "Needs approval".
McpManagerPage.tsx:171rendersmcp.status.needsApprovalfor anyapprovalState === 'pending', so every approved project-/workspace-scoped stdio server shows as needing approval in the Web Shell even while it is connected and serving tools. Everyone who already has such a server approved sees this on upgrade — the stored hash was computed beforecwdexisted. - Approving makes it worse. Clicking Approve rebinds the stored decision to the cwd-decorated config. The gate still hashes the raw entry, so on the next daemon start
isMcpServerPendingApproval()is true anddiscoverAllMcpToolsIncrementalskips the server before any spawn. Step 5 above is that: 0 new processes,entrySummary: null. A worktree session makes it worse still — the injectedcwddiffers per worktree, so no single stored hash can ever match all of them.
Only scope: 'project' (.mcp.json) and scope: 'workspace' (.qwen/settings.json) servers are gated (isGatedMcpScope), so user-scope and --mcp-config servers are unaffected.
Candidate fix (validated locally)
Keep getMcpServers() an identity-stable view of configuration and bind the implicit cwd one layer down, at the transport-config seam that every spawn path already goes through — populateMcpServerCommand(servers, cmd, cwd), which this PR already threads getTargetDir() into at all six mcp-client-manager.ts call sites plus discoverMcpTools:
--- a/packages/core/src/config/config.ts
getMcpServers(): Record<string, MCPServerConfig> | undefined {
...
- return Object.fromEntries(
- Object.entries(mcpServers).map(([name, server]) => [ ...cwd injection... ]),
- );
+ return mcpServers;
}
--- a/packages/core/src/tools/mcp-client.ts
export function populateMcpServerCommand(mcpServers, mcpServerCommand, cwd?) {
if (mcpServerCommand) { ... }
- return mcpServers;
+ if (cwd === undefined) return mcpServers;
+ return Object.fromEntries(
+ Object.entries(mcpServers).map(([name, server]) => [
+ name,
+ server.command !== undefined && server.httpUrl === undefined &&
+ server.url === undefined && server.type !== 'sdk' && server.cwd === undefined
+ ? { ...server, cwd } : server,
+ ]),
+ );
}
Rebuilt and re-run through the same two harnesses: worktree isolation is unchanged (2 MCP processes, one per worktree; per-session direct-shell cwd; base checkout untouched; base HEAD unchanged) and the approval regression is gone (every row of the table above matches the BEFORE column). It also stops populateMcpServerCommand from mutating the caller's live map, which is what lets the synthesized --mcp-server-command entry leak into config state today.
Cost: 5 existing assertions encode "getMcpServers() returns cwd" and would move down a layer — core/src/config/config.test.ts (2: binds command MCP servers without an explicit cwd…, relocateWorkingDirectory should reconcile…), core/src/config/config.safe-mode.test.ts (2), cli/src/config/config.test.ts (1: should parse inline JSON with mcpServers wrapper). Everything else stays green (717/721 core, 314/315 cli with the patch applied, and the 4+1 failures are exactly those assertions).
4. 🟡 Worktree session creation is now serialized behind MCP connect
relocateWorkingDirectory() gained await this.waitForMcpReady(); await this.refreshMcpServers(); (config.ts:4582-4588), and POST /session {worktree} awaits bridge.changeSessionCwd(...) before it answers (routes/session.ts:1717). Session creation therefore now blocks on the MCP handshake. Measured with a mock stdio server that stalls its initialize reply by 25 s:
POST /session |
BEFORE 1643a6c9ae |
AFTER 1b4aefc819 |
|---|---|---|
| plain session (no worktree) | 658 ms | 663 ms |
| worktree session | 801 ms | 25,865 ms |
The default MCP timeout is 10 minutes (MCP_DEFAULT_TIMEOUT_MSEC) while the cd deadline is max(initTimeoutMs, 30_000) (bridge.ts:6441), and the create route treats a cd failure as fatal: it kills the session, removes the worktree and its branch, and returns 500 worktree_relocate_failed. So one slow or wedged MCP server can now stop worktree sessions from being created at all. Making the relocation-time MCP reconcile fire-and-forget (it already reports through mcpRefreshError, which is only a warning) would decouple them.
5. 🟡 Nit — one retained abort listener per direct shell command
The new race in executeShellCommand (bridge.ts:7794-7803) registers signal.addEventListener('abort', …, {once: true}) and never removes it when the cwd queue wins. The REST route builds a fresh AbortController per request, but the ACP path passes the long-lived per-session binding.abort.signal (serve/acp-http/dispatch.ts:2793), so listeners accumulate for the session's lifetime — measured 300 retained listeners after 300 commands. A finally { signal?.removeEventListener('abort', onAbort); } closes it.
6. Observed, but not caused by this PR
- The first of two concurrent
POST /session/:id/shellcalls on a freshly created session sometimes returnsexitCode: 0with an emptyoutput(session B in every arm). Reproduces identically on the base build; sequential calls always return the correct output. - Daemon restart.
createSessionEntryseedseffectiveCwd: workspaceCwdand onlychangeSessionCwdever updates it; the restore path (bridge.ts:4375 restoreSession) never re-issues a cd. So a restored worktree session's direct shell falls back to the base checkout — matching the PR's own "Not validated / out of scope" note. Code-read only, not exercised here.
7. Targeted unit tests at 1b4aefc819 — 1,865 passed
packages/acp-bridge src/bridge.test.ts 464 passed
packages/core src/config/config.test.ts 471 passed
src/config/config.safe-mode.test.ts 24 passed
src/tools/mcp-client.test.ts 109 passed
src/tools/mcp-client-manager.test.ts 117 passed
packages/cli src/acp-integration/acpAgent.test.ts 343 passed
src/config/config.test.ts 315 passed
src/ui/commands/cdCommand.test.ts 22 passed
Recommendation
The isolation fix itself is solid and I'd merge it. I'd hold on §3 first: it silently stops an approved MCP server from starting, it is triggered by the most natural user action (clicking Approve), and it reaches beyond worktrees to every daemon workspace with a gated stdio server. §4 and §5 are follow-ups.
中文版
独立本地验证 —— Linux、真实 daemon、before/after 双构建
我从源码重新构建了本 PR，并在 Linux（PR 中标记为 qwen serve daemon，每个场景都跑两遍：一遍在 PR base 1643a6c9ae，一遍在 PR head 1b4aefc819。
结论： 修复是真实有效的，并且端到端复现了——before 分支确实会按 PR 描述的方式丢数据。但 getMcpServers() 注入 cwd 的做法在 worktree 路径之外产生了副作用，其中一个会静默地让已批准的 MCP server 不再启动。我还验证了一个候选修复：它保留全部隔离保证，同时消除该回归。
| 核心结论（worktree 隔离） | ✅ Linux 上 before/after 确认 |
| 针对性单元测试 | ✅ 1,865 通过 |
| 阻塞性问题 | 🔴 1 个 —— effective config 的 cwd 破坏了 MCP 审批绑定 |
| 非阻塞问题 | 🟡 2 个 —— worktree session 创建被 MCP 连接串行化；每条 shell 命令泄漏一个 abort listener |
1. 环境
worktree /root/wt/wt8068-pr @ 1b4aefc819 (base 分支: git checkout 1643a6c9ae -- <PR 的 15 个文件>)
构建 npm run build (core, acp-bridge, cli) && npm run bundle -> dist/cli.js
daemon node dist/cli.js serve --workspace <tmp>/base --hostname 127.0.0.1 --port <p> --enable-session-shell
隔离的 HOME、QWEN_RUNTIME_DIR、QWEN_SERVER_TOKEN；所有场景都不需要真实模型流量
workspace 临时 git 仓库；.qwen/settings.json 声明一个**没有** `cwd` 的 stdio MCP server
（一个 Node 脚本，每次进程启动都把 {pid, cwd} 追加写入日志）
驱动 POST /session {cwd, worktree:{slug}} x2，然后在两个 session 上并发 POST /session/:id/shell
2. 已确认：修复有效
（截图见上方英文部分。）
| 检查项（同一 workspace 下 2 个并发 worktree session） | BEFORE 1643a6c9ae |
AFTER 1b4aefc819 |
|---|---|---|
| Session A direct-shell cwd | <repo> 🔴 |
<repo>/.qwen/worktrees/e2e-a ✅ |
| Session B direct-shell cwd | <repo> 🔴 |
<repo>/.qwen/worktrees/e2e-b ✅ |
<repo>/same-name.txt（主 checkout） |
'B-shell' —— A 的写入被覆盖 🔴 |
不存在 ✅ |
worktrees/e2e-a/same-name.txt |
不存在 🔴 | 'A-shell' ✅ |
worktrees/e2e-b/same-name.txt |
不存在 🔴 | 'B-shell' ✅ |
两个 session 各自 git commit |
都提交到 main；base HEAD 发生移动 🔴 |
分别落在 worktree-e2e-a / worktree-e2e-b ✅ |
| Base HEAD | 2d57202 → 40ac730 🔴 |
未变 ✅ |
| 隐式 cwd 的 stdio MCP 进程 | 1 个共享进程，cwd = <repo>，refs=2 🔴 |
2 个，cwd 各为对应 worktree ✅ |
MCP 证据取自进程自身的启动记录，因此可以证明第一次 spawn 就已经在 worktree 中——延迟 discovery 生效，不存在"先在主 checkout 启动再迁移"的窗口。两个 session 均以 204 关闭；主 checkout 最终只剩 README.md、.gitignore、.qwen/。
3. 🔴 阻塞项 —— effective config 的 cwd 破坏了 MCP 审批绑定
mcpApprovals.json 把用户决定绑定到 hashMcpServerConfig(config)，而 cwd 属于行为性字段（不在 NON_BEHAVIORAL_FIELDS 中，packages/core/src/mcp/configHash.ts:16）。本 PR 之后，这份契约的两端开始对不同的对象求哈希：
- raw settings 条目 ——
qwen mcp approve/list，以及关键的 discovery gategetPendingGatedMcpServers(mcpServers, cwd)（packages/cli/src/config/config.ts:2036），它决定一个受管控的 server 是否允许启动； - effective config —— daemon/ACP 侧使用
config.getMcpServers()（packages/cli/src/acp-integration/acpAgent.ts:5220状态、:8290审批），因此现在会看到packages/core/src/config/config.ts:5071-5081注入的cwd: this.targetDir。
针对一个用户已经批准过的 workspace-settings stdio server，在真实 daemon 上实测：
| 步骤 | BEFORE 1643a6c9ae |
AFTER 1b4aefc819 |
|---|---|---|
1. qwen mcp approve cwdprobe |
存入 hash 37637fa790db… |
存入 hash 37637fa790db… |
| 2. daemon 启动 session —— server 确实 spawn 并连接成功 | GET /workspace/mcp → 已批准 ✅ |
→ approvalState: "pending"、status: "warning" 🔴 |
| 3. 操作者在 Web Shell 点击 Approve | hash 不变 | hash 被改写为 3803e8b53e46…（绑定了 cwd）🔴 |
4. qwen mcp list |
Disconnected（即已批准） |
Pending approval 🔴 |
| 5. 重启 daemon 并新建 session | 有新的 MCP 进程 spawn ✅ | 没有任何进程 spawn —— server 被静默拦截 🔴 |
两个用户可见的故障：
- 虚假的"需要批准"。
McpManagerPage.tsx:171只要approvalState === 'pending'就渲染mcp.status.needsApproval，因此每个已批准的 project/workspace scope stdio server 即便正在连接并提供 tool，也会在 Web Shell 里显示为待批准。所有已经批准过此类 server 的用户在升级后都会看到——因为存量 hash 是在还没有cwd的时候算出来的。 - 点了 Approve 反而更糟。 点击 Approve 会把存储的决定重新绑定到带 cwd 的 config 上。而 gate 仍然对 raw 条目求哈希，于是下次 daemon 启动时
isMcpServerPendingApproval()为真，discoverAllMcpToolsIncremental会在任何 spawn 之前跳过该 server。上表第 5 步就是这个结果：0 个新进程，entrySummary: null。worktree session 只会更糟——注入的cwd每个 worktree 都不同，任何单一 hash 都不可能同时匹配。
只有 scope: 'project'（.mcp.json）和 scope: 'workspace'（.qwen/settings.json）会被 gate（isGatedMcpScope），user scope 与 --mcp-config 不受影响。
候选修复（已在本地验证）
让 getMcpServers() 保持为配置的稳定视图，把隐式 cwd 的绑定下沉一层，放到所有 spawn 路径都必经的 transport-config 接缝 —— populateMcpServerCommand(servers, cmd, cwd)。本 PR 已经把 getTargetDir() 传到了 mcp-client-manager.ts 的全部 6 个调用点以及 discoverMcpTools：
--- a/packages/core/src/config/config.ts
getMcpServers(): Record<string, MCPServerConfig> | undefined {
...
- return Object.fromEntries(
- Object.entries(mcpServers).map(([name, server]) => [ ...cwd 注入... ]),
- );
+ return mcpServers;
}
--- a/packages/core/src/tools/mcp-client.ts
export function populateMcpServerCommand(mcpServers, mcpServerCommand, cwd?) {
if (mcpServerCommand) { ... }
- return mcpServers;
+ if (cwd === undefined) return mcpServers;
+ return Object.fromEntries(
+ Object.entries(mcpServers).map(([name, server]) => [
+ name,
+ server.command !== undefined && server.httpUrl === undefined &&
+ server.url === undefined && server.type !== 'sdk' && server.cwd === undefined
+ ? { ...server, cwd } : server,
+ ]),
+ );
}重新构建后跑同样两套 harness：worktree 隔离完全不变（2 个 MCP 进程各在自己的 worktree；每个 session 独立的 direct-shell cwd；主 checkout 未被写入；base HEAD 未移动），审批回归消失（上表每一行都与 BEFORE 列一致）。它同时还避免了 populateMcpServerCommand 直接修改调用方的活配置 map —— 那正是目前 --mcp-server-command 合成条目会渗入 config 状态的原因。
代价：有 5 处断言写死了"getMcpServers() 应返回 cwd"，需要下移一层 —— core/src/config/config.test.ts（2 处：binds command MCP servers without an explicit cwd…、relocateWorkingDirectory should reconcile…）、core/src/config/config.safe-mode.test.ts（2 处）、cli/src/config/config.test.ts（1 处：should parse inline JSON with mcpServers wrapper）。其余全部保持通过（打上补丁后 core 717/721、cli 314/315，失败的正好就是这 4+1 条断言）。
4. 🟡 worktree session 创建现在被 MCP 连接串行化
relocateWorkingDirectory() 新增了 await this.waitForMcpReady(); await this.refreshMcpServers();（config.ts:4582-4588），而 POST /session {worktree} 在返回前会 await bridge.changeSessionCwd(...)（routes/session.ts:1717）。于是创建 session 现在会阻塞在 MCP 握手上。用一个把 initialize 响应拖延 25 秒的 mock stdio server 实测：
POST /session |
BEFORE 1643a6c9ae |
AFTER 1b4aefc819 |
|---|---|---|
| 普通 session（无 worktree） | 658 ms | 663 ms |
| worktree session | 801 ms | 25,865 ms |
MCP 默认超时是 10 分钟（MCP_DEFAULT_TIMEOUT_MSEC），而 cd 的截止时间是 max(initTimeoutMs, 30_000)（bridge.ts:6441）；创建路由把 cd 失败视为致命错误：kill session、删除 worktree 及其分支，返回 500 worktree_relocate_failed。也就是说，一个慢的或卡死的 MCP server 现在可以让 worktree session 完全建不起来。把 relocation 时的 MCP reconcile 改成 fire-and-forget（它本来就只通过 mcpRefreshError 报 warning）即可解耦。
5. 🟡 小问题 —— 每条 direct shell 命令残留一个 abort listener
executeShellCommand 新增的竞速（bridge.ts:7794-7803）注册了 signal.addEventListener('abort', …, {once: true})，但在 cwd 队列先完成时不会移除。REST 路由每个请求都新建 AbortController，但 ACP 路径传入的是长生命周期的 per-session binding.abort.signal（serve/acp-http/dispatch.ts:2793），因此 listener 会在 session 生命周期内不断累积 —— 实测 300 条命令后残留 300 个 listener。加一句 finally { signal?.removeEventListener('abort', onAbort); } 即可。
6. 观察到，但并非本 PR 引入
- 在刚创建的 session 上并发发起两条
POST /session/:id/shell时，第一条有时返回exitCode: 0但output为空（每个分支里的 session B 都是如此）。base 构建上同样复现；顺序调用则始终返回正确输出。 - daemon 重启。
createSessionEntry把effectiveCwd初始化为workspaceCwd，且只有changeSessionCwd会更新它；restore 路径（bridge.ts:4375 restoreSession）从不重新发起 cd。因此被恢复的 worktree session 的 direct shell 会退回主 checkout —— 这与 PR 自己"Not validated / out of scope"中的说明一致。此项仅为代码阅读结论，未实际驱动验证。
7. 1b4aefc819 上的针对性单元测试 —— 1,865 通过
packages/acp-bridge src/bridge.test.ts 464 passed
packages/core src/config/config.test.ts 471 passed
src/config/config.safe-mode.test.ts 24 passed
src/tools/mcp-client.test.ts 109 passed
src/tools/mcp-client-manager.test.ts 117 passed
packages/cli src/acp-integration/acpAgent.test.ts 343 passed
src/config/config.test.ts 315 passed
src/ui/commands/cdCommand.test.ts 22 passed
建议
隔离修复本身很扎实，值得合入。但建议先处理第 3 节：它会静默地让已批准的 MCP server 不再启动，触发条件是最自然的用户操作（点击 Approve），而且影响范围超出 worktree，波及每一个配置了受管控 stdio server 的 daemon workspace。第 4、5 节可作为后续跟进。
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Review feedback dispositionsAddressed the one correctness bug in the new cwd-stamping predicate plus a ImplementedFinding 5 — stdio predicate diverges from Nit — redundant expect ( Declined (with reasons)Finding 1 — per-agent (frontmatter) MCP servers bypass the cwd binding. Finding 2 — stamping makes a stale Finding 3 — Finding 4 — a failed deferred discovery is silent for worktree sessions. Finding 6 — Finding 7 — Nit — abort listener stays attached after the cwd-queue wins the race Nit — race unblocks only on caller abort; reuse Nit — Changes
Verification
No settings-schema source changed, so 中文说明评审反馈处理修复了新 cwd 标记谓词中唯一的一个正确性 bug，以及一个冗余测试的小问题；其余各项均记录了理由后予以拒绝。无 base 冲突（ 已实现Finding 5 — stdio 谓词与 小问题 — 冗余的 expect（ 已拒绝（附理由）Finding 1 — 每个 agent（frontmatter）的 MCP 服务器绕过了 cwd 绑定。 Finding 2 — 标记使得过期的 Finding 3 — Finding 4 — worktree 会话中延迟发现失败时是静默的。 拒绝（建议后续处理）。在 Finding 6 — Finding 7 — 会话恢复时 小问题 — cwd 队列赢得竞争后 abort 监听器仍然挂着（ 小问题 — 竞争只在调用方 abort 时解除；复用 小问题 — 变更内容
验证
未改动任何 settings-schema 源文件，因此不需要 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
未审查：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
| return Object.fromEntries( | ||
| Object.entries(mcpServers).map(([name, server]) => [ | ||
| name, | ||
| server.command !== undefined && | ||
| server.httpUrl === undefined && | ||
| server.url === undefined && | ||
| server.tcp === undefined && | ||
| server.type !== 'sdk' && | ||
| server.cwd === undefined | ||
| ? { ...server, cwd: this.targetDir } | ||
| : server, | ||
| ]), | ||
| ); |
There was a problem hiding this comment.
[Critical] Injecting cwd: this.targetDir into getMcpServers() breaks the MCP approval hash contract. cwd is a behavioral field in hashMcpServerConfig (it is not in NON_BEHAVIORAL_FIELDS in configHash.ts), but the boot-time discovery gate getPendingGatedMcpServers (cli/src/config/config.ts) and qwen mcp approve hash the raw config, while the ACP status/approve paths (acpAgent.ts) hash the cwd-decorated config returned here. The two hashes never match. — Failure scenario: an already-approved project-/workspace-scoped stdio server shows approvalState: 'pending' ("Needs approval") in the Web Shell even while it is connected and serving tools; if the operator clicks Approve, a cwd-bound hash is persisted, and on the next daemon boot the gate (which hashes the raw config) no longer matches, so discoverAllMcpToolsIncremental skips the server before any spawn — silently gating it off. For worktree sessions the injected cwd differs per worktree, so no single stored hash can ever match all of them. Verified by probe: raw config hash aae5909… vs cwd-decorated 14b4b95… → mismatch; also reproduced against a real daemon in the PR thread (comment 5129580948).
Fix (spans two files, so not a one-click suggestion): keep getMcpServers() an identity-stable view of configuration and bind the implicit cwd one layer down, at the transport-config seam every spawn path already goes through — populateMcpServerCommand in mcp-client.ts, which this PR already threads getTargetDir() into. That is, return the merged map unchanged here and stamp cwd (same stdio predicate, fresh object) inside populateMcpServerCommand when a cwd argument is supplied.
中文说明
[Critical] 在 getMcpServers() 中注入 cwd: this.targetDir 破坏了 MCP 审批哈希契约。cwd 是 hashMcpServerConfig 中的行为性字段（不在 configHash.ts 的 NON_BEHAVIORAL_FIELDS 中），但启动期的 discovery gate getPendingGatedMcpServers（cli/src/config/config.ts）和 qwen mcp approve 对原始配置求哈希，而 ACP 的状态/审批路径（acpAgent.ts）对这里返回的、带 cwd 的配置求哈希。两端哈希永远不一致。— 故障场景：一个已批准的 project/workspace scope stdio server，即便已连接并正常提供 tool，也会在 Web Shell 中显示 approvalState: 'pending'（"需要批准"）；若操作者点击 Approve，会持久化一个绑定 cwd 的哈希，下次 daemon 启动时 gate（对原始配置求哈希）不再匹配，于是 discoverAllMcpToolsIncremental 在任何 spawn 之前跳过该 server——静默将其拦截。对 worktree session，注入的 cwd 每个 worktree 都不同，任何单一存储哈希都不可能同时匹配。已通过 probe 验证：原始配置哈希 aae5909… 与带 cwd 的 14b4b95… 不一致；PR 讨论区（评论 5129580948）也在真实 daemon 上复现了该问题。
修复（跨两个文件，故不用一键 suggestion）：让 getMcpServers() 保持配置的稳定视图，把隐式 cwd 的绑定下沉一层，放到所有 spawn 路径都必经的 transport-config 接缝——mcp-client.ts 的 populateMcpServerCommand（本 PR 已经把 getTargetDir() 传入其中）。即此处直接返回合并后的 map，而在 populateMcpServerCommand 收到 cwd 参数时（用相同的 stdio 谓词、构造新对象）打上 cwd。
— qwen3.8-max-preview via Qwen Code /review
| new Promise<void>((resolve) => { | ||
| if (signal?.aborted) return resolve(); | ||
| signal?.addEventListener('abort', () => resolve(), { once: true }); | ||
| }), |
There was a problem hiding this comment.
[Suggestion] The anonymous abort listener added inside this Promise.race is never removed when cwdChangeQueue wins the race (the normal path). The finally block below removes only the separate named onSignalAbort, not this closure, and { once: true } auto-removes a listener only after it fires. — Concrete cost: the ACP session/shell path passes a long-lived per-session signal (serve/acp-http/dispatch.ts), so each completed direct shell command leaks one dead abort listener on that signal for the session's lifetime (measured 300 retained listeners after 300 commands). Fix: hoist the listener to a named reference (e.g. onCwdQueueAbort) and remove it in the existing finally (signal?.removeEventListener('abort', onCwdQueueAbort)), mirroring the onSignalAbort pattern.
中文说明
[Suggestion] 在这个 Promise.race 内部添加的匿名 abort 监听器，在 cwdChangeQueue 赢得竞争（正常路径）时永远不会被移除。下方的 finally 块只移除了另一个具名的 onSignalAbort，并不会移除这个闭包；而 { once: true } 只在监听器触发后才自动移除。— 具体代价：ACP 的 session/shell 路径传入的是长生命周期的 per-session signal（serve/acp-http/dispatch.ts），因此每条完成的 direct shell 命令都会在该 signal 上泄漏一个失效的 abort 监听器，持续整个 session 生命周期（实测 300 条命令后残留 300 个监听器）。修复：把监听器提升为具名引用（如 onCwdQueueAbort），并在已有的 finally 中移除它（signal?.removeEventListener('abort', onCwdQueueAbort)），与 onSignalAbort 的写法保持一致。
— qwen3.8-max-preview via Qwen Code /review
getMcpServers() injected cwd into the effective config, which changed the hash used for MCP approval binding (hashMcpServerConfig). This caused approved project/workspace-scoped stdio servers to show as pending approval, and re-approving silently gated them off on the next daemon start. Move the cwd stamping down to populateMcpServerCommand — the transport-config seam every spawn path already goes through — so getMcpServers() stays an identity-stable view of configuration and the approval hash contract holds. Also make populateMcpServerCommand non-mutating (spread instead of in-place assignment). Additionally, clean up the abort listener registered during the direct-shell cwd-queue race so it does not accumulate on the long-lived per-session signal (ACP path).
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round summaryAddressed the blocking finding and one non-blocking nit from @wenshao's independent verification report. Deferred one non-blocking finding per the reviewer's explicit recommendation. §3 🔴 Blocking — MCP approval binding broken by effective-config cwd → FixedDecision: Required — correctness bug. Changes:
This matches the candidate fix validated by @wenshao: worktree isolation is preserved (cwd stamping still happens at spawn time via §5 🟡 Abort listener leak → FixedDecision: Optional nit — small, valuable, in scope. Changes:
§4 🟡 Worktree session creation serialized behind MCP connect → DeferredDecision: Deferred — the reviewer explicitly recommends this as a follow-up ("§4 and §5 are follow-ups"). Making the relocation-time MCP reconcile fire-and-forget changes the error-reporting semantics ( §6 Observed, not caused by this PR → No actionBoth items (concurrent shell empty output, daemon restart cwd restore) reproduce on the base build and are out of scope. Verification
中文说明本轮总结处理了 @wenshao 独立验证报告中的阻塞性问题和一个小问题。按评审者明确建议，推迟了一项非阻塞问题。 §3 🔴 阻塞项 —— effective config 的 cwd 破坏 MCP 审批绑定 → 已修复决定： 必须修复 —— 正确性 bug。 变更：
与 @wenshao 验证过的候选修复一致：worktree 隔离不受影响（cwd 注入仍在 spawn 时通过 §5 🟡 abort listener 泄漏 → 已修复决定： 可选小问题 —— 改动小、有价值、在范围内。 变更：
§4 🟡 worktree session 创建被 MCP 连接串行化 → 推迟决定： 推迟 —— 评审者明确建议作为后续跟进（"§4 and §5 are follow-ups"）。将 relocation 时的 MCP reconcile 改为 fire-and-forget 会改变错误报告语义（ §6 观察到但非本 PR 引入 → 不处理两项（并发 shell 空输出、daemon 重启 cwd 恢复）均在 base 构建上复现，不在本 PR 范围内。 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Round summary — no changes neededBoth inline findings were already resolved by the HEAD commit [rc:3682390409] Critical — MCP approval hash contract broken by
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：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
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action takenThis round contains no actionable feedback:
The only content is deferred non-Critical feedback from the automated reviewer, which is in Critical-only mode and explicitly excluded from action. No code changes were made. 中文说明未采取任何操作本轮没有可处理的反馈：
唯一的内容是来自自动评审器的已延后非 Critical 反馈，当前处于仅处理 Critical 的模式，明确排除在操作范围之外。 未做任何代码更改。 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds: the automated reviewer's non-Critical suggestions below are deferred and stay open for human follow-up — do not modify code, resolve threads, or reply on their behalf. Maintainer feedback defers only once its author has already had 2 regular feedback batches addressed in this window's Critical-only tail — an account can host an automated reviewer loop, so the brake keys on measured regeneration, not identity; authors at their budget, if any, are named below. (A maintainer can lift the mode itself: 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式：以上为自动评审的非 Critical 建议，予以延后、保持开放并留待人工跟进——不要为其修改代码、解决线程或代为回复。维护者的反馈仅在其本人于本窗口 Critical-only 阶段已被处理 2 批常规反馈之后才会被延后——账号可能挂着自动评审循环，因此刹车依据实测的再生频度而非身份；达到预算的作者（如有）在下方点名。（如需解除该模式，评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Local runtime validation (Linux) — the fix works, but it adds a blocking regressionI rebuilt both sides of this PR from source and drove a real Arms — both built into real
1. The isolation fix reproduces on Linux ✅Two concurrent Web Shell worktree sessions under one daemon workspace; direct session shell, agent-side shell writes, git commits, and an implicit-cwd stdio MCP server, all driven over the real REST API.
The BEFORE arm reproduces the bug exactly as described, and slightly worse than the PR body states — both sessions' direct shells ran in the base checkout, so session A's The MCP evidence is process-level: the mock stdio server records its OS pid and 2. Both stated invariants hold ✅Probed with two MCP servers configured simultaneously — one implicit-cwd, one with an explicit
So the process-count tradeoff in "Risk & Scope" is bounded by distinct directories, not by session count — worth stating that way in the description. 3. Tests pass and have teeth ✅All seven changed test files, on Linux / Node 22.22.2: I then neutered each production hunk one at a time and re-ran the PR's own suites. 11/11 mutants killed — no hunk is untested:
4. Review commit
|
| one stdio MCP server configured | BEFORE | AFTER |
|---|---|---|
answers initialize immediately |
200 in 1.0 s |
200 in 1.0 s |
initialize takes 6 s |
200 in 1.0 s |
200 in 7.2 s |
never answers initialize |
200 in 1.0 s |
500 worktree_relocate_failed in 33.1 s, worktree deleted, session killed |
The failure is deterministic (reproduced twice: 33.1 s / 33.2 s). This is not an exotic case — a first-run npx -y @some/mcp-server cold start routinely exceeds 30 s, and a wedged server (auth prompt, dead network mount) is the everyday failure mode. Today those users get degraded MCP; after this PR they cannot create a worktree session at all.
The same wait blocks /cd in the TUI, where there is no 30 s cap at all — the UI is frozen on the command for ~60 s (30 s for waitForMcpReady() on the old directory, then another 30 s for the reconcile in the new one):
Causality is confirmed, not inferred. I patched only that one call site in the built artifact — MCP refresh moved off the relocation critical path, everything else identical — and the wedged case went back to 200 in 1.0 s while the direct shell still correctly reported the worktree cwd. The isolation fix does not depend on the refresh being awaited.
Suggested direction
- Drop
await this.waitForMcpReady()here. It waits for discovery in the directory we are about to leave, which the following reconcile supersedes anyway — it is pure added latency on both the daemon and/cdpaths. - Bound the reconcile (a short
Promise.racedeadline) or run it after the relocation returns, deliveringmcpRefreshErrorthrough the existing warning/event stream instead of holding the caller. ThemcpRefreshErrorplumbing and its tests stay useful either way. - Worth a follow-up thought: the
session/loadrestore path (routes/session.ts) catches achangeSessionCwdfailure and only logs it, so with a slow MCP server a restored session would silently stay in the base checkout after a daemon restart — i.e. the exact bug this PR fixes. That path is already listed as out of scope, but the new timeout pressure makes it easier to hit.
Minor, not caused by this PR
When two direct shell commands run concurrently, the POST /session/:id/shell response occasionally comes back with exitCode: 0 but an empty output even though the command ran (the file it wrote is present). Observed on both arms, so it predates this PR — flagging it only so it is not mistaken for fallout here.
Verdict
The isolation fix itself is correct, well-scoped, genuinely tested, and reproduces end-to-end on Linux — the relocateWorkingDirectory() addressed before merge; with that one change I'm happy to approve.
中文版
本地运行时验证（Linux）——修复有效，但引入了一个阻塞性回归
我从源码分别构建了本 PR 的前后两个版本，在 Linux（本 PR 标记为 qwen serve daemon 和真实 TUI 进行验证。PR 描述中关于 worktree 隔离的所有结论都能复现。同时发现一个我认为应在合并前修复的回归。
两个构建——都构建成真实 dist 产物（node_modules link farm，无需 npm install），daemon 从 packages/cli/dist/index.js 启动：
| 版本 | commit |
|---|---|
| BEFORE | 1643a6c9ae（PR 父提交） |
| AFTER | 356ab6c203（PR head） |
1. 隔离修复在 Linux 上复现 ✅
同一 daemon workspace 下并发运行两个 Web Shell worktree session，通过真实 REST API 驱动 direct session shell、agent 侧 shell 写文件、git commit，以及一个未显式配置 cwd 的 stdio MCP server。
BEFORE 版本完整复现了该 bug，而且比 PR 描述的更严重：两个 session 的 direct shell 都运行在主 checkout 中，session A 的 same-name.txt 被 session B 静默覆盖；单个隐式 cwd 的 MCP 进程也被两个 session 共享，且位于主目录。AFTER 版本中每一次写入、每一个 commit、每一个 MCP 进程都留在各自的 worktree 内，base HEAD 不变，主 checkout 中没有出现任何 session 文件。
MCP 证据是进程级的：mock stdio server 在启动时记录自己的 OS pid 与 process.cwd()。BEFORE = 1 个进程在 <ws>；AFTER = 2 个进程，每个 worktree 一个，并且没有在主 checkout 中出现短暂的启动（deferMcpDiscovery 交接生效）。
2. 两个既定不变量成立 ✅
同时配置两个 MCP server（一个隐式 cwd，一个显式 cwd）后探测：
| BEFORE | AFTER | |
|---|---|---|
显式 cwd server |
1 个进程，固定在配置目录 | 1 个进程，仍然固定（含 worktree session） |
| 同目录下 2 个普通 session | 共享 1 个隐式 cwd 进程 | 仍然共享 1 个 |
| 隐式 cwd 进程数 | 1 | 2（按不同目录计，而非按 session 计） |
所以 "Risk & Scope" 中提到的进程数上涨是以不同目录数为上界，而不是 session 数——建议在描述里这样表述。
3. 测试通过且有实际约束力 ✅
七个被改动的测试文件，Linux / Node 22.22.2：
acp-bridge src/bridge.test.ts 464 passed
core config.test.ts, config.safe-mode.test.ts,
mcp-client.test.ts, mcp-client-manager.test.ts 725 passed
cli acpAgent.test.ts, cdCommand.test.ts 365 passed
─────────────
1554 passed, 0 failed
随后我逐个把生产代码的改动点还原/破坏，再跑 PR 自带的测试套件。11/11 变异全部被杀死——没有任何一个改动点缺少测试覆盖：
| # | 变异 | 结果 |
|---|---|---|
| M1 | direct shell 重新读 entry.workspaceCwd |
killed |
| M2 | 去掉 direct shell 前的 cwdChangeQueue 等待 |
killed |
| M3 | worktree newSession 不再打 defer 标记 |
killed |
| M4 | shouldDeferMcpDiscovery 恒为 false |
killed |
| M5 | 去掉 acpAgent 的 mcpRefreshError warning |
killed |
| M6 | 去掉 cdCommand 的 mcpRefreshError warning |
killed |
| M7 | relocateWorkingDirectory 跳过 refreshMcpServers() |
killed |
| M8 | populateMcpServerCommand 不再 stamp cwd |
killed |
| M9 | stamping 判定去掉 tcp 守卫 |
killed |
| M10 | getEffectiveMcpServers() 不传 getTargetDir() |
killed |
| M11 | ACP 注入的 stdio MCP cwd 重新固定到 newSession 的 cwd |
killed |
4. Review commit 356ab6c是必要的 ✅
我在构建产物中重建了中间提交 b694051 的行为（在 getMcpServers() 内 stamp cwd），然后在一个 gated MCP server 已被批准的 workspace 中启动 TUI：审批弹窗会再次出现——因为 useMcpApproval.ts:83 对 config.getMcpServers() 做哈希，而 cwd 在 hashMcpServerConfig 中属于行为字段。使用最终提交则不会。持久化的审批哈希在 BEFORE / AFTER 之间完全一致（d734c5bf…），审批绑定契约得以保持。
⛔ 阻塞性问题——relocation 现在会等待 MCP discovery
relocateWorkingDirectory() 新增了内联的 await this.waitForMcpReady(); await this.refreshMcpServers();，于是整个 MCP 连接/发现往返被放到了每次 relocation 的关键路径上。这里有两个常量正面冲突：
- stdio MCP discovery 预算：30 秒（
mcp-client-manager.ts:2533—isRemote ? 5_000 : 30_000） changeSessionCwd超时：30 秒（bridge.ts:6441—Math.max(initTimeoutMs, 30_000)）
因此只要有一个慢的或卡住的 stdio MCP server，这个竞争就会输，而 POST /session {worktree} 是事务性的——会杀掉 session 并删除 worktree。
| 配置一个 stdio MCP server | BEFORE | AFTER |
|---|---|---|
立即响应 initialize |
200，1.0 秒 |
200，1.0 秒 |
initialize 耗时 6 秒 |
200，1.0 秒 |
200，7.2 秒 |
从不响应 initialize |
200，1.0 秒 |
500 worktree_relocate_failed，33.1 秒，worktree 被删除，session 被杀 |
该失败是确定性的（复现两次：33.1 秒 / 33.2 秒）。这并非罕见场景——首次执行 npx -y @some/mcp-server 冷启动经常超过 30 秒，而卡住的 server（等待授权、网络挂载失效）本来就是最常见的故障形态。目前这些用户只是 MCP 降级；合并本 PR 后他们将完全无法创建 worktree session。
同一个等待也会阻塞 TUI 中的 /cd，而那条路径上没有任何 30 秒上限——界面在命令上冻结约 60 秒（旧目录上的 waitForMcpReady() 占 30 秒，新目录上的 reconcile 再占 30 秒）。
因果关系是实测的，不是推断的。 我只在构建产物中改了这一个调用点——把 MCP refresh 移出 relocation 关键路径，其余完全不变——卡住场景立刻恢复为 200、1.0 秒，同时 direct shell 仍然正确报告 worktree cwd。也就是说，隔离修复本身并不依赖这个 await。
建议方向
- 去掉这里的
await this.waitForMcpReady()。它等待的是即将离开的那个目录上的 discovery，随后的 reconcile 本来就会取代它——在 daemon 和/cd两条路径上都是纯粹的额外延迟。 - 给 reconcile 加上界（用一个较短的
Promise.racedeadline），或者在 relocation 返回之后再执行，把mcpRefreshError通过现有的 warning/event 通道投递出去，而不是让调用方一直等待。无论采用哪种方式，mcpRefreshError的管线和相关测试都仍然有价值。 - 一个值得后续关注的点：
session/load恢复路径（routes/session.ts）对changeSessionCwd失败只做 catch + log，因此在 MCP 较慢时，daemon 重启后被恢复的 session 会静默地停留在主 checkout——正是本 PR 要修的那个 bug。该路径已被列为 out of scope，但新的超时压力会让它更容易触发。
次要问题，与本 PR 无关
两个 direct shell 命令并发执行时，POST /session/:id/shell 偶尔会返回 exitCode: 0 但 output 为空，尽管命令确实执行了（它写的文件存在）。两个版本上都能观察到，说明早于本 PR——在此说明，以免被误认为是本次改动的副作用。
结论
隔离修复本身是正确的、范围合理、测试真实有效，并且在 Linux 上端到端复现——描述中 Linux 的 relocateWorkingDirectory() 里内联的 MCP 等待；解决这一点后我可以 approve。
|
🤖 AutoFix could not start — a setup step failed (or the run was cancelled) before the agent ran, so no fix was attempted. This is normally a transient infra issue, a broken base build, or a cancelled run — not this PR. It will retry on the next scan. AutoFix failed before producing a verified commit (the run crashed or timed out before it could explain why). Run log: https://github.com/QwenLM/qwen-code/actions/runs/30551642622 🧠 Handled by Qwen Code · model/模型 |
Local verification — real daemon, A/B against the merge baseVerdict: merge-ready. Every claim in the PR description that I could test reproduced, and the defect reproduces cleanly on the merge base with the same harness. Verified head One correction to the PR description is in the Corrections section below — it understates one of the failure modes. 中文摘要（点击展开）结论：可以合并。 用真实 daemon 在临时 Git 仓库里跑了 A/B 对照：同一 workspace 下并发两个 worktree session，分别执行 direct session shell、agent shell 写文件、
How I verifiedA standalone harness boots a real
The MCP server is a mock stdio server configured with no The same harness ran unchanged on both commits. Note that A/B matrix
The last two rows are the controls that make the rest meaningful: built-in agent tools were already isolated before this PR (so the diff is not papering over a broader breakage), and a plain non-worktree session still gets the workspace directory as its MCP cwd (so the new stamping did not over-reach). Before: what the merge base actually did to the base checkout
The MCP evidence is worth calling out separately: on the merge base the spawn log holds one entry, cwd Test efficacy
The PR's 7 test files were checked out onto merge-base source and run there. 16 new assertions fail, each naming the behaviour this PR adds; all 1,554 tests in those files pass on the PR head.
Also green on the PR head: CorrectionsThe PR description says direct shell commands "previously used the base workspace directory". That is true but reads as a cosmetic path bug. What the harness observed is stronger and worth putting in the description: because the direct shell ran in the base checkout, Two smaller notes, neither blocking:
Not coveredDaemon restart / Harness, raw result JSON, and the screenshots above are reproducible from the description in How I verified; evidence images hosted at |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The cwd isolation is well-structured: shell commands serialize behind the cwd queue with a proper abort race, worktree sessions defer MCP discovery until relocation, and populateMcpServerCommand stamps cwd only on implicit stdio servers without mutating the input. The getEffectiveMcpServers() extraction removes 6 duplicate call sites cleanly. Test coverage is thorough across all layers.
|
@qwen-code /resolve |
Resolve import conflict in bridge.test.ts by keeping both WORKTREE_MCP_DEFER_META_KEY (this PR) and LOAD_REPLAY_HIDE_INHERITED_META_KEY (main) in the bridgeTypes import block.
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution: PR #8068 ← origin/mainRoot causeMain's Textual, not semanticPurely textual. The sides only touched adjacent lines of one import statement; neither modified the same logic. Both constants are independently exported by the auto-merged import {
CHANNEL_STARTUP_PROFILE_META_KEY,
CHANNEL_STARTUP_PROFILE_VERSION,
WORKTREE_MCP_DEFER_META_KEY, // this PR
LOAD_REPLAY_HIDE_INHERITED_META_KEY, // main (#7929)
} from './bridgeTypes.js';This was the only conflicted file. The other six files touched on both sides ( What is load-bearingNothing order-sensitive: import order is irrelevant here; the only requirement is that both names stay imported from What I could not verifyNo build, typecheck, or tests were run (out of scope). I confirmed symbol presence, not semantic coherence, of the six auto-merged files. Those merges combine this PR's worktree-isolation logic with unrelated main features landed since the merge base — notably #7929 and the daemon session-maintenance writer changes (#7975, #8144). If any interacts with the worktree isolation path, only the PR's CI will surface it; I must not edit those non-conflicted files here. 中文说明根因main 的 文本冲突，非语义冲突纯文本冲突。双方只改动同一条 import 语句的相邻行，未修改相同逻辑。两个常量都由自动合并后的 关键点无顺序敏感之处：导入顺序在此无关紧要，唯一要求是两个名称都继续从 无法验证的部分本命令不运行构建、类型检查或测试。对那六个自动合并文件，我只确认符号存在，未验证语义一致性。这些合并把本 PR 的 worktree 隔离逻辑与 merge base 之后落地的若干无关 main 功能组合在一起，尤其是 #7929 与守护进程会话维护写入方改动（#7975、#8144）。若它们与 worktree 隔离路径有交互，只有本 PR 的 CI 才能发现；我不应也不能在此修改那些未冲突文件。 |
23f08bc
|
@qwen-code /triage |
Follow-up: a validated fix for the blocking findingTwo notes since my previous comment:
The fixBound the reconcile instead of dropping it. --- a/packages/core/src/config/config.ts
+++ b/packages/core/src/config/config.ts
@@ -1661,6 +1661,14 @@ export type SubSessionSpawner = (
class SessionWriterShutdownError extends SessionWriterUnavailableError {}
+/**
+ * How long {@link Config.relocateWorkingDirectory} waits for the MCP reconcile
+ * against the new directory before letting it finish in the background. Kept
+ * well under the daemon's 30s `changeSessionCwd` timeout so a slow MCP server
+ * degrades the reconcile, never the relocation itself.
+ */
+const MCP_RELOCATE_REFRESH_BUDGET_MS = 5_000;
+
export class Config {
@@ -4580,11 +4588,42 @@ export class Config {
}
let mcpRefreshError: unknown;
- try {
+ // Rebinding MCP to the new directory must never hold the caller longer
+ // than the relocation itself is allowed to take. stdio discovery carries
+ // its own 30s budget (McpClientManager) and the daemon's
+ // `changeSessionCwd` times out at 30s, so awaiting the full reconcile
+ // lets a single slow MCP server fail a Web Shell worktree relocation
+ // outright (500 `worktree_relocate_failed`, worktree deleted) and freeze
+ // `/cd` for as long as discovery runs. Bound the wait; a reconcile that
+ // overruns keeps running and still reports per-server failures through
+ // the `mcp-client-update` stream.
+ const mcpRefresh = (async () => {
await this.waitForMcpReady();
await this.refreshMcpServers();
- } catch (error) {
- mcpRefreshError = error;
+ })().then(
+ () => 'settled' as const,
+ (error: unknown) => {
+ mcpRefreshError = error;
+ return 'settled' as const;
+ },
+ );
+ let budgetTimer: ReturnType<typeof setTimeout> | undefined;
+ const refreshOutcome = await Promise.race([
+ mcpRefresh,
+ new Promise<'overran'>((resolve) => {
+ budgetTimer = setTimeout(
+ () => resolve('overran'),
+ MCP_RELOCATE_REFRESH_BUDGET_MS,
+ );
+ budgetTimer.unref?.();
+ }),
+ ]);
+ if (budgetTimer) clearTimeout(budgetTimer);
+ if (refreshOutcome === 'overran') {
+ this.debugLogger.debug(
+ '[mcp-hot-reload] relocation reconcile overran its budget — ' +
+ 'continuing in the background',
+ );
}
return {One file, +42/−3. Measured on the same harness
Relocation overhead is now capped at the budget regardless of how the MCP server behaves. Re-running the full two-session E2E on the patched build shows the isolation fix is untouched:
5 s is a starting point, not a considered constant — pick whatever you prefer as long as it is comfortably below the 30 s Take it, adapt it, or fix it differently — with the relocation no longer able to lose the 30 s race, I'm happy to approve. 中文版补充：针对阻塞问题的已验证修复在上一条评论之后有两点更新：
修复方案不是删掉 reconcile，而是给它加上界。对于在预算内完成的 reconcile， 补丁见上方英文部分（单文件，+42/−3）。 在同一套 harness 上的实测
无论 MCP server 表现如何，relocation 的额外开销现在都被限制在预算之内。在打了补丁的构建上重跑完整的双 session E2E，隔离修复毫发无损：
5 秒只是一个起点，并非精心论证过的常量——只要明显小于 30 秒的 可以直接采用、按需调整，或换一种方式修复——只要 relocation 不会再输掉那个 30 秒竞争，我这边就可以 approve。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.21.2. |
5 participants
What this PR does
This change makes execution inside daemon-managed Web Shell worktree sessions consistently use the session's effective working directory rather than the owning workspace checkout. Direct session shell commands wait for pending directory relocation before running, and local command-based MCP servers without an explicit
cwdare bound to the current session directory across legacy, incremental, and pooled discovery paths.Fresh worktree sessions defer MCP discovery until their trusted relocation into the worktree completes, preventing an MCP process from briefly starting in the base checkout. Subsequent directory changes reconcile MCP connections against the new directory, while explicitly configured MCP working directories and remote or SDK transports retain their existing behavior.
Why it's needed
Multiple worktree sessions can run concurrently under one Web Shell workspace. Direct shell commands previously used the base workspace directory, and implicit stdio MCP processes could inherit or initially start in that same directory. Concurrent sessions could therefore overwrite files in the base checkout or share an MCP transport with the wrong filesystem context even though built-in file tools were operating in separate worktrees.
Reviewer Test Plan
How to verify
Start the daemon from TypeScript source against a temporary Git repository, create two Web Shell worktree sessions under the same workspace, and run direct shell commands, agent shell writes, Git commits, and an implicit-cwd stdio MCP tool concurrently in both sessions. Confirm that each operation reports and modifies only its own worktree, each worktree branch receives only its own commit, the base branch HEAD does not move, and no business files appear in the base checkout. Also confirm from the mock MCP server's startup record that its first process cwd is the corresponding worktree rather than the base checkout.
Targeted automated coverage verifies direct-shell cwd serialization, worktree MCP discovery deferral, implicit versus explicit MCP cwd handling, MCP reconciliation after relocation, pooled discovery fingerprints, ACP-injected stdio configuration, and warning behavior when MCP refresh fails. The affected test files passed with 1,523 tests total; Core and ACP bridge type checks, ESLint, Prettier, and
git diff --checkalso passed. The full CLI type check remains blocked by pre-existing channel module and Ink type errors unrelated to this change.Evidence (Before & After)
Before: two worktree sessions shared the base workspace cwd for direct session shell execution, and implicit stdio MCP processes could start in the base checkout and overwrite the same file.
After: a real concurrent daemon run from the TypeScript source produced the following observations:
/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-a/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-bsame-name.txt = A-agent-shellsame-name.txt = B-agent-shellagent-same.txt = A-agentagent-same.txt = B-agentworktree-e2e-aworktree-e2e-b75d553f5d96a541e91f09aa5fffd694fee685686e486e1b63007565bea13f8859ff3edc05945a480/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-a/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-b/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-a/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-bThe base HEAD remained
17a5cbfbeb347a594cfbf9846e525e98e0712513before and after both commits. Neithersame-name.txtnoragent-same.txtexisted in the base checkout. Both agent turns completed without a turn error, both sessions closed with HTTP 204, the daemon port was no longer listening after cleanup, and the temporary repository was removed.Automated verification:
npm run typecheckpassed inpackages/coreandpackages/acp-bridge. ESLint, Prettier, andgit diff --checkpassed for the modified files. Full CLI typecheck was not counted as passing because it remains blocked by pre-existing missing channel modules and Ink type drift unrelated to this diff.Tested on
Environment (optional)
macOS with
npm run dev -- serve, daemon-managed Git worktrees, direct session shell enabled, and a local mock stdio MCP server.Risk & Scope
cwdremains authoritative.Linked Issues
N/A
中文说明
What this PR does
此改动让 daemon 管理的 Web Shell worktree session 中的执行统一使用 session 的有效工作目录，而不是所属 workspace 的主 checkout。Direct session shell 会等待尚未完成的目录迁移后再执行；未显式配置
cwd的本地 command MCP server 会在 legacy、incremental 和 pooled discovery 路径中绑定到当前 session 目录。新建 worktree session 会在可信的 worktree relocation 完成前延迟 MCP discovery，避免 MCP 进程短暂地在主 checkout 中启动。后续目录变更会按新目录重新 reconcile MCP connection；显式配置的 MCP 工作目录以及 remote 或 SDK transport 保持现有语义。
Why it's needed
同一个 Web Shell workspace 下可以并发运行多个 worktree session。此前 direct shell command 使用主 workspace 目录，隐式 stdio MCP 进程也可能继承或首次启动在同一个目录。因此，即使 built-in file tool 已经在不同 worktree 中运行，并发 session 仍可能覆盖主 checkout 中的文件，或者共享带有错误文件系统上下文的 MCP transport。
Reviewer Test Plan
How to verify
使用临时 Git 仓库从 TypeScript 源码启动 daemon，在同一 workspace 下创建两个 Web Shell worktree session，并在两个 session 中并发执行 direct shell、agent shell 文件写入、Git commit 和未显式配置 cwd 的 stdio MCP tool。确认每项操作只报告并修改自己的 worktree，每个 worktree branch 只获得自己的 commit，base branch HEAD 不发生变化，并且主 checkout 中没有出现业务文件。同时检查 mock MCP server 的启动记录，确认进程第一次启动时的 cwd 就是对应 worktree，而不是主 checkout。
针对性自动化覆盖验证了 direct-shell cwd 串行化、worktree MCP discovery 延迟、隐式与显式 MCP cwd 处理、relocation 后的 MCP reconcile、pool discovery fingerprint、ACP 注入的 stdio 配置，以及 MCP refresh 失败时的 warning。受影响测试文件共 1,523 个测试全部通过；Core 和 ACP bridge typecheck、ESLint、Prettier、
git diff --check也通过。CLI 全量 typecheck 仍被与本改动无关的既有 channel module 和 Ink 类型错误阻塞。Evidence (Before & After)
Before：两个 worktree session 的 direct session shell 共享主 workspace cwd，隐式 stdio MCP 进程可能启动在主 checkout，并覆盖同名文件。
After：从 TypeScript 源码启动 daemon 进行真实并发测试，得到以下实际观测值：
/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-a/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-bsame-name.txt = A-agent-shellsame-name.txt = B-agent-shellagent-same.txt = A-agentagent-same.txt = B-agentworktree-e2e-aworktree-e2e-b75d553f5d96a541e91f09aa5fffd694fee685686e486e1b63007565bea13f8859ff3edc05945a480/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-a/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-b/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-a/private/tmp/qwen-wt-e2e.U29woU/base/.qwen/worktrees/e2e-b两个 commit 前后 base HEAD 都是
17a5cbfbeb347a594cfbf9846e525e98e0712513。主 checkout 中不存在same-name.txt和agent-same.txt。两个 agent turn 都正常完成且没有 turn error；两个 session 均以 HTTP 204 关闭；清理后 daemon 端口不再监听，临时仓库已删除。自动化验证：
packages/core和packages/acp-bridge的npm run typecheck通过；修改文件的 ESLint、Prettier 和git diff --check通过。CLI 全量 typecheck 没有计为通过，因为仍被与本 diff 无关的既有 channel module 缺失和 Ink 类型漂移阻塞。Tested on
Environment (optional)
macOS，使用
npm run dev -- serve、daemon 管理的 Git worktree、已启用 direct session shell，以及本地 mock stdio MCP server。Risk & Scope
cwd仍具有最高优先级。Linked Issues
N/A