fix(cli): use target dir for ACP core settings - #9897
Conversation
Load ACP core settings from the active target directory when clients omit cwd so worktree sessions write workspace settings into the worktree instead of the daemon process root. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Gate check below. Template — the three main sections are present (same shape as your recently merged #9888). The Problem — real, not theoretical. Linked issue #8138 is open, triaged ( Direction — aligned. Workspace settings should follow the active workspace, the same way file operations are already isolated inside worktrees. Size — 10 production lines (+6/−4 in Approach — minimal, and consistent with the file: six sibling call sites already do Risk — Moving on to code review. 🔍 中文说明感谢贡献!准入检查如下。 模板 —— 三个主要小节齐全(和你刚合并的 #9888 结构一致)。缺少 问题 —— 真实存在,不是理论加固。关联 issue #8138 处于 open 状态、已分诊( 方向 —— 对齐。workspace 设置理应跟随当前工作区,正如文件操作已经在 worktree 内做了隔离。 规模 —— 生产代码 10 行( 方案 —— 克制,且与本文件现有写法一致:同文件已有 6 处调用 风险 —— 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal, written before reading the diff: swap the What I verified in the code:
No critical findings. Non-blocking, carried from Stage 1: the ~eight sibling Files changed (2 of 2 shown)
TestingWhat this pass carries: the PR's own CI checks, quoted below. Per triage rules I don't execute PR code, and there is no local tmux run here — the changed surface is the ACP The unit suite and the Real daemon E2E job are still running; the windows/macos/integration jobs are skipped pending fork-CI approval, so the table below is a mid-flight snapshot. Not verified: end-to-end persistence in a real daemon + worktree ("the file actually lands in the worktree's Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The 中文说明代码审查先说我不看 diff 时的独立方案:恰好就是在 issue 点名的两个处理器里,把 核实过的要点:
无阻断性问题。非阻断、承接 Stage 1:同一 switch 里约 8 个 测试本次证据:PR 自身 CI 检查(下表)。按分诊规则不执行 PR 代码;也没有本地 tmux 实测——改动面是 ACP 单测套件与 Real daemon E2E 仍在运行;windows/macos/集成测试因 fork CI 需审批而 skipped,下表是进行中的快照。未验证:真实 daemon + worktree 下的端到端落盘("文件确实写进 worktree 的 CI 表格区域将由 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — small, well-aimed fix that does exactly what the maintainer-triaged issue asks, matches the file's own established pattern, and ships tests that pin the wiring; the remaining reservations are the unfixed sibling handlers and an end-to-end daemon claim that rests on those wiring tests. Reflection: my independent proposal and the diff are the same change — I found no simpler path, and no reason one exists. The problem is real and maintainer-accepted (#8138, Two things keep this from a 5: roughly eight sibling CI on the reviewed commit is still running (unit suite and Real daemon E2E), so approval is deferred until CI lands green on 中文说明置信度:4/5 —— 小而准的修复,完全对应维护者已分诊的 issue,与本文件既有模式一致,并附带钉住接线的测试;剩余保留意见是尚未修复的兄弟处理器,以及端到端 daemon 结论目前由接线测试承载。 复盘:我的独立方案与 diff 完全一致——没找到更简单的路径,也不认为存在。问题真实且已被维护者认可(#8138, 没给到 5 分的两点:同一 switch 里约 8 个 被审提交的 CI 仍在运行(单测套件与 Real daemon E2E),因此批准推迟到 CI 在该提交上全绿之后。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
| } | ||
| case 'qwen/settings/getCore': { | ||
| const settings = loadSettings(cwd); | ||
| const coreSettingsCwd = requestedCwd || this.config.getTargetDir(); |
There was a problem hiding this comment.
[Critical] This fallback swap is observably a no-op in every reachable ACP topology, so the PR does not fix #8138. this.config is the bootstrap Config built from process.cwd() (gemini.tsx:853 → targetDir: cwd) and is never relocated: the only relocateWorkingDirectory call inside the agent mutates session.getConfig() (acpAgent.ts:9514 → 9718) with skipProcessChdir: true, sessions get fresh Configs via loadCliConfig, --worktree + --acp is rejected at startup, and nothing in the ACP process calls process.chdir. So this.config.getTargetDir() === process.cwd() for the agent process's whole life, and settings I/O still lands in <launch-dir>/.qwen/settings.json — the exact symptom the issue reports. The endorsement of getTargetDir() in the issue thread rested on a static-analysis-only claim that it tracks the worktree, which this trace disproves.
The new tests pass only because they stub getTargetDir to /worktree/.qwen — a value production never produces. A/B probe in a scratch tree (PR arm with production wiring vs reverted base):
PR arm : PROBE-RESULT process.cwd()=.../packages/cli getCoreCalls=[".../packages/cli"] redirectedToWorktree=false
BASE : identical PROBE-RESULT; the two new tests flip to FAILED (expected spy called with '/worktree/.qwen')
Resolve the settings directory from the session that owns the request instead — e.g. thread sessionId through the qwen/settings/* methods and use session.getConfig().getTargetDir() (the object ACP worktree relocation actually mutates) — and add a test that simulates a relocated session config and shows getCore/setCoreValue actually follow it.
中文说明
这个 fallback 替换在所有可达的 ACP 拓扑中都没有可观察的行为变化(no-op),因此本 PR 并没有修复 #8138。this.config 是 bootstrap Config,由 process.cwd() 构建(gemini.tsx:853 → targetDir: cwd),且从不被重定位:agent 内唯一的 relocateWorkingDirectory 调用只修改 session.getConfig()(acpAgent.ts:9514 → 9718,且 skipProcessChdir: true),session 通过 loadCliConfig 获得全新的 Config,--worktree + --acp 在启动时被拒绝,ACP 进程中没有任何地方调用 process.chdir。因此 this.config.getTargetDir() === process.cwd() 在整个 agent 进程生命周期内恒成立,settings 读写仍然落在 <启动目录>/.qwen/settings.json —— 正是 issue 报告的症状。issue 讨论中对 getTargetDir() 的认可基于"它会跟随 worktree"的纯静态分析结论,而上述代码追踪证明该结论不成立。
新增的两个测试之所以通过,仅仅是因为它们把 getTargetDir 打桩为 /worktree/.qwen —— 生产环境从不产生这个值。在 scratch tree 中做的 A/B 探针(PR 侧按生产接线 vs 回退 base):
PR arm : PROBE-RESULT process.cwd()=.../packages/cli getCoreCalls=[".../packages/cli"] redirectedToWorktree=false
BASE : PROBE-RESULT 完全相同;两个新测试变为 FAILED(期望 spy 被以 '/worktree/.qwen' 调用)
建议改为从拥有该请求的 session 解析 settings 目录 —— 例如给 qwen/settings/* 方法透传 sessionId,使用 session.getConfig().getTargetDir()(ACP worktree 重定位实际修改的对象)—— 并新增一个模拟已重定位 session config 的测试,证明 getCore/setCoreValue 确实跟随它。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const settings = makeCoreSettings(); | ||
| const { agent, agentPromise } = await bootCoreSettingsAgent(settings); | ||
|
|
||
| await agent.extMethod('qwen/settings/getCore', {}); |
There was a problem hiding this comment.
[Suggestion] The requestedCwd || half of this fallback is pinned by zero tests: all 11 getCore/setCoreValue call sites in the suite omit cwd, and loadSettings is mocked to return the same object regardless of argument. The mutation requestedCwd || this.config.getTargetDir() → this.config.getTargetDir() survives the entire settings suite (probe-verified: Tests 25 passed | 440 skipped). Since the desktop client always sends an explicit cwd, that regression would silently redirect desktop settings calls to the config target dir while the suite stays green. Add a variant per method in which the explicit cwd must win:
it('qwen/settings/getCore prefers explicit cwd over config target dir', async () => {
mockConfig.getTargetDir = vi.fn().mockReturnValue('/worktree/project');
const settings = makeCoreSettings();
const { agent, agentPromise } = await bootCoreSettingsAgent(settings);
await agent.extMethod('qwen/settings/getCore', { cwd: '/explicit/dir' });
expect(loadSettings).toHaveBeenCalledWith('/explicit/dir');
mockConnectionState.resolve();
await agentPromise;
});中文说明
这个 fallback 中 requestedCwd || 的一半没有任何测试覆盖:套件中全部 11 处 getCore/setCoreValue 调用都省略了 cwd,且 loadSettings 被 mock 为无论参数如何都返回同一对象。变异 requestedCwd || this.config.getTargetDir() → this.config.getTargetDir() 在整个 settings 套件中存活(已通过探针验证:Tests 25 passed | 440 skipped)。由于 desktop 客户端总是显式传入 cwd,这种回归会在套件全绿的情况下悄悄把 desktop 的 settings 调用重定向到 config target dir。建议为每个方法补一个"显式 cwd 必须优先"的变体测试(代码见上)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const settings = loadSettings(coreSettingsCwd); | ||
| this.settings = settings; | ||
| return this.buildCoreSettings(settings, cwd); | ||
| return this.buildCoreSettings(settings, coreSettingsCwd); |
There was a problem hiding this comment.
[Suggestion] The diff changes four sites (loadSettings + buildCoreSettings in each handler), but these new tests assert only the loadSettings calls. buildCoreSettings feeds its second argument to new ExtensionManager({ workspaceDir: cwd, ... }), and the module-level ExtensionManager mock ignores constructor arguments while mockExtensionManagerState.extensions resets to [] — so the mutation buildCoreSettings(settings, coreSettingsCwd) → buildCoreSettings(settings, cwd) compiles (cwd is still in scope from extMethodInternal) and survives all tests (probe-verified). A partial revert would load settings from the worktree while resolving workspace extensions against process.cwd(), undetected. Assert the second argument as well:
expect(vi.mocked(ExtensionManager)).toHaveBeenCalledWith(
expect.objectContaining({ workspaceDir: targetDir }),
);中文说明
本次改动修改了四处(每个 handler 中的 loadSettings + buildCoreSettings),但这些新测试只断言了 loadSettings 的调用。buildCoreSettings 会把第二个参数传给 new ExtensionManager({ workspaceDir: cwd, ... }),而模块级 ExtensionManager mock 忽略构造参数,mockExtensionManagerState.extensions 又被重置为 [] —— 因此变异 buildCoreSettings(settings, coreSettingsCwd) → buildCoreSettings(settings, cwd) 可以编译(cwd 在 extMethodInternal 作用域内仍然可见)且在所有测试中存活(已通过探针验证)。部分回退时会从 worktree 加载 settings、却按 process.cwd() 解析 workspace 扩展,且不会被察觉。建议同时断言第二个参数(代码见上)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Use the live session config when ACP core settings requests omit cwd, while preserving explicit cwd priority for desktop callers. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
Thanks for the fix! This overlaps with #8152, which was opened earlier (Jul 30) for the same issue #8138 and covers worktree-aware settings/context-file resolution. To keep the discussion in one place, I'll close this in favor of that one — thanks again for the effort, and you're very welcome to pick up any other unclaimed issues. 🙏 |
What this PR does
cwd.qwen/settings/getCoreandqwen/settings/setCoreValueso omittedcwdresolves throughconfig.getTargetDir().Why it's needed
Worktree-backed ACP sessions can run from a daemon process root that differs from the active workspace. Falling back to
process.cwd()makes core settings reads and writes target the wrong.qwen/settings.json; using the target directory keeps workspace settings scoped to the active worktree.Fixes #8138.
Reviewer Test Plan
cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.tscd packages/cli && npm run typecheck中文说明
本 PR 做了什么
cwd时,改用当前 Config 的 target directory。qwen/settings/getCore和qwen/settings/setCoreValue的回归测试,覆盖省略cwd时走config.getTargetDir()的路径。为什么需要
worktree 场景下,ACP daemon 的进程根目录可能不是当前工作区。继续 fallback 到
process.cwd()会把 core settings 读写到错误的.qwen/settings.json;使用 target directory 后,workspace 设置会落在当前 worktree。修复 #8138。
验证
cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.tscd packages/cli && npm run typecheck