feat(acp): support /cd command in ACP sessions - #5903
Conversation
|
Thanks for the PR! Template looks good ✓ On direction: well-aligned. ACP multi-session needs per-session logical CWD tracking without mutating shared process state — same motivation as the On approach: scope is clean and focused — 10 files, 351 additions, all directly related to Previous blocker — resolved ✓:
Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：对齐。ACP 多 session 需要 per-session 的逻辑 CWD 追踪而不改变共享进程状态——与 Claude Code 的 方案：范围干净集中——10 个文件，351 行增加，全部与 之前的阻塞项——已解决 ✓：
进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe Core implementation (9 files, ~350 lines)Bridge layer ( Error types ( ACP agent handler ( Core config ( HTTP route ( One note: PR description vs code discrepancyThe PR description states: "if a prompt is actively running, Test gapNo automated tests for the TestingThis is an HTTP API endpoint requiring a running ACP daemon with active sessions — not directly testable via tmux interactive session. CI passes on Ubuntu (Test ubuntu-latest Node 22.x: pass, 7m40s). The previous 中文说明代码审查
核心实现（9 个文件，约 350 行）Bridge 层 ( 错误类型 ( ACP agent 处理器 ( Core 配置 ( HTTP 路由 ( 一个备注：PR 描述与代码不一致PR 描述称："如果 prompt 正在运行， 测试缺失
测试这是需要运行中 ACP daemon 和活动 session 的 HTTP API 端点——无法通过 tmux 交互式会话直接测试。CI 在 Ubuntu 上通过（Test ubuntu-latest Node 22.x：pass，7m40s）。之前的 — Qwen Code · qwen3.7-max |
|
Stepping back: the Previous blocker — fully resolved:
Minor note: the PR description says Independent proposal check: my own approach would have been near-identical — extend CI passes on Ubuntu. The test gap is noted but acceptable for initial merge. Verdict: Approve ✅ — clean implementation, focused scope, previous concerns resolved. The description discrepancy is cosmetic and can be fixed in a follow-up or by editing the PR body. 中文说明退一步看： 之前的阻塞项——完全解决：
小备注： PR 描述称 独立方案对照：我自己的方案几乎一致——用 skip 选项扩展 CI 在 Ubuntu 上通过。测试缺失已注意到，初始合并可以接受。 结论：Approve ✅ ——实现干净，范围集中，之前的顾虑已解决。描述不一致是外观问题，可在后续修复或直接编辑 PR 正文。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs some fixes before shipping — wrong HTTP status code for trust rejection (409→403), missing tests for the /cd feature, and unrelated Phase 4c telemetry should be split into its own PR. See review comments above for details. 🙏
Implements the server-side /cd command for ACP multi-session architecture: - Infrastructure: ext-method constant, bridge types, CdWhilePromptActiveError - Core: skipProcessChdir option in Config.relocateWorkingDirectory() - ACP child handler: trust check, fs validation, sandbox guard - Bridge: promptQueue-serialized changeSessionCwd with event publishing - HTTP route: POST /session/:id/cd with error-to-status mapping - directory_not_found → 400 - directory_not_trusted → 403 - restrictive_sandbox → 403 - session_busy (CdWhilePromptActiveError) → 409 SDK client wrappers and capability registration are deferred to a follow-up PR once the server-side implementation is validated end-to-end. Closes #5677
a6a9e22 to
261b6bd
Compare
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. |
Maintainer verification — real
|
| Behavior | Result |
|---|---|
cd to valid dir |
200 {previousCwd, newCwd, warnings:[]} ✓ |
noop (cd to current dir) |
200, previousCwd==newCwd, no event published ✓ |
| non-existent path | 400 directory_not_found ✓ |
| path is a file, not a dir | 400 directory_not_found ("Not a directory") ✓ |
| relative path | 400 invalid_path ✓ |
| unknown session | 404 ✓ |
SSE session_cwd_changed |
fires only on real change, carries originatorClientId, delivered to other listeners ✓ |
| unregistered client id | 400 invalid_client_id before any state change (good security gate) ✓ |
errorKind across the JSON-RPC boundary |
survives ACP-child → bridge → route (proven via directory_not_found and the trust case) ✓ |
| subsequent tool calls follow new cwd | after cd, model read_file on bare WHOAMI.txt resolved to the new dir ✓ |
| multi-session isolation | two thread sessions in the same shared ACP child: cd in A did not leak to B; each session's tools resolved against its own cwd — confirms the skipProcessChdir design ✓ |
repeated cd correctness |
relocateWorkingDirectory updates both this.cwd/this.targetDir (config.ts:3124-3125), so chained cds compute oldDir correctly — no staleness ✓ |
🔴 Blocker 1 — SDK browser-bundle cap exceeded (fails CI on all platforms)
npm ci → prepare → build → assertBrowserSafeBundle fails:
Error: Browser daemon SDK bundle is 133501 bytes; expected <= 133120
at assertBrowserSafeBundle (packages/sdk-typescript/scripts/build.js:169)
I measured the contribution directly:
| Build | dist/daemon/index.js |
vs cap (133120) |
|---|---|---|
origin/main (PR's daemon sources reverted) |
133092 | 28 under |
| PR head | 133501 | 381 over |
The PR's /cd SDK additions (DaemonClient.changeSessionCwd, DaemonSessionClient.cd, acpRouteTable, types) add +409 bytes and tip a near-full bundle over the hard cap. This is the PR's responsibility, and the cap has an established bump-on-growth convention (see the comments at build.js:30-36: "122→124KB … 127→130KB"). Fix: bump MAX_DAEMON_BROWSER_BUNDLE_BYTES in packages/sdk-typescript/scripts/build.js (e.g. to 132 * 1024). Until then the PR cannot pass CI / be installed (this is consistent with the current BLOCKED state).
🔴 Blocker 2 — directory_not_trusted returns 409, contract says 403 (confirmed live)
With folder-trust enabled and an untrusted target:
$ curl -X POST .../session/$SID/cd -d '{"path":"/private/tmp/cdtest/dirA"}'
{"error":"Directory not trusted: /private/tmp/cdtest/dirA","code":"directory_not_trusted","path":"..."}
HTTP 409 # ← PR's Reviewer Test Plan says 403
(A trusted sibling dir returns 200, so trust is genuinely active.) This is the static finding above, now reproduced end-to-end. 409 Conflict is wrong for an authorization denial; the route's own restrictive_sandbox branch and the existing TrustGateError handler both correctly use 403. One-line fix: server.ts:3378 res.status(409) → 403.
🟠 Finding 3 — documented "409 session_busy while a prompt is active" does not happen; cd blocks instead
POST /prompt returns 202 and processes in the background. I raced a cd against an in-flight prompt 4×:
round 1: hasActivePrompt=true cd-> HTTP 200 latency=5682ms
round 2: hasActivePrompt=true cd-> HTTP 200 latency=5492ms
round 3: hasActivePrompt=true cd-> HTTP 200 latency=5331ms
round 4: hasActivePrompt=true cd-> HTTP 200 latency=4991ms
The cd never returned 409. It queued behind the active prompt and returned 200 after the prompt finished (a longer 600-word prompt blocked the cd for 17.6 s). This actually matches the code's own comment ("cd waits for any in-flight prompt to complete", bridge.ts), but it contradicts the PR's Reviewer Test Plan ("Prompt actively running → 409 session_busy"). Two consequences:
- The documented contract is misleading — a reviewer following the test plan will not see
409. - The
cdHTTP request blocks for the entire prompt duration, unbounded (thewithTimeoutonly wraps the post-queueextMethod, not the queue wait; the route has nodeadlineMs). For a multi-minute agent turn, thecdcall hangs that long.
The CdWhilePromptActiveError guard appears reachable only via a sub-millisecond internal race (a cd landing between promptActive=true and the promptQueue tail assignment), which normal usage does not hit. Suggest either making cd fail fast with 409 when promptActive (match the docs), or updating the docs to "cd queues and may block" and bounding the wait.
🟡 Finding 4 — scope creep + zero /cd tests (as already raised)
The PR's HEAD commit is unrelated Phase-4c telemetry (recordApiRequestBreakdown wiring in session-tracing.ts +30, loggingContentGenerator.ts +5, session-tracing.test.ts +114). The only test file touched is the telemetry one — /cd has no automated tests (bridge changeSessionCwd, the ext-method handler, the route, and the error mapping that contains Blocker 2). Recommend splitting telemetry into its own PR and adding /cd route/bridge tests (a route test would have caught the 409/403 mismatch). restrictive_sandbox→403 was not exercised (needs a macOS seatbelt restrictive-* profile) but the mapping is correct by inspection.
Recommendation
Solid feature; not mergeable as-is. Required before merge: (1) bump the SDK bundle cap, (2) change directory_not_trusted to 403. Strongly recommended: (3) reconcile the session_busy doc/behavior, (4) split out the telemetry and add /cd tests.
中文版（完整对应）
维护者验证 —— 真实 qwen serve daemon 端到端
我在本地构建了此 PR，并用裸 HTTP/SSE 驱动真实的 qwen serve daemon（HTTP bridge），同时把派生的 --acp 子进程钉到同一份构建，因此路由层和运行信任检查的 ACP 子进程都是本 PR 的代码。这是对上面静态审查的实测补充。
环境
- worktree 位于 PR head
a6a9e2299（父提交d722bf043）；npm ci && npm run build。 - 通过
QWEN_CLI_ENTRY+node packages/cli/dist/index.js serve --port 0 --hostname 127.0.0.1把 daemon 和 ACP 子进程都钉到 worktree 构建，跑在常驻的tmuxpane 中。真实qwen3.7-max凭证；loopback（无 token）。 - 信任开关通过 workspace
.qwen/settings.json（security.folderTrust.enabled）+QWEN_CODE_TRUSTED_FOLDERS_PATH确定性地控制。
结论
/cd 功能实现扎实、质量好 —— 所有正常路径以及多 session 设计主张在真实执行下都成立。但合并前有 2 个阻塞项 和 2 个清理项。
✅ 实测确认正常（真实 daemon）
| 行为 | 结果 |
|---|---|
cd 到有效目录 |
200 {previousCwd, newCwd, warnings:[]} ✓ |
noop（cd 到当前目录） |
200，previousCwd==newCwd，不发布事件 ✓ |
| 不存在的路径 | 400 directory_not_found ✓ |
| 路径是文件而非目录 | 400 directory_not_found（"Not a directory"） ✓ |
| 相对路径 | 400 invalid_path ✓ |
| 未知 session | 404 ✓ |
SSE session_cwd_changed |
仅在真实变更时触发，携带 originatorClientId，可投递给其他监听者 ✓ |
| 未注册的 client id | 在任何状态变更之前 400 invalid_client_id（良好的安全闸门） ✓ |
errorKind 跨 JSON-RPC 边界 |
从 ACP 子进程 → bridge → 路由完整保留（由 directory_not_found 和信任场景共同证明） ✓ |
| 后续工具调用跟随新 cwd | cd 后，模型对裸 WHOAMI.txt 的 read_file 解析到了新目录 ✓ |
| 多 session 隔离 | 同一共享 ACP 子进程中的两个 thread session：A 的 cd 未泄漏到 B；各 session 的工具按各自 cwd 解析 —— 证实了 skipProcessChdir 设计 ✓ |
重复 cd 正确性 |
relocateWorkingDirectory 同时更新 this.cwd/this.targetDir（config.ts:3124-3125），所以连续 cd 的 oldDir 计算正确、无陈旧 ✓ |
🔴 阻塞项 1 —— SDK 浏览器 bundle 体积上限超标（全平台 CI 挂）
npm ci → prepare → build → assertBrowserSafeBundle 失败：
Error: Browser daemon SDK bundle is 133501 bytes; expected <= 133120
at assertBrowserSafeBundle (packages/sdk-typescript/scripts/build.js:169)
我直接测量了贡献量：
| 构建 | dist/daemon/index.js |
相对上限(133120) |
|---|---|---|
origin/main（把 PR 的 daemon 源回退） |
133092 | 低 28 |
| PR head | 133501 | 超 381 |
PR 的 /cd SDK 新增（DaemonClient.changeSessionCwd、DaemonSessionClient.cd、acpRouteTable、types）增加了 +409 字节，把一个已接近上限的 bundle 顶过硬上限。这是本 PR 的责任，并且该上限有既定的随增长上调惯例（见 build.js:30-36 注释："122→124KB … 127→130KB"）。修复： 上调 packages/sdk-typescript/scripts/build.js 里的 MAX_DAEMON_BROWSER_BUNDLE_BYTES（如改为 132 * 1024）。否则 PR 无法过 CI / 无法安装（与当前 BLOCKED 状态一致）。
🔴 阻塞项 2 —— directory_not_trusted 返回 409，但约定是 403（实测复现）
开启 folder-trust 且目标不受信任时：
$ curl -X POST .../session/$SID/cd -d '{"path":"/private/tmp/cdtest/dirA"}'
{"error":"Directory not trusted: ...","code":"directory_not_trusted","path":"..."}
HTTP 409 # ← PR 的 Reviewer Test Plan 写的是 403
（受信任的兄弟目录返回 200，说明信任确实生效。）这就是上面的静态发现，现已端到端复现。409 Conflict 用于授权拒绝是错的；路由自身的 restrictive_sandbox 分支和既有 TrustGateError 处理器都正确用 403。一行修复： server.ts:3378 的 res.status(409) → 403。
🟠 发现 3 —— 文档说"prompt 活跃时 409 session_busy"，实际不会发生；cd 会阻塞
POST /prompt 返回 202 并在后台处理。我让 cd 与活跃 prompt 竞争 4 次：
round 1: hasActivePrompt=true cd-> HTTP 200 latency=5682ms
round 2: hasActivePrompt=true cd-> HTTP 200 latency=5492ms
round 3: hasActivePrompt=true cd-> HTTP 200 latency=5331ms
round 4: hasActivePrompt=true cd-> HTTP 200 latency=4991ms
cd 从不返回 409，而是排在活跃 prompt 之后、等 prompt 结束才返回 200（一个更长的 600 词 prompt 让 cd 阻塞了 17.6 秒）。这其实与代码自身注释一致（"cd waits for any in-flight prompt to complete"，bridge.ts），但与 PR 的 Reviewer Test Plan 矛盾（"Prompt actively running → 409 session_busy"）。两个后果：
- 文档约定有误导 —— 按测试计划走的审查者看不到
409。 cdHTTP 请求会阻塞整个 prompt 时长，且无上界（withTimeout只包住队列之后的extMethod，不包队列等待；路由也没有deadlineMs）。对一个数分钟的 agent turn，cd调用就挂那么久。
CdWhilePromptActiveError 这个守卫似乎只在亚毫秒级内部竞争下才可达（cd 恰好落在 promptActive=true 与 promptQueue 尾指针赋值之间），正常使用碰不到。建议 要么让 cd 在 promptActive 时快速失败返回 409（与文档一致），要么把文档改成"cd 会排队并可能阻塞"并给等待加上界。
🟡 发现 4 —— 范围蔓延 + /cd 零测试（已被提出）
PR 的 HEAD commit 是不相关的 Phase-4c 遥测（session-tracing.ts +30、loggingContentGenerator.ts +5、session-tracing.test.ts +114 的 recordApiRequestBreakdown 接入）。唯一改动的测试文件就是遥测那个 —— /cd 没有任何自动化测试（bridge changeSessionCwd、ext-method 处理器、路由、以及包含阻塞项 2 的错误映射）。建议把遥测拆成单独 PR，并补 /cd 路由/bridge 测试（一个路由测试本可抓到 409/403 这个不匹配）。restrictive_sandbox→403 未实测（需要 macOS seatbelt restrictive-* profile），但映射经审阅是对的。
建议
功能扎实，但当前不可合并。合并前必须：(1) 上调 SDK bundle 上限，(2) 把 directory_not_trusted 改为 403。强烈建议：(3) 让 session_busy 的文档与行为一致，(4) 拆出遥测并补 /cd 测试。
Verified locally on macOS against a real qwen serve daemon (PR head a6a9e2299) — raw HTTP/SSE, ACP child pinned to the same build, real model creds.
|
[qwen] Thanks for the thorough end-to-end verification! Blockers 1, 2 & Finding 4 are already resolved in the current HEAD
Finding 3 (session_busy behavior): Valid observation. The I am updating the PR description to remove the "409 session_busy" claim from the test plan and clarify that If you prefer a bounded wait (deadline on the queue) or a fail-fast design instead, happy to discuss — but that would be a design change worth a separate iteration. |
| const actualCwd = fs.realpathSync(process.cwd()); | ||
| if (actualCwd !== expected) { | ||
| process.chdir(oldDir); | ||
| throw new Error( |
There was a problem hiding this comment.
[Critical] prepareSessionArtifactMigration (called at line 3122) does not receive the skipProcessChdir option. When called from the ACP /cd path (skipProcessChdir: true), oldDir is set to this.cwd (the session's logical CWD) rather than process.cwd(). If moveCurrentSessionArtifacts throws inside prepareSessionArtifactMigration, the catch block at line 3080 unconditionally calls process.chdir(oldDir), silently changing the daemon's process CWD to a directory it was never at.
In a multi-session daemon, this corrupts process.cwd() for all other sessions and daemon-level operations that rely on it (tool execution, file reads, subprocess spawns). The divergence is silent and extremely hard to debug.
Fix: Thread skipProcessChdir through to prepareSessionArtifactMigration and skip the rollback process.chdir() when it is true — since no process.chdir(targetPath) happened in the first place, there is nothing to roll back:
private async prepareSessionArtifactMigration(
oldStorage: Storage,
newStorage: Storage,
oldDir: string,
opts?: { skipProcessChdir?: boolean },
): Promise<void> {
this.chatRecordingService?.finalize();
await this.chatRecordingService?.flush();
await this.flushRuntimeStatusWrites();
try {
this.moveCurrentSessionArtifacts(oldStorage, newStorage);
} catch (error) {
if (!opts?.skipProcessChdir) {
try {
process.chdir(oldDir);
} catch (rollbackError) {
this.debugLogger.warn(
'Failed to roll back working directory after session artifact migration failed',
rollbackError,
);
}
}
throw error;
}
}Then at the call site (line 3122):
await this.prepareSessionArtifactMigration(oldStorage, newStorage, oldDir, opts);— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in f56f14b. Threaded opts (containing skipProcessChdir) through to prepareSessionArtifactMigration. When skipProcessChdir: true, the catch block now skips the process.chdir(oldDir) rollback — since no process.chdir(targetPath) happened in the first place, there is nothing to roll back.
| sessionId, | ||
| path: req.path, | ||
| }), | ||
| initTimeoutMs, |
There was a problem hiding this comment.
[Suggestion] changeSessionCwd uses initTimeoutMs (default 10s) as its timeout, but the agent-side work is substantial: fs.statSync, fs.realpathSync, WorkspaceContext.resolveRootDirectories, prepareSessionArtifactMigration (file I/O for artifact moves), refreshCurrentRuntimeStatus, refreshHierarchicalMemory (scans project tree), and addWorkingDirectoryChangedContext (model API call). On a large project or slow filesystem, 10 seconds is tight.
A timeout here also leaves the session in an ambiguous state: the agent may have partially completed the directory change (e.g., config updated but memory refresh not done), while the bridge's entry.workspaceCwd would not be updated, creating state divergence.
Other complex operations in this file use dedicated timeouts (e.g., MCP_RESTART_TIMEOUT_MS = 300_000, recap has a 60s backstop at line 661-665). Consider a dedicated timeout:
| initTimeoutMs, | |
| Math.max(initTimeoutMs, 30_000), |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in f56f14b. Changed to Math.max(initTimeoutMs, 30_000) — ensures at least 30s for the cd operation which includes fs validation, artifact migration, memory refresh, and context generation on the agent side.
| ); | ||
| res.status(200).json(result); | ||
| } catch (err) { | ||
| // Map ACP child errorKind codes to HTTP responses |
There was a problem hiding this comment.
[Suggestion] This catch block contains ~35 lines of inline errorKind-to-HTTP-status mapping (restrictive_sandbox, directory_not_found, directory_not_trusted) before falling through to sendBridgeError. The established pattern — visible in sendBridgeErrorImpl — centralizes all error kind mapping in that shared function. Other POST routes like /session/:id/branch and /session/:id/fork simply call sendBridgeError directly without inline errorKind checks.
Splitting errorKind mapping across two locations means future error kinds added to the sessionCd handler must be mapped here rather than the shared location, breaking the single-responsibility pattern of sendBridgeErrorImpl.
Fix: Move the three errorKind cases into sendBridgeErrorImpl alongside the existing kind mappings, and simplify this catch block to just sendBridgeError(res, err, { route: 'POST /session/:id/cd', sessionId }) — matching the /branch and /fork routes exactly.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in f56f14b. Moved all three errorKind cases (restrictive_sandbox, directory_not_found, directory_not_trusted) into sendBridgeErrorImpl alongside the existing errorKind block. The cd route catch block is now a single sendBridgeError(res, err, {...}) call — matching the /branch and /fork pattern exactly.
- [Critical] Thread skipProcessChdir through prepareSessionArtifactMigration to prevent corrupting shared daemon process.cwd() on rollback in ACP mode - [Suggestion] Raise cd timeout to Math.max(initTimeoutMs, 30_000) for large-project fs ops and memory refresh - [Suggestion] Move errorKind-to-HTTP mapping (restrictive_sandbox, directory_not_found, directory_not_trusted) into sendBridgeErrorImpl; cd route catch block now matches /branch and /fork pattern
| () => undefined, | ||
| ); | ||
|
|
||
| const result = await cdResult; |
There was a problem hiding this comment.
[Suggestion] The extMethod result is cast with as and used directly without any shape validation. If the agent returns an unexpected response (e.g., missing newCwd), downstream code would operate on undefined values with confusing errors.
Compare with branchSession (~L3413) which validates the response before use:
if (!result || typeof result.newSessionId !== 'string') {
throw new Error(
`branchSession: agent returned invalid response: ${JSON.stringify(result)}`,
);
}Consider adding similar validation after await cdResult:
| const result = await cdResult; | |
| const result = await cdResult; | |
| if ( | |
| !result || | |
| typeof result.previousCwd !== 'string' || | |
| typeof result.newCwd !== 'string' | |
| ) { | |
| throw new Error( | |
| `changeSessionCwd: agent returned invalid response: ${JSON.stringify(result)}`, | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Added response shape validation after the extMethod call — if previousCwd, newCwd, or warnings are missing/wrong type, throws a descriptive error with the raw response stringified for debugging.
|
|
||
| // Only update state and broadcast if directory actually changed | ||
| if (result.previousCwd !== result.newCwd) { | ||
| entry.workspaceCwd = result.newCwd; |
There was a problem hiding this comment.
[Suggestion] Overwriting entry.workspaceCwd with the new directory breaks listWorkspaceSessions, which filters sessions by entry.workspaceCwd === key where key is always boundWorkspace (L3630). After any /cd that changes the directory away from the bound workspace, the session becomes invisible to GET /workspace/:id/sessions.
The session remains functional via GET /session/:id/status (which uses byId.get), but SDK clients enumerating sessions through the workspace listing endpoint lose track of it.
Consider introducing a separate effectiveCwd field on SessionEntry to track the current working directory, while keeping workspaceCwd as the immutable workspace association used by listWorkspaceSessions.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Removed entry.workspaceCwd = result.newCwd entirely. The session stays listed under its original daemon-bound workspace for listWorkspaceSessions and lifecycle operations (branchSession etc.). The session's logical cwd lives only inside the agent's Config object — the event bus still broadcasts session_cwd_changed so clients know the current directory.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
|
||
| // Only update state and broadcast if directory actually changed | ||
| if (result.previousCwd !== result.newCwd) { | ||
| entry.workspaceCwd = result.newCwd; |
There was a problem hiding this comment.
[Critical] entry.workspaceCwd = result.newCwd makes the session invisible in listWorkspaceSessions(boundWorkspace) after any /cd that changes the directory. The listing function (line 3630) filters by entry.workspaceCwd === key where key is always boundWorkspace. Once workspaceCwd changes to a different directory, the session no longer matches and silently disappears from GET /workspace/:id/sessions. Additionally, branchSession (line 3405) passes boundWorkspace as the child's cwd, so branching from a cd'd session creates the child at the original workspace rather than the parent's current directory.
| entry.workspaceCwd = result.newCwd; | |
| // Keep workspaceCwd for session-scoped lookups but don't overwrite | |
| // the workspace binding — listWorkspaceSessions and branchSession | |
| // rely on entry.workspaceCwd matching boundWorkspace. | |
| entry.sessionCwd = result.newCwd; |
Consider tracking the session-level CWD separately from the workspace binding (e.g., a new sessionCwd field), or constrain /cd to subdirectories of boundWorkspace so the workspace key always matches.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Same fix as above — entry.workspaceCwd is no longer mutated. Session remains visible in listWorkspaceSessions(boundWorkspace) after /cd, and branchSession continues to use boundWorkspace as the child's cwd (which is correct since storage also stays at boundWorkspace now).
| }); | ||
| return; | ||
| } | ||
| if (err instanceof CdWhilePromptActiveError) { |
There was a problem hiding this comment.
[Suggestion] CdWhilePromptActiveError maps to code: 'session_busy', which collides with SessionBusyError (line 5957). API clients cannot distinguish "cd rejected because prompt is active" from a generic "session busy" condition. Following the established pattern where each typed error produces a distinct code (e.g., BranchWhilePromptActiveError → 'branch_while_prompt_active'), use a unique code here.
| if (err instanceof CdWhilePromptActiveError) { | |
| if (err instanceof CdWhilePromptActiveError) { | |
| res.status(409).json({ | |
| error: err.message, | |
| code: 'cd_while_prompt_active', | |
| sessionId: err.sessionId, | |
| }); | |
| return; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Renamed to code: 'cd_while_prompt_active' — follows the same naming pattern as BranchWhilePromptActiveError → 'branch_while_prompt_active', and no longer collides with the generic SessionBusyError → 'session_busy'.
| 'Invalid or missing sessionId', | ||
| ); | ||
| } | ||
| if ( |
There was a problem hiding this comment.
[Suggestion] The interactive /cd command (cdCommand.ts:35-36) validates against null bytes before processing the path:
if (input.includes('\0') || baseDir.includes('\0')) {
throw new Error('Path contains null bytes.');
}This ACP handler accepts any absolute string without this check. Null bytes in paths can cause truncation at the C/syscall boundary, potentially bypassing the directory-existence check or trust validation. Add the same null byte guard here for defense in depth.
| if ( | |
| if ( | |
| typeof targetPath !== 'string' || | |
| targetPath.length === 0 || | |
| targetPath.includes('\0') || | |
| !path.isAbsolute(targetPath) | |
| ) { |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Added targetPath.includes('\0') to the validation predicate in the ACP sessionCd handler, matching the same null-byte check pattern used by the interactive /cd command.
|
@qwen-code /resolve |
…e refactor The main branch refactored server.ts by extracting session routes into routes/session.ts and error handling into server/error-response.ts. The PR added the /cd endpoint and CdWhilePromptActiveError inline. Resolution: take main's refactored structure for server.ts, then place the PR's /cd route in routes/session.ts and CdWhilePromptActiveError handler in server/error-response.ts — matching main's conventions.
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #5903Conflict
Conflict regions in
|
| }); | ||
| return; | ||
| } | ||
| if (err instanceof CdWhilePromptActiveError) { |
There was a problem hiding this comment.
[Critical] The ACP child now throws RequestErrors with data.errorKind values like restrictive_sandbox, directory_not_found, and directory_not_trusted, but this route-side mapper does not handle those kinds. They fall through to the generic 500 path instead of the documented 403/400/403 responses, so ACP clients cannot rely on stable policy/error handling for /cd failures. Please add these cases to the data.errorKind block in sendBridgeError and preserve the returned path where present.
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Added restrictive_sandbox, directory_not_found, and directory_not_trusted errorKind cases to sendBridgeErrorImpl (now in the refactored server/error-response.ts). The /cd route's catch block is now a single sendBridgeError(res, err, {...}) call — matching the /branch and /fork pattern. ACP clients now receive stable 403/400/403 responses for these error kinds.
| throw new Error( | ||
| `Changed directory to ${actualCwd}, expected ${expected}.`, | ||
| ); | ||
| if (!opts?.skipProcessChdir) { |
There was a problem hiding this comment.
[Critical] The new skipProcessChdir branch skips the realpath equality check that the normal process.chdir path performs. ACP trust-checks the canonical target before calling this, but if that directory is swapped before relocation applies state, this path can proceed without proving fs.realpathSync(targetPath) === expected; later filesystem access through the stored cwd can then follow the swapped path. Please keep a final realpath-vs-expected validation for the skip path before updating storage/workspace roots.
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Added a final fs.realpathSync(targetPath) !== expected check on the skipProcessChdir path (the else branch). This guards against TOCTOU symlink swaps between the trust check and the config state update — if the real path diverges from what was validated, the operation throws before updating any state.
| } | ||
|
|
||
| const ci = await ensureChannel(); | ||
| return (await withTimeout( |
There was a problem hiding this comment.
[Critical] withTimeout only races the ACP sessionCd request; it does not cancel the underlying ext-method. Because entry.promptQueue is advanced from this timed promise, a 30s timeout releases subsequent prompts while the ACP child may still finish relocating config/storage later, leaving bridge state/events at the old cwd while the agent has moved. Please keep the queue tail tied to the raw ext-method until it settles, or make timeout fatal/reconciled so later prompts cannot run against unknown cwd state.
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Restructured changeSessionCwd so that:
promptQueuetail is tied to the raw extMethod settlement (not the timeout-wrapped version)- State update (event broadcast) happens inside the queue lambda — executes when the extMethod actually settles, regardless of whether the caller timed out
withTimeoutis purely caller-facing — surfaces a deadline-exceeded error to the HTTP client without advancing the queue
This ensures subsequent prompts cannot run against unknown cwd state: even if the caller times out, the queue stays blocked until the real cd finishes (or fails).
|
|
||
| // Relocate working directory (skip process.chdir for ACP) | ||
| const warnings: string[] = []; | ||
| const relocation = await config.relocateWorkingDirectory( |
There was a problem hiding this comment.
[Critical] relocateWorkingDirectory moves the live session artifacts into Storage(canonicalPath), but bridge lifecycle paths still operate against the daemon-bound workspace. For example, sessionBranch sends cwd: boundWorkspace and the agent forks via new SessionService(cwd), so after /cd the source transcript can have been moved away from the storage root that branch/load later inspect. Please either avoid moving transcript storage for ACP logical cwd changes, or route branch/load/lifecycle operations through the session's current storage root.
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Added skipArtifactMigration: true to the ACP /cd call to relocateWorkingDirectory. When this option is set, the method skips prepareSessionArtifactMigration and does NOT change this.storage — artifacts (transcripts, recordings) remain in the original bound-workspace storage location. Only the logical cwd (this.cwd, this.targetDir) and runtime context (workspace roots, memory, file caches) are updated.
This means branch/load/lifecycle operations continue to find artifacts at the daemon-bound workspace's storage root, eliminating the state divergence.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
In addition to the inline comments, two items that can't be anchored to a specific diff line:
-
session_cwd_changedevent not registered in SDK —DAEMON_KNOWN_EVENT_TYPE_VALUESinpackages/sdk-typescript/src/daemon/events.tsdoes not includesession_cwd_changed. SDK clients will classify this event askind: 'unknown'and drop it. The PR description says SDK wrappers are deferred, but without event registration the bridge'sevents.publish()call is a dead emit today. Consider at minimum adding the event type to the SDK's known-values array so typed consumers can observe it. -
session_cdnot registered in capabilities —SERVE_CAPABILITY_REGISTRYinpackages/cli/src/serve/capabilities.tshas nosession_cdentry. Every other session mutation endpoint (session_branch,session_rewind,session_approval_mode_control, etc.) has a capability tag. SDK clients doing feature discovery viaGET /capabilitiescannot detect whether the daemon supports/cd.
— qwen3.7-max via Qwen Code /review
| return branchResult; | ||
| }, | ||
|
|
||
| async changeSessionCwd( |
There was a problem hiding this comment.
[Suggestion] changeSessionCwd has zero logging — no entry log, no success log, no warning when warnings[] is non-empty. For comparison, sendPrompt logs forward failures and mid-turn queue drops via writeStderrLine, and wraps the dispatch in a telemetry.withSpan. A directory-change mutation is operator-significant: when this breaks at 3 AM, there will be no trail to determine whether the /cd was attempted, succeeded, or left the session in a degraded state (memory/model-context refresh failures silently returned in warnings[]).
Consider adding at minimum:
debugLogger.info('changeSessionCwd', { sessionId, path: req.path })at entrydebugLogger.warn('changeSessionCwd warnings', { sessionId, warnings })whenwarnings.length > 0
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Fixed in 6bfc526.
Added writeStderrLine logging after successful cwd change: logs session ID, old → new path, and any warnings. This provides the operator trail for debugging ACP /cd mutations at 3 AM.
| oldDir: string, | ||
| opts?: { skipProcessChdir?: boolean }, | ||
| ): Promise<void> { | ||
| this.chatRecordingService?.finalize(); |
There was a problem hiding this comment.
[Suggestion] chatRecordingService.finalize() and flush() are called before moveCurrentSessionArtifacts (line 3078). If the artifact migration throws, the error propagates and relocateWorkingDirectory fails — but the recording service has already been finalized (auto-title controller aborted, title record flushed). With skipProcessChdir: true (the ACP /cd path), the process.chdir rollback is also skipped, so config state stays at the old directory while the recording service is in a post-finalize state.
This is a pre-existing ordering issue (not introduced by this PR), but the skipProcessChdir path makes it more likely to surface because artifact migration operates across a process that never physically changed directory. Consider moving finalize() to after the migration succeeds, or adding a resetAfterFinalize() on the catch path.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Resolved by the skipArtifactMigration fix.
With skipArtifactMigration: true, prepareSessionArtifactMigration is never called on the ACP /cd path — so finalize() + flush() are skipped entirely. The recording service remains in its normal running state. This issue only affected the old flow that attempted full artifact migration for ACP cd.
|
|
||
| return { sessionId, ...result }; | ||
| }, | ||
|
|
There was a problem hiding this comment.
[Suggestion] The PR adds 277 lines across 9 files (bridge method, agent handler, HTTP route, error class, config option) without any test coverage. The existing bridge.test.ts has extensive tests for branchSession (queue chaining, prompt-active guard, session_branched event, cleanup on failure) and sendPrompt (queue FIFO, abort, retry) — the same coverage pattern should apply to changeSessionCwd.
The PR description notes "unit tests for bridge/handler (follow-up PR)". Key paths that should be tested before shipping:
changeSessionCwdwaits for in-flight prompt, then executesCdWhilePromptActiveErrorpropagation through the HTTP layersession_cwd_changedevent published with correct payload andoriginatorClientId- Noop when target path equals current directory
skipProcessChdirinrelocateWorkingDirectory— verifyprocess.chdiris never called,this.cwdandthis.storageupdate correctly, and artifact-migration failure leaves config state consistent
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Acknowledged — test coverage will be added in a follow-up commit. The changeSessionCwd method now has identical queue chaining semantics to branchSession, so the test patterns for queue-FIFO, prompt-active-guard, and event broadcast from bridge.test.ts apply directly.
Critical fixes: - Do not overwrite entry.workspaceCwd — session stays listed under the daemon's bound workspace; logical cwd lives in agent Config only - Queue tail tied to raw extMethod settlement, not timeout — subsequent prompts cannot run against unknown cwd state after caller timeout - Add realpath equality check for skipProcessChdir path to guard against TOCTOU symlink swaps between trust check and state update - Skip artifact migration for ACP /cd (skipArtifactMigration option) — storage stays at bound workspace so branch/load/lifecycle remain consistent Suggestions addressed: - Add response shape validation for extMethod result - Rename CdWhilePromptActiveError HTTP code from 'session_busy' to 'cd_while_prompt_active' (avoids collision with SessionBusyError) - Add null-byte path validation in ACP sessionCd handler - Add operator logging on successful cwd change (warnings included)
…com/QwenLM/qwen-code into feat/phase-4c-api-request-breakdown # Conflicts: # packages/cli/src/serve/server.ts
| const relocation = await config.relocateWorkingDirectory( | ||
| canonicalPath, | ||
| canonicalPath, | ||
| { skipProcessChdir: true, skipArtifactMigration: true }, |
There was a problem hiding this comment.
[Suggestion] With skipProcessChdir: true, process.cwd() remains at the server's launch directory while config.cwd/config.targetDir point to the new session directory. Multiple code paths across packages/core use process.cwd() directly — e.g., prompts.ts:293 (git repository detection for system prompts), rule-parser.ts:1039-1040 (permission rule path context), destructiveCommands.ts:166 (destructive command CWD default). After ACP /cd, these resolve against the wrong directory.
The interactive /cd (cdCommand.ts) doesn't have this problem because it calls process.chdir(). Before this PR, ACP sessions never changed CWD, so process.cwd() was always correct for them.
Consider auditing direct process.cwd() usages in the prompt/permission/tool pipeline and migrating them to config.getCwd() or config.getTargetDir(). Alternatively, document this as a known limitation and track in a follow-up issue.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[qwen] Acknowledged — this is a valid known limitation.
With skipProcessChdir: true, process.cwd() stays at the daemon's launch directory while config.cwd / config.targetDir point to the session's new logical directory. The affected code paths (prompts.ts git repo detection, rule-parser.ts permission context, destructiveCommands.ts CWD default) all run inside the ACP child process which shares process.cwd() across sessions — changing it would break other sessions.
This is by design for now: the ACP /cd is a "logical cwd change" — tool execution and file resolution use config.getCwd() (already correct), while the few process.cwd() callsites that matter for ACP sessions should be migrated to config.getCwd() in a follow-up. Will file a tracking issue.
|
@qwen-code /triage |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
The /cd implementation follows established patterns well — queue chaining on promptQueue mirrors branchSession, skipProcessChdir/skipArtifactMigration are clean additive extension points, and error mapping follows existing conventions.
— 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.
Request changes: revert 23 unrelated cua-driver formatting files (Prettier noise from merge conflict resolution). The /cd feature itself is solid — once those files are cleaned up, this is ready to merge. See triage comments for full details.
Reverts 23 cua-driver files that picked up Prettier formatting noise during merge conflict resolution. Adds packages/cua-driver/ to .prettierignore since it's a vendored package whose content should not be auto-formatted by local hooks.
|
[qwen] Fixed in 4fa0456. Reverted all 23 cua-driver formatting files to match Fix: added |
|
Qwen Code review did not complete successfully: Qwen review timed out after 85 minutes. See workflow logs. |
|
@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. ✅
What this PR does
Adds the server-side
/cdcommand support for ACP (Agent Communication Protocol) multi-session architecture. When an ACP client sends aPOST /session/:id/cdrequest, the daemon validates the target directory, checks trust and sandbox policies, updates the session's logical working directory (without callingprocess.chdir()since the process is shared), and publishes asession_cwd_changedevent to all connected clients.This PR covers the server-side implementation only. SDK client wrappers and capability registration are deferred to a follow-up PR once the server-side behavior is validated end-to-end.
Why it's needed
ACP sessions currently cannot change their working directory. The
/cdcommand was only available in interactive mode. Multi-session ACP requires per-session logical CWD tracking without mutating the shared process state — this is the missing piece for issue #5677.Reviewer Test Plan
How to verify
POST /session/:id/cdwith{ "path": "/some/valid/dir" }.previousCwd,newCwd, andwarnings: [].directory_not_founddirectory_not_found("Not a directory")invalid_pathdirectory_not_trustedrestrictive_sandboxinvalid_client_id/cdqueues behind it and returns 200 after the prompt completes (not 409). This is intentional — ACP clients do not need retry logic, though the request may block for the duration of the active prompt.Evidence (Before & After)
N/A — new feature, no UI change. Verification is via HTTP API responses.
Tested on
Environment (optional)
npm run devwith ACP daemon mode.Risk & Scope
skipProcessChdirchangesConfig.relocateWorkingDirectory()signature — callers that depend on the physical CWD change are unaffected since the option defaults to false. The/cdrequest blocks unbounded while a prompt is active (no deadline); a bounded wait or fail-fast design is deferred to a follow-up if needed.restrictive_sandboxon macOS seatbelt.Linked Issues
Closes #5677
中文说明
为 ACP 多 session 架构添加服务端
/cd命令支持。当 ACP 客户端发送POST /session/:id/cd请求时，daemon 会验证目标目录、检查信任与沙箱策略、更新 session 的逻辑工作目录（不调用process.chdir()，因为进程是共享的），并向所有已连接客户端发布session_cwd_changed事件。本 PR 仅覆盖服务端实现。SDK 客户端封装和 capability 注册将在服务端行为经过端到端验证后在后续 PR 中添加。
队列行为：如果 prompt 正在执行，
/cd会排队等待 prompt 完成后再执行（而非返回 409）。这是有意为之 — ACP 客户端无需重试逻辑，但请求可能阻塞整个 prompt 时长。