Skip to content

fix(cli): route the transcript turn-index handler through the pin choke point - #11047

Merged
wenshao merged 3 commits into
mainfrom
fix/acp-transcript-pin-chokepoint
Sep 5, 2026
Merged

fix(cli): route the transcript turn-index handler through the pin choke point#11047
wenshao merged 3 commits into
mainfrom
fix/acp-transcript-pin-chokepoint

Conversation

@wenshao

@wenshao wenshao commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

The per-request runtime-root pinning has a single choke point: one private method that composes the settings-to-cwd routing, with a guard test that fails the build if any other line in the agent names the underlying context runner directly. One handler still composed the routing by hand — the session transcript turn-index page reader — loading the settings for the caller-supplied working directory and calling the context runner itself. This routes that handler through the per-request choke-point form, which resolves the settings for the request's cwd in one place, exactly as the choke point's own documentation lists for transcript page handlers.

Why it's needed

The guard test is red on main today: it enumerates every direct mention and expects only the choke point itself, so every PR inheriting main fails Test (ubuntu-latest) on this assertion. The hand-composed call is also the bug class the choke point exists to prevent — a handler deciding by itself which settings pin the operation — so routing it through the per-request form both clears the gate and restores the invariant the guard protects. Behaviour is unchanged: the per-request form resolves the same cached settings for the same cwd and pins identically; the handler's operation never used the locally loaded settings object.

Reviewer Test Plan

How to verify

  1. Run the agent test file and confirm the choke-point case ("routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir") passes, and that grepping the agent source for the context runner's name yields only the choke point's own body.
  2. Exercise the transcript turn-index endpoint (session/transcript/turnIndexPage or the equivalent ACP request) against a daemon with a session that has recorded turns: paging with start/limit and a snapshot cursor returns the same pages as before this change.
  3. Confirm invalid cursors and limits still surface their structured errors (invalid transcript cursor / invalid limit), since the handler's catch chain is untouched but now wraps the routed call.

Evidence (Before & After)

Before: the choke-point test fails on main with the turn-index handler's direct call listed as a second mention; after: the test passes and the mention list contains only the choke point. The handler's own behaviour is covered by the existing transcript navigation tests, which stay green.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Local unit run of the agent test file plus lint, formatting and type checks.

Risk & Scope

  • Main risk or tradeoff: none behavioural — the routed form performs the identical settings resolution and pin; the only change is where the routing is composed.
  • Not validated / out of scope: two unrelated agent tests fail locally on a pristine main checkout in this environment (an env-parsing case and a status-snapshot case) and pass in CI; they are untouched here.
  • Breaking changes / migration notes: none.

Linked Issues

None; clears the red guard test inherited from #10988.

中文说明

这个 PR 做了什么

每次请求的 runtime-root pin 有一个单一 choke point:一个私有方法负责组合 settings 到 cwd 的路由,并有守卫测试——agent 里任何其他行直接点名底层 context runner 即构建失败。仍有一个 handler 手工组合路由:会话 transcript turn-index 分页读取器——它自己为调用方提供的 cwd 加载 settings 并直接调用 context runner。本 PR 让该 handler 走 per-request 形式的 choke point(由它在一处解析请求 cwd 的 settings),与 choke point 自身文档为 transcript page handler 列出的做法一致。

为什么需要

该守卫测试今天在 main 上是红的:它枚举所有直接点名并只期望 choke point 自身,因此每个继承 main 的 PR 都会在 Test (ubuntu-latest) 上因这条断言失败。手工组合调用也正是 choke point 要防止的 bug 类(handler 自行决定用哪份 settings pin 操作),改走 per-request 形式既清门禁又恢复守卫所保护的不变式。行为不变:per-request 形式对同一 cwd 解析同样的缓存 settings 并同样 pin;handler 的 operation 从未使用本地加载的 settings 对象。

审阅测试计划

如何验证

  1. 跑 agent 测试文件,确认 choke point 用例通过;grep agent 源码中 context runner 的名字应只剩 choke point 自身函数体。
  2. 对着有已记录回合的 daemon 会话调用 transcript turn-index 端点:带 start/limit 与 snapshot 游标的分页应返回与本改动前相同的页。
  3. 确认非法游标与 limit 仍返回结构化错误(invalid transcript cursor / invalid limit)——catch 链未动,但现在包裹的是路由后的调用。

证据(前后对比)

改动前:choke point 测试在 main 上失败,列出 turn-index handler 的直接调用为第二处点名;改动后:测试通过,点名列表只剩 choke point。handler 自身行为由既有 transcript navigation 测试覆盖,保持绿。

测试环境

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

本地跑 agent 测试文件,加 lint、格式化与类型检查。

风险与范围

  • 主要风险或取舍:无行为变化——路由形式执行完全相同的 settings 解析与 pin;唯一变化是路由在哪里组合。
  • 未验证 / 范围之外:两个无关的 agent 测试在本环境干净 main 上本地失败(env 解析与 status 快照用例),在 CI 上通过;此处未触碰。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

无;清除继承自 #10988 的红色守卫测试。

…ke point

The #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 #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.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Qwen Triage ended earlyview run. It stopped before finishing; check the run log.

⚠️ Qwen Triage 提前结束 —— 查看运行。未跑完,请查看运行日志。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, not theoretical — and it is currently main's only red test. On the base commit 74fe3a65, Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) both fail with exactly one failing test out of 28,589: QwenAgent runtime-root pinning choke point > routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir. The assertion names 9200: return await runWithAcpRuntimeOutputDir(settings, cwd, async () => { as the second direct mention — which is precisely the line this PR removes.

Direction: aligned. This is a gate repair rather than a feature. The guard landed expecting only the choke point to name runWithAcpRuntimeOutputDir, but the turn-index handler was left composing the routing by hand, so every PR inheriting main fails Test. Routing that handler through the per-request form clears the gate and restores the #10095 invariant the guard protects — and the choke point's own doc comment already lists transcript page handlers among the per-request handlers that must not make the settings decision themselves, so this is the documented shape, not a new one.

Size: not applicable. packages/cli/src/acp-integration/ is not a core-infra path and the change stays inside one package: 1 file, +22/−20, all production lines (no test or generated files). The semantic delta is 2 lines; the rest is re-indentation from the operation moving one argument position deeper.

Approach: minimal, and I do not see a better path. The only smaller diff would be relaxing the guard test, which discards the invariant it pins. No drive-by refactors and no unrelated edits: the removed settings local had exactly one use (the call being replaced), and both loadSettingsCached and runWithAcpRuntimeOutputDir keep other uses in the file, so no import goes unused.

Risk: Stage 1e matches one high-risk path — acp-integration is in the revert-history set (10 of 31 reverted PRs touched these paths vs 5 of 60 controls). Not a blocker; it just means the review should not be shallow and CI evidence should land before approval. Both are handled in Stage 2.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 是已观测到的问题,不是理论性加固——而且它正是 main 目前唯一红着的测试。在 base commit 74fe3a65 上,Test (macos-latest, Node 22.x)Test (windows-latest, Node 22.x) 均失败,28,589 个测试中恰好只有 1 个失败:QwenAgent runtime-root pinning choke point > routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir。断言把 9200: return await runWithAcpRuntimeOutputDir(settings, cwd, async () => { 列为第二处直接点名——而这正是本 PR 删掉的那一行。

方向: 对齐。这是修门禁,不是加功能。守卫测试落地时期望只有 choke point 点名 runWithAcpRuntimeOutputDir,但 turn-index handler 仍然手工组合路由,于是每个继承 main 的 PR 都会在 Test 上失败。让该 handler 走 per-request 形式既清了门禁,也恢复了守卫所保护的 #10095 不变式——而且 choke point 自己的文档注释早已把 transcript page handler 列入"不得自行决定用哪份 settings"的 per-request handler 名单,所以这是文档里既有的形态,不是新形态。

规模: 不适用。packages/cli/src/acp-integration/ 不属于核心基础设施路径,改动也只在一个包内:1 个文件,+22/−20,全部是生产代码行(无测试或生成文件)。语义上的变化只有 2 行,其余是因为 operation 多嵌套一层参数而产生的重新缩进。

方案: 足够小,也想不到更好的路径。唯一更小的 diff 是放宽守卫测试,但那等于丢掉它要钉住的不变式。没有顺手重构,也没有夹带无关改动:被删掉的 settings 局部变量只有一处使用(就是被替换的那个调用),而 loadSettingsCachedrunWithAcpRuntimeOutputDir 在文件里都还有其他使用点,所以不会有 import 变成未使用。

风险: Stage 1e 命中一个高风险路径——acp-integration 在 revert 历史分析的集合里(31 个被 revert 的 PR 中有 10 个触及这些路径,对照组 60 个中只有 5 个)。这不是阻塞项,只是意味着 review 不能太浅、且批准前应等 CI 证据落地。这两点在 Stage 2 都处理了。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 65e4f9fb9cd9bc07075ddb5fcc3621d7001d57f8 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Code review

What I would have done from the title and motivation alone: find the one remaining hand-composed pin in the turn-index handler and replace runWithAcpRuntimeOutputDir(loadSettingsCached(cwd), cwd, fn) with the per-request helper, dropping the local that becomes dead. The only real judgement call is which helper — runWithPinnedRuntimeBaseDir (caller passes deliberately scoped settings) versus runWithPinnedRuntimeBaseDirForRequest (helper resolves settings from the request cwd) — and that turns on whether the handler's cwd is caller-supplied.

The diff is exactly that, and the judgement call is right. I checked the cwd provenance independently rather than taking the description's word: extMethodInternal computes const cwd = requestedCwd || process.cwd() from params['cwd'], so this handler serves a caller-supplied cwd and belongs in the per-request class the choke point's doc comment already enumerates (list, delete, rename, transcript page, settled turn status, non-live loadUpdates). The two sibling handlers in that class — transcript page and settled turn result — already call the same helper in the same shape.

Semantic equivalence, traced end to end rather than assumed:

  • Before: loadSettingsCached(cwd)runWithAcpRuntimeOutputDir(settings, cwd, op)Storage.runWithRuntimeBaseDir(settings.merged.advanced?.runtimeOutputDir, cwd, op).
  • After: runWithPinnedRuntimeBaseDirForRequest(cwd, op)loadSettingsCached(cwd)runWithPinnedRuntimeBaseDir(settings, cwd, () => op(settings)) → the same runWithAcpRuntimeOutputDir → the same Storage.runWithRuntimeBaseDir.

Same settings resolution, same cwd, same pin, same operation. The only deltas are one extra arrow frame and a settings argument the operation ignores.

Things that could have broken and did not:

  • The removed settings local had exactly one use — the call being replaced. The operation body never referenced it, so nothing downstream loses its settings object.
  • Neither import goes unused: loadSettingsCached keeps seven other call sites in the file, and runWithAcpRuntimeOutputDir keeps the canonical import plus the choke point's own delegation.
  • this binding survives. The arrow still closes over this for this.sessions.get(sessionId); the helper's () => operation(settings) wrapper rebinds nothing.
  • The catch chain is untouched and still wraps the routed call. Neither helper catches, so InvalidSessionTranscriptCursorError / RangeError still map to -32602 with invalid_transcript_cursor / invalid_transcript_limit exactly as before.
  • Formatting matches the sibling call sites, so this should not trip Prettier.

The guard test now passes — by construction, not by hope. After the change the AST walk finds runWithAcpRuntimeOutputDir as an identifier in exactly two places: the canonical un-aliased import specifier (explicitly exempt) and the choke point's return runWithAcpRuntimeOutputDir(settings, cwd, operation);. That is a one-element array matching the expected StringMatching, and the doc-comment mention is prose, not an identifier node, so it cannot false-positive. The choke point sits above the edited region, so its line number does not even shift.

Findings: none. No correctness bug, no regression, no AGENTS.md violation. One honest observation about coverage, not a defect: the handler's behavioural tests (flushes latest but not frozen turn-index pages and the error-path it.each) mock SessionTranscriptReader and assert call args, flush-once, and the structured errors. They stay green through this change — but they would also have stayed green through the original regression, which is precisely why the source-level guard exists. Nothing to add here.

Test evidence

The decisive evidence is on the base, not the head. This PR's base SHA is main's tip 74fe3a65, where Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) both completed failure. The failing job's log names exactly one failing test out of 28,589, and it is this one:

FAIL  src/acp-integration/acpAgent.test.ts > QwenAgent runtime-root pinning choke point > routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir

AssertionError: acpAgent.ts must not name runWithAcpRuntimeOutputDir directly. Handlers serving a caller-supplied cwd route through this.runWithPinnedRuntimeBaseDirForRequest; only callers holding deliberately scoped settings may use this.runWithPinnedRuntimeBaseDir (see #10095). Direct mentions at:
4595: return runWithAcpRuntimeOutputDir(settings, cwd, operation);
9200: return await runWithAcpRuntimeOutputDir(settings, cwd, async () => {: expected [ …(2) ] to deeply equal [ StringMatching{…} ]

 Test Files  1 failed | 1010 passed (1011)
      Tests  1 failed | 28488 passed | 100 skipped (28589)

So "the guard test is red on main today, and the turn-index handler's direct call is the second mention" is confirmed verbatim from main's own CI log — not from the PR description. The diff deletes precisely the line the assertion names, and nothing else fails on base that this PR could affect.

On the head, Test (ubuntu-latest, Node 22.x) and Lint & Static were still in_progress at the single fetch above (no polling — this suite runs ~30 min). Test (macos/windows) are skipped on PR runs in this repo; they only execute post-merge. So the head's green is not yet in hand: the base-is-red evidence plus the by-construction argument above is what carries this review, and the pending ubuntu run is the same suite that is red on base, which makes it a real A/B signal once it lands. The finalize workflow rewrites the table below in place when CI settles.

Not verified: the head's own Test (ubuntu-latest) and Lint & Static conclusions (still running at fetch time). Also not verified live: the paging round-trip against a real daemon from the test plan — I did not run any PR-derived code, per the static-review rule.

Final CI results for 65e4f9f (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Test (ubuntu-latest, Node 22.x) 🚫 cancelled
web-shell E2E Smoke (ubuntu-latest, Node 22.x) 🚫 cancelled
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
macos-latest / Java 21 ✅ success
OpenTUI no-flicker gate ✅ success
Real daemon E2E / Java 11 ✅ success
Secret scan (TruffleHog) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle the remaining gap: @qwen-code /verify — an A/B against base 74fe3a65 would prove the guard test flips red→green because of this diff rather than incidentally, and that the 28,488 currently-passing tests still pass with the routing moved. This is belt-and-braces rather than load-bearing: the claim is a source-shape assertion, so it is decidable by reading, and the pending ubuntu Test run is already the same suite that is red on base. There is no TUI surface here, so /tmux would add nothing.

中文说明

代码审查

只看标题和动机时我会怎么做: 找到 turn-index handler 里最后一处手工组合的 pin,把 runWithAcpRuntimeOutputDir(loadSettingsCached(cwd), cwd, fn) 换成 per-request helper,并删掉因此失效的局部变量。唯一真正需要判断的是用哪个 helper——runWithPinnedRuntimeBaseDir(调用方传入刻意限定作用域的 settings)还是 runWithPinnedRuntimeBaseDirForRequest(helper 自己按请求 cwd 解析 settings)——而这取决于该 handler 的 cwd 是否由调用方提供。

diff 正是如此,且这个判断是对的。我没有采信 PR 描述,而是自己查了 cwd 的来源:extMethodInternal 里是 const cwd = requestedCwd || process.cwd(),取自 params['cwd'],所以该 handler 服务的是调用方提供的 cwd,属于 choke point 文档注释已经列举的 per-request 一类(list、delete、rename、transcript page、settled turn status、非 live 的 loadUpdates)。这一类里的两个同级 handler——transcript page 与 settled turn result——本来就以同样的形态调用同一个 helper。

语义等价(是完整追出来的,不是假设的):改动前后最终都走到同一个 Storage.runWithRuntimeBaseDir(settings.merged.advanced?.runtimeOutputDir, cwd, op),settings 解析方式相同、cwd 相同、pin 相同、operation 相同。唯一差别是多了一层箭头函数,以及 operation 收到一个它并不使用的 settings 参数。

可能出问题但实际没有的地方:

  • 被删掉的 settings 局部变量只有一处使用,就是被替换的那个调用;operation 体内从未引用它,所以下游不会丢失 settings 对象。
  • 两个 import 都不会变成未使用:loadSettingsCached 在文件里还有 7 处调用,runWithAcpRuntimeOutputDir 还保留规范 import 与 choke point 自身的委托调用。
  • this 绑定不受影响:箭头函数仍然闭包捕获 this 用于 this.sessions.get(sessionId),helper 里的 () => operation(settings) 不改变任何绑定。
  • catch 链未动,且仍然包裹路由后的调用。两个 helper 都不捕获异常,所以 InvalidSessionTranscriptCursorError / RangeError 仍照原样映射为 -32602invalid_transcript_cursor / invalid_transcript_limit
  • 格式与同级调用点一致,应该不会触发 Prettier。

守卫测试现在是构造性通过的。 改动之后,AST 遍历在 acpAgent.ts 里只会在两处遇到 runWithAcpRuntimeOutputDir 标识符:规范的非别名 import specifier(明确豁免)与 choke point 自身的 return runWithAcpRuntimeOutputDir(settings, cwd, operation);。这正好是长度为 1、匹配期望 StringMatching 的数组;文档注释里的提及是散文而非标识符节点,不会误报。而且 choke point 位于被改区域之前,行号根本不会移动。

结论:没有发现问题。 无正确性缺陷、无回归、无 AGENTS.md 违规。关于覆盖率有一点如实说明(不是缺陷):该 handler 的行为测试(flushes latest but not frozen turn-index pages 与错误路径的 it.each)mock 了 SessionTranscriptReader,断言调用参数、flush 一次以及结构化错误。它们在本次改动后依然为绿——但它们在最初那次回归时同样会是绿的,这也正是需要源码级守卫的原因。此处无需补测试。

测试证据

决定性证据在 base 上,不在 head 上。本 PR 的 base SHA 就是 main 的 tip 74fe3a65,在该 commit 上 Test (macos-latest, Node 22.x)Test (windows-latest, Node 22.x) 均为 failure。失败 job 的日志显示 28,589 个测试中恰好只有 1 个失败,而且就是这一个(原文见上方英文代码块)。

所以"守卫测试今天在 main 上是红的,且 turn-index handler 的直接调用是第二处点名"这一点,是从 main 自己的 CI 日志里逐字确认的,不是来自 PR 描述。diff 删掉的正是断言点名的那一行,base 上也没有其他失败会受本 PR 影响。

在 head 上,Test (ubuntu-latest, Node 22.x)Lint & Static 在上面那次单次抓取时仍是 in_progress(没有轮询——该套件约需 30 分钟)。本仓库 PR 运行中 Test (macos/windows)skipped,只在合并后执行。因此 head 的绿灯尚未到手:支撑本次审查的是 base 红灯证据加上上面的构造性论证,而待完成的 ubuntu 运行与 base 上红灯的是同一套件,落地后就是一个真实的 A/B 信号。CI 结束后 finalize workflow 会就地重写上面的表格。

未验证:head 自身的 Test (ubuntu-latest)Lint & Static 结论(抓取时仍在运行)。同样未做实机验证:测试计划里对真实 daemon 的分页往返——按静态审查规则,我没有执行任何 PR 带来的代码。

沙箱验证可以补上剩下的缺口:@qwen-code /verify——对 base 74fe3a65 做 A/B,可以证明守卫测试是由本 diff 才从红变绿,而非顺带变绿,并证明当前通过的 28,488 个测试在路由搬移后依然通过。这属于双保险而非关键依赖:该断言是源码形态层面的,靠阅读即可判定,而待完成的 ubuntu Test 运行本身就是 base 上红着的那个套件。此处没有 TUI 界面,所以 /tmux 无补充价值。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 65e4f9fb9cd9bc07075ddb5fcc3621d7001d57f8 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — about as tight as a review gets: main's own CI log names the exact line this diff deletes, and the replacement is provably the same call.

My independent proposal and this diff are the same change, including the one judgement call (per-request form, because the handler's cwd comes from params['cwd']). I checked that provenance in the source rather than accepting the description's framing, and the call chain resolves to the identical Storage.runWithRuntimeBaseDir(settings.merged.advanced?.runtimeOutputDir, cwd, …) either way. There is no behavioural surface to get wrong here — the operation, the settings resolution, the cwd and the pin are all unchanged, and the only structural delta is one extra arrow frame.

How main actually went red — worth recording, because it changes the story slightly and it is not what the description says. The description frames this as a handler #10988 left behind. It was not: at #10988's head (2c4d313e) the turn-index handler did not exist in the file at all, and runWithAcpRuntimeOutputDir appeared exactly once outside the import — which is why its own Test (ubuntu-latest) run was legitimately green. The handler came from #10751 (feat(serve): add session turn navigation protocol, merged 2026-09-04 06:34), whose tree predates the guard and which was also legitimately green — at that head the file still had direct calls at six sites, none of them guarded yet. #10988 merged at 16:07 the same day from a branch that had never seen #10751's handler. Git found no textual conflict, both PRs passed their own CI, and main went red at the merge. A textbook semantic conflict, and the guard did its job — it caught a reintroduced pattern that no behavioural test could see. It just caught it post-merge.

That is a non-blocking observation for whoever owns the gate, not a criticism of this PR: a source-text guard cannot be enforced across concurrent branches, and since Test (macos/windows) only run post-merge here, the ubuntu PR run is the sole pre-merge defence. Expect this shape to recur occasionally when a large routing refactor races a large feature PR. Nothing to fix in this diff.

On the maintain-it-in-six-months question: thank, not curse. The handler now reads exactly like its two siblings in the same class, and the file has one place that composes the routing instead of two.

Being explicit about the pushover check, since this is the profile where a rubber stamp is likeliest — an admin author with 18 open PRs, a 2-line semantic diff, and a description that reads plausibly. The approval does not rest on any of that. It rests on the failing assertion quoted verbatim from main's CI log naming line 9200, the AST-walk arithmetic showing the array collapses to the single expected entry after the change, and a traced call chain. I looked for a reason to say no and the only candidate — that the guard test itself is an unusual thing to ship — is a question about #10988, already merged, and relitigating it here would be scope creep.

My one real reservation is timing, not content: the head's Test (ubuntu-latest, Node 22.x) and Lint & Static were still running at fetch time, so I am not holding a green head run. Approval is therefore deferred until CI lands green on 65e4f9fb9cd9bc07075ddb5fcc3621d7001d57f8 — the marker below carries that precondition and the finalize workflow posts the commit-pinned approval once every check completes. If anything lands red or the head moves, it withholds.

中文说明

Confidence: 5/5 —— 证据 tight 到几乎没得挑:main 自己的 CI 日志点名的正是本 diff 删掉的那一行,而替换后的调用可证明与原调用完全相同。

我独立想到的方案与这个 diff 是同一个改动,包括那个唯一需要判断的点(用 per-request 形式,因为该 handler 的 cwd 来自 params['cwd'])。这个来源我是自己在源码里查的,没有采信描述的措辞;两条路径最终都落到同一个 Storage.runWithRuntimeBaseDir(settings.merged.advanced?.runtimeOutputDir, cwd, …)。这里没有可能出错的行为面——operation、settings 解析、cwd 与 pin 全都没变,唯一的结构差别是多了一层箭头函数。

main 究竟是怎么变红的 —— 值得记下来,因为它与描述的说法略有不同。描述把这件事写成 #10988 漏掉了一个 handler。事实并非如此:在 #10988 的 head(2c4d313e)上,turn-index handler 在文件里根本不存在,而 runWithAcpRuntimeOutputDir 在 import 之外只出现一次——所以它自己的 Test (ubuntu-latest) 是合理地为绿。该 handler 来自 #10751feat(serve): add session turn navigation protocol,2026-09-04 06:34 合并),其代码树早于守卫测试,而且同样合理地为绿——在那个 head 上文件里还有六处直接调用,当时都还没有守卫。#10988 在同一天 16:07 从一个从未见过 #10751 那个 handler 的分支上合并。git 没有发现文本冲突,两个 PR 各自的 CI 都通过,而 main 在合并后变红。这是典型的语义冲突,而守卫确实起了作用——它抓到了一次行为测试看不见的模式复发。只是它是在合并后才抓到的。

这一点对门禁的负责人来说是一个非阻塞的观察,不是对本 PR 的批评:源码文本级守卫无法在并发分支之间强制执行,而且本仓库的 Test (macos/windows) 只在合并后运行,所以 ubuntu 的 PR 运行是唯一的合并前防线。当一个大范围路由重构与一个大功能 PR 撞车时,预期这种形态还会偶发。本 diff 里无需修任何东西。

关于"六个月后维护它"这个问题:会感谢作者,不会骂。该 handler 现在与同类里的两个同级 handler 读起来完全一致,而且文件里组合路由的地方从两处变成一处。

明确说明一下"是否太好说话"这项检查,因为这正是最容易被盖章通过的情形——admin 作者、18 个 open PR、2 行语义改动、描述读起来也很合理。但本次批准不建立在任何这些之上。它建立在:从 main 的 CI 日志里逐字引出的、点名第 9200 行的失败断言;AST 遍历的推导——改动后数组收敛为唯一的期望项;以及完整追出来的调用链。我找过说不的理由,唯一的候选是"守卫测试本身这种东西是否该上线",但那是对已合并的 #10988 的问题,在这里重新翻案属于范围蔓延。

我唯一真正的保留是时机而非内容:抓取时 head 的 Test (ubuntu-latest, Node 22.x)Lint & Static 仍在运行,所以我手上没有 head 的绿灯。因此批准延后到 CI 在 65e4f9fb9cd9bc07075ddb5fcc3621d7001d57f8 上全绿之后——上面的标记承载了这个前置条件,全部检查完成后 finalize workflow 会发出绑定该 commit 的批准。若有检查变红或 head 移动,它会 withholding(不发批准)。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 65e4f9fb9cd9bc07075ddb5fcc3621d7001d57f8 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "agent 6c": running the guard test in packages/cli (vitest) — packages/core/dist is absent in this worktree so the suite's globalSetup guard would block the run, and a ….

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):"agent 6c"running the guard test in packages/cli (vitest) — packages/core/dist is absent in this worktree so the suite's globalSetup guard would block the run, and a …

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/acp-integration/acpAgent.ts Outdated
Comment thread packages/cli/src/acp-integration/acpAgent.ts Outdated
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.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running. Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

⚠️ 已从批准降级为评论:CI still running。 仅完成部分审查,审查缺口已披露。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

— qwen3.8-max via Qwen Code /review (v0.23.0)

qqqys
qqqys previously approved these changes Sep 5, 2026

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE (verified at head dba1ac7)

What this change actually is

At head, runWithPinnedRuntimeBaseDirForRequest resolves settings via loadSettingsCached(cwd) and delegates through the pass-through wrapper runWithPinnedRuntimeBaseDir to the same runWithAcpRuntimeOutputDir(settings, cwd, op) the handler called directly before — so this is a behavior-preserving consolidation, not a semantic fix: it removes the last hand-composed instance of the routing pattern that #10095 established the choke point to eliminate, which is precisely what makes it safe. I verified the equivalence against both definitions at head (acpAgent.ts:4591 and :4609) and confirmed the surrounding InvalidSessionTranscriptCursorError catch, the pre-read flush, and the reader construction are byte-identical inside the moved closure.

Historical items

Round-1's two findings were both Suggestions, and both are fixed in this exact head's code, not just marked resolved: the behavioral per-request routing test now exists (mirroring the six siblings, and it genuinely discriminates — the assertions on loadSettings(cwd) and the pin receiving the per-request settings object would fail on the boot-cache shape the AST guard permits), and the choke point's normative roster comment now lists all seven handlers including "transcript turn index". The round-2 pass at head posted zero findings.

CI at head

  • 23 checks green including Lint & Static and Integration Tests (no-AK); Test (ubuntu-latest) is still in progress (never gates per policy); the Integration Tests (CLI, No Sandbox) and macOS/Windows lanes were route-skipped, and one duplicate route meta-run was cancelled while the others succeeded — none attributable to this two-file change.

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@wenshao
wenshao enabled auto-merge September 5, 2026 01:11
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 65 passed · 0 failed · 65 total

Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:65 通过 · 0 失败 · 65 总计

抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence

Verification report

PR #11047 — deep verification

Verdict: merge-ready — 65 scripted assertions executed, 65 pass / 0 fail
(08-consolidated-assertions-65-pass.png).
Verified head: dba1ac7b45dc097ab5f91ea2797f7f88beba24e6 (git rev-parse HEAD^2).
Control base: 74fe3a659dde2859f152d6c860e04cfddca86d05 (HEAD^1, equals the snapshot's baseRefOid).
Three non-blocking findings (one suggestion, one nit, one observation) and one correction to
the PR description below; nothing blocking.

中文摘要

结论:merge-ready —— 65 条脚本化断言全部通过(65 pass / 0 fail),验证 head 为 dba1ac7b

A/B 结论(核心主张成立):守卫测试 routes every per-request runtime-root pin through runWithPinnedRuntimeBaseDir 在 base 上确实是红的,失败信息精确列出两处点名(4595 choke point 自身 + 9200 turn-index handler 的直接调用);在 head 上变绿。整文件跑:base 622 个测试恰好 1 红(就是这条守卫),head 622 全绿。所以本 PR 的生产代码改动对恰好一条测试是 load-bearing 的,见下表「中央 A/B」与图 01-ab-guard-base-vs-head.png02-mutation-matrix.png

行为不变(端到端实证):起了真实 ACP 子进程(tsx 直跑 TS 源码,零 mock),boot 在 boot-ws、请求 cwd=ws-a,两个 workspace 的 advanced.runtimeOutputDir 指向不同目录,同一 sessionId 在两个 runtime root 下放了带不同标记文本的真实 transcript。head 与 base 都读到 WSA-MARKER,page digest 完全相同(3afdac3bf910abda);正控制(把 handler 改成用 this.settings pin,即 #10095 bug)读到 BOOTROOT-MARKER,digest 不同(ec4b13bb34978c01)——证明探针不是瞎的,head==base 不是假阴性。分页(snapshot+start+limit)与三类结构化错误(非法 cursor / 非法 limit / start 无 snapshot)在两臂完全一致。见图 04-e2e-wire-runtime-root-ab.png

新增测试的定位(重要澄清,非缺陷):新测试在 base 上同样是绿的,因此它并不区分本 PR 的两臂——它不是这次改动的「回归捕手」,守卫测试才是。但它并非空转:更细的变异证明它精确钉住了它名字所声称的 bug 类(用 this.settings pin → 红)、以及「完全没有 pin」(→ 红)。两条测试是互补的:守卫钉「拼写」、对语义盲;新测试钉「语义」、对拼写盲。

覆盖普查(正向发现):把同样的 stale-cache pin 逐个注入 choke point 文档列举的全部 7 个 per-request handler,结果 7/7 全被杀。本 PR 之前,turn-index 是七者中唯一没有行为级 pin 的;这个 PR 正好补齐了这个不对称。见图 03-sibling-stale-cache-probe.png07-coverage-census-stale-pin.png

findings(均非阻塞):新测试 mock 的 readTurnIndexPage 返回的是 SessionTranscriptRecordPage 的字段集(records/hasMore/gaps),而不是真实的 SessionTranscriptTurnIndexPagev/snapshot/totalTurns/start/turns)——同文件 17056 行的姊妹测试用的是正确形状;doc comment 重排留下一行孤行。另有一处对 PR 描述的更正:作者称本地 pristine main 上另有两个无关 agent 测试失败,本容器中 base 臂整文件只有守卫这一条红,那两个失败未复现

未覆盖范围:逐 commit 归因(depth-2 浅克隆,元数据 2 个 commit 只有 dba1ac7b 可达,65e4f9fb 不是有效对象);base worktree 无法跑 vitest(globalSetup 前置构建守卫拦截),控制臂改用「已构建 head 树中单文件替换」;未跑仓库全量测试(只跑了 src/acp-integration/ 2038 与 acpAgent.test.ts 622);未驱动真实 acp-bridge HTTP 路由;未覆盖 live session 的 pre-read flush 真实路径;仅 Linux。

Central claim + A/B

Central claim. Per-request runtime-root pinning has one choke point, guarded by a
source-pinning test that enumerates every direct mention of runWithAcpRuntimeOutputDir
and expects exactly one (the choke point's own body). One handler still composed the
routing by hand — the transcript turn-index page reader — so the guard is red on main.
Routing that handler through runWithPinnedRuntimeBaseDirForRequest clears it, with no
behavioural change.

The claim has two halves and they were tested differently: the gate half by A/B on the
guard test, the no-behaviour-change half by a real-wire harness with a discriminating
oracle. Witness: 01-ab-guard-base-vs-head.png.

Cells — the gate flips

The base worktree could not run vitest at all (the repo's globalSetup prerequisite guard
requires 12 workspace dist/ outputs plus src/generated/git-commit.ts, none of which
exist in a fresh worktree). The control arm is therefore a single-file source swap
inside the already-built head tree: packages/cli/src/acp-integration/acpAgent.ts is
replaced by the base version and nothing else differs — same node_modules, same dist/,
same generated files, same test file. Cell identity is pinned by sha256[:16] of that file.

cell arm acpAgent.ts sha oracle observed expected assertion
C1 head 9adcd497bc94a442 guard test exit status GREEN (1 passed | 621 skipped) GREEN PASS
C2 base df8aedad82f10f6d guard test exit status RED (1 failed) RED PASS
C3 head 9adcd497bc94a442 new test exit status GREEN GREEN PASS
C4 base df8aedad82f10f6d new test exit status GREEN (measured) see below

C2's failure message is the intended expected-vs-actual assertion, not an import or compile
break, and it names both mentions:

AssertionError: acpAgent.ts must not name runWithAcpRuntimeOutputDir directly. …
Direct mentions at:
4595: return runWithAcpRuntimeOutputDir(settings, cwd, operation);
9200: return await runWithAcpRuntimeOutputDir(settings, cwd, async () => {: expected [ …(2) ] to deeply equal [ StringMatching{…} ]

Whole-file counts (02-mutation-matrix.png): base = 622 tests / 1 failure (only the
guard) and head = 622 / 0. So the production hunk is load-bearing for exactly one test,
and it breaks nothing else in the file.

C4 is the interesting cell. The new test passes on base too, so it does not
discriminate this PR's two arms. That is not vacuity — base was already correct for this
handler (it called loadSettingsCached(cwd) with the request cwd). Escalating to finer
mutations, as required before calling a survivor vacuous, shows the test pins real
semantics; see the next section.

Mutation matrix — what each test actually pins

Each mutant changes one thing in acpAgent.ts and runs the full 622-test file, so
coverage living anywhere in that file is collected (the control is landed in the same file
as the mutants). C0 is the unmutated control and is green, which is what makes the kills
meaningful. Witness: 02-mutation-matrix.png.

arm mutation tests failed guard test new test reading
C0 none (head) 622 0 green green control green
M1 revert to base (hand-composed routing) 622 1 RED green the PR's hunk is load-bearing for the guard only
M2 pin with this.settings via the non-request choke form (the #10095 bug class) 622 1 green RED new test kills the bug its name claims
M3 drop the pin from the handler entirely 622 1 green RED new test kills a missing pin
M4 invert the handler's sessionId validation (positive control) 622 8 green RED harness can turn this file red decisively

Two conclusions the two-cell A/B alone could not reach:

  1. The pair is complementary, each blind to exactly what the other catches. The guard
    is an AST walk for one identifier, so M2 and M3 — which never name
    runWithAcpRuntimeOutputDir — leave it green. The new test mocks the runner, so it
    cannot see the spelling M1 changes. Neither is redundant; the guard enforces "routing
    is composed in one place", the behavioural test enforces "composed with the right
    settings".
  2. M1 is a clean single-test kill. Reverting the PR turns exactly one test red in 622,
    which bounds the blast radius of the change to the gate it was written to clear.

Real-wire A/B — behaviour preservation, with a positive control

Reviewer Test Plan steps 2 and 3 were executed against a real ACP child process driven
over the real stdio ndjson JSON-RPC wire. No mocks anywhere: real loadSettings reading
real .qwen/settings.json, real Storage.runWithRuntimeBaseDir, real
SessionTranscriptReader, real transcript bytes taken from the repo's own contract fixture
(integration-tests/fixtures/chat-transcript-contract/v1/cases/representative/chat-records.jsonl,
with only the session id, cwd and marker text rewritten). The child runs from TypeScript
source under tsx, so a source swap changes the arm with no rebuild and both arms share one
byte-identical packages/core. Witness: 04-e2e-wire-runtime-root-ab.png.

The oracle is a runtime-root discriminator. The child boots in boot-ws
(advanced.runtimeOutputDirruntime-boot) and then receives a turn-index request naming
a different cwd, ws-a (→ runtime-a). A transcript for the same session id exists
under both roots with different marker text, so the returned turn label says unambiguously
which workspace's settings pinned the read.

arm acpAgent.ts sha which runtime root was read page digest turn label
head 9adcd497bc94a442 runtime-a (request cwd's settings) 3afdac3bf910abda 0:prompt:WSA-MARKER Inspect the contract
base df8aedad82f10f6d runtime-a (request cwd's settings) 3afdac3bf910abda 0:prompt:WSA-MARKER Inspect the contract
stale (M2, positive control) aecf380c853ea4fa runtime-boot (boot settings — the #10095 bug) ec4b13bb34978c01 0:prompt:BOOTROOT-MARKER Inspect the contract
  • head and base page digests are identical, and so are the paged digests and turn labels
    → behaviour preserved, as the description claims.
  • the stale arm reads a different root and produces a different digest → the probe
    discriminates, so "head == base" is a measurement and not a blind harness. Without this
    arm the equality would be worthless.
  • The scenario is production-shaped: through acp-bridge the request cwd is always the
    connection's boundWorkspace, but this.settings is a process-wide "latest loaded"
    cache, so in a multi-workspace daemon it can hold another workspace's
    advanced.runtimeOutputDir while a request names a different one. Booting in boot-ws
    and requesting ws-a reproduces exactly that split.

Test Plan step 3 (structured errors survive the routed call), identical on all three arms:

probe head base
start without snapshot -32602 "Invalid params: Invalid transcript turn index start" identical
limit: 0 -32602 "Invalid params: Invalid transcript limit" identical
bogus snapshot cursor -32602 errorKind=invalid_transcript_cursor identical

Paging with snapshot + start: 0 + limit: 1 returned turns=1 totalTurns=1 start=0
with an identical digest on both arms.

Coverage census — the invariant is now pinned for all seven handlers

The choke point's doc comment enumerates seven per-request handlers that must use the
ForRequest form. A call-site census (grep plus reading each enclosing method) confirmed the
code matches that list exactly: 7 ForRequest
call sites (5837 list, 9092 transcript page, 9200 turn index, 12155 settled turn status,
12376 delete, 12426 rename, 12657 loadUpdates non-live), and all 6 runWithPinnedRuntimeBaseDir
call sites are accounted for by the comment's other two sentences — 4614 is the ForRequest
form's own delegation, 3903 createWorkspaceMcpDiscoveryConfig / 4657 assertLiveSessionScope
/ 13673 newSessionConfig are the "callers that already hold deliberately scoped settings",
and 5270 loadSessionWithProfiler / 5664 resumeSessionWithProfiler are the "session load
and resume … under profiler instrumentation" pair their method names already advertise. Then
the M2 bug was injected into each of the other six per-request handlers, one at a time.
Witnesses: 03-sibling-stale-cache-probe.png (list/delete, whole src/acp-integration/
directory, 2038 tests) and 07-coverage-census-stale-pin.png (the other four, 622-test file).

handler mutant verdict killed by
unstable_listSessions M5 killed (1/2038) resolves unstable_listSessions settings per request… (pre-existing)
deleteSession M6 killed (1/2038) resolves deleteSession settings per request… (pre-existing)
renameSession M7 killed (1/622) resolves dead-session renameSession settings per request… (pre-existing)
transcript page M8 killed (19/622) resolves qwen/status/session/transcript settings per request… + 18 more (pre-existing)
transcript turn index M2 killed (1/622) resolves sessionTurnIndex settings per request… (added by this PR)
settled turn status M9 killed (1/622) resolves sessionTurnStatus settings per request… (pre-existing)
loadUpdates non-live M10 killed (1/622) resolves non-live qwen/session/loadUpdates settings per request… (pre-existing)

Every mutant broke exactly the one test named for its handler (M8 aside, which also breaks
18 neighbouring transcript tests) — no collateral. Before this PR, turn-index was the only
one of the seven with no behavioural pin against the stale-cache bug class.
The new test
follows the file's established naming and shape and closes that asymmetry; that, rather than
catching this PR's own diff, is what it is worth.

Corrections

The description's local-failure caveat does not reproduce here. It states that "two
unrelated agent tests fail locally on a pristine main checkout in this environment (an
env-parsing case and a status-snapshot case) and pass in CI". In this container the base arm
ran the complete acpAgent.test.ts at baseRefOid: 622 tests, exactly 1 failure — the
guard test.
No env-parsing or status-snapshot case failed. Nothing beyond the gate the PR
clears is red on the base, so the caveat can be dropped rather than carried by reviewers.

This is a correction to the description, not a request to change the code.

Findings

Non-blocking. None of them affects a single assertion above, and all three were measured
rather than inferred.

1. Suggestion — the new test's mocked return value is the wrong page shape

acpAgent.test.ts:15335 mocks readTurnIndexPage to resolve:

{ sessionId, records: [], hasMore: false, gaps: [], startTime: 'start', lastUpdated: 'end' }

That is the field set of SessionTranscriptRecordPage
(packages/core/src/services/session-transcript-reader.ts:222), which is what readPage
returns. readTurnIndexPage returns SessionTranscriptTurnIndexPage (line 197):
{ v, sessionId, snapshot, totalTurns, start, turns[], startTime?, lastUpdated? }. The
real wire response confirms it — the E2E harness above received totalTurns: 1,
start: 0, turns: [ … ] and no records/hasMore/gaps key at all.

The neighbouring turn-index test at acpAgent.test.ts:17056 mocks the correct shape, so
this is not a local convention being followed — the same file has the right example 1,700
lines below. The as unknown as InstanceType<typeof SessionTranscriptReader> cast is what
hides it from tsc.

Impact is limited and worth stating precisely: the test's assertions only inspect which
settings and cwd reached the pin, so the fictional payload does not weaken them — M2 and M3
prove the test still kills real regressions. But the fixture cannot catch a response-shape
regression, and it teaches the next reader a shape that does not exist. Aligning it with the
line-17056 example is a like-for-like six-field swap, not a redesign.

2. Nit — the doc-comment reflow leaves an orphan line

The comment edit at acpAgent.ts:4585-4587 now reads:

   * decision themselves — they use `runWithPinnedRuntimeBaseDirForRequest`
   * below. Session load and
   * resume resolve the request's settings at the call site deliberately,

* below. Session load and is a short orphan that splits the "Session load and resume"
pair across the wrap. Prettier does not reflow comments, so prettier --check stays green
(verified, 06-gates-prettier-tsc.png) and no gate objects — purely cosmetic. The
substance of the edit is correct: the enumeration now names seven handlers and the code has
exactly seven ForRequest call sites.

3. Observation, not a defect — the guard test is spelling-only

Recorded so the invariant is not over-trusted: M2 (pin with this.settings) and M3 (drop
the pin) both leave the guard test green, because it is an AST walk for one identifier
and neither mutant names it. What catches those shapes is the per-handler behavioural test.
That is by design — the guard's own comment says the per-request handlers are "pinned
behaviorally … by the routing tests above" — and after this PR all seven exist (census
above). No change requested; the PR improves this position rather than weakening it.

Targeted gates

All run against the two changed files / the affected workspace, each with a live probe first
— a gate that exits 0 because it matched nothing looks identical to one that passed.
Witnesses: 05-eslint-live-probe-corrected.png, 06-gates-prettier-tsc.png.

gate live probe (must fail) real run
prettier --check planted const x=1;; in src/acp-integration/ → exit 1, [warn] Code style issues found exit 0, All matched files use Prettier code style!
eslint planted export const x: any = 1 → exit 1, @typescript-eslint/no-explicit-any; planted camelCase filename → exit 1, check-file/filename-naming-convention exit 0 on both changed files, no output
tsc --noEmit (packages/cli) n/a — the eslint probe file was removed before this ran exit 0, no diagnostics
vitest run src/acp-integration/acpAgent.test.ts M4 positive control turns 8 tests red in this file 622 passed / 622 at head
vitest run src/acp-integration/ M5/M6 each turn 1 test red 2038 passed / 2038 across 39 files at head (separate clean, unmutated run — the mutants only reported this collection size while mutated)

The first eslint live probe was mis-designed — it planted an exported unused const, which
this config legitimately does not flag, so it exited 0 and proved nothing. That was a harness
defect, not a PR problem; the probe was rebuilt with violations the repo's own rules forbid
and re-run, and the superseded row is excluded from the assertion count rather than counted
as a failure against the diff.

No repo-wide gate is claimed: only src/acp-integration/ was run.

Not covered

  • Per-commit attribution. The checkout is depth 2. The snapshot lists two commits
    (65e4f9fb9cd9, dba1ac7b45dc); git rev-list HEAD^1..HEAD^2 reaches only dba1ac7b,
    and git cat-file -t 65e4f9fb9c returns fatal: Not a valid object name. Note that
    git rev-list --count HEAD^1..HEAD^2 returns 1 without erroring, so the gap is invisible
    unless the count is compared against the snapshot. Only the aggregate HEAD^1..HEAD diff
    was verified; the split between the fix commit and the test commit is out of reach.
  • Base worktree test execution. tmp/base-tree at HEAD^1 could not run vitest: the
    repo's globalSetup guard reported 12 unbuilt workspace dist/ outputs
    (@qwen-code/acp-bridge, @qwen-code/web-templates, 10 channel packages) plus a missing
    generated packages/cli/src/generated/git-commit.ts. Building all of them did not fit the
    budget, so every control arm is a single-file source swap in the built head tree instead.
    The worktree was removed after the A/B cells were captured.
  • Internal workspace links. From inside the base worktree,
    readlink -f node_modules/@qwen-code/qwen-code-core resolves to
    /__w/qwen-code/qwen-code/packages/core — i.e. the head tree, since the base tree has no
    node_modules and Node walks up to the root. This is inert here and was checked rather
    than assumed: git diff --name-only HEAD^1..HEAD touches 0 files under packages/core
    and no package.json/package-lock.json, so the shared dependency and the dependency tree
    are byte-identical on both arms. acpAgent.ts itself always resolved from the arm's own
    tree, which the per-cell sha256 column witnesses.
  • The acp-bridge HTTP route. The harness spoke raw JSON-RPC to the ACP child. The
    production path (requestSessionTurnIndexPage, packages/acp-bridge/src/bridge.ts:7382)
    additionally wraps the call in withEnsuredWorkspaceControl and a timeout and forces
    cwd: boundWorkspace; that wrapping was not exercised.
  • Live-session pre-read flush. With no live session, the handler's
    this.sessions.get(sessionId)?.getConfig().getChatRecordingService()?.flush() is an
    optional-chained no-op, so the E2E ran the non-live branch only. Flush behaviour stays
    covered by the pre-existing mocked tests (flushes latest but not frozen turn-index pages,
    still scans the transcript when the pre-read flush fails), not by the wire harness.
  • Repo-wide test suite, bundle, integration tests. Not run. Only acpAgent.test.ts
    (622) and src/acp-integration/ (2038).
  • Other platforms. Linux container only. The description's own table marks Windows and
    Linux ⚠️; nothing here speaks to Windows path or ino-reporting behaviour in the settings
    cache.
  • Trial merge into current main. Not performed: HEAD^1 is origin/main's tip
    (74fe3a65) in this checkout, so the base is not stale and there was nothing to re-measure.
  • First round. No previous-report.md in the snapshot directory, so there are no
    carried-forward findings to re-measure.

Methodology

Everything ran in the CI verify container at the merge-ref checkout, against the
already-installed root node_modules and the already-built dist/; no npm ci or full
npm run build was needed. Because the base worktree could not run vitest, every control arm
was produced by swapping the single changed production file inside the built head tree and
restoring it afterwards — each harness records sha256[:16] of the active file per cell and
re-verifies the restore against the head hash, and git status --porcelain is empty at the
end (asserted). Three harness families drove the code: (1) vitest runs of the affected test
file, filtered for the A/B cells and complete (622 tests) for every mutant, with failures
read out of junit.xml rather than scraped from console output; (2) a mutation set of ten
mutants of acpAgent.ts — one whole-file revert plus nine single-point mutations — each built
by a uniqueness-checked string replacement
that throws if its anchor matches more than once, so a mutant that silently landed in the
wrong handler cannot happen; (3) a wire harness that spawns the real CLI in --acp mode with
tsx, an isolated HOME, and no credentials (the turn-index path makes no model call and the
ACP bootstrap runs config.initialize({ skipLlmInitialization: true })), performs the real
initialize handshake, and sends qwen/status/session/turn_index as a JSON-RPC request over
stdin. Harnesses, per-arm raw JSON, per-mutant vitest logs and junit.xml copies live in
tmp/pr11047-verify-20260905-013249/ (*.mjs, logs/); assertions.json is written by
score.mjs, which re-derives every assertion from those recorded outputs so each number in
this report maps to a comparison that executed.

Flakiness gate log

rounds=5 files=1 skipped=0
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/acp-integration/acpAgent.test.ts: PPPPP

verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)

Evidence images

01-ab-guard-base-vs-head

02-mutation-matrix

03-sibling-stale-cache-probe

04-e2e-wire-runtime-root-ab

05-eslint-live-probe-corrected

06-gates-prettier-tsc

07-coverage-census-stale-pin

08-consolidated-assertions-65-pass

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge resolution — PR #11047

Root cause

main's #11036 (eaef97e634 "route turn-index reads through runtime-root pin") collided with this PR. Both independently made the same fix in acpAgent.ts: the sessionTurnIndex handler read settings via loadSettingsCached(cwd) + a hand-composed runWithAcpRuntimeOutputDir(settings, cwd, ...) call; both sides replaced that with the pin choke point this.runWithPinnedRuntimeBaseDirForRequest(cwd, ...).

Textual or semantic

Semantic — both sides rewrote the same try block with identical behavior but different shapes: this PR inlined the callback; main extracted it as a named function. The PR's other pieces (doc-comment handler list, new test) auto-merged cleanly.

Resolution keeps main's shape — it matches the sibling handlers (readTranscriptPage, readSettledTurnResult) and is already CI-green on main:

const readTurnIndexPage = async () => {
  if (rawSnapshot === undefined) { /* flush live session recording */ }
  return (await new SessionTranscriptReader(cwd).readTurnIndexPage(
    sessionId, { /* snapshot/start/limit */ },
  )) as unknown as Record<string, unknown>;
};
return await this.runWithPinnedRuntimeBaseDirForRequest(cwd, readTurnIndexPage);

What is load-bearing

  • The handler must route through runWithPinnedRuntimeBaseDirForRequest(cwd, ...), never resolve settings itself — the choke point pins the transcript read to the request's cwd settings instead of the stale process-wide this.settings (fix(cli): resolve session-management settings per request, not from the stale this.settings cache #10095 bug class). A direct loadSettingsCached/runWithAcpRuntimeOutputDir composition here breaks it.
  • The PR's new test stays valid against main's shape: the test mocks settings-cache.js so loadSettingsCached(cwd) delegates to mocked loadSettings(cwd), and asserts the choke point was traversed — shape-independent.

What I could not verify

No build/tests were run. Post-merge, acpAgent.ts is byte-identical to origin/main except the PR's doc-comment hunk, so production behavior is exactly what main's CI validated; the only code not yet executed anywhere is the PR's new test, now exercising main's shape — consistent by reading the mocks, not by running vitest.

中文说明

冲突根因

main 上的 #11036eaef97e634)与本 PR 相撞:两边各自独立地在 acpAgent.tssessionTurnIndex 处理器里做了同一个修复——把 loadSettingsCached(cwd) + 手工组合的 runWithAcpRuntimeOutputDir(...) 替换为统一的 pin 收口函数 runWithPinnedRuntimeBaseDirForRequest(cwd, ...)

文本冲突还是语义冲突

语义冲突——两边改写了同一段 try 逻辑,行为一致,仅回调形态不同(本 PR 内联回调,main 提取为具名函数)。PR 的其余改动(文档注释、新增测试)均自动合并成功。

解决时保留 main 的形态:它与相邻处理器(readTranscriptPagereadSettledTurnResult)写法一致,且已在 main 上通过 CI。

关键点

  • 该处理器必须经由 runWithPinnedRuntimeBaseDirForRequest(cwd, ...) 路由,不能自行解析 settings——正是这个收口保证按请求方 cwd 的 settings 钉住运行时根目录,而不是用进程级过期缓存(fix(cli): resolve session-management settings per request, not from the stale this.settings cache #10095 一类 bug)。若在此恢复直接组合 loadSettingsCached/runWithAcpRuntimeOutputDir,即破坏该不变量。
  • PR 新增测试对 main 形态依然成立:测试把 settings-cache.js mock 成 loadSettingsCached(cwd) 委托给被 mock 的 loadSettings(cwd),只断言收口被走过,与回调形态无关。

无法验证的部分

按指令未运行构建/测试。合并后 acpAgent.ts 与 origin/main 逐字节一致(除 PR 的文档注释改动),生产行为即 main CI 已验证的行为;唯一未执行过的是 PR 新增测试——已通过阅读 mock 确认其与 main 形态一致,但未实际跑 vitest。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI failing: web-shell E2E Smoke (ubuntu-latest, Node 22.x), Test (ubuntu-latest, Node 22.x). Reviewed.

中文说明

⚠️ 已从批准降级为评论:CI failing: web-shell E2E Smoke (ubuntu-latest, Node 22.x), Test (ubuntu-latest, Node 22.x)。 已审查。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI failing: web-shell E2E Smoke (ubuntu-latest, Node 22.x), Test (ubuntu-latest, Node 22.x). ⚠️ This run could not certify that any of this diff was reviewed.

Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read.

Not reviewed: verification — could not check that Step 4 and Step 5 ran (no subagent transcripts at /home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.).

中文说明

⚠️ 已从批准降级为评论:CI failing: web-shell E2E Smoke (ubuntu-latest, Node 22.x), Test (ubuntu-latest, Node 22.x)。 ⚠️ 本次运行无法证明这个 diff 的任何部分经过了审查。

未审查:覆盖情况——无法读取 agent 的运行记录(no subagent transcripts at /home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.),本次运行无法证明 diff 的任何部分被读过。

未审查:验证——无法检查步骤 4 与步骤 5 是否运行(no subagent transcripts at /home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-hk1-3/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-hk1-3--work-qwen-code-qwen-code/subagents/e4776ba9-11a6-40ca-b1e3-ef788d29ee8a'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running. Partially reviewed — gaps disclosed.

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 3 by the review time budget.

Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/acp-integration/acpAgent.test.ts:15335 — [probe] turn-index routing test stubs the record-page shape (records/hasMore/gaps) and asserts only toBeDefined(), so the stub is not load-bearing
  • packages/cli/src/acp-integration/acpAgent.test.ts:15343 — [probe] test witnesses the pin's arguments but never its effect — hoisting the reader construction out of the pinned scope keeps all 622 tests green
  • packages/cli/src/acp-integration/acpAgent.test.ts:15352 — [probe] comment claims a live session in the boot workspace; the fixture creates none, so the pre-read flush branch is uncovered
  • packages/cli/src/acp-integration/acpAgent.test.ts:15334 — [probe] AST guard permits this.runWithPinnedRuntimeBaseDir(this.settings, cwd, ...), so the invariant rests on one bespoke test per handler
  • packages/cli/src/acp-integration/acpAgent.ts:4583 — [probe] PR title and 'Why it's needed' describe a routing change and a red gate this residual diff does not contain — main's #11036 landed both
中文说明

⚠️ 已从批准降级为评论:CI still running。 仅完成部分审查,审查缺口已披露。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未审查:反向审计——评审时间预算不足,未能开始第 3 轮。

收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 70 passed · 0 failed · 70 total

Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:70 通过 · 0 失败 · 70 总计

抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence

Verification report

<!-- qwen-triage:verify -->
<!-- qwen-triage:verify-substantive -->

Sandboxed verification: ⚠️ findings — non-blocking (agent verdict) - follow-up round 2

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free mutation harnesses, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 70 passed · 0 failed · 70 total

Verdict: findings. 70 scripted assertions executed, 70 pass / 0 fail
(assertions.json; witnesses 01-ab-guard-already-green-on-base.png,
02-mutation-matrix-0-of-621-to-1-of-622.png, 03-sibling-census-7-of-7-pinned.png,
04-gates-live-probes-8-of-8.png).
Verified head: 2b80865d520ea81b6d69dc9ba35189e3a902f72a (git rev-parse HEAD^2).
Control base: e3d26283e63c19b17e00862addae86c2ecdd86a4 (HEAD^1, equals the snapshot's baseRefOid).

The code in this PR is verified good and safe to merge — but the PR no longer does what its
title and description say.
The production fix it describes already landed in main, so at the
verified base the guard test it claims to clear is already green and the diff contains zero
executable production changes
. What remains is one new behavioural test (verified load-bearing:
0/621 → 1/622) and a JSDoc roster fix. The one finding worth a reviewer's attention is that
mismatch between the PR's stated purpose and its actual diff; the two carried non-blocking
findings from round 1 both still stand.

中文摘要

结论:findings(非阻塞) —— 70 条脚本化断言全部通过(70 pass / 0 fail),验证 head 为 2b80865d

核心变化(与上一轮不同):本 PR 描述的生产改动已经合入 main。在验证 base(e3d26283)上,PR 声称要清除的守卫测试已经是绿的(真实执行 1.28s,非被过滤;且 live probe 证明该门是活的——植入一处直接点名即变红)。acpAgent.ts 在 base 与 head 之间去掉块注释后逐字节相同git diff 的 7 行改动全部是 JSDoc * 行。因此本 PR 现在的实际贡献只有:一条新行为测试 + 一处 JSDoc 名单修正。

A/B 结论(见「中央 A/B」表与图 01-ab-guard-already-green-on-base.png:门不翻转——base 绿、head 绿;base 全文件 621/0,head 622/0(恰好 +1 测试)。目录级普查(src/acp-integration/ 39 文件 2041 测试)在 base 测试下对 stale-cache 变异 0 失败,证明该 bug 类在 base 上无任何 pin。

新测试的价值(见「变异矩阵」表与图 02-mutation-matrix-0-of-621-to-1-of-622.png:把 #10095 stale-cache bug 注入 turn-index handler,旧(base)测试文件 0/621 全绿,新(head)测试文件 1/622 红,且红的恰好是本 PR 新增的那条测试;「完全去掉 pin」的变异同样 0/621 → 1/622。即 0/621 → 1/622:新测试是整个包里唯一能看见该 bug 的东西。七个 per-request handler 的兄弟普查(图 03-sibling-census-7-of-7-pinned.png)在 head 上 7/7 全被 pin,base 上只有 turn-index 缺失。

findings:① PR 的标题/commit 类型(fix(cli))与描述(「守卫今天在 main 上是红的」)与当前 diff 不符——修复已在 main,建议改题为 test(cli) 并重写动机;②(沿用)新测试 mock 的是 SessionTranscriptRecordPage 字段而非真实的 SessionTranscriptTurnIndexPage;③(沿用)JSDoc 重排留下一行孤行;④(沿用,观察)守卫只做拼写检查;⑤(新,观察,既有)invalid_transcript_limitInvalid transcript snapshot 两类错误在全目录测试中无任何断言。以上均不阻塞合并。

未覆盖范围:逐 commit 归因(HEAD^2 是零父浅 graft);base worktree 无法跑 vitest(globalSetup 前置构建守卫);未重跑上一轮的真实 ACP 子进程 wire harness(生产代码逐字节相同,wire A/B 无意义);未跑仓库全量测试/集成测试;仅 Linux;抖动门由 workflow 自带门覆盖。

Previous-round findings, re-measured at the new head

# finding (round 1) severity status at 2b80865d re-measurement
1 new test mocks SessionTranscriptRecordPage fields (records/hasMore/gaps) instead of SessionTranscriptTurnIndexPage (v/snapshot/totalTurns/start/turns) Suggestion stands score.mjs F1–F3: the new test at acpAgent.test.ts:15335 still mocks the wrong shape; the real interface in packages/core/src/services/session-transcript-reader.ts:197 has no records field; the same file's flushes latest but not frozen turn-index pages (line 17053) mocks the correct one
2 doc-comment reflow leaves an orphan line * below. Session load and Nit stands score.mjs F4: the line is present verbatim at head
3 the guard test is spelling-only (semantic mutants leave it green) Observation stands score.mjs G12: M2 and M3 (which never name the runner) leave the guard green on the head test file
C correction: "two unrelated agent tests fail locally on a pristine main checkout" Correction stands re-measured at the new base e3d26283: complete base-arm run = 621 tests, 0 failures (F1-full-base). The caveat still does not reproduce in this container
+ coverage census: 7/7 per-request handlers pinned, turn-index was the only gap positive finding re-measured, holds 6 sibling stale-cache mutants each killed by exactly its own named pre-existing routing test; M2/M3 killed by the new test → 7/7 at head, 6/7 at base (03-sibling-census-7-of-7-pinned.png)
+ real-wire ACP-child E2E proving behaviour preservation head==base positive finding superseded by a stronger proof this round proves acpAgent.ts base==head after stripping block comments (score.mjs C1) and that all 7 changed lines are JSDoc * lines (C2). A wire A/B would compare byte-identical behaviour by construction, so it was not re-run
+ round-1 central A/B: guard RED at base → GREEN at head central claim no longer reproduces — see below at the new base the guard is GREEN (C1). Not a regression: main absorbed the production fix between rounds

Central claim + A/B

Central claim as written by the PR: per-request runtime-root pinning has one choke point guarded
by a source-pinning test; the turn-index handler still composed the routing by hand, so the guard
test is red on main today
; routing it through the choke point clears it with no behaviour change.

What is true at the verified base: the routing is already in main. git show HEAD^1:packages/cli/src/acp-integration/acpAgent.ts lines 9218–9221 are the
this.runWithPinnedRuntimeBaseDirForRequest(cwd, readTurnIndexPage) call — the exact form the PR
says it introduces. Consequently the diff's only production change is a JSDoc roster edit, and the
gate the PR exists to clear does not flip. Witness: 01-ab-guard-already-green-on-base.png.

The claim therefore splits into a stale half (the fix, already in main) and a live half (the
test and the roster fix). The live half is what this round verifies, with the test-only-PR method:
mutants of the production file the PR does not change executably, run against the old and the new
test file.

Cells — the gate does not flip, and the gate is provably live

cell arm acpAgent.ts sha test sha oracle observed expected assertion
C1 base (HEAD^1) e55d68cd3b662f3e e2a78585e672bf00 guard test exit + junit status GREEN, testcase PASSED in 1.278 s (executed, not filtered; 620/621 skipped) PR predicts RED PASS — the premise is gone
C2 head e8c0ae637c47c7d2 07ae87856893796f guard test exit + junit status GREEN, PASSED 1.175 s GREEN PASS
C3 head e8c0ae637c47c7d2 07ae87856893796f the test this PR adds GREEN, PASSED 0.081 s GREEN PASS
P1 base + planted direct runWithAcpRuntimeOutputDir mention 30a6db0383ed15e0 e2a78585e672bf00 guard test RED, 1 failure = the guard RED PASS — C1's green means "base is genuinely clean", not "the gate is dead"
F1 base, complete file e55d68cd3b662f3e e2a78585e672bf00 whole-file counts 621 tests, 0 failures 0 PASS
F2 head, complete file e8c0ae637c47c7d2 07ae87856893796f whole-file counts 622 tests, 0 failures 0 PASS
H1 base tests + M2 mutant, whole src/acp-integration/ 502d58feb10cc23c e2a78585e672bf00 directory-wide counts 39 files, 2041 tests, 0 failures 0 PASS — nothing anywhere in the package pins the bug at base

P1 is the control that makes C1 meaningful: without it, "green at base" could have meant a dead gate.
With it, C1 is a measurement of a clean base.

Mutation matrix — what the PR's remaining diff is worth

Each mutant is a uniqueness-checked single-point edit of acpAgent.ts; every run collects the whole
622-test file, so coverage living anywhere in it is counted. Witness:
02-mutation-matrix-0-of-621-to-1-of-622.png.

mutant edit old (base) test file new (head) test file reading
M2 turn-index pins with this.settings (the #10095 bug class) SURVIVED 0/621 KILLED 1/622 — exactly resolves sessionTurnIndex settings per request… the new test is the only pin for this handler
M3 turn-index drops the runtime-root pin entirely SURVIVED 0/621 KILLED 1/622 — same single test the new test also catches a missing pin
Mdoc revert the PR's only other hunk (the JSDoc roster) SURVIVED 0/621 SURVIVED 0/622 expected: no test can observe a comment; classified unobservable, not a coverage gap
M4 SUPERSEDED control: append a suffix to an error message SURVIVED 0/621 SURVIVED 0/622 harness defect, excluded from the verdict (see Methodology)
M4b rebuilt control: alter the asserted substring itself KILLED 1/621 KILLED 1/622rejects a turn-index start without its snapshot on both arms the full-file command collects pre-existing coverage of the mutated file on both arms

Two conclusions the two-cell A/B could not reach:

  1. 0/621 → 1/622 with exact attribution. On the old test file the fix(cli): resolve session-management settings per request, not from the stale this.settings cache #10095 bug in this handler is
    invisible to all 621 tests, and directory-wide to all 2041; on the new one exactly the added test
    goes red. No mutant regressed from killed to survived (G11).
  2. The roster hunk is the only other thing in the diff, and it fixes a real mismatch. At base the
    JSDoc names six per-request handlers while the code has seven ForRequest call sites; at head the
    roster names seven and matches. Mdoc surviving both arms is the expected unobservable outcome,
    not a vacuity signal.

Sibling census — the invariant is now pinned for all seven handlers

Witness: 03-sibling-census-7-of-7-pinned.png. The same stale-cache mutant injected into each
per-request handler, run against the head tests (filtered to the seven routing tests, so the claim
measured is attribution; whole-file collateral for these mutants was measured in round 1).

handler verdict killed by
unstable_listSessions KILLED 1/622 resolves unstable_listSessions settings per request… (pre-existing)
deleteSession KILLED 1/622 resolves deleteSession settings per request… (pre-existing)
renameSession KILLED 1/622 resolves dead-session renameSession settings per request… (pre-existing)
transcript page KILLED 1/622 resolves qwen/status/session/transcript settings per request… (pre-existing)
transcript turn index KILLED 1/622 resolves sessionTurnIndex settings per request… (added by this PR)
settled turn status KILLED 1/622 resolves sessionTurnStatus settings per request… (pre-existing)
loadUpdates non-live KILLED 1/622 resolves non-live qwen/session/loadUpdates settings per request… (pre-existing)

Every sibling broke exactly its own named test — no collateral in the filtered collection. At base the
count was 6/7; turn-index was the only unpinned handler, and this PR's test closes that asymmetry.
That, rather than clearing a gate, is the PR's actual contribution.

Corrections

  1. "The guard test is red on main today" is false at the verified base. The description, the
    commit message ("The refactor(cli): route every per-request runtime-root pin through runWithPinnedRuntimeBaseDir #10988 guard fails the build on any direct mention…"), and the Evidence
    section ("Before: the choke-point test fails on main with the turn-index handler's direct call
    listed as a second mention") all describe a state that no longer exists at e3d26283: base has
    exactly three mentions of runWithAcpRuntimeOutputDir (import, JSDoc, choke-point body) and the
    guard passes. Evidence: C1 (green, executed) + P1 (gate live). This is a correction to the
    description, not a request to change the code.
  2. The description's local-failure caveat still does not reproduce. "Two unrelated agent tests
    fail locally on a pristine main checkout" — the complete base-arm run measured 621 tests, 0
    failures. Carried from round 1 and re-measured at the new base.

Findings

Non-blocking unless stated. None affects any assertion above.

1. Suggestion — the PR's title, commit type and stated purpose no longer match its diff

The title is fix(cli): route the transcript turn-index handler through the pin choke point, and the
body's "Why it's needed" rests on a red guard. At the verified base the routing already exists
(git show HEAD^1:packages/cli/src/acp-integration/acpAgent.ts | sed -n '9215,9222p' shows the
runWithPinnedRuntimeBaseDirForRequest call), the guard is green (C1), and the diff's production
change is comment-only (score.mjs C1/C2: base==head after stripping block comments; 7/7 changed
lines are JSDoc * lines).

Consequences a reviewer should weigh: a fix(cli): entry lands in the changelog for a change that
fixes nothing at this base; Reviewer Test Plan step 1 ("run the agent test file and confirm the
choke-point case passes") is green on main without this PR, so it cannot distinguish the arms; and
a future reader of the commit will believe a CI gate was red when it was not.

The code itself is not the problem: the added test is verified load-bearing (0/621 → 1/622, exact
attribution) and closes the last unpinned handler of the seven. Merging is safe; what needs fixing is
the framing.

Minimal suggested edit (measured against the same harnesses)

Retitle to test(cli): pin the turn-index handler's per-request routing and roster, and replace the
"Why it's needed" paragraph with the coverage-gap rationale that is true at any base: of the seven
per-request handlers the choke point's JSDoc enumerates, turn-index was the only one with no
behavioural test against the #10095 stale-cache class (6 routing tests vs 7 ForRequest call sites
at base, score.mjs E1–E5); the roster sentence is updated to name it. The mutation matrix above is
the evidence for that sentence, and it holds at base and at head alike — unlike the red-guard
sentence, which holds at neither.

2. Suggestion (carried from round 1, stands) — the new test's mocked return value is the wrong page shape

acpAgent.test.ts:15335 mocks readTurnIndexPage to resolve
{ sessionId, records: [], hasMore: false, gaps: [], startTime, lastUpdated } — the field set of
SessionTranscriptRecordPage. readTurnIndexPage returns SessionTranscriptTurnIndexPage
(packages/core/src/services/session-transcript-reader.ts:197):
{ v, sessionId, snapshot, totalTurns, start, turns[], startTime?, lastUpdated? }, with no
records/hasMore/gaps keys. The as unknown as InstanceType<typeof SessionTranscriptReader>
cast hides it from tsc. The same file mocks the correct shape 1,700 lines below
(flushes latest but not frozen turn-index pages, line 17053), so this is not a local convention.
Impact is bounded — M2/M3 prove the test still kills real regressions — but the fixture cannot catch
a response-shape regression and teaches a shape that does not exist. Reproducing check:
sed -n '15335,15345p' packages/cli/src/acp-integration/acpAgent.test.ts against
sed -n '/export interface SessionTranscriptTurnIndexPage/,/^}/p' packages/core/src/services/session-transcript-reader.ts.

3. Nit (carried from round 1, stands) — the doc-comment reflow leaves an orphan line

acpAgent.ts now reads * below. Session load and on its own line, splitting the "Session load and
resume" pair across the wrap. Prettier does not reflow comments, so prettier --check stays green
(04-gates-live-probes-8-of-8.png); purely cosmetic. Reproducing check:
grep -n 'below. Session load and' packages/cli/src/acp-integration/acpAgent.ts.

4. Observation (carried from round 1, stands) — the guard test is spelling-only

M2 and M3 leave the guard green on both test files because neither names
runWithAcpRuntimeOutputDir; the guard is an AST walk for one identifier. What catches those shapes
is the per-handler behavioural test, and after this PR all seven exist (census above). Recorded so the
invariant is not over-trusted; no change requested.

5. Observation, new, pre-existing — two turn-index error kinds have no assertion anywhere

grep -rc 'invalid_transcript_limit' packages/cli/src/acp-integration/*.test.ts returns 0, and
'Invalid transcript snapshot' likewise, while invalid_transcript_cursor has 2. The handler emits
both kinds (acpAgent.ts catch chain). Not introduced by this PR and outside its scope; noted only
because the census walked the handler's error paths.

Not covered

  • Per-commit attribution. HEAD^2 (2b80865d) is a shallow graft with zero parents
    (git rev-parse HEAD^2^ fails), so git rev-list HEAD^1..HEAD^2 reaches only it (count 1) while
    the snapshot lists three commits; dba1ac7b (round 1's head) is no longer a valid object and
    65e4f9fb exists as an object but is unreachable from HEAD^2. Only the aggregate HEAD^1..HEAD
    diff was verified.
  • Base worktree test execution. Same globalSetup prerequisite blocker as round 1 (12 unbuilt
    workspace dist/ outputs plus a missing generated file), so every control arm is a single-file
    source swap inside the built head tree. Each cell records sha256[:16] of both swapped files and
    every group re-verifies the byte-exact restore to head (L-* assertions); git status --porcelain
    is empty at the end (asserted).
  • Internal workspace links. Not re-checked this round: round 1 established that a base worktree
    resolves node_modules/@qwen-code/* into the head tree, and that this PR touches no packages/core
    file and no package.json/package-lock.json; the same holds at this base, and every arm's
    acpAgent.ts resolved from the arm's own tree (per-cell sha column).
  • Real-wire ACP-child E2E. Not re-run. The previous round's wire harness proved head==base
    behaviour with a discriminating oracle; this round proves the two arms' executable code is
    byte-identical, so a wire A/B would compare identical behaviour by construction. Round 1's wire
    result remains the last behavioural measurement of the handler, and main now carries that code.
  • acp-bridge HTTP route and live-session pre-read flush. Untouched by this diff and not
    re-exercised; round 1's scope notes still apply.
  • Collateral for the six sibling mutants. Measured filtered (attribution only) this round to fit
    the budget; whole-file collateral for the same mutants was measured in round 1.
  • Flakiness. The workflow's own flakiness gate covers the changed test file; no separate 5-round
    loop was run this round.
  • Repo-wide suite, bundle, integration tests, other platforms. Not run; Linux container only.
  • Trial merge into current main. Not needed: HEAD^1 is main's tip, so the base is not stale.
  • The superseded M4 control row is excluded from the verdict and from the assertion count as a
    harness defect (see Methodology), not counted as a failure against the diff.

Methodology

Everything ran in the CI verify container at the merge-ref checkout against the already-installed
root node_modules and built dist/; no npm ci or full rebuild was needed. Because the base
worktree cannot run vitest, each arm is produced by swapping the single changed production file
and/or the test file inside the built head tree and restoring afterwards; run.mjs records
sha256[:16] of both files per cell and asserts the byte-exact restore. Harnesses: lib.mjs
(git-object arm sources, uniqueness-checked mutate(), junit parsing), mutants.mjs (12 mutants,
each anchor matched exactly once or the run aborts), run.mjs/run-dir.mjs (vitest cells),
gates.mjs (prettier/eslint/tsc, each with a planted violation first), score.mjs (re-derives all
70 assertions from the recorded junit XML and JSON, so every number maps to an executed comparison),
evidence.mjs + scripts/verify-capture.mjs (the four PNGs). One harness defect is disclosed rather
than hidden: the first positive control (M4) appended a suffix to an error message, but
rejects.toThrow(string) is a substring match, so it could not kill; it survived both arms, was
excluded from the verdict, and was replaced by M4b, which alters the asserted substring and kills on
both arms. Raw per-cell logs, junit XML and per-group JSON live in
tmp/pr11047-verify-20260905-153356/logs/ and results-*.json.

Flakiness gate log

rounds=5 files=1 skipped=0
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/acp-integration/acpAgent.test.ts: PPPPP

verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)

Evidence images

01-ab-guard-already-green-on-base

02-mutation-matrix-0-of-621-to-1-of-622

03-sibling-census-7-of-7-pinned

04-gates-live-probes-8-of-8

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE (verified at head 2b80865 — re-review of the moved head; the previous approval at dba1ac7b was procedurally dismissed by the merge commit, not by any new finding)

State after the merge

The merge of main brought in #11036, which landed the same production routing this PR introduced — so the PR's net delta against current main is now exactly two items: the normative roster comment (7 handlers enumerated, matching the 7 call sites of runWithPinnedRuntimeBaseDirForRequest at head) and the behavioral per-request routing test for the turn-index handler. I verified at head that the handler routes through the choke point (acpAgent.ts:9219) and that the AST guard's load-bearing property holds: runWithAcpRuntimeOutputDir has exactly one direct call — the shared delegation at :4596 — so the guard the previous main-red satisfied by two merged fixes, not by this PR's diff alone, and nothing here breaks it.

Historical items

Round 1's two Suggestions were both fixed at dba1ac7b (the routing test — still present and passing against the merged shape since the test drives extMethod(sessionTurnIndex) end to end through the same code path #11036 shipped — and the roster completion). The three bot rounds at this exact head posted zero findings (no Critical in any ledger), and a human maintainer approved this head before this pass.

CI at head

23 checks green, zero failures; Test (ubuntu-latest) and web-shell E2E Smoke show this week's recurring shared-pool cancellations (cancellation, not failure, on a diff that is comment + test), non-gating per policy.

@wenshao
wenshao added this pull request to the merge queue Sep 5, 2026
Merged via the queue into main with commit 9b188ea Sep 5, 2026
141 of 147 checks passed

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review pass on head 2b80865. Context: the handler routing itself already landed on main via issue#11036 (eaef97e), so after this branch merged main the remaining diff is exactly the two pieces main still lacks — the regression test pinning that sessionTurnIndex resolves settings per request (loadSettings called with the request cwd, and the pin receiving the per-request settings object rather than the boot-workspace cache, the #10095 bug class), and the choke-point doc roster gaining 'transcript turn index'. The test follows the established per-request mock pattern used by the sibling handler tests (loadSettingsCached passes through to loadSettings), so it is consistent with the current main implementation. Note: Test (ubuntu) and web-shell E2E Smoke show cancelled on this head rather than green; the merge queue will re-verify required checks on the merge result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants