test(core): close the deferred test gaps recorded in #9930's review rounds - #10465
Conversation
…view rounds Three items QwenLM#9930's rounds 3-4 recorded as non-blocking deferrals, now resolved by the same author: - Rotation fallback branch (config.test.ts): pinned via the re-claim scenario — the fallback holds a live Config reference, so only an interloper Config taking the fallback between construction and an un-contexted rotation makes the rotation-time claim observable. Deletion mutant now fails the test. - Streak-cap recovery (debugLogger.test.ts): the cap must act as a circuit breaker, not a latch — a capped streak still attempts on a session change, and one success re-opens retries. The never-attempt-again latch mutant now fails the test. - Drain-side sessionIdContext.exit (scheduler.ts): removed rather than tested — every path into the drain (start-side kick, .finally re-kick, retry timers) already runs context-free behind the start-side exit, so the second wrapper was unreachable defensive code no test could pin. The inner `return` becomes `continue` (equivalent: finally still runs, the while condition exits the loop). Verification: core config 580/580 + debugLogger 41/41, cli scheduler 14/14 + acpAgent 531/531; both mutants killed by their named tests; typecheck/ESLint/Prettier clean. Relates to QwenLM#9535, QwenLM#9538, QwenLM#9930. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the follow-through — re-run on the current head
Moving on to code review. 🔍 中文说明感谢跟进——本次在当前 head
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review — re-run at My independent proposal for the three deferred items matches what landed: an interloper-Config test for the rotation re-claim, a circuit-breaker-recovery test for the streak cap, and removal of the unreachable wrapper rather than an unpinnable comment-only wrapper. What I verified on this pass:
One residual, named by the PR itself: if a future caller ever kicks the worker from inside a bound session context, the invariant lives solely in the start-side exit. Acceptable — every worker entry is module-private today and the comment records the invariant — but worth knowing when extending this scheduler. Testing — unattended CI run; no PR code was built or executed here. Evidence below is the PR's own CI on
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Mutation testing is not part of CI, so green suites prove the tests pass but not that they pin the mutants. The sponsored 中文说明代码审查——在 我对三个遗留条目的独立提案与最终落地一致：用"闯入者" Config 测试 rotation 重新认领、用断路器恢复测试钉住 streak 上限、直接移除不可达包装而非留一个无法钉住的注释版包装。 本轮核实内容：
PR 自己点名的残留：若未来有调用方在绑定会话上下文内启动 worker，不变式将只靠 start 侧 exit 维系。可以接受——当前所有 worker 入口均为模块私有，注释也记录了该不变式——但后续扩展此调度器时值得留意。 测试——无人值守 CI 运行；此处未构建或执行任何 PR 代码。下方证据是 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal follow-through on three recorded deferrals; the only check this lane cannot re-run itself is the mutation testing, which the in-flight sponsored Reflection: this is what a good deferral follow-through looks like, and the review rounds made it better rather than bigger. The R1-1 fix didn't just extract a helper — it closed a real isolation hole (the missing Approving now: CI is fully green on this head, no PR-CI runs are pending, and the fork- 中文说明反思：这是一次遗留项跟进的理想形态，而且评审轮次让它变得更好而非更臃肿。R1-1 修复不只是抽取辅助函数——它补上了两个原测试共享的真实隔离漏洞（遗漏的 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 3c": executing the two new tests to confirm they are green — the shared review worktree has no node_modules / dist , and npm ci + build exceeds this lens's tool b….
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 3c"：executing the two new tests to confirm they are green — the shared review worktree has no node_modules / dist , and npm ci + build exceeds this lens's tool b…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const previousDebugLogFileEnv = process.env['QWEN_DEBUG_LOG_FILE']; | ||
| const previousSessionIdEnv = process.env['QWEN_CODE_SESSION_ID']; | ||
| const initialSessionId = '550e8400-e29b-41d4-a716-446655440000'; | ||
| const rotatedSessionId = '7ba7b810-9dad-11d1-80b4-00c04fd430c8'; |
There was a problem hiding this comment.
[Suggestion] R1-1: This new test copies ~28 lines of load-bearing debug-fallback isolation boilerplate verbatim from the adjacent test does not replace the global debug fallback during daemon Config creation or rotation (config.test.ts:617): the four fs.promises spies (mkdir/appendFile/unlink/symlink), the QWEN_DEBUG_LOG_FILE/QWEN_CODE_SESSION_ID env save, and the entire finally teardown (mockRestore() ×4, resetDebugLoggingState(), setDebugLogSession(null), env restore). These spies are the only thing standing between the two tests and the real filesystem — the module-level vi.mock('node:fs') factory at config.test.ts:135 spreads ...actual and overrides only the sync fs API, so any un-spied fs.promises call runs the real implementation. If the debug fallback's fs surface changes (e.g. the alias/claim path gains a rename or stat call), both copies must gain the matching spy in lockstep; updating only one leaves that test doing real mkdir/appendFile writes into the actual global debug dir during a test run — the exact leakage this boilerplate exists to prevent. Neither copy spies fs.promises.readlink, so both tests already make a real (read-only) readlink against the actual debug dir — harmless today, but a shared helper is the natural place to make the isolation surface complete and single-sourced.
Witness:
A/B probe (scratch tree at this commit, only the new test's four spies neutralized):
real writes landed in QWEN_RUNTIME_DIR —
debug/7ba7b810-9dad-11d1-80b4-00c04fd430c8.txt (real "[CLI_ROTATION] post-rotation message" line)
debug/latest -> 7ba7b810-9dad-11d1-80b4-00c04fd430c8.txt
Spies intact (committed code), same run: Tests 1 passed, RUNTIME DIR NEVER CREATED.
Extract a helper in this file used by both tests, e.g. async function withIsolatedDebugFallback(fn) that installs the four vi.spyOn(fs.promises, …) mocks (plus readlink to complete the surface), saves the two env vars, calls resetDebugLoggingState(), runs fn, and in finally restores spies, singleton state (resetDebugLoggingState(); setDebugLogSession(null)), and env — each test keeps only its scenario and assertions. The helper must keep vi.spyOn on the real fs.promises methods with mockRestore() teardown: the module-level factory at config.test.ts:137-150 returns {...actual} without overriding promises (unlike debugLogger.test.ts:27-37, whose vi.mock('node:fs') replaces promises with vi.fn()s), so vi.mocked(...)/clearAllMocks() alone would not isolate or uninstall them.
中文说明
[建议] 新测试从相邻测试 does not replace the global debug fallback during daemon Config creation or rotation（config.test.ts:617）逐字复制了约 28 行承重性的 debug-fallback 隔离样板：四个 fs.promises spy（mkdir/appendFile/unlink/symlink）、QWEN_DEBUG_LOG_FILE/QWEN_CODE_SESSION_ID 环境变量保存，以及整个 finally 清理（mockRestore() ×4、resetDebugLoggingState()、setDebugLogSession(null)、环境变量恢复）。这些 spy 是这两个测试与真实文件系统之间唯一的屏障——config.test.ts:135 处的模块级 vi.mock('node:fs') 工厂展开 ...actual 且只覆盖同步 fs API，因此任何未被 spy 的 fs.promises 调用都会执行真实实现。如果 debug fallback 的 fs 调用面发生变化（例如 alias/claim 路径新增 rename 或 stat 调用），两份副本必须同步补上对应的 spy；只更新其中一份，该测试就会在测试运行期间向真实的全局 debug 目录执行真实的 mkdir/appendFile 写入——正是这份样板所要防止的泄漏。两份副本都没有 spy fs.promises.readlink，因此两个测试其实都已经对真实 debug 目录执行了一次真实的（只读）readlink——目前无害，但抽取共享 helper 正是把隔离面补全并收敛到一处的自然位置。
验证证据：在同一提交的临时树中做 A/B 探针（仅禁用新测试的四个 spy）：真实写入落进了 QWEN_RUNTIME_DIR——debug/7ba7b810-9dad-11d1-80b4-00c04fd430c8.txt（含真实的 "[CLI_ROTATION] post-rotation message" 日志行）以及真实符号链接 debug/latest -> 7ba7b810-9dad-11d1-80b4-00c04fd430c8.txt；spy 保持原样（提交代码）时同样运行：测试通过且运行目录从未被创建。
建议在本文件抽取一个两个测试共用的 helper，例如 async function withIsolatedDebugFallback(fn)：安装四个 vi.spyOn(fs.promises, …) mock（并加上 readlink 以补全隔离面）、保存两个环境变量、调用 resetDebugLoggingState()、执行 fn，并在 finally 中恢复 spy、单例状态（resetDebugLoggingState(); setDebugLogSession(null)）与环境变量——每个测试只保留自身场景与断言。helper 必须保留对真实 fs.promises 方法的 vi.spyOn 及 mockRestore() 清理：config.test.ts:137-150 的模块级工厂返回 {...actual} 且未覆盖 promises（与 debugLogger.test.ts:27-37 不同，那里的 vi.mock('node:fs') 把 promises 替换为 vi.fn()），因此仅靠 vi.mocked(...)/clearAllMocks() 无法隔离或卸载它们。
— qwen3.8-max via Qwen Code /review (v0.22.3)
…1-1) The two debug-fallback rotation tests copied ~28 lines of fs-spy + env save/restore boilerplate verbatim, and both omitted a readlink spy — so both already made a real readlink against the actual global debug dir, and any future fs call added to the fallback/alias path would leak real writes from whichever copy wasn't updated in lockstep. Extract withDebugFallbackIsolation: it spies the full surface (mkdir/appendFile/ unlink/symlink/readlink) once, hands the body only the appendFile spy, and restores env + logger state on exit. Behavior unchanged; mutation-verified that the rotation-claim deletion still fails its test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
R1-1 addressed in |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6b": execute the two debug-fallback tests in packages/core/src/config/config.test.ts — the review worktree has no node_modules and I declined a full monorepo install….
中文说明
未探索到全部深度（达到工具调用预算）："agent 6b"：execute the two debug-fallback tests in packages/core/src/config/config.test.ts — the review worktree has no node_modules and I declined a full monorepo install…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| async function withDebugFallbackIsolation( | ||
| run: (appendFileSpy: ReturnType<typeof vi.spyOn>) => Promise<void>, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
[Critical] R1-1: (fix-induced) [fails-closed] [regression] The new withDebugFallbackIsolation helper types its callback as ReturnType<typeof vi.spyOn>, which resolves to vitest's generic-constraint overload MockInstance<(this: unknown, ...args: unknown[]) => unknown> — the concrete vi.spyOn(fs.promises, 'appendFile') spy is not assignable to it, so npm run build --workspace=packages/core fails with TS2345. The round-1 fix for R1-1 (extracting this shared helper) introduced it: every build or typecheck of packages/core — the CI Test job, npm run typecheck, npm run preflight, and dependent workspaces — exits non-zero at src/config/config.test.ts(650,17) on await run(appendFileSpy);. vitest transpiles without type-checking, so the two tests still run green and the failure is invisible to a test-only run; live CI on this commit is red with exactly this error.
Witness:
npm run build --workspace=packages/core → exit 1, single error:
src/config/config.test.ts(650,17): error TS2345: Argument of type
'MockInstance<(path: PathLike | FileHandle, data: string | Uint8Array<ArrayBufferLike>, ...) => Promise<...>>'
is not assignable to parameter of type 'MockInstance<(this: unknown, ...args: unknown[]) => unknown>'.
Identical failure in live CI: Test (ubuntu-latest, Node 22.x), run 33240215927.
Fix (two spots — repo precedent at packages/core/src/services/sessionService.test.ts:74):
// config.test.ts:8 — extend the existing type import
import type { Mock, MockInstance } from 'vitest';
// helper signature
async function withDebugFallbackIsolation(
run: (appendFileSpy: MockInstance<typeof fs.promises.appendFile>) => Promise<void>,
): Promise<void> {The annotation must stay assignable where the spy is passed — await run(appendFileSpy) at config.test.ts:650, with the spy produced as vi.spyOn(fs.promises, 'appendFile').mockResolvedValue(undefined) (config.test.ts:637-639) and consumed in both test bodies via toHaveBeenCalledWith(path, expect.stringContaining(...), 'utf8') — so it must not be widened to a bare Mock/vi.fn() type that drops the call-argument typing.
Acceptance criterion: npm run build --workspace=packages/core must go green — it is red today with TS2345 at config.test.ts:650, and reverting the annotation to ReturnType<typeof vi.spyOn> reproduces the failure (no runtime test pins a type annotation, so the build itself is the mutation check).
中文说明
R1-1：（修复引入）新的 withDebugFallbackIsolation 辅助函数把回调参数标注为 ReturnType<typeof vi.spyOn>，它解析到 vitest 的泛型约束重载 MockInstance<(this: unknown, ...args: unknown[]) => unknown> —— 具体的 vi.spyOn(fs.promises, 'appendFile') spy 无法赋给该类型，导致 npm run build --workspace=packages/core 报 TS2345 失败。本缺陷由第 1 轮 R1-1 的修复（提取这个共享辅助函数）引入：所有对 packages/core 的构建或类型检查 —— CI 的 Test 任务、npm run typecheck、npm run preflight、以及依赖它的工作区 —— 都会在 src/config/config.test.ts(650,17) 的 await run(appendFileSpy); 处非零退出。vitest 转译时不做类型检查，所以这两个测试仍然绿灯，仅跑测试看不到该失败；当前提交上的 CI 正是因为这个错误而红。
修复（两处 —— 仓库先例见 packages/core/src/services/sessionService.test.ts:74）：把类型导入扩展为 import type { Mock, MockInstance } from 'vitest';，并把辅助函数签名改为 run: (appendFileSpy: MockInstance<typeof fs.promises.appendFile>) => Promise<void>。
约束：该标注必须保持可赋值 —— await run(appendFileSpy)（config.test.ts:650）处的 spy 由 vi.spyOn(fs.promises, 'appendFile').mockResolvedValue(undefined)（config.test.ts:637-639）产生，并被两个测试体以 toHaveBeenCalledWith(path, expect.stringContaining(...), 'utf8') 消费 —— 因此不能放宽为丢失调用参数类型的裸 Mock/vi.fn() 类型。
验收标准：npm run build --workspace=packages/core 必须变绿 —— 当前因 config.test.ts:650 的 TS2345 而红，把标注还原为 ReturnType<typeof vi.spyOn> 会复现该失败（类型标注没有运行时测试可钉，构建本身就是变异检验）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
The helper typed its callback arg as ReturnType<typeof vi.spyOn>, which resolves to vi.spyOn's generic-overload return type; the concrete appendFile spy is not assignable to it, so tsc/build/CI failed with TS2345 at the call site (vitest transpiles without type-checking, so the tests still ran green and the failure was invisible to a test-only run — I missed it by not running typecheck on the previous commit). Drop the callback arg; the two tests read the spy back via vi.mocked(fs.promises.appendFile), which is correctly typed. appendFile folded into the spies array. Verification: tsc --noEmit exit 0, config suite 580/580, rotation-claim mutation still fails its test, ESLint + Prettier clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed in Fix: dropped the callback arg entirely; the two tests read the spy back via |
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.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": empirical mutation check — removing the sessionIdContext.getStore() === undefined re-claim branch in config.ts:startNewSession and re-running the test to pr…; "agent reverse-audit (round 2)": repeated soak with full error capture of the flake (15 post-failure runs all passed, so the full failure message was never captured).
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/config/config.test.ts:708 — [probe] rotation test never asserts the interloper steals the fallback
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：empirical mutation check — removing the sessionIdContext.getStore() === undefined re-claim branch in config.ts:startNewSession and re-running the test to pr…；"agent reverse-audit (round 2)"：repeated soak with full error capture of the flake (15 post-failure runs all passed, so the full failure message was never captured)。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
CanReader
left a comment
CanReader
left a comment
There was a problem hiding this comment.
Deleting defensive code is the right call when it genuinely cannot execute, and the reasoning here holds up as far as I could follow it statically.
The claim is that every path into drainNonInteractiveQueue already runs outside the session-id store, so the inner sessionIdContext.exit was dead. The pieces are where the comment says they are at this commit: startNonInteractiveOpenAILogHousekeeping wraps its enqueue and worker start in sessionIdContext.exit (scheduler.ts:197), the worker's re-kick hangs off the .finally at 257, and the retry timers are scheduled from inside the drain itself (357). Since AsyncLocalStorage.exit runs its callback outside the store and anything scheduled inside that callback inherits the outside context, the chain does hold — a timer registered within the exited scope fires context-free, so the drain and everything it schedules stay outside the store without a second wrapper.
The part I like is that this is framed as removing something no test could pin. That is the honest reason to delete defensive code, and it is better than the usual "this looked redundant". Keeping the explanation as a comment at the site is the right trade: the next person to read this will wonder why there is no exit here, and now they will not have to re-derive the answer.
One suggestion. The invariant that makes this safe lives at the call site (line 197), not here, and nothing fails if someone later adds a fourth way into the drain that does not go through the exited scope — the code would silently start propagating a session id into housekeeping work. If there is a cheap way to assert it, an invariant check in the drain (sessionIdContext.getStore() === undefined) behind a debug/dev guard would turn a silent regression into a visible one without reintroducing the wrapper you are removing. If that is more machinery than it is worth, a pointer in this comment to scheduler.ts:197 as the choke point would at least make the coupling greppable.
I reviewed this statically and did not run the housekeeping suite.
…d-2 suggestion) The invariant that makes the removed drain-side sessionIdContext.exit safe lives at the start-side exit, not here. Spell that out and warn that a new entry into the drain must go through the exited scope — makes the coupling greppable, per the review suggestion. Comment-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Took the cheap option in |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /triage |
|
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: 40 passed · 0 failed · 40 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：40 通过 · 0 失败 · 40 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10465 Deep Verification — merge-readyVerdict: 中文摘要结论： A/B 结论：
Findings：无。仅有一处对描述的更正（测试总数因 base 前移为 640，而非描述中的 621，见 Corrections）。 未覆盖范围：逐提交归因（浅克隆 depth 2，4 个提交仅 head 可达）、acpAgent.test.ts（整体 mock 调度器模块，无本 PR 信号）、ESLint 门禁、真实计时器下的重试定时器路径（以两臂假定时钟套件等价 + ALS 继承语义佐证）、Windows/macOS。 ScopeThis PR closes three deferred test gaps recorded in #9930's review rounds 3–4: two become mutation-killing tests (core), and the third — the drain-side
Mutation matrix (central claim 1)Production sources are identical on both arms (the PR changes no core production code), so the matrix isolates what the new tests pin that the old tests did not. Mutants are single-guard surgical mutations applied with exact-match replacement (
Re-run: Reading:
cli A/B: drain-side wrapper removal (central claim 2)
Harness: The base-arm observation is the load-bearing one: the drain-side exit call fired with the store already undefined — the wrapper stripped nothing, so it was dead code, and its removal changes no observable. The debug-routing oracle corroborates without trusting the spy: had any context leaked into the drain, Remaining coverage of the removed layer, per the skill's layered-guard rule: the hazard (session context leaking into process-scoped housekeeping) is still pinned by the start-side exit, which two existing tests exercise (
Corrections
FindingsNone. Every claim the PR makes was measured and held; no unexpected failure occurred in any cell. Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Maintainer verification — built and run locally against a real merged treeI rebuilt this PR locally, merged it onto current Environment. PR head 1. The stated test runs reproduce
2. Mutation matrix — 11 mutants, 0 survivors, and the counterfactual holdsI ran the two mutants the PR names, plus nine more around the same two guards, against both the PR-head suites (640 tests) and the pre-PR suites (
The other nine were already covered, but the two new tests are not narrow one-mutant pins: the streak test also fires on
3. The production change: verified as a no-op, and the residual risk measuredThis is the only part of the PR that ships. The argument in the comment is a static one, so I checked it dynamically instead. I wrote an AsyncLocalStorage ledger probe that records
Five of six ledgers are byte-identical. P6 is the one that matters most: it starts housekeeping from a bound, ACP-shaped context exactly as P4 also settles the
The single differing row is the honest measurement of what @CanReader flagged. P2 is not a production path — I hand-invoked the retry callback from inside 4. Bonus: the round-1 helper's
|
| 套件 | 声明 | 合并树实测 |
|---|---|---|
core：config.test.ts + debugLogger.test.ts |
621/621 | 640/640（599 + 41；main 自 PR 基线以来有增长） |
cli：non-interactive-scheduler.test.ts |
14/14 | 14/14 |
cli：+ scheduler.test.ts + acpAgent.test.ts |
531/531 | 604/604（564 + 26 + 14） |
2. 变异矩阵 —— 11 个变异体、0 存活，且反事实成立
我把 PR 点名的两个变异体、加上围绕同两处守卫的另外九个，分别对 PR head 套件（640 条） 与 PR 之前的套件（两个测试文件的 origin/main 版本，638 条） 各跑一遍。真正能证明这个 PR 有价值的是反事实那一半：
C-M1（删掉 rotation 路径的setDebugLogSession(this)守卫）—— 在 638 条 PR 前测试下全部存活；在 PR head 下被且仅被一条测试杀死：claims the global debug fallback on un-contexted rotation (single-session CLI)。断言逐字复现。D-M1（把 streak 上限变成永不重试的闩锁）—— 在 638 条 PR 前测试下全部存活；在 PR head 下被且仅被一条测试杀死：recovers from the streak cap when a later alias update succeeds。断言逐字复现。
另外九个原本就有覆盖，但两条新测试并不是只钉一个变异体的窄用例：streak 那条同时对 D-M2（成功不再重置 streak）、D-M3（<= 差一）、D-M4（streak 从不自增）、D-M5（reset 不再清零）、D-M7（上限 3→2）报红。rotation 那对是双向约束：把守卫取反（C-M3）会同时打红两条 fallback 测试；把任一处 claim 改成无条件（C-M2、C-M4）则打红 daemon 侧那条。
3. 生产改动：实测为空操作，并把残留风险量化
这是 PR 里唯一会上线的部分。代码注释给的是静态论证，所以我改用动态方式核对：写了一个 AsyncLocalStorage 流水账探针，在 drainNonInteractiveQueue 的每次进入处记录 sessionIdContext.getStore()，然后用完全相同的探针文件跑同一棵树的两臂，只替换 scheduler.ts（BASE = origin/main，带包装；HEAD = 本 PR，去掉包装）。
| # | 驱动的入口 | BASE | HEAD | |
|---|---|---|---|---|
| P1 | 在 sessionIdContext.run 内的 start 侧启动 |
<none> |
<none> |
相同 |
| P3 | 第二个会话入队的 job 被 FIFO 取走 | <none> |
<none> |
相同 |
| P4 | 在飞任务中途 stop()，且仍有第二个 job 排队 |
<none> |
<none> |
相同 |
| P5 | fresh/locked 的重排延迟 |
60000ms |
60000ms |
相同 |
| P6 | 重试定时器的真实事件循环派发 | <none> |
<none> |
相同 |
| P2 | 合成：在绑定上下文内手工调用重试回调 | <none> |
S-timer-firer |
不同 |
六个流水账里五个逐字节相同。P6 最关键：它按 acpAgent.ts 的生产形态在绑定上下文里启动 housekeeping，让任务抛错，再让那个 10 分钟重试定时器真的由事件循环派发（只缩短延迟，不改派发方式）—— 两臂下重入 drain 都是无上下文的。所以注释里的归纳论证在实测上成立，这次移除在今天存在的任何路径上都不改变行为。
P4 同时从行为上结清了 return → continue 的替换：仍有排队 job 时调用 stop()，在飞任务被 abort、排队 job 不再执行，两臂完全一致。
那唯一不同的一行，正是 @CanReader 所提风险的诚实测量值。P2 不是生产路径 —— 我在 sessionIdContext.run 里手工触发重试回调，用来合成"未来某个不经过 start 侧 exit 就进入 drain 的调用方"。BASE 会把上下文剥掉；HEAD 会把 session id 静默带进进程级 housekeeping。这就是残留风险的确切大小：今天为零，等到有人加第四个入口那天变成一次静默回归。
4. 附带核实：round-1 helper 里的 readlink spy 确实是承重件
commit 36e3749fdb 抽取 withDebugFallbackIsolation 的理由之一是"两处都漏了 readlink spy……都已经在对真实全局 debug 目录做真实 readlink"。我用一个装在模块作用域的记录器做了实测（因此它只能看到逃过测试自身 vi.spyOn 的调用）：按现状的 helper —— 真实 fs 调用 0 次；仅删掉 readlink spy 那一行 —— 两条测试依然通过，但对我真实的 ~/.qwen/debug/latest 发起了 2 次真实 readlink。重构给出的理由属实，而且这个泄漏是静默的。
观察项（非阻塞，不作为合入条件）
- O1 —— drain 的三个入口里有两个在 CI 中仍未被钉住。 现有上下文测试（
keeps process-scoped cleanup outside session contexts）只覆盖 start 侧启动。我的 P6 证明重试定时器路径当前是无上下文的，但仓库里没有任何东西能在它回归时报红。若想用最低成本收掉 @CanReader 那条，加一个 P6 形态的测试、或他建议的那行sessionIdContext.getStore() === undefined开发期断言都可以；鉴于上面 P1–P6 的结果，a6cab44148里"只加注释"的处理是站得住的，我不会为此卡住这个 PR。 - O2 —— 无其他问题。 PR 的 CI 为 17 成功 / 43 跳过，无失败。
结论
描述中的每条断言都在真实构建下复现。两条新测试确实是收掉这两个缺口的原因（由 PR 前反事实证明，而非声称），唯一的生产改动经测量为空操作。建议合入。
复现物料（探针、变异脚本、原始流水账、原始矩阵输出）：wenshao/qwen-code@assets-pr10465。
|
Released in v0.23.0. |
4 participants
What this PR does
Closes the three test gaps that #9930's review rounds 3–4 recorded as non-blocking deferrals, submitted by the same author as a follow-through. Two gaps become mutation-killing tests; the third — an
sessionIdContext.exitwrapper the review probe showed to be mutation-unreachable — is removed as dead defensive code rather than left permanently unpinnable.Why it's needed
Deferred review findings rot unless someone owns them. All three were recorded against code this author shipped in #9930, so closing them here keeps the debug-log-routing work fully pinned:
Configreference, so rotating the same Config reroutes writes even without the rotation-time claim — the claim is only observable when another Config (transcript replay, bootstrap) took the fallback in between and the un-contexted rotation must re-claim it. The new test drives exactly that interloper scenario.sessionIdContext.exit(round-4: "mutation-unreachable"): every path into the drain — the start-side kick, the.finallyre-kick, and the retry timers — already runs context-free behind the start-side exit instartNonInteractiveOpenAILogHousekeeping(timers registered inside that scope inherit it). A second wrapper was unreachable defensive code no test could pin, so it is removed; the start-side exit remains the single tested choke point. The innerreturnbecomescontinue, which is behaviorally identical (finallystill runs, then thewhilecondition ends the loop).Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/config/config.test.ts src/utils/debugLogger.test.ts→ 621/621.cd packages/cli && npx vitest run src/services/housekeeping/non-interactive-scheduler.test.ts→ 14/14 (unchanged tests still pass against the simplified drain).setDebugLogSession(this)guard block inconfig.ts→claims the global debug fallback on un-contexted rotationfails;if (aliasFailureStreak >= MAX_CONSECUTIVE_ALIAS_FAILURES) return;before alias scheduling indebugLogger.ts→recovers from the streak cap when a later alias update succeedsfails.Evidence (Before & After)
Before: both mutants survive the full suites (as recorded in #9930's round-3/round-4 review ledgers). After: each mutant fails its named test; real code passes 621/621 + 14/14. No user-visible TUI change — N/A for screenshots.
Tested on
Environment (optional)
vitest from each package dir on macOS (Darwin 25.4.0, Node 22). N/A beyond unit tests.
Risk & Scope
Linked Issues
Relates to #9535, #9538, #9930 (closes the deferred items recorded in #9930's round-3/round-4 reviews; no standalone issue was filed since the findings live in those review ledgers).
中文说明
本 PR 做了什么：收掉 #9930 评审第 3、4 轮记录在案的三条非阻塞遗留（同一作者跟进）。两条补上可杀变异体的测试；第三条（评审探针证明"变异不可达"的 drain 侧
sessionIdContext.exit包装）按死防御代码移除，而非留下永远无法钉住的缺口。为什么需要：deferred 评审发现无人认领就会腐烂。三条都出自本作者在 #9930 交付的代码：(1) rotation 回退分支——全局回退持有 Config 活引用，只有"另一个 Config 中途抢走回退、无上下文 rotation 必须抢回"的场景能观察到该调用，新测试正是驱动这个闯入者场景（删除变异体现在会红）；(2) streak 上限恢复——上限必须是断路器而非闩锁：触顶后换会话仍要尝试，一次成功即重开重试（永不再试的闩锁变异体现在会红）；(3) drain 侧 exit——进入 drain 的所有路径（start 侧启动、.finally 重启、重试定时器）都已在 start 侧 exit 之后运行、天然无上下文，第二层包装不可达故移除；内层
return改为行为等价的continue。验证：core 621/621、cli scheduler 14/14；两个变异体均被点名测试杀死；typecheck/ESLint/Prettier 干净。macOS 本地已测，Windows/Linux 交 CI。
风险与范围：唯一生产改动是移除不可达包装，所有可达路径行为不变（现有 drain 侧上下文测试仍通过）；不变更 #9930 的路由语义本身。无破坏性变更。
关联：#9535、#9538、#9930（收掉其 round-3/4 评审台账中的 deferred 项；发现记录在评审台账中，故未另开 issue）。
🤖 Generated with Claude Code