refactor(cli): route every per-request runtime-root pin through runWithPinnedRuntimeBaseDir - #10988
Conversation
…thPinnedRuntimeBaseDir Follow-up to QwenLM#10095 (review item D2-1 + maintainer nit). QwenAgent already had a private runWithPinnedRuntimeBaseDir helper, but five handlers composed the loadSettingsCached -> runWithAcpRuntimeOutputDir routing by hand instead: unstable_listSessions, deleteSession, renameSession (the three QwenLM#10095 fixed) plus the older qwen/status/session/transcript and sessionTurnStatus readers. Hand-composed routing is exactly the shape that picked up the stale this.settings cache in QwenLM#10095, so make the helper the single choke point and document why it exists. Pure delegation swap: the helper's body is runWithAcpRuntimeOutputDir(settings, cwd, operation), so no runtime behavior changes. The two wide sites name their inline callbacks (readTranscriptPage / readSettledTurnResult) so the reroute does not re-indent 60-line bodies. Because the two spellings reach the same function, no behavioral test can distinguish them; a source-level test pins the invariant instead (the only remaining runWithAcpRuntimeOutputDir call is the helper's own delegation), following the cli.test.ts source-pin precedent. Mutation check, one site at a time: re-adding a direct call is killed only by the new pin; routing this.settings through the helper at deleteSession is killed only by the QwenLM#10095 deleteSession test; making the helper itself use this.settings is killed by exactly the three QwenLM#10095 tests. Also rename the describe block that holds those three tests -- it was named "extMethod renameSession routing" but covers rename / delete / list / branch / close, so a listSessions failure reported under "renameSession routing" (wenshao's nit on QwenLM#10095). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA
|
Thanks for the follow-up! Template looks good ✓ — every section filled in, Problem: this is not a bug fix, so "no reproduction" is the right answer here rather than a gap. More importantly the motivation checks out, and it checks out better than the description claims. I read back through #10095: it merged on 2026-09-03, and wenshao recorded both items this PR closes, in writing, on that thread — D2-1 ("folding the hand-composed Direction: aligned. #10465 is a real merged precedent of the same shape (closing deferred items recorded in #9930's review rounds), so this is an established pattern here rather than a one-off. Nothing touches auth, model selection, telemetry, release, or a public contract — the helper is a pass-through to the same function these five sites already called, so route ownership and failure semantics are unchanged. Size: not applicable for the core-module gate. Approach: scope feels right and genuinely minimal — five call-site swaps, one doc comment explaining why the choke point exists, one invariant test, one describe rename. No drive-by edits and no formatting churn hiding in it. I also checked the obvious alternative before reading the diff: enforcing this with the repo's existing Risk: Stage 1e matched. Moving on to code review. 🔍 中文说明感谢这个跟进 PR！ 模板完整 ✓——各节都填了，无行为变化的重构在 Before/After 下正确写了 问题： 这不是修 bug，所以"没有复现"在这里是正确答案，而不是缺口。更重要的是动机经得起核对，而且比 PR 描述里说的更硬。我回看了 #10095：它已于 2026-09-03 合并，wenshao 在那个线程里白纸黑字记录了本 PR 关闭的两条项——D2-1（"把手写的 方向： 对齐。#10465 是同一形态的真实已合并先例（关闭 #9930 评审轮次记录的延后项），说明这在本仓库是既有做法而不是一次性动作。不涉及 auth、模型选择、telemetry、发布或公共契约——辅助方法只是转发到这五个调用点原本就调用的同一个函数，路由归属与失败语义都没变。 规模： 核心模块闸门不适用。 方案： 范围合理且确实最小——五处调用点替换、一段说明该入口为何存在的注释、一条不变量测试、一次 describe 改名。没有夹带无关改动，也没有藏着纯格式化改动。我在看 diff 之前也先核对了最明显的替代方案：用仓库已有的 风险： Stage 1e 命中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI read the title and the "Why it's needed" section first and wrote down what I would have done before opening the diff: find every direct call to the context function, check which settings each one passes, route all of them through the existing private helper, then pin the invariant somehow. That is what this PR does, so we agree on the shape. The one place I would have gone differently — an ESLint The delegation swap is behavior-preserving. I checked this against The choke-point claim holds repo-wide, not just in this file. No production file outside The existing #10095 pins really do survive the reroute. Those three regression tests mock The describe rename is accurate, not just cosmetic. The block spans lines 20206–21705 and contains rename (20525), delete (20666) and list (20741) tests plus branch tests (20871–21437) and close tests (21437–21621). So "session-management routing (rename / delete / list / branch / close)" describes what is actually in there, which is what wenshao asked for on #10095. On the enforcement mechanism — I looked for the more idiomatic route before accepting the source pin. The repo already bans specific call expressions through One thing I'd change before mergeThe new test reads its source with That matters because Nits (non-blocking)
sequenceDiagram
participant P1 as session handlers 5 sites
participant P2 as runWithPinnedRuntimeBaseDir
participant P3 as runWithAcpRuntimeOutputDir
participant P4 as Storage.runWithRuntimeBaseDir
P1->>P2: pin this request runtime root
P2->>P3: pass through unchanged
P3->>P4: runtimeOutputDir plus cwd
P4-->>P1: operation result
The single path above is the point of the PR: on Test evidenceThis was an unattended CI run, so I did not build or execute anything from this PR — the gate is static and the evidence below is this PR's own CI, read through the API for commit
The two checks that matter most for this diff are still running. The one red check is not this PR's.
I'm classifying that from the diff and the check identity, not from anything the log asserts about itself. Not verified: that the new source-pin test executes and passes, and that the rerouted sites keep #10095's three regression tests green. Both rest on the author's local macOS run (602/602, plus a mutation matrix), which is the author's claim and not evidence I re-ran — this gate never executes PR code. The ubuntu unit job settles it, and it is still in flight. Sandboxed verification would close that gap now rather than waiting: Real-scenario testing: N/A — unattended CI run, and a pure delegation refactor with no user-visible behavior to drive. 中文说明代码审查我先只读了标题和"为什么需要"，在看 diff 之前写下自己会怎么做：找出所有对 context 函数的直接调用，确认每处传的 settings，全部改道经过已有的私有辅助方法，再想办法钉住这个不变量。这个 PR 做的正是这件事，所以整体形态我认同。唯一我会走不同路的地方——用 ESLint 的 委托替换确实保持行为不变。 这一点我是对着 "唯一入口"这个说法在整个仓库成立，不只是这一个文件。 #10095 原有的三条钉确实不受改道影响。 那三条回归测试在模块级 mock 了 describe 改名是准确的，不只是外观。 该块跨 20206–21705 行，里面有 rename（20525）、delete（20666）、list（20741），也有 branch（20871–21437）和 close（21437–21621）的测试。所以"session-management routing (rename / delete / list / branch / close)"描述的是里面真实存在的内容，这正是 wenshao 在 #10095 上提的要求。 关于约束机制——我在接受源码钉之前先找了更地道的路子。仓库已经通过 合并前我会改的一处新测试用 之所以要紧，是因为 小问题（非阻塞）
上面那张时序图就是这个 PR 的意义所在：在 测试证据本轮是无人值守的 CI 运行，所以我没有构建或执行本 PR 的任何代码——闸门是静态的，下面的证据来自 PR 自己的 CI，通过 API 读取 commit 对这个 diff 最关键的两个检查仍在运行。 唯一的红检查不是本 PR 造成的。 未验证： 新源码钉是否真的执行并通过，以及改道后 #10095 的三条回归测试是否仍绿。这两点目前只有作者本地 macOS 的结果（602/602 加一份变异矩阵）支撑，那是作者的说法，不是我再跑一遍的证据——本闸门从不执行 PR 代码。ubuntu 单元测试 job 能定论，它还在跑。 沙箱验证可以现在就补上这个缺口而不必等待： 真实场景测试：N/A——无人值守 CI 运行，且本 PR 是纯委托重构，没有可驱动的用户可见行为。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the refactor itself is clean and I would merge it; the 3 is the fork- Stepping back: this does the one thing worth doing after a bug like #10095. The original defect was three handlers each independently deciding "which settings pin this operation" and getting it wrong — the shape of the code was the bug's enabling condition. Collapsing six direct calls into one documented choke point removes that condition rather than patching its symptoms, and the doc comment tells the next person why the helper exists instead of leaving them to rediscover it from a git blame. If I picked this file up in six months I would thank whoever wrote this, which is the test I actually care about. It also matches what I would have done independently, which is not nothing: I wrote my proposal down before reading the diff, and the only place I diverged — enforcing the invariant with an ESLint selector instead of a source-reading test — does not survive contact with the flat config, because the helper's own legitimate delegation lives in the same file as the calls a selector would ban. The author chose the workable option and cited real precedent for it. Scope is minimal, there is no drive-by churn, and every line serves the stated goal. On the "does this problem exist" question that normally sinks refactors: it does, and I checked rather than accepted the framing. #10095 merged on 2026-09-03, and wenshao recorded both items this PR closes on that thread — the deferred D2-1 fold-into-helper item ("I would not want it in this diff") and the describe-block nit ("it will mislead the next person grepping a failure"). This is sanctioned follow-up work on an observed bug, not speculative tidying. On the volume question: the author has exactly one open PR and seven merged ones, so this is not a spray of plausible-looking diffs to wear a reviewer down — I'm judging it on its merits and it holds up. Why I'm not approving. This is a cross-repository PR whose title is a The two reservations a maintainer should weigh:
On the escalation itself: the guardrail wants this in a named maintainer's hands, and I could not resolve one deterministically. If a maintainer wants the load-bearing claim pinned before deciding rather than after, ⏸️ Deferring to a maintainer — clean refactor, blocked only by the fork- 中文说明Confidence: 3/5——重构本身是干净的，我会合并它；这个 3 分来自 fork 退一步看：这个 PR 做的正是 #10095 那类 bug 之后唯一值得做的事。原缺陷是三个处理器各自独立决定"用哪份 settings 钉这次操作"并且都决定错了——代码的形态本身就是 bug 的成因。把六处直接调用收拢成一个带注释的唯一入口，是消除这个成因，而不是补症状；注释也告诉下一个人这个辅助方法为何存在，而不必靠 git blame 重新发现。如果六个月后我接手这个文件，我会感谢写这段的人——这才是我真正在意的检验。 它也与我独立想到的做法一致，这点并非无关紧要：我在看 diff 之前先写下了自己的方案，唯一分歧处——用 ESLint 选择器而不是读源码的测试来钉不变量——在 flat config 面前站不住，因为辅助方法自己那次合法委托与选择器要禁的调用在同一个文件里。作者选了可行的那条路，并且引用了真实先例。范围最小，没有夹带无关改动，每一行都服务于既定目标。 关于通常会让重构沉掉的"这个问题真的存在吗"：存在，而且我是去核过的，没有接受现成叙事。#10095 已于 2026-09-03 合并，wenshao 在那个线程里记录了本 PR 关闭的两条项——延后的 D2-1 折进辅助方法（"我也不希望它进这个 diff"）和 describe 块的小建议（"会误导下一个 grep 失败信息的人"）。这是针对已观测 bug、经认可的后续工作，不是投机性整洁。关于数量的疑问：作者当前只有这一个 open PR，另有七个已合并，所以这不是一堆看着合理、意在磨软审查者的 diff——我是就其本身判断的，它站得住。 为什么我不 approve。 这是一个跨仓库（fork）PR，标题是 维护者需要权衡的两条保留意见：
关于上报本身： 护栏希望它落到一位具名维护者手里，而我无法确定性地解析出这个人。本轮 如果维护者想在决定之前而不是之后把承重主张钉住， ⏸️ 转交维护者——重构干净，只被 fork — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
…e its failures (round-1) Triage round 1 on QwenLM#10988 asked for two things on the new source pin: - Read the source with the spelling every other source-text assertion in packages/cli uses (readFileSync('src/...', 'utf8') against vitest's package-root cwd) instead of new URL(..., import.meta.url), which cli.test.ts warns Vite may rewrite to a non-file URL under vitest. - Make the failure self-locating. The pin now scans line by line, skips comment lines, and puts "<line>: <text>" for every direct call into the assertion message, so re-adding a direct call reports e.g. "12099: const success = await runWithAcpRuntimeOutputDir(" next to the helper's own delegation line. A doc comment that spells the call with a paren no longer trips it. The surviving call is asserted to be the helper's delegation body, which subsumes the previous "helper still exists" check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA
|
Round-1 items addressed in
Re-verified on Agree with the CVE-audit classification: the job printed @wenshao this is the D2-1 + describe-rename follow-up you recorded on #10095; the gate deferred it to a human on the fork- |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 601 passed — this review observed 28138 passed; 599 passed — this review observed 28138 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan（非阻断）：601 passed — this review observed 28138 passed; 599 passed — this review observed 28138 passed。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * operation to the settings loaded for THAT request's cwd. Every handler | ||
| * that resolves session storage for a caller-supplied cwd goes through | ||
| * here rather than calling `runWithAcpRuntimeOutputDir` directly, so the | ||
| * "which settings pin this operation" decision lives in one place — the |
There was a problem hiding this comment.
[Suggestion] R1-1: The new doc comment claims the "which settings pin this operation" decision "lives in one place", but runWithPinnedRuntimeBaseDir is a pure pass-through that still receives settings from every caller — that decision remains in the call sites, and the new source-pin test pins only the call shape, not the decision. If a future session-management handler is written as this.runWithPinnedRuntimeBaseDir(this.settings, cwd, ...), it compiles and keeps the new pin green, yet in a multi-workspace daemon whose this.settings cache holds another workspace's advanced.runtimeOutputDir it pins the wrong runtime root — unstable_listSessions returns an empty/foreign list, and deleteSession reports success:false for an existing session or deletes a stale same-id copy under the wrong root. The #10095 bug class regresses while the guard created to prevent it reports safe.
Witness:
scratch-tree probe: added probeStaleSettingsPin(cwd) { return this.runWithPinnedRuntimeBaseDir(this.settings, cwd, () => null); }
plus one direct-call probe. The pin test failed listing ONLY the direct call:
Direct calls at:
4572: return runWithAcpRuntimeOutputDir(settings, cwd, operation);
4589: return runWithAcpRuntimeOutputDir(settings, cwd, operation);
(the stale-settings call appears nowhere in the list)
baseline on unmodified PR: Tests 1 passed | 601 skipped
Put the decision where the comment says it lives: add a per-request variant that calls loadSettingsCached(cwd) itself, and migrate the five per-request sites (acpAgent.ts:5786, 8926, 11877, 12099, 12150):
private runWithPinnedRuntimeBaseDirForRequest<T>(cwd: string, operation: () => T): T {
return this.runWithPinnedRuntimeBaseDir(loadSettingsCached(cwd), cwd, operation);
}Minimal fallback if the shape stays: rewrite the doc comment and the test comment to claim only what the shape guarantees — "the wrapper call lives in one place" — not that the settings decision does.
The settings parameter cannot be removed from the shared helper: createWorkspaceMcpDiscoveryConfig(settings) at acpAgent.ts:3887 is fed deliberately workspace-scoped settings (loadSettings(this.config.getTargetDir()), acpAgent.ts:3948) and assertLiveSessionScope at acpAgent.ts:4613 receives caller-scoped settings — the fix must be an added per-request variant, not a signature change. If you add the variant, please seed this.settings with a foreign workspace's settings, invoke one per-request pin for cwd, and assert the settings argument reaching the mocked runWithAcpRuntimeOutputDir equals loadSettingsCached(cwd) (mirroring acpAgent.test.ts:20682-20768) — reverting any converted handler to pass this.settings must turn that test red while the source pin stays green; please confirm by running that mutation.
中文说明
新增的 doc 注释声称 "which settings pin this operation"（用哪份 settings 钉住本次操作）的决定 "lives in one place"（只在一处做出），但 runWithPinnedRuntimeBaseDir 是一个纯透传，仍然从每个调用方接收 settings —— 这个决定实际上留在各调用点，新加的源码级 pin 测试也只钉住了调用形态，没钉住这个决定。如果将来某个会话管理处理器写成 this.runWithPinnedRuntimeBaseDir(this.settings, cwd, ...)，它能编译、新 pin 测试也保持绿色，但在多 workspace 守护进程里（this.settings 缓存装着另一个 workspace 的 advanced.runtimeOutputDir）会钉错运行时根目录 —— unstable_listSessions 返回空/别的 workspace 的列表，deleteSession 对存在的会话报 success:false，或者删掉错误根目录下同 id 的陈旧副本。#10095 那类 bug 回归了，而为防止它而建的守卫却报告安全。
建议：把决定放到注释所说的那个位置 —— 新增一个自己调用 loadSettingsCached(cwd) 的按请求变体，并把五个按请求调用点（acpAgent.ts:5786、8926、11877、12099、12150）迁移过去。若保持现有形态的最小替代方案：改写 doc 注释和测试注释，只声称该形态真正保证的内容 —— "wrapper 调用集中在一处" —— 而不是 settings 的决定也集中了。
settings 参数不能从共享辅助方法里移除：acpAgent.ts:3887 的 createWorkspaceMcpDiscoveryConfig(settings) 传入的是刻意的 workspace 级 settings（loadSettings(this.config.getTargetDir())，acpAgent.ts:3948），acpAgent.ts:4613 的 assertLiveSessionScope 接收调用方作用域的 settings —— 所以修复必须是新增按请求变体，而不是改签名。若新增变体，请用另一个 workspace 的 settings 塞进 this.settings，对 cwd 发起一次按请求 pin，并断言到达被 mock 的 runWithAcpRuntimeOutputDir 的 settings 参数等于 loadSettingsCached(cwd)（参照 acpAgent.test.ts:20682-20768）—— 把任一已转换的处理器还原为传 this.settings 必须让该测试变红，而源码 pin 保持绿色；请跑这个变异确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| } as Record<string, unknown>; | ||
| }); | ||
| }; | ||
| return await this.runWithPinnedRuntimeBaseDir( |
There was a problem hiding this comment.
[Suggestion] R1-2: Of the five handlers this PR re-routes, sessionTranscript (call at acpAgent.ts:8926) and sessionTurnStatus (call at acpAgent.ts:11877) have no behavioral test pinning which settings/cwd reach the runtime-root pin — every runWithAcpRuntimeOutputDir mock-call-argument assertion lives in the deleteSession / renameSession / unstable_listSessions tests, and the added source-level test pins call shape only. If a future edit resolves settings for either of these two handlers from this.settings instead of loadSettingsCached(cwd) — the exact stale-cache regression #10095 fixed — every test stays green, and in a multi-workspace daemon the transcript page / settled turn result is read under the wrong runtime root, returning an empty or foreign-workspace result for the request's cwd.
Witness:
sweep of acpAgent.test.ts — runWithAcpRuntimeOutputDir references:
854 (module mock factory), 1084 (import),
20682-20768 (mock.calls[0]![0]/[1] assertions — deleteSession ~20690,
renameSession ~20730, unstable_listSessions ~20764),
29920-29951 (the new pin test — pins call shape only)
pin-argument assertions for sessionTranscript/sessionTurnStatus: 0
pattern runs green: Tests 45 passed | 557 skipped
Add two behavioral tests mirroring the existing per-request pattern (acpAgent.test.ts:20667-20770): mock loadSettings to return per-request settings, invoke agent.extMethod(SERVE_STATUS_EXT_METHODS.sessionTranscript, { cwd: '/tmp/workspace-a', sessionId, ... }) and agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionTurnStatus, { cwd: '/tmp/workspace-a', ... }), then assert vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![0] is the per-request settings and mock.calls[0]![1] is '/tmp/workspace-a'. The new tests must go red if loadSettingsCached(cwd) in the sessionTranscript handler (acpAgent.ts:8871) or the sessionTurnStatus handler (acpAgent.ts:11802) is replaced with this.settings — please confirm by running that mutation.
中文说明
本 PR 改道的五个处理器中，sessionTranscript（调用在 acpAgent.ts:8926）和 sessionTurnStatus（调用在 acpAgent.ts:11877）没有行为测试钉住到达运行时根 pin 的 settings/cwd —— 所有对 runWithAcpRuntimeOutputDir mock 调用参数的断言都在 deleteSession / renameSession / unstable_listSessions 的测试里，而新增的源码级测试只钉调用形态。如果将来某次修改把这两个处理器之一的 settings 来源从 loadSettingsCached(cwd) 换成 this.settings —— 正是 #10095 修复的陈旧缓存回归 —— 所有测试都保持绿色，而在多 workspace 守护进程里，transcript 分页 / settled turn 结果会在错误的运行时根下读取，对该请求的 cwd 返回空或别的 workspace 的结果。
建议：仿照现有按请求模式（acpAgent.test.ts:20667-20770）补两条行为测试：mock loadSettings 返回按请求的 settings，调用 agent.extMethod(SERVE_STATUS_EXT_METHODS.sessionTranscript, { cwd: '/tmp/workspace-a', sessionId, ... }) 和 agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionTurnStatus, { cwd: '/tmp/workspace-a', ... })，然后断言 vi.mocked(runWithAcpRuntimeOutputDir).mock.calls[0]![0] 是按请求的 settings、mock.calls[0]![1] 是 '/tmp/workspace-a'。若把 sessionTranscript 处理器（acpAgent.ts:8871）或 sessionTurnStatus 处理器（acpAgent.ts:11802）里的 loadSettingsCached(cwd) 换成 this.settings，新测试必须变红 —— 请跑这个变异确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| .map((line, index) => ({ line: index + 1, text: line.trim() })) | ||
| .filter( | ||
| ({ text }) => | ||
| /\brunWithAcpRuntimeOutputDir\(/.test(text) && |
There was a problem hiding this comment.
[Suggestion] R1-3: The choke-point pin only matches the literal identifier immediately followed by ( on one line, so an aliased or indirect call to runWithAcpRuntimeOutputDir is invisible to it and the property it advertises — every per-request pin goes through runWithPinnedRuntimeBaseDir — is not enforced. A future handler that binds const pin = runWithAcpRuntimeOutputDir; and calls pin(this.settings, cwd, ...) resurrects the stale-cache bug class of #10095 (delete/rename/list of a caller-supplied cwd runs against another workspace's advanced.runtimeOutputDir) while this pin test stays green.
Witness:
scratch-tree mutations against the real vitest harness:
baseline (unmodified PR): Tests 1 passed | 601 skipped
alias-only mutant: test stays GREEN (alias invisible to the regex)
alias + direct-call mutants: red, listing only the direct call
(the `const pin = ...` line appears nowhere)
strengthened regex (\b) + import skip: RED on alias mutant
("4580: const pin = runWithAcpRuntimeOutputDir;"),
green on intact source
Match identifier occurrences rather than same-line calls only, so const pin = runWithAcpRuntimeOutputDir; itself becomes a flagged occurrence:
| /\brunWithAcpRuntimeOutputDir\(/.test(text) && | |
| /\brunWithAcpRuntimeOutputDir\b/.test(text) && | |
| !text.startsWith('import ') && |
Loosening the regex makes the import line at acpAgent.ts:320 (import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js';) newly match, so the import skip is part of the fix; the helper's delegation at acpAgent.ts:4572 remains the sole expected element. Please re-apply the alias probe (const pin = runWithAcpRuntimeOutputDir; return pin(...)) and confirm the strengthened test goes red naming that line, then remove the probe and confirm it is green again.
中文说明
这个 choke-point pin 只匹配同一行里紧跟 ( 的字面标识符，因此对 runWithAcpRuntimeOutputDir 的别名或间接调用是不可见的，它所宣称的性质 —— 每个按请求的 pin 都经过 runWithPinnedRuntimeBaseDir —— 实际上没有被强制。将来某个处理器若绑定 const pin = runWithAcpRuntimeOutputDir; 再调用 pin(this.settings, cwd, ...)，会让 #10095 的陈旧缓存 bug 类复活（对调用方给定 cwd 的 delete/rename/list 会跑在另一个 workspace 的 advanced.runtimeOutputDir 下），而这个 pin 测试保持绿色。
建议：匹配标识符的出现而不是仅匹配同行调用，让 const pin = runWithAcpRuntimeOutputDir; 本身也成为被标记的出现。注意：放宽正则后，acpAgent.ts:320 的 import 行（import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js';）会新匹配上，所以跳过 import 行是修复的一部分；辅助方法在 acpAgent.ts:4572 的委托仍是唯一期望的元素。请重新加上别名探针（const pin = runWithAcpRuntimeOutputDir; return pin(...)）确认加强后的测试变红并指名该行，移除探针后确认恢复绿色。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…in (round-1 R1-1/R1-2/R1-3) review-pr round 1 on QwenLM#10988 showed the shared helper still let a handler hand it `this.settings`, so the doc comment's "decision lives in one place" was not true and the source pin only checked call shape. - R1-1: add runWithPinnedRuntimeBaseDirForRequest(cwd, operation): it loads the settings for the request's cwd itself and hands them to the operation, so a per-request handler has no settings parameter to get wrong. All five per-request sites (unstable_listSessions, deleteSession, renameSession, sessionTranscript, sessionTurnStatus) now use it; the three-argument helper stays for callers that hold deliberately scoped settings (workspace MCP discovery, live-session scope checks, session creation). Both doc comments now claim only what each helper guarantees. - R1-2: add behavioral pins for the two handlers that had none: sessionTurnStatus and qwen/status/session/transcript each assert the settings/cwd reaching the mocked context function are the request's (the transcript test also checks the replay config was seeded from the request's settings, distinguished by outputLanguage). - R1-3: the source pin matches identifier occurrences, not only same-line calls, and skips import lines, so an alias (`const pin = runWithAcpRuntimeOutputDir;`) is flagged with its line. Mutation matrix, one mutant at a time, whole file each run (604 tests): deleteSession -> this.settings via 3-arg helper: 1 failed (its pin) sessionTranscript -> this.settings: 3 failed (new pin + 2 replay-config tests) sessionTurnStatus -> this.settings: 1 failed (new pin) alias probe in the helper: 1 failed (source pin, names the alias line) ForRequest helper itself uses this.settings: 7 failed (all five handlers' pins + 2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA
|
Round-1 review items (R1-1 / R1-2 / R1-3) addressed in R1-1 — the decision now lives where the comment says. Added R1-2 — the two uncovered handlers now have behavioral pins. R1-3 — the source pin matches identifier occurrences. Applied your regex ( Mutation matrix on
Row 5 is the one I'd point a reader at: the choke point is now load-bearing for every handler that goes through it, which is the property the first push only claimed. Also re-verified on |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * session creation) pass them in. Handlers that serve a caller-supplied cwd | ||
| * must not make that decision themselves — they use | ||
| * `runWithPinnedRuntimeBaseDirForRequest` below. |
There was a problem hiding this comment.
[Suggestion] R1-1: (fix-induced) The replacement doc comment written by the R1-1 fix overclaims in the same shape R1-1 condemned. It says handlers serving a caller-supplied cwd "must not make that decision themselves — they use runWithPinnedRuntimeBaseDirForRequest", but the qwen/session/loadUpdates non-live branch (acpAgent.ts:12396-12405) still hand-composes loadSettingsCached(cwd) + runWithPinnedRuntimeBaseDir, and loadSession restore (5240-5244) / unstable_resumeSession (5629-5634) resolve settings at the call site under profiler.timeSync('settings_load') instrumentation. The decision the comment claims is made in one place is still made at three call sites, and neither the source-pin test nor any behavioral test covers those shapes. Behavior is correct today, but swapping loadSettingsCached(cwd) for this.settings at acpAgent.ts:12397 — the exact #10095 bug shape — compiles and keeps all 604 tests green, so the guard reports safe while the bug class regresses for a foreign-cwd loadUpdates; conversely, a maintainer enforcing the documented contract migrates load/resume into the variant and breaks the profiler instrumentation (5240, 5629) plus their downstream this.settings adoption.
Witness:
C1 mutant (acpAgent.ts:12397 const settings = this.settings;):
full acpAgent.test.ts run → "Test Files 1 passed (1) / Tests 604 passed (604)"
baseline unmodified: 604/604 — the bug-shape swap is invisible to every test.
| * session creation) pass them in. Handlers that serve a caller-supplied cwd | |
| * must not make that decision themselves — they use | |
| * `runWithPinnedRuntimeBaseDirForRequest` below. | |
| * session creation) pass them in. Per-request session-management handlers | |
| * (list, delete, rename, transcript page, settled turn status) must not make | |
| * that decision themselves — they use `runWithPinnedRuntimeBaseDirForRequest` | |
| * below. (`qwen/session/loadUpdates`' non-live branch still composes at the | |
| * call site; session load/resume resolve there deliberately, under profiler | |
| * instrumentation, before adopting the settings.) |
The fix must not disturb profiler.timeSync('settings_load', () => loadSettingsCached(params.cwd)) at acpAgent.ts:5240-5242 and 5629-5631 — deliberate instrumentation whose resolved settings are adopted downstream (this.settings = settings at 5258/5645). If consolidation is chosen instead of the comment rewrite, each new behavioral pin must go red when loadSettingsCached at acpAgent.ts:5241 / 5630 / 12397 is replaced with this.settings — please confirm by running those mutations.
中文说明
R1-1 修复写入的替代 doc 注释又以 R1-1 所批评的形态过度声称。它说服用调用方给定 cwd 的处理器"不得自己做这个决定——它们使用 runWithPinnedRuntimeBaseDirForRequest"，但 qwen/session/loadUpdates 的非 live 分支（acpAgent.ts:12396-12405）仍然手写 loadSettingsCached(cwd) + runWithPinnedRuntimeBaseDir，且 loadSession 恢复（5240-5244）/ unstable_resumeSession（5629-5634）在 profiler.timeSync('settings_load') 埋点下于调用点解析 settings。注释声称只在一处做出的决定实际上仍在三个调用点做出，且源码级 pin 测试与行为测试都不覆盖这些形态。当前行为是正确的，但在 acpAgent.ts:12397 把 loadSettingsCached(cwd) 换成 this.settings——正是 #10095 的 bug 形态——能编译且 604 条测试全部保持绿色，即守卫报告安全而该 bug 类已在外部 cwd 的 loadUpdates 上回归；反过来，若维护者按文档契约强制把 load/resume 迁入该变体，会破坏 5240、5629 处的 profiler 埋点及其后续的 this.settings 承接。修复不得改动 acpAgent.ts:5240-5242 与 5629-5631 的 profiler.timeSync('settings_load', () => loadSettingsCached(params.cwd))——那是刻意的埋点，解析出的 settings 还会在下游被承接（5258/5645 的 this.settings = settings）。若选择收拢而不是改注释，每条新的行为钉必须在 acpAgent.ts:5241 / 5630 / 12397 的 loadSettingsCached 被替换为 this.settings 时变红——请跑这些变异确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| /\brunWithAcpRuntimeOutputDir\b/.test(text) && | ||
| !text.startsWith('import ') && |
There was a problem hiding this comment.
[Suggestion] R2-1: The choke-point pin's !text.startsWith('import ') exclusion makes an aliased import invisible. With import { runWithAcpRuntimeOutputDir as pinDirect }, the import line is skipped by the exclusion and the alias call site names no identifier, so a direct pin via the alias re-ships the stale-this.settings bug class while the test — whose own comment claims to catch direct naming "as a call or via an alias" — stays green. A future change adding that import and calling pinDirect(this.settings, cwd, op) in a per-request handler keeps located equal to exactly the helper's delegation line, and the #10095 bug class re-ships through the one guard meant to catch it generically.
Witness:
intact filter + alias-import mutant (pinDirect(this.settings, '/tmp/probe-alias', () => 1))
→ "Tests 1 passed | 603 skipped"
canonical-only exclusion + same mutant
→ AssertionError "Direct calls at:
321: import { runWithAcpRuntimeOutputDir as pinDirect } from './runtimeOutputDirContext.js';
4575: return runWithAcpRuntimeOutputDir(settings, cwd, operation);"
| /\brunWithAcpRuntimeOutputDir\b/.test(text) && | |
| !text.startsWith('import ') && | |
| /\brunWithAcpRuntimeOutputDir\b/.test(text) && | |
| !text.startsWith("import { runWithAcpRuntimeOutputDir } from") && |
The exclusion must keep matching the canonical import import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js'; at packages/cli/src/acp-integration/acpAgent.ts:320.
中文说明
choke-point pin 的 !text.startsWith('import ') 排除条件对别名 import 不可见。若写成 import { runWithAcpRuntimeOutputDir as pinDirect }，import 行会被该排除条件跳过，而别名调用点不含该标识符，因此经由别名直接钉定会重新引入陈旧 this.settings 的 bug 类，而这个测试——其自身注释声称能捕获"直接调用或经由别名"的直接引用——仍保持绿色。将来若添加该 import 并在某个按请求处理器里调用 pinDirect(this.settings, cwd, op)，located 仍恰好等于辅助方法的委托行，#10095 的 bug 类就会穿过这个本应通用地捕获它的守卫重新出现。注意：修复必须继续匹配 acpAgent.ts:320 的规范 import import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js';。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const located = directCalls.map(({ line, text }) => `${line}: ${text}`); | ||
| expect( | ||
| located, | ||
| `acpAgent.ts must not call runWithAcpRuntimeOutputDir directly; route the operation through this.runWithPinnedRuntimeBaseDir (see #10095). Direct calls at:\n${located.join('\n')}`, |
There was a problem hiding this comment.
[Suggestion] R2-2: The source-pin test's failure message directs fixers to route through this.runWithPinnedRuntimeBaseDir — the variant whose caller still chooses the settings — instead of runWithPinnedRuntimeBaseDirForRequest, contradicting the base helper's own new doc comment (acpAgent.ts:4558-4567). A fixer following the message literally writes this.runWithPinnedRuntimeBaseDir(this.settings, cwd, op), which compiles and turns the pin green because the test only bans direct token mentions — and in a multi-workspace daemon whose this.settings cache holds another workspace's advanced.runtimeOutputDir the handler then pins the wrong runtime root (empty/foreign session list; deleteSession deleting a stale same-id copy under the wrong root): the exact #10095 bug class, shipped with a green guard.
Witness:
deleteSession mutant (this.runWithPinnedRuntimeBaseDir(this.settings, cwd, …) at acpAgent.ts:12116):
✓ QwenAgent runtime-root pinning choke point > routes every per-request runtime-root pin…
× resolves deleteSession settings per request
→ AssertionError: expected { merged: { mcpServers: {} }, … } to be … // Object.is equality
at acpAgent.test.ts:20812
| `acpAgent.ts must not call runWithAcpRuntimeOutputDir directly; route the operation through this.runWithPinnedRuntimeBaseDir (see #10095). Direct calls at:\n${located.join('\n')}`, | |
| `acpAgent.ts must not call runWithAcpRuntimeOutputDir directly. Handlers serving a caller-supplied cwd must route through this.runWithPinnedRuntimeBaseDirForRequest; only callers holding deliberately scoped settings may use this.runWithPinnedRuntimeBaseDir (see #10095). Direct calls at:\n${located.join('\n')}`, |
中文说明
源码级 pin 测试的失败信息把修复者指向 this.runWithPinnedRuntimeBaseDir——仍由调用方选择 settings 的那个变体——而不是 runWithPinnedRuntimeBaseDirForRequest，与基础辅助方法自身的新 doc 注释（acpAgent.ts:4558-4567）相矛盾。照字面执行该信息的修复者会写出 this.runWithPinnedRuntimeBaseDir(this.settings, cwd, op)：它能编译、能让 pin 变绿（因为测试只禁止直接出现该标识符）——而在 this.settings 缓存装着另一个 workspace 的 advanced.runtimeOutputDir 的多 workspace 守护进程里，该处理器会钉错运行时根目录（空/别的 workspace 的会话列表；deleteSession 删掉错误根目录下同 id 的陈旧副本）：正是 #10095 的 bug 类，且在守卫全绿的情况下被放出。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…lper (round-2 R1-1/R2-1/R2-2) review-pr round 2 on QwenLM#10988: - R1-1 (fix-induced): the shared helper's comment claimed every caller-supplied-cwd handler already used the per-request variant, but qwen/session/loadUpdates' non-live branch still composed loadSettingsCached + the three-argument helper by hand, and session load/resume resolve settings at the call site on purpose. Migrate the loadUpdates branch (settings are not needed outside the pin), add a behavioral pin for it, and make the comment say exactly which handlers use the variant and why load/resume do not (profiler-instrumented resolution whose settings are adopted for the session afterwards). - R2-1: the source pin skipped every import line, so an aliased import (`import { runWithAcpRuntimeOutputDir as pinDirect }`) was invisible. It now skips only the canonical import line. - R2-2: the pin's failure message pointed fixers at the three-argument helper; it now names runWithPinnedRuntimeBaseDirForRequest for caller-supplied-cwd handlers. Mutation matrix, one mutant at a time, whole file (605 tests): loadUpdates non-live -> this.settings: 1 failed (its new pin) aliased import + helper pins this.settings: 7 failed (source pin names line 321 + six handler pins) deleteSession -> this.settings: 1 failed (its pin), source pin green, message now names the per-request helper Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA
|
Round-2 items addressed in R1-1 (fix-induced) — took the consolidation route for the one site that fits, and the comment-rewrite route for the two that don't. R2-1 — the pin now skips only the canonical import line ( R2-2 — applied your message wording: fixers are pointed at Mutation matrix on
Re-verified on |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 603 passed — this review observed 28141 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.test.ts:30107 — [probe] D3-1 the source-pin test's comment exclusion only handles whole-line comments — a trailing comment or string literal mentioning runWithAcpRuntimeOutputDir false-positives as …
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.test.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan（非阻断）：603 passed — this review observed 28141 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 3 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/acpAgent.test.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
| !text.startsWith( | ||
| "import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js';", | ||
| ) && |
There was a problem hiding this comment.
[Suggestion] R2-1: (fix-induced) The R2-1 fix replaced the choke-point pin's !text.startsWith('import ') exclusion with an exact match against today's single-line canonical import. That closed the aliased-import blind spot, but made the exemption format-sensitive: any benign reformat of that import now trips the tripwire. A future change that adds a second specifier to the ./runtimeOutputDirContext.js import pushes the line past Prettier's 80-column width; Prettier splits it, the continuation line no longer starts with the exact exempted string, so the filter counts it and the test goes RED with the "must not name runWithAcpRuntimeOutputDir directly" message — actively accusing the change of re-introducing the #10095 routing regression when it merely reformatted an import. The failure is loud rather than silent, so this is maintenance tax, not a hole — but the tripwire's stated purpose is to fire only on the regression shape.
Witness:
PR tree (benign multi-line import, scratch probe):
× routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir
Direct mentions at:
321: runWithAcpRuntimeOutputDir,
4581: return runWithAcpRuntimeOutputDir(settings, cwd, operation);: expected [ …(2) ] to deeply equal [ StringMatching{…} ]
RESET (unmodified tree, same test): Tests 1 passed | 604 skipped (605)
Exempt the import statement structurally instead of one exact spelling — additionally filter the multi-line continuation shapes, or strip the import statement before scanning. Alias detection stays intact: an aliased import plus renamed delegation still fails the expected-delegation assertion, and an aliased import with an unchanged delegation is a compile error. For example, after the exact-match line:
!/^runWithAcpRuntimeOutputDir( as \w+)?,$/.test(text) &&中文说明
[Suggestion] R2-1：（修复引入）R2-1 的修复把钉点测试的 !text.startsWith('import ') 排除换成了对当前单行规范 import 的精确匹配。这关闭了别名 import 的盲区，但让豁免变得依赖格式：对该 import 的任何无害重排现在都会触发这个绊线。将来若给 ./runtimeOutputDirContext.js 的 import 增加第二个符号，该行会超过 Prettier 的 80 列宽度而被拆成多行；续行不再以精确豁免串开头，过滤器把它计入，测试变红，报错信息指控该改动重新引入了 #10095 的路由回归——而它只是重排了一个 import。失败是显式的而非静默的，所以这是维护成本而不是漏洞——但该绊线的既定目的是只在回归形态上触发。
（Witness 见英文部分：探针实测显示多行 import 使测试变红，还原后恢复绿色。）
建议改为按结构豁免 import 语句，而不是只认一种精确拼写——额外过滤多行 import 的续行形态，或在扫描前把 import 语句剥掉。别名检测不受影响：别名 import 加上改名的委托仍会使期望的委托断言失败；别名 import 而委托未改名则是编译错误。示例见英文部分代码块。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…1 root cause) review-pr rounds 2 and 3 kept finding false positives in the line-based source pin (exact-match import exemption trips on a Prettier reformat; whole-line comment exclusion misses trailing comments and string literals). The root cause is scanning text lines, so scan the AST instead, following the fast-path.test.ts precedent: every Identifier named runWithAcpRuntimeOutputDir is a mention unless it is the name of an un-aliased import specifier. Comments, string literals and import formatting cannot false-positive; an aliased import keeps the identifier under propertyName and is reported with its line. Probe matrix on the pin alone (restored between runs): benign multi-line import reformat green trailing comment naming the function green string literal naming the function green aliased import used by the helper red, names the import line direct call at deleteSession red, names the call line const pin = runWithAcpRuntimeOutputDir red, names the alias line Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TGftREDUQDei396TrMLQHA
|
Round 3 (R2-1 fix-induced) and the deferred D3-1 addressed together in Probe matrix on
Re-verified: 605/605, |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max via Qwen Code /review (v0.23.0)
tmux E2E report — real
|
| Head verified | 2c4d313eca40f27964eb362f1a221a91d6fa5765 — the head qwen-code-ci-bot APPROVED at 11:16:36Z, and the head CI is green on (27 success / 26 skipped / 0 failure / 0 pending / 0 cancelled across all 53 check-runs) |
| Merge base | d4e3e4fc87 |
| Build | git archive 2c4d313eca extracted outside the repo → npm ci && npm run bundle → BUNDLE_OK (dist 155 M) |
| Environment | Linux 5.10.134-19.3.1.al8.x86_64 (Alibaba Cloud Linux 3), Node v24.18.1, tmux 2.7, isolated HOME=/tmp/r31_e2e/home |
| Model calls | none possible — OPENAI_API_KEY is a dummy and OPENAI_BASE_URL=http://127.0.0.1:9/v1 (discard port, nothing listening). Every prompt fails locally; that failure is what makes the product write a genuine transcript |
QWEN_RUNTIME_DIR |
deliberately unset. Storage.getRuntimeBaseDir() lets that env var override a non-pinned contextual dir, so setting it would have made the whole A/B vacuous |
| Base arm | not rebuilt (disk budget). Behavioural equivalence to base is argued from the diff below instead |
Build witness — the measurements are against this PR, not a stale tree
dist/chunks/acpAgent-2IT3RGWT.js (sha256 888440697d329f9187168cfa9b86de3f79db035d5aa0231786fbc480a2274bd3) contains the compiled new helper with its body intact:
runWithPinnedRuntimeBaseDirForRequest(cwd, operation) {
const settings = loadSettingsCached(cwd);
return this.runWithPinnedRuntimeBaseDir(
settings, cwd, () => operation(settings)
);
}- 6 call sites of the new helper in the shipped chunk (lines 18332, 20780, 23216, 23396, 23434, 23624) — exactly the six converted handlers.
runWithAcpRuntimeOutputDirhas exactly ONE call site in the whole bundle (line 17342, the shared helper's own delegation; the other textual hits are the function definition, its__nameregistration and a doc comment). That independently confirms in production wiring the invariant the PR's new AST test asserts about the source.
Code review — five hypotheses measured dead, please don't re-raise them
- "A per-request handler was missed and still pins with the process-wide
this.settings" → DEAD. At headrunWithAcpRuntimeOutputDiris called in exactly one place, insiderunWithPinnedRuntimeBaseDir. Base had 5 direct call sites (5777, 8864, 11789, 12080, 12131); all 5 are converted (5811, 8950, 11899, 12120, 12170). The 5 remainingrunWithPinnedRuntimeBaseDircallers all hold deliberately scoped settings, matching the new doc comment:createWorkspaceMcpDiscoveryConfig(settings)(3887),assertLiveSessionScope(config, settings, cwd)(4639), the non-liveloadSession(5248) andresumeSession(5638) branches — both resolvingloadSettingsCached(params.cwd)at the call site underprofiler.timeSync('settings_load')and only then adopting it viathis.settings = settings— andnewSessionConfig(cwd, mcpServers, settings, …)(13417). - "A converted operation no longer receives
settingsbut silently resolves the name from an enclosing scope" — the exact fix(cli): resolve session-management settings per request, not from the stale this.settings cache #10095 shape surviving the refactor, and it would compile → DEAD. Only one converted body referencessettingsat all: the transcript page, at 8919 (this.getTranscriptReplayConfig(cwd, settings)), and that operation takes it as its parameter at 8897. The other five bodies reference nosettingsidentifier (only comments). - "The helper changes when/how settings are resolved" → DEAD.
loadSettingsCached(cwd)is evaluated before the pin is entered in both the old and the new shape, andrunWithPinnedRuntimeBaseDiris a pure pass-through torunWithAcpRuntimeOutputDir.loadSettingsCachedis a per-workspace LRU keyed bypath.resolve(workspaceDir)with freshness fingerprints (settings-cache.ts:161), not a last-caller cache, so it really returns the request workspace's settings. - "The new AST source-pin test's relative
readFileSync('src/acp-integration/acpAgent.ts')breaks under a different cwd" → not a defect. Established convention in this package (cli.test.ts:1151,cli.test.ts:1188,serve/fast-path.test.ts:397, …) and vitest runs per package. - "Workspace settings could be trust-gated, making the pin untestable/vacuous" → DEAD.
workspaceSettingsActive = !opts.skipWorkspaceSettings && realWorkspaceDir !== realHomeDir(settings.ts:1089) — no folder-trust gate onadvanced.runtimeOutputDir.
E2E design — a decoy makes "read rootB" distinguishable from "read rootA" by content
Two workspaces whose own settings point at different runtime roots:
wsA/.qwen/settings.json {"advanced":{"runtimeOutputDir":"/tmp/r31_e2e/rootA"}}
wsB/.qwen/settings.json {"advanced":{"runtimeOutputDir":"/tmp/r31_e2e/rootB"}}
Process 1 boots in wsB and creates a real session (sidB=9719936c-…); the product writes its own transcript to rootB/projects/-tmp-r31-e2e-wsB/chats/<sidB>.jsonl (1413 b). I then plant a decoy — a copy of that genuine transcript with its marker rewritten — at exactly the path a wrong-root pin would read:
rootA/projects/-tmp-r31-e2e-wsB/chats/<sidB>.jsonl 1418 b "DECOY-UNDER-ROOTA"
(The project segment is sanitizeCwd(wsB) in both roots, so a pin carrying wsA's runtimeOutputDir while the SessionService is constructed with cwd=wsB lands precisely on the decoy.)
Process 2 boots in wsA and creates a real session there (sidA=c1a87e68-…, transcript written under rootA/projects/-tmp-r31-e2e-wsA/chats/), so this.settings genuinely holds wsA's settings — the multi-workspace shape the pin exists for. Process 2 then serves requests naming cwd=wsB. Every result is read back from raw disk bytes, not from handler-reported success.
Pristine head — all five reachable handlers resolve workspace B's root
| # | Handler (head line) | Result | Which root |
|---|---|---|---|
| M0 | unstable_listSessions(cwd=wsA) — control |
sidA / title RECORD-FOR-A control marker |
rootA ✓ |
| M1 | unstable_listSessions(cwd=wsB) (5811) |
sidB / title RECORD-FOR-B e2e marker |
rootB |
| M2 | qwen/status/session/transcript (8950) |
RECORD-FOR-B=true, DECOY=false |
rootB |
| M3 | qwen/control/session/turn_status (11899) |
Session not found — see "not verified" |
unreachable |
| M4 | qwen/session/loadUpdates non-live (12401) |
1 update, RECORD-FOR-B=true, DECOY=false |
rootB |
| M5 | renameSession non-live (12170) |
success:true; rootB 1413→1765 b, rename marker present; decoy byte- and mtime-identical (1418 b, 14:30:15.581Z) |
rootB only |
| M6 | deleteSession (12120) |
success:true; rootB original ABSENT; decoy intact (1418 b, same mtime) |
rootB only |
| M7 | unstable_listSessions(cwd=wsB) after delete |
sessions: [] |
rootB |
Mutant arm — the same harness, one line changed, every measurement flips
I patched the shipped chunk to re-introduce the bug class (const settings = this.settings; instead of loadSettingsCached(cwd)), re-ran the whole flow from scratch, then restored the chunk and verified the sha256 matches the pristine value byte-for-byte.
| # | Pristine head | Mutant (this.settings) |
|---|---|---|
| M0 control | RECORD-FOR-A control marker |
RECORD-FOR-A control marker (unchanged, as a control should be) |
| M1 list cwd=wsB | RECORD-FOR-B e2e marker |
DECOY-UNDER-ROOTA e2e marker |
| M2 transcript | RECORD-FOR-B=true, DECOY=false |
RECORD-FOR-B=false, DECOY=true |
| M4 loadUpdates | RECORD-FOR-B=true, DECOY=false |
RECORD-FOR-B=false, DECOY=true |
| M5 rename | rootB 1413→1765 b, marker set; decoy untouched | rootB untouched (1413 b, mtime unchanged); decoy 1418→1770 b, marker set |
| M6 delete | rootB ABSENT; decoy intact | rootB INTACT; decoy ABSENT |
M6 is the one that matters most: the mutant still returns {"success":true} while the user's real session survives and a foreign workspace's file is destroyed. So the harness does not merely observe "non-empty output" — it discriminates the two behaviours on read, on write and on delete. The pristine head is on the correct side of every row.
The patch was also narrow, which is what makes the comparison fair: the mutant arm's own setup step (session creation for wsB, which goes through runWithPinnedRuntimeBaseDir with caller-scoped settings, not through the per-request helper) still wrote its transcript to rootB/projects/-tmp-r31-e2e-wsB/chats/ exactly as the pristine arm did. Only the six handlers under test changed behaviour.
What I did NOT verify (disclosed, not glossed)
turn_status(M3) is unverified and unverifiable in this shape.this.sessionOrThrow(sessionId)at 11824 runs before the pin at 11899, so a non-live session throwsSession not foundand the pinned block is never entered; both arms failed identically. Reaching it needs a session that is live in the same process and a request naming a different cwd, which this two-process design cannot produce. Its correctness here rests on the code review (hypotheses 1–3), not on measurement.- Base arm not rebuilt. Equivalence is argued from the diff: each converted site previously did
loadSettingsCached(cwd)→runWithAcpRuntimeOutputDir(settings, cwd, op), which is literally what the helper composes. - A harness bug of mine, which does not change any conclusion: for M2 I extracted the record count from
value.updates ?? value.records, but the response key isevents— hence the reportedrecordCount: 0in both arms. The root attribution for M2 comes from the marker scan over the whole response body (RECORD-FOR-BvsDECOY-UNDER-ROOTA), which is unaffected. - Not exercised: sandbox/seatbelt relaunch paths, the archive-state variants of delete/rename, and concurrent requests from two workspaces.
Two observations that are NOT Criticals
I am not requesting changes on either; both are recorded so the merge decision is made with them visible.
- (a)
turn_statusmixes a live session with a request-supplied cwd. Inside the pinned block it flushes the live session's recording (session.getConfig().getChatRecordingService()?.flush()) but buildsnew SessionTranscriptReader(cwd)from the request's cwd, and unlikeloadSession/resumeSessionit does not callassertLiveSessionScope. A client naming a different cwd for a live session would flush one workspace and read another. Pre-existing and untouched by this diff — base resolved the samecwdand the sameloadSettingsCached(cwd)— so it is a question for a follow-up, not a blocker here. - (b) A foreign-cwd request's debug log lands under the boot workspace's root. After process 2 served
cwd=wsBrequests,rootA/debug/<sidB>.txt(432 b) appeared alongsiderootB/debug/<sidB>.txt. Debug-log placement follows the process's ambient runtime root, not the per-request pin. Also not touched by this diff (which changes only the six call sites and adds two helpers); noted only so a reader who spots the file does not mistake it for a leak the pin was supposed to prevent.
Conclusion
Merge-ready from my side. The refactor is semantics-preserving at every one of the six converted sites, no per-request pin was missed, the choke point the new AST test asserts really holds in the shipped bundle, and on a kernel and Node version nobody else has reported here the handlers resolve the request workspace's runtime root for reads, writes and deletes — with a mutant arm proving the measurement can tell the difference. Observations (a) and (b) are accuracy and follow-up items, not blockers. This is my first report on this PR, so the approve follows next round rather than landing in the same breath.
tmux E2E 报告 —— 在 ci-bot 已 approve 的 head 上驱动真实 qwen --acp agent，并附带一个证明测量灵敏度的变异臂
我用本 PR 的 head 构建产物，在 tmux 中驱动真实的 ACP agent 进程穿过被改造的各个 handler，随后对同一个测量流程又跑了一个故意改坏的构建。未发现 Critical。从我这边看可以合入。 我不会在同一口气里 approve —— 按我自己的流程，approve 要跟在一份已结论"可以合入"的既有报告之后，所以留到下一轮。
| 验证的 head | 2c4d313eca40f27964eb362f1a221a91d6fa5765 —— 即 qwen-code-ci-bot 于 11:16:36Z APPROVED 的 head，也是 CI 全绿的 head（53 个 check-run 中 27 success / 26 skipped / 0 failure / 0 pending / 0 cancelled） |
| Merge base | d4e3e4fc87 |
| 构建 | git archive 2c4d313eca 在仓库外解包 → npm ci && npm run bundle → BUNDLE_OK（dist 155 M） |
| 环境 | Linux 5.10.134-19.3.1.al8.x86_64（Alibaba Cloud Linux 3）、Node v24.18.1、tmux 2.7、隔离 HOME=/tmp/r31_e2e/home |
| 模型调用 | 不可能发生 —— OPENAI_API_KEY 是假值，OPENAI_BASE_URL=http://127.0.0.1:9/v1（discard 端口，无人监听）。每次 prompt 都在本地失败，而正是这个失败让产品写出了真实的 transcript |
QWEN_RUNTIME_DIR |
刻意不设置。 Storage.getRuntimeBaseDir() 允许该环境变量覆盖非 pinned 的上下文目录，一旦设置整个 A/B 就失去意义 |
| Base 臂 | 未重新构建（磁盘预算）。与 base 的行为等价性改由下面的 diff 论证 |
构建见证 —— 测量对象确实是本 PR，不是旧树
dist/chunks/acpAgent-2IT3RGWT.js（sha256 888440697d329f9187168cfa9b86de3f79db035d5aa0231786fbc480a2274bd3）中包含编译后的新 helper，函数体完整：
runWithPinnedRuntimeBaseDirForRequest(cwd, operation) {
const settings = loadSettingsCached(cwd);
return this.runWithPinnedRuntimeBaseDir(
settings, cwd, () => operation(settings)
);
}- 出货 chunk 中新 helper 的调用点为 6 处（18332、20780、23216、23396、23434、23624），正好对应六个被改造的 handler。
- 整个 bundle 中
runWithAcpRuntimeOutputDir只有 1 个调用点（17342，即共享 helper 自己的委托；其余文本命中是函数定义、__name注册和文档注释）。这在生产接线中独立证实了本 PR 新增 AST 测试对源码所断言的那条不变量。
代码审查 —— 五个假设已被测死，请勿重复提出
- "某个 per-request handler 被漏掉，仍用进程级
this.settings去 pin" → 已死。 head 上runWithAcpRuntimeOutputDir只有一处调用，就在runWithPinnedRuntimeBaseDir内部。base 有 5 个直接调用点（5777、8864、11789、12080、12131），全部被改造（5811、8950、11899、12120、12170）。剩下 5 个runWithPinnedRuntimeBaseDir调用方都持有刻意限定作用域的 settings，与新文档注释一致：createWorkspaceMcpDiscoveryConfig(settings)（3887）、assertLiveSessionScope(config, settings, cwd)（4639）、非 live 的loadSession（5248）与resumeSession（5638）分支 —— 两者都在调用点于profiler.timeSync('settings_load')下解析loadSettingsCached(params.cwd)，之后才通过this.settings = settings采纳 —— 以及newSessionConfig(cwd, mcpServers, settings, …)（13417）。 - "某个被改造的 operation 不再接收
settings，却从外层作用域静默解析到同名变量" —— 这正是 fix(cli): resolve session-management settings per request, not from the stale this.settings cache #10095 的形态在重构后残留，而且能编译通过 → 已死。 只有一个被改造的函数体引用了settings：transcript page 的 8919 行（this.getTranscriptReplayConfig(cwd, settings)），而它在 8897 行以参数形式接收。其余五个函数体完全没有引用settings标识符（只有注释）。 - "新 helper 改变了 settings 的解析时机或方式" → 已死。 新旧两种写法中
loadSettingsCached(cwd)都在进入 pin 之前求值，且runWithPinnedRuntimeBaseDir是对runWithAcpRuntimeOutputDir的纯转发。loadSettingsCached是以path.resolve(workspaceDir)为键、带新鲜度指纹的按工作区 LRU（settings-cache.ts:161），不是"最后一个调用者"缓存，因此确实返回请求工作区自己的 settings。 - "新增的 AST 源码钉测试用相对路径
readFileSync('src/acp-integration/acpAgent.ts')，换个 cwd 就会坏" → 不是缺陷。 这是本包既有惯例（cli.test.ts:1151、cli.test.ts:1188、serve/fast-path.test.ts:397等），且 vitest 按包运行。 - "工作区 settings 可能被 trust 门禁挡住，导致 pin 不可测/无意义" → 已死。
workspaceSettingsActive = !opts.skipWorkspaceSettings && realWorkspaceDir !== realHomeDir（settings.ts:1089）——advanced.runtimeOutputDir上没有 folder-trust 门禁。
E2E 设计 —— 用诱饵让"读 rootB"与"读 rootA"在内容上可区分
两个工作区，各自的 settings 指向不同的运行时根：
wsA/.qwen/settings.json {"advanced":{"runtimeOutputDir":"/tmp/r31_e2e/rootA"}}
wsB/.qwen/settings.json {"advanced":{"runtimeOutputDir":"/tmp/r31_e2e/rootB"}}
进程 1 在 wsB 启动并创建真实会话（sidB=9719936c-…），产品自己把 transcript 写到 rootB/projects/-tmp-r31-e2e-wsB/chats/<sidB>.jsonl（1413 b）。随后我植入诱饵 —— 复制这份真实 transcript 并改写其标记 —— 放在"错误根 pin"恰好会读到的路径上：
rootA/projects/-tmp-r31-e2e-wsB/chats/<sidB>.jsonl 1418 b "DECOY-UNDER-ROOTA"
（两个根下的项目段都是 sanitizeCwd(wsB)，所以一个携带 wsA 的 runtimeOutputDir、却用 cwd=wsB 构造 SessionService 的 pin，会精确落在诱饵上。）
进程 2 在 wsA 启动并在当地创建真实会话（sidA=c1a87e68-…，transcript 写在 rootA/projects/-tmp-r31-e2e-wsA/chats/），因此 this.settings 确实持有 wsA 的 settings —— 正是 pin 为之存在的多工作区形态。随后进程 2 处理指名 cwd=wsB 的请求。所有结论都从磁盘原始字节读回，而不是采信 handler 自报的成功。
原始 head —— 五个可达的 handler 全部解析到工作区 B 的根
| # | Handler（head 行号） | 结果 | 落在哪个根 |
|---|---|---|---|
| M0 | unstable_listSessions(cwd=wsA) —— 对照 |
sidA / 标题 RECORD-FOR-A control marker |
rootA ✓ |
| M1 | unstable_listSessions(cwd=wsB)（5811） |
sidB / 标题 RECORD-FOR-B e2e marker |
rootB |
| M2 | qwen/status/session/transcript（8950） |
RECORD-FOR-B=true、DECOY=false |
rootB |
| M3 | qwen/control/session/turn_status（11899） |
Session not found —— 见"未验证" |
不可达 |
| M4 | qwen/session/loadUpdates 非 live（12401） |
1 条 update，RECORD-FOR-B=true、DECOY=false |
rootB |
| M5 | renameSession 非 live（12170） |
success:true；rootB 1413→1765 b，含重命名标记；诱饵字节与 mtime 均未变（1418 b，14:30:15.581Z） |
仅 rootB |
| M6 | deleteSession（12120） |
success:true；rootB 原文件 ABSENT；诱饵完好（1418 b，mtime 不变） |
仅 rootB |
| M7 | 删除后 unstable_listSessions(cwd=wsB) |
sessions: [] |
rootB |
变异臂 —— 同一套装置，只改一行，每项测量都翻转
我把出货 chunk 改成重新引入该 bug 类（用 const settings = this.settings; 取代 loadSettingsCached(cwd)），从零重跑整个流程，随后还原该 chunk 并验证 sha256 与原始值逐字节一致。
| # | 原始 head | 变异体（this.settings） |
|---|---|---|
| M0 对照 | RECORD-FOR-A control marker |
RECORD-FOR-A control marker（不变，正符合对照组应有的表现） |
| M1 list cwd=wsB | RECORD-FOR-B e2e marker |
DECOY-UNDER-ROOTA e2e marker |
| M2 transcript | RECORD-FOR-B=true、DECOY=false |
RECORD-FOR-B=false、DECOY=true |
| M4 loadUpdates | RECORD-FOR-B=true、DECOY=false |
RECORD-FOR-B=false、DECOY=true |
| M5 rename | rootB 1413→1765 b，标记写入；诱饵未动 | rootB 未动（1413 b，mtime 不变）；诱饵 1418→1770 b，标记写入 |
| M6 delete | rootB ABSENT；诱饵完好 | rootB 完好；诱饵 ABSENT |
M6 最关键：变异体依然返回 {"success":true}，而用户真实的会话幸存下来，被销毁的是另一个工作区的文件。所以这套装置并非只观察"输出非空" —— 它在读、写、删三个维度上都能区分两种行为。原始 head 在每一行上都站在正确的一侧。
我没有验证的部分（如实披露，不粉饰）
turn_status（M3）未验证，且在该形态下无法验证。this.sessionOrThrow(sessionId)在 11824 行，先于 11899 行的 pin 执行，因此非 live 会话会抛Session not found，pinned 代码块根本不会进入；两臂表现完全相同。要触达它需要"同一进程内 live 的会话"+"指名另一个 cwd 的请求"，我的双进程设计造不出这种组合。它的正确性在这里只由代码审查（假设 1–3）支撑，而非测量。- 未重建 base 臂。 等价性由 diff 论证：每个被改造的点此前都是
loadSettingsCached(cwd)→runWithAcpRuntimeOutputDir(settings, cwd, op)，而这正是 helper 所组合的内容。 - 我自己的一个装置 bug，不影响任何结论： M2 我从
value.updates ?? value.records取记录数，但响应的键是events—— 这就是两臂都报recordCount: 0的原因。M2 的根归属判断来自对整个响应体的标记扫描（RECORD-FOR-BvsDECOY-UNDER-ROOTA），不受影响。 - 未触及：sandbox/seatbelt 重启路径、delete/rename 的 archive-state 变体、以及来自两个工作区的并发请求。
两条不是 Critical 的观察
两条我都没有据此请求变更；记录下来是为了让合入决策在可见这些信息的前提下做出。
- （a）
turn_status把 live 会话与请求提供的 cwd 混用。 在 pinned 代码块内，它 flush 的是 live 会话的录制（session.getConfig().getChatRecordingService()?.flush()），却用请求的 cwd 构造new SessionTranscriptReader(cwd)；而且与loadSession/resumeSession不同，它没有调用assertLiveSessionScope。若客户端为一个 live 会话指名另一个 cwd，就会 flush 一个工作区、读取另一个工作区。属既有问题，本 diff 未触碰 —— base 解析的是同一个cwd和同一个loadSettingsCached(cwd)—— 所以这是后续 PR 的问题，不是此处的阻塞项。 - （b）外部 cwd 请求的 debug 日志落在启动工作区的根下。 进程 2 处理完
cwd=wsB的请求后，rootA/debug/<sidB>.txt（432 b）与rootB/debug/<sidB>.txt同时出现。debug 日志的落点跟随进程的环境运行时根，而非 per-request 的 pin。同样未被本 diff 触碰（diff 只改了六个调用点并新增两个 helper）；记录此条只是为了让看到该文件的人不会误以为这是 pin 本该防住的泄漏。
结论
从我这边看可以合入。 六个被改造的调用点每一处都保持语义不变，没有漏掉任何 per-request pin，新 AST 测试所断言的收敛点在出货 bundle 中确实成立；并且在别人尚未报告过的内核与 Node 版本上，这些 handler 在读、写、删三类操作中都解析到了请求工作区的运行时根 —— 同时变异臂证明了这套测量能够分辨二者之差。观察（a）与（b）属于措辞准确性与后续事项，不是阻塞项。这是我在本 PR 上的第一份报告，所以 approve 留到下一轮，不在同一口气里给出。
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving on the strength of my tmux E2E report above, which concluded this is merge-ready.
Gate re-verified at approve time (2026-09-04T15:00Z):
| Condition | Value |
|---|---|
| Head | 2c4d313eca40f27964eb362f1a221a91d6fa5765 — unchanged since the report |
| Latest review | qwen-code-ci-bot APPROVED at this head, 2026-09-04T11:16:36Z (5112259581); no review after it |
| CI at head | 53 check-runs = 27 success / 26 skipped / 0 failure / 0 pending / 0 cancelled |
| Activity since the report | none — no new commit, review or comment |
No Critical found. The five hypotheses I measured against this head are recorded as dead in the report. Both observations I raised there are pre-existing and non-blocking, so neither should hold this merge:
turn_statusbuilds itsSessionTranscriptReaderfrom the request's cwd while flushing the live session's recording, and unlikeloadSession/resumeSessionnever callsassertLiveSessionScope. Base resolved the same cwd, so this is a follow-up question rather than a regression introduced here.- A foreign-cwd request's debug log lands under the boot workspace's runtime root rather than the per-request pin (I measured
rootA/debug/<sidB>.txtappearing alongsiderootB/debug/<sidB>.txt). This diff touches only the 6 converted call sites and the 2 helpers, so debug-log placement is out of its scope.
Still unverified by anyone, carried over from the report so the merge decision owns it: turn_status end-to-end (unreachable in my harness shape — sessionOrThrow runs before the pin, so a non-live session throws Session not found), a base arm (equivalence argued from the diff instead), sandbox relaunch, and two-workspace concurrency.
Maintainer verification — recommend mergeVerified Linux, Node 22.22.2, 1. A live E2E the plan doesn't have: does the pin actually work on a running agent?The plan is a grep count plus a mutation matrix over mocks. Since the whole point is that both spellings reach the same function, I wanted the process to answer which runtime root each handler opens. Setup:
All six rerouted handlers pin the request workspace's root, and base and PR are byte-for-byte the same verdict — the behavior-preservation claim, measured on a running agent rather than argued from the diff:
The interesting half is non-vacuity. I rebuilt the PR head with one line changed inside the new seam —
2. The Reviewer Test Plan, step by step
Step 1 — grep. On Step 2 — suites. Step 3 — mutation matrix. Ran 13 mutants one at a time, whole file each run, tree restored between runs. Every row lands where the PR says, and I added the two handler mutants the body doesn't list ( Step 4 — whitespace. Beyond the plan — is the taxonomy in the doc comment actually true? I audited all six remaining three-argument call sites, since a per-request handler hiding among them would defeat the seam. CI is now fully green on the head (23 pass, 30 skipping, 0 fail) — the 3. Non-blocking observations
(a) The source pin asserts the surviving mention's raw line text, so a formatting-only reflow of the delegation fails with a message that misdiagnoses it. The AST walk is correct — a trailing comment on another line, a multi-line canonical import and a string literal naming the function are all green, exactly as round 3 claims. But the assertion is (b) (c) The PR body's step-3 mutation matrix wasn't refreshed after the None of these change the recommendation. The refactor does the thing worth doing after #10095 — it removes the "which settings pin this operation" decision from six call sites instead of re-auditing them — and the E2E above shows the seam holds the property on a real process, not just under mocks. 中文说明维护者验证 —— 建议合并在本地专门搭建的真实环境中验证了 环境：Linux、Node 22.22.2，两侧各在干净 worktree 中 1. 计划里没有的一项：在真正跑起来的 agent 上，钉定到底生效了吗原计划是 grep 计数加一套跑在 mock 上的变异矩阵。既然整件事的前提就是"两种写法最终到达同一个函数"，我希望由进程本身回答每个处理器实际打开的是哪个 runtime root。搭法：
改道的六个处理器全部钉在请求 workspace 的 root 上，且 base 与 PR 的判定完全一致——"行为不变"这一点是在跑起来的 agent 上量出来的，不是从 diff 推出来的（见上方第一张图）。 更关键的是非空洞性。我把 PR head 里新入口内的一行改掉—— 2. 逐条复核 Reviewer Test Plan第 1 步 grep。 在 第 2 步 测试。 PR head 上 第 3 步 变异矩阵。 逐个跑了 13 个变异，每次跑整个文件、每次恢复。每一行都落在 PR 所述位置；我另补了 body 未列出的两个处理器变异（ 第 4 步 空白。 计划之外——文档注释里的分类是否属实？ 我核了剩下全部六处三参数调用点，因为若有按请求处理器混在其中就会架空这个入口。 当前 head 的 CI 已全绿（23 通过、30 跳过、0 失败）——你提到的 3. 非阻断观察(a) 源码钉断言的是存活引用所在行的原始文本，因此对委托行做纯格式化重排会以一条误导性的信息失败。 AST 遍历本身是对的——另一行上的行尾注释、被拆成多行的规范 import、字符串字面量，第 3 轮声称的三种良性形状实测全绿。但断言是 (b) (c) PR body 第 3 步的变异矩阵在新增 以上都不改变结论。这次重构做的正是 #10095 之后值得做的事——把"用哪份 settings 钉这次操作"的决定从六个调用点移走，而不是逐个复查它们——而上面的 E2E 表明这个入口在真实进程上、而不仅在 mock 下，确实承载着这个性质。 |
…ke point (QwenLM#11047) * fix(cli): route the transcript turn-index handler through the pin choke point The QwenLM#10988 guard fails the build on any direct mention of the runtime-root context runner outside the single choke point; the session turn-index page handler (from QwenLM#10751) still composed the routing by hand with loadSettingsCached, the exact decision runWithPinnedRuntimeBaseDirForRequest exists to make in one place (transcript pages are listed in the choke point's own doc). Behaviour is identical: same cached settings for the same cwd, same pin; the operation never used the local settings object. * test(cli): pin the turn-index handler's per-request routing and roster The choke-point guard permits the scoped-settings shape, so an edit back to this.runWithPinnedRuntimeBaseDir(this.settings, cwd, ...) would stay green without a behavioural test; mirror the six sibling routing tests. Also add the turn-index handler to the choke point's normative handler roster. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
4 participants
What this PR does
Makes the agent's runtime-root pinning go through one documented seam instead of five hand-composed copies, and puts the "which settings pin this operation" decision inside that seam for per-request handlers. The existing private three-argument helper stays as the single place that composes the routing (every caller in the class goes through it; nothing else names the underlying context function). On top of it, a per-request variant takes only the request's cwd, resolves that cwd's settings itself, and hands them to the operation — so a handler serving a caller-supplied cwd has no settings argument it could fill with the process-wide cache. The six per-request sites (session listing, delete, rename, status transcript page, settled turn status, and the non-live branch of session loadUpdates) all use the variant; callers that hold deliberately scoped settings (workspace MCP discovery, live-session scope checks, session creation) keep the three-argument form. The helper bodies are one-line delegations, so runtime behavior is unchanged; the two wide call sites name their inline callbacks so the reroute does not re-indent two 60-line bodies.
Three tests pin this. A source-level test walks the file's TypeScript AST and asserts that the only mention of the underlying context function, other than the un-aliased import specifier, is the shared helper's own delegation — so comments, string literals and import formatting cannot false-positive, while an alias, an aliased import, or a direct call is reported with its line. Three behavioral tests cover the rerouted handlers that had none (settled turn status, status transcript page, non-live loadUpdates): each asserts the settings and cwd reaching the mocked context function are the request's, and the transcript test also checks the replay config built inside the pinned scope was seeded from the request's settings. The test describe block holding #10095's three regression tests is renamed to match what it actually covers.
Why it's needed
Follow-up to #10095, closing the two non-blocking items recorded there: the round-2 review's deferred D2-1 (the near-miss helper already existed, the fix hand-pasted the routing three more times) and the maintainer's nit that the three regression tests were reported under a
renameSession routingdescribe. Hand-composed routing is exactly the shape that regressed in #10095 — each handler made its own "which settings pin this operation" decision and three of them made it wrong. Round 1 of this PR's own review showed that merely routing through a pass-through helper does not remove that decision from the call sites; the per-request variant does, and the mutation matrix below shows the seam is now load-bearing for every handler that uses it.The choke-point invariant needs a source pin rather than a behavioral test because both spellings reach the same function. The pin follows the precedent in
serve/fast-path.test.ts(TypeScript AST walk over a source file read with a package-relative path).Reviewer Test Plan
How to verify
grep -n 'runWithAcpRuntimeOutputDir' packages/cli/src/acp-integration/acpAgent.ts— outside the canonical import line and comments, expect exactly one hit: the shared helper's delegation. Onmainthere are six call sites.grep -c 'runWithPinnedRuntimeBaseDirForRequest(' …— expect 6 call sites (the definition line carries a type parameter and does not match).cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts— 605/605 (601 existing + the source pin + three behavioral pins). fix(cli): resolve session-management settings per request, not from the stale this.settings cache #10095's three regression tests report underQwenAgent session-management routing (rename / delete / list / branch / close) > ….deleteSessionreverted to the three-argument helper withthis.settings→ 1 failed / 603 passed: onlyresolves deleteSession settings per request; the source pin stays green (it is not the test that guards this).sessionTranscripthandedthis.settings→ 3 failed / 601: the new transcript pin plus the two existing replay-config tests.sessionTurnStatushandedthis.settings→ 1 failed / 603: only the new turn-status pin (this handler had no coverage before).loadUpdateshandedthis.settings→ 1 failed / 604: only the new loadUpdates pin (on the round-1 head this mutant was invisible, 605/605).const pin = runWithAcpRuntimeOutputDir; return pin(…)) inside the helper → 1 failed / 603: the source pin, whose message lists the alias line next to the delegation line.import { runWithAcpRuntimeOutputDir as pinDirect }) used by the helper withthis.settings→ 7 failed / 598: the source pin lists the import line, and the six handler pins catch the settings half.this.settings→ 7 failed / 597: all five per-request handler pins plus the two replay-config tests. This is the row that shows the seam is load-bearing.git diff -w --stat mainequalsgit diff --stat main(+272 / −16 over the two files): no whitespace-only churn hides in the diff.Evidence (Before & After)
N/A — refactor with no runtime behavior change. Evidence is the grep count in step 1 (six direct call sites → one) and the mutation matrix in step 3.
Tested on
Environment (optional)
Local unit tests only (
vitest,tsc --noEmitonpackages/cliwith zero diagnostics underacp-integration/*,eslintandprettier --checkon both files clean). Core and acp-bridgedistrebuilt from this branch's merge-base before running.Risk & Scope
loadSettingsCached(cwd)the sites called themselves. The source-pin test parses the file with the TypeScript compiler API already used byfast-path.test.ts; if a second legitimate direct mention is ever introduced, the failure names its line and the assertion is a one-line update.qwen --acpclient's cwd matches the workspace it asks about) is a separate topic and not touched here. Session load / resume keep resolving the request's settings at the call site on purpose (profiler-instrumented, and the settings are adopted for the session afterwards); they and the other three-argument callers are unchanged.Linked Issues
Relates to #10095 (closes its deferred round-2 item D2-1 and the maintainer's describe-block nit; no standalone issue was filed since both findings live in that PR's review thread). Same follow-through shape as #10465 for #9930.
中文说明
本 PR 做了什么
把 agent 的运行时根目录钉定收拢到一个有注释的入口，取代五处手写的复制，并把"用哪份 settings 钉这次操作"的决定放进这个入口（针对按请求处理器）。原有的私有三参数辅助方法仍然是唯一组合路由的地方（类内所有调用方都经过它，此外没有任何地方直接引用底层的 context 函数）。在它之上新增一个按请求变体：只接收请求的 cwd，自己解析该 cwd 的 settings 并交给操作回调，这样服务于调用方指定 cwd 的处理器就没有可以被进程级缓存填错的 settings 参数。六处按请求调用点（会话列表、删除、重命名、status transcript 分页、settled turn status、以及 session loadUpdates 的非 live 分支）全部改用该变体；持有明确作用域 settings 的调用方（workspace MCP 发现、live session 作用域检查、会话创建）保留三参数形式。两个辅助方法的本体都是一行委托，运行时行为不变；两处较宽的调用点给内联回调起了名字，避免重排两段 60 行函数体。
三类测试钉住这些。一条源码级测试遍历该文件的 TypeScript AST，断言除未加别名的 import 说明符外，对底层 context 函数的唯一引用是共享辅助方法自己的委托——因此注释、字符串字面量和 import 的排版不会误报，而别名、别名 import 或直接调用都会连同行号被报出。三条行为测试覆盖此前没有覆盖的改道处理器（settled turn status、status transcript 分页、非 live 的 loadUpdates）：各自断言到达被 mock 的 context 函数的 settings 与 cwd 是本次请求的；transcript 测试还检查在钉定作用域内构建的 replay config 是由请求的 settings 生成的。承载 #10095 三条回归测试的 describe 块改名为它实际覆盖的内容。
为什么需要
#10095 的跟进，关闭那里记录的两条非阻塞项：第 2 轮评审延后的 D2-1（近似的辅助方法已存在，修复却又手工粘贴了三遍路由），以及维护者指出三条回归测试被报在
renameSession routing这个 describe 下。手写路由正是 #10095 里出问题的形态——每个处理器各自决定"用哪份 settings 钉这次操作"，其中三个决定错了。本 PR 自己的第 1 轮评审表明：仅仅经过一个透传的辅助方法并不能把这个决定从调用点移走；按请求变体做到了，下面的变异矩阵显示这个入口对每个使用它的处理器都是承重的。唯一入口这个不变量需要源码级钉住而不是行为测试，因为两种写法最终到达同一个函数。做法沿用
serve/fast-path.test.ts的先例（用包内相对路径读源码后遍历 TypeScript AST）。审阅者验证方案
如何验证
grep -n 'runWithAcpRuntimeOutputDir' packages/cli/src/acp-integration/acpAgent.ts——除规范 import 行和注释外应恰好命中一处：共享辅助方法的委托。main上有六处调用。grep -c 'runWithPinnedRuntimeBaseDirForRequest(' …——应为 6 处调用（定义行带类型参数，不匹配）。cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts——605/605（601 条原有 + 源码钉 + 三条行为钉）。fix(cli): resolve session-management settings per request, not from the stale this.settings cache #10095 的三条回归测试报在QwenAgent session-management routing (rename / delete / list / branch / close) > …下。deleteSession还原为三参数形式并传this.settings**→1 失败 / 603 通过：只有resolves deleteSession settings per request；源码钉保持绿色（它不是守这一点的测试）。sessionTranscript传this.settings**→3 失败 / 601：新 transcript 钉加两条已有的 replay-config 测试。sessionTurnStatus传this.settings**→1 失败 / 603：只有新的 turn-status 钉（该处理器此前没有覆盖）。loadUpdates传this.settings**→1 失败 / 604：只有新的 loadUpdates 钉（在第 1 轮的 head 上这个变异不可见，605/605）。const pin = runWithAcpRuntimeOutputDir; return pin(…)）**→1 失败 / 603：源码钉，报错信息把别名行与委托行并列列出。import { runWithAcpRuntimeOutputDir as pinDirect }）并在辅助方法里用它传this.settings**→7 失败 / 598：源码钉列出该 import 行，六个处理器的钉抓住 settings 那一半。this.settings**→7 失败 / 597：五个按请求处理器的钉全部失败，加两条 replay-config 测试。这一行说明入口是承重的。git diff -w --stat main与git diff --stat main一致（两个文件 +272 / −16）：diff 里没有藏着纯空白改动。前后对比证据
N/A——无运行时行为变化的重构。证据是第 1 步的 grep 计数（六处直接调用→一处）和第 3 步的变异矩阵。
测试平台
环境（可选）
仅本地单元测试（
vitest；packages/cli的tsc --noEmit在acp-integration/*下零报错；两个文件的eslint与prettier --check干净）。运行前从本分支的 merge-base 重建了 core 与 acp-bridge 的dist。风险与范围
loadSettingsCached(cwd)。源码钉测试用的是fast-path.test.ts已在用的 TypeScript 编译器 API 解析文件；若将来引入第二处合理的直接引用，失败信息会指出行号，改一行断言即可。qwen --acp客户端的 cwd 与被查询的 workspace 一致）是独立话题，本 PR 不涉及。session load / resume 有意保留在调用点解析请求的 settings（有 profiler 埋点，且解析出的 settings 随后被会话采用）；它们和其余三参数调用方均未改动。关联 Issue
关联 #10095（关闭其第 2 轮延后项 D2-1 与维护者的 describe 块小建议；两条发现都在该 PR 的评审线程里，未另开 issue）。与 #10465 之于 #9930 是同一种跟进形态。