fix(cli): route turn-index reads through runtime-root pin - #11036
Conversation
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical — and it is red on Direction: aligned. This restores an invariant #10095 introduced for a real bug class — handlers composing the runtime-root routing by hand and pinning another workspace's root via the process-wide Size: Stage 0 not applicable — Approach: this is what I would have written. The per-request tier is the right one — the handler serves a caller-supplied Risk: Stage 1e matched Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 是已观测到的问题，不是理论性加固——而且 方向： 对齐。这恢复的是 #10095 为真实 bug 类别引入的约束——handler 手工组合 runtime-root 路由，经由进程级 规模： Stage 0 不适用—— 方案： 换我来写也是这样。per-request 这一层是对的——该 handler 服务调用方传入的 风险： Stage 1e 命中 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI formed my own answer before reading the diff: the invariant test demands exactly one direct mention of No critical blockers, and no AGENTS.md violations. What I checked: Behavior equivalence. The helper chain is Nothing lost by hoisting the closure. The extracted body never referenced Error mapping intact. The flush stays inside the pin. The No collateral. Two non-blocking observations, neither worth a round trip:
Test evidenceUnattended CI run, so per the triage rules I did not build or execute anything from this PR — the evidence below is the PR's own CI plus a static re-derivation I did myself against the base commit. I did not take the described failure on faith. On base The decisive check, Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查在读 diff 之前我先给出了自己的答案：约束测试要求 无阻断性问题，也没有违反 AGENTS.md。核对过的点： 行为等价。 helper 链路为 提出闭包没有丢东西。 被提取的函数体从未引用 错误映射保持。 flush 仍在 pin 内。 无附带影响。 两点非阻断观察，都不值得为此多走一轮：
测试证据本次为无人值守 CI 运行，按 triage 规则我没有构建或执行本 PR 的任何代码——下面的证据来自 PR 自身的 CI，以及我在基线提交上独立做的静态推导。 我没有直接采信所描述的失败。在基线 决定性检查 上方表格为 CI 检查的真实名称与结论（英文部分），此处不再重复。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the fix is exactly the right one and I confirmed the failure independently; my only reservation is a stale doc-comment enumeration, not the code. The part that actually mattered here was not taking the framing on faith. "A source-level invariant test fails" is an easy claim to wave through, and this failure is invisible to every behavioral test in the file — the The approach matches what I would have written, and it lands on the shape two sibling handlers in the same file already use, so On volume — the author has roughly twenty open PRs at the moment. I evaluated this one on its own evidence rather than on the strength of the description, and the count does not change the answer. It does make me glad this one is small enough to verify completely. The single thing I would add, and would not block on: the doc comment enumerating which handlers must take the per-request form still doesn't name turn index. Two words, next time someone is in the file. Approval is deferred, not withheld. 中文说明Confidence: 4/5 —— 修法完全正确，且我独立确认了失败本身；唯一的保留意见是一处文档注释的列举已过时，与代码无关。 这里真正要紧的是不轻信 PR 的表述。“某个源码级约束测试失败”是一个很容易被放行的说法，而这次失败对文件里所有行为测试都是不可见的—— 方案与我会写的一致，并且落在同文件中两个同类 handler 已有的形态上，因此 关于数量——作者目前约有二十个开放 PR。我是依据本 PR 自身的证据、而非描述的说服力来评估的，数量不改变结论。但也确实让我庆幸这一个足够小，可以完整验证。 我唯一会补、且不会因此阻断的一点：列举哪些 handler 必须使用 per-request 形式的文档注释仍未提到 turn index。两个词的事，等下次有人改到这个文件时顺手加上。 批准是推迟，不是否决。我完成审查时 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
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: reverse audit — stopped before round 1 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| return await this.runWithPinnedRuntimeBaseDirForRequest( | ||
| cwd, | ||
| readTurnIndexPage, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-1: The turn-index handler is the only one of the seven handlers routed through runWithPinnedRuntimeBaseDirForRequest that has no behavioral routing test pinning that the request's settings and cwd reach the pin. The six sibling handlers each have one (sessionTurnStatus, sessionTranscript, deleteSession, renameSession, listSessions, non-live loadUpdates — acpAgent.test.ts:15272, 16905, 21214, 21252, 21289, 21323). The existing turn-index tests pass no caller-supplied cwd and assert nothing about loadSettings/runWithAcpRuntimeOutputDir, and the AST choke-point test only forbids naming runWithAcpRuntimeOutputDir directly — its own comment ("pinned behaviorally … by the routing tests above") presumes coverage this handler does not have.
If a future edit re-pins this handler from the process-wide cache (e.g. this.runWithPinnedRuntimeBaseDir(this.settings, cwd, readTurnIndexPage)), the AST choke-point test stays green and no existing turn-index test detects it, so in a multi-workspace daemon a turn-index request naming another workspace's cwd scans the boot workspace's runtime root and returns the wrong transcript page or a spurious ENOENT (-32002/-32010) — the exact #10095 bug class this choke point exists to prevent.
Witness:
Probe (scratch tree) at 40ae7cf:
MUTANT this.runWithPinnedRuntimeBaseDir(this.settings, cwd, readTurnIndexPage) + full suite:
Tests 621 passed (621) ← nothing detects the regression
MUTANT + suggested routing test:
× expected "spy" to be called with arguments: [ '/tmp/workspace-a' ]
INTACT + suggested routing test:
Tests 1 passed | 621 skipped
Suggested fix: add a routing test beside the sibling ones, mirroring resolves qwen/status/session/transcript settings per request, not from the this.settings cache (acpAgent.test.ts:16905): boot the agent for one workspace, mock loadSettings to return a distinguishable per-request settings object, call extMethod(SERVE_STATUS_EXT_METHODS.sessionTurnIndex, { cwd: '/tmp/workspace-a', sessionId }), and assert loadSettings was called with '/tmp/workspace-a' and runWithAcpRuntimeOutputDir received (perRequestSettings, '/tmp/workspace-a').
Note the fix must respect that this route derives cwd as const cwd = requestedCwd || process.cwd() (acpAgent.ts:8500), so the test must pass an explicit cwd in the request params, and the assertions must target the module-mocked runWithAcpRuntimeOutputDir (acpAgent.test.ts:878) because the helper delegates to it (acpAgent.ts:4595).
Acceptance check: the new test must go red if line 9218 is mutated to pin from this.settings (this.runWithPinnedRuntimeBaseDir(this.settings, cwd, readTurnIndexPage)) — please run that mutation once to confirm.
中文说明
turn-index 处理器是七个经由 runWithPinnedRuntimeBaseDirForRequest 路由的处理器中唯一没有行为级路由测试的——该测试用于确保请求的 settings 与 cwd 真正传入 pin。其余六个同级处理器各有一个（sessionTurnStatus、sessionTranscript、deleteSession、renameSession、listSessions、非 live 分支的 loadUpdates，见 acpAgent.test.ts:15272、16905、21214、21252、21289、21323）。现有的 turn-index 测试不传调用方指定的 cwd，也不对 loadSettings/runWithAcpRuntimeOutputDir 做任何断言，而 AST 扼制点测试只禁止直接写出 runWithAcpRuntimeOutputDir——它自己的注释（"pinned behaviorally … by the routing tests above"）预设了该处理器并不具备的覆盖。
如果未来某次修改把这个处理器重新固定到进程级缓存（例如 this.runWithPinnedRuntimeBaseDir(this.settings, cwd, readTurnIndexPage)），AST 扼制点测试仍会通过，现有 turn-index 测试也不会发现；在多工作区 daemon 中，一个指定了其他工作区 cwd 的 turn-index 请求会扫描启动工作区的 runtime root，返回错误的 transcript 页面或误报 ENOENT（-32002/-32010）——正是该扼制点要防的 #10095 一类 bug。
验证证据（临时树探针，提交 40ae7cf）：将处理器突变为从 this.settings 固定后，整套测试仍然全部通过（621 passed），没有任何测试发现该回归；加上建议新增的路由测试后，突变版本失败（期望 "spy" 以 [ '/tmp/workspace-a' ] 被调用），完整代码通过。
建议修复：在同级测试旁新增一个路由测试，仿照 resolves qwen/status/session/transcript settings per request, not from the this.settings cache（acpAgent.test.ts:16905）：以一个工作区启动 agent，mock loadSettings 返回可区分的按请求 settings，调用 extMethod(SERVE_STATUS_EXT_METHODS.sessionTurnIndex, { cwd: '/tmp/workspace-a', sessionId })，并断言 loadSettings 以 '/tmp/workspace-a' 被调用、runWithAcpRuntimeOutputDir 收到 (perRequestSettings, '/tmp/workspace-a')。
注意：该路由以 const cwd = requestedCwd || process.cwd()（acpAgent.ts:8500）派生 cwd，因此测试必须在请求参数中显式传入 cwd；断言必须针对模块级 mock 的 runWithAcpRuntimeOutputDir（acpAgent.test.ts:878），因为 helper 会委托给它（acpAgent.ts:4595）。
验收方式：把第 9218 行突变为从 this.settings 固定（this.runWithPinnedRuntimeBaseDir(this.settings, cwd, readTurnIndexPage)）时，新测试必须变红——请跑一次该突变以确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head 40ae7cf1.
- The change is exactly what main needs: it routes the turn-index reader through
runWithPinnedRuntimeBaseDirForRequest— the same helper the other six caller-supplied-cwd handlers use, whose body performs the identicalloadSettingsCached(cwd)+ delegation the inline code did — closing the stale-base violation that has been failing #10988's runtime-root guard on main's Test lane since the #10751 merge (the failure I flagged on #11033, #11025 and #11026). Wrapping the callback body is the established sibling pattern (the transcript reader two scopes away reads identically), and the guard's own AST walk now sees only the permitted delegation at :4595. - The one open thread is ci-bot's Suggestion that the turn-index handler lacks a behavioral routing test of its own; six sibling handlers ride the same helper with the source-level invariant as their pin, and this is S-grade under the repo's own rule.
- CI facts, with the failure attributed by execution: 23 checks pass; the lone
Test (ubuntu)red on this head is NOT the guard and NOT this PR — I reproduced it locally at this tree and it isno-ak-integration-ci.test.jsexpecting the no-AK script list without./cli/_prompt-latency-policy.test.ts, a merge-window skew between this PR's base and #11004's package.json/test pair that passes on currentmain(verified: 12 passed on the tip). This PR touches neither file; a base sync/rebase clears it. Theweb-shell E2E Smokered is the recurring 20-minute pool cap cancellation. Per the channel convention the call is on the review itself.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Scope: acpAgent.ts diff only (6 additions, 3 deletions, 1 file). The change is a rebase of 40ae7cf1 onto current main with no semantic delta.
What I checked:
- Behavioral equivalence:
runWithPinnedRuntimeBaseDirForRequest(cwd, op)internally callsloadSettingsCached(cwd)thenrunWithAcpRuntimeOutputDir(settings, cwd, () => op(settings))— identical to the removed inline code. The operation body capturesthis.sessions,sessionId,rawSnapshot,rawCursor,rawLimitfrom the outer closure; it ignores thesettingsargument the helper passes, which is correct because the original also did not usesettingsinside the operation. - Error handling:
catchblock is identical to the base — error types and request codes are unchanged. - Choke-point test:
acpAgent.test.ts:30537walks the AST and expects exactly one non-import mention ofrunWithAcpRuntimeOutputDir:4595: return runWithAcpRuntimeOutputDir(settings, cwd, operation). After this PR the turn-index handler no longer names it directly, so the test passes.
Confirmed suggestion (ci-bot R1-1): The turn-index handler is the only one of the seven handlers routed through runWithPinnedRuntimeBaseDirForRequest that has no behavioral routing test verifying the request's cwd and settings reach the pin. The six siblings have one each (acpAgent.test.ts:15272, 16905, 21214, 21252, 21289, 21323). The choke-point test's own comment says "pinned behaviorally … by the routing tests above" — that presupposition does not hold for this handler. A mutation probe (from ci-bot's report) confirmed: replacing line 9218 with this.runWithPinnedRuntimeBaseDir(this.settings, cwd, readTurnIndexPage) leaves all 621 tests green. The suggested fix is a routing test mirroring the sibling at acpAgent.test.ts:16905. Non-blocking — the current routing is correct; the gap is in regression-prevention coverage.
Not covered: No working tree available for a local build or mutation run; rung 3 not applicable (no environment-dependent change).
Reviewed with AI assistance.
| )) as unknown as Record<string, unknown>; | ||
| }); | ||
| }; | ||
| return await this.runWithPinnedRuntimeBaseDirForRequest( |
There was a problem hiding this comment.
[Confirmed — Suggestion] R1-1: This handler is the only one of the seven runWithPinnedRuntimeBaseDirForRequest callers without a behavioral routing test verifying that the request's cwd and settings reach the pin. Six sibling handlers each have one (acpAgent.test.ts:15272, 16905, 21214, 21252, 21289, 21323). The choke-point test's own comment — "pinned behaviorally … by the routing tests above" — presupposes this coverage, but this handler has none.
Mutation witness (from ci-bot's probe): mutating this call to this.runWithPinnedRuntimeBaseDir(this.settings, cwd, readTurnIndexPage) leaves all 621 tests green, so the #10095 regression class can recur here undetected. The suggested fix (per ci-bot) is a routing test mirroring resolves qwen/status/session/transcript settings per request, not from the this.settings cache (acpAgent.test.ts:16905): boot the agent for one workspace, mock loadSettings to return a distinguishable per-request settings object, call extMethod(SERVE_STATUS_EXT_METHODS.sessionTurnIndex, { cwd: '\/tmp\/workspace-a', sessionId }), and assert loadSettings was called with '\/tmp\/workspace-a' and runWithAcpRuntimeOutputDir received (perRequestSettings, '\/tmp\/workspace-a'). The test must go red when mutated to pin from this.settings.
Non-blocking — current routing is correct.
(cherry picked from commit eaef97e)
The shared parser cast three capability members through unchecked while every consumer reads them strictly, so a mistyped value in an unvalidated `modelProviders` settings entry silently changed the declaration instead of failing it. A truthy non-boolean `toggleOnly` collapsed a declared ladder to toggle-only: the pickers refused every tier while `disableField` still reshaped the wire. `efforts: []` passed `[].every()` and emitted a zero-tier capability that suppressed the manifest fallback and stripped the effort from the request. A non-`false` `canDisable` degenerated to absent, so a route declaring that disabling is forbidden still shipped the disable shape that declaration exists to prevent. All three are now rejected rather than coerced, along with a repeated tier, which duplicated picker rows, ACP options and React keys. Well-formed input keeps the identity contract. Both effort pickers forced the cursor onto the first tier when the stored global `model.reasoningEffort` is not one the active model exposes, and confirming without moving persisted that tier over the stored value -- the rewrite measured live as F2. A forced cursor now cancels in the ink dialog and closes without writing in the OpenTUI one; navigating first still selects, and the unset case is unchanged. The OpenTUI footer discloses the unset case too, matching ink. Four output-style dialog tests drove keys before the effect deriving the cursor from the async catalog had run, so that derivation could overwrite the navigation and pick the wrong row; they now wait for it. Every new guard condition has a mutation witness: removing the toggleOnly, canDisable, empty-efforts or uniqueness check, the ink cursor-moved or stored-tier condition, or the OpenTUI forced-cursor condition each reddens the test that pins it, and acceptance of a well-formed `canDisable: false` or `toggleOnly: false` is pinned as well. Not changed here: the pipeline still drops a carried-over tier the capability does not list instead of clamping it onto the ladder. The maintainer's real-stack A/B records refusing an unsupported tier as correct and files the carried-over case as a picker problem, and the author's triage answer names the drop as the requested migration contract, so the clamp stays a maintainer call and its thread stays open. The turn-index per-request settings test is deferred: this round's merge of main absorbed the identical handler fix (#11036), leaving only the roster docstring here.
4 participants
What this PR does
Routes the session turn-index reader through the existing per-request runtime-root helper. The helper performs the same cwd-based settings lookup and delegates to the same runtime context operation as the previous inline code.
Why it's needed
PR CI job 101103789058 deterministically failed the runtime-root choke-point test three times. #10751 added the turn-index reader after #10988's branch point; #10988 later added the invariant that caller-supplied-cwd handlers must use the shared helper. Both PRs passed independently, but the stale-base merge left the concurrently added reader outside the helper.
Reviewer Test Plan
How to verify
Confirm that the turn-index path still flushes the active recording when required, reads the same cwd and pagination arguments, and preserves the existing error mapping. Confirm that the source-level runtime-root invariant sees only the shared helper's permitted delegation.
Evidence (Before & After)
Before: the linked CI job reported a second direct
runWithAcpRuntimeOutputDiridentifier at the turn-index reader and failed 1 of 28,585 CLI tests.After: replaying the test's TypeScript AST walk reports only
4595: return runWithAcpRuntimeOutputDir(settings, cwd, operation);.git diff --checkand Prettier pass.Tested on
Environment (optional)
Static AST and formatting checks only.
Risk & Scope
Linked Issues
Follow-up to #11033 and #11030.
中文说明
本 PR 做了什么
让 session turn-index reader 经过现有的 per-request runtime-root helper。该 helper 会执行与原内联代码相同的基于 cwd 的 settings 查找，并委托给相同的 runtime context 操作。
为什么需要
PR CI job 101103789058 中的 runtime-root choke-point 测试连续三次确定性失败。#10751 在 #10988 的分支基线之后新增了 turn-index reader；#10988 随后加入 caller-supplied-cwd handler 必须使用共享 helper 的约束。两个 PR 独立运行时都通过，但陈旧基线合并使并发新增的 reader 留在了 helper 之外。
Reviewer Test Plan
如何验证
确认 turn-index 路径仍会在需要时 flush 活跃记录，使用相同的 cwd 和分页参数读取，并保留现有错误映射。确认源码级 runtime-root 约束只发现共享 helper 中唯一允许的委托。
证据（修改前后）
修改前：链接中的 CI job 在 turn-index reader 处发现第二个直接
runWithAcpRuntimeOutputDir标识符，28,585 个 CLI 测试中有 1 个失败。修改后：复刻该测试的 TypeScript AST 遍历后，只报告
4595: return runWithAcpRuntimeOutputDir(settings, cwd, operation);。git diff --check和 Prettier 均通过。测试平台
环境（可选）
仅执行静态 AST 与格式检查。
风险与范围
关联 Issue
#11033 和 #11030 的 follow-up。