fix(core): stop daemon Configs and background work from hijacking the debug-log session - #9930
Conversation
… debug-log session Follow-up to QwenLM#9538 (relates to QwenLM#9535). That PR routed debug logs through sessionIdContext where a context was bound; this closes the entry points that still ran without one, so a multi-session daemon no longer leaks one session's debug lines into another session's file: - Config construction and rotateSessionId only claim the process-wide debug-log fallback when no sessionIdContext is active, so creating or /clear-rotating session B can no longer redirect session A's in-flight background logs. - All ACP session Config creation (new/load/resume/transcript replay) runs inside sessionIdContext; transcript replay inherits the target session's validated context instead of minting an id. - MCP budget-event callbacks re-bind their owning session's context at invocation time (the invoker may be a shared transport). - Dead-session deleteSession/renameSession dispatch binds the validated target session id even when no live Session object exists in this process. - Non-interactive OpenAI log housekeeping explicitly exits sessionIdContext: its queue is process-scoped (deduped by log dir), so its logs stay in the bootstrap log instead of whichever session started it. - The latest-alias dedup marker is cleared when an alias update fails, so the next write retries instead of leaving `latest` stale; docstrings now describe the three-tier resolution order. Verification: debugLogger 37/37, config 552/552, acpAgent 467/467, non-interactive-scheduler 13/13; typecheck clean for the changed files; ESLint + Prettier clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re-run: the head moved from
Moving on to code review. 🔍 中文说明复跑：分支头已从
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (head
|
| File | What changed |
|---|---|
docs/design/2026-08-24-debug-log-session-routing-residuals.md |
Design notes: ownership classes for the residual entry points, the bounded-retry rationale, non-goals |
packages/cli/src/acp-integration/acpAgent.ts |
Binds sessionIdContext around all session Config creation, workspace MCP discovery, budget callbacks, and dead-session dispatch; hoists SESSION_ID_RE to module scope |
packages/cli/src/acp-integration/acpAgent.test.ts |
Pins the bound context at each entry point, generated-id argv handling, and traversal-id rejection without binding |
packages/cli/src/acp-integration/acpAgent.worktree.test.ts |
Passes the real sessionIdContext through the core mock so binding is observable |
packages/cli/src/config/config.ts |
Adds internal sessionIdGenerated flag; daemon-generated UUIDs skip the caller-id occupancy check |
packages/cli/src/config/config.test.ts |
Pins the occupancy skip and that a failing scan cannot reject a generated id |
packages/cli/src/services/housekeeping/scheduler.ts |
Housekeeping start and drain run explicitly outside session contexts via sessionIdContext.exit |
packages/cli/src/services/housekeeping/non-interactive-scheduler.test.ts |
Asserts cleanup observes no session context even when started inside one |
packages/core/src/config/config.ts |
Constructor and rotation claim the process-wide debug fallback only when no context is bound |
packages/core/src/config/config.test.ts |
Asserts daemon Config creation and rotation leave the bootstrap fallback intact, via real appendFile routing |
packages/core/src/utils/debugLogger.ts |
Verifies the latest-alias target with readlink; bounded retry with failure-streak cap and generation guard |
packages/core/src/utils/debugLogger.test.ts |
Retry, streak reset, sticky cap, and stale-failure isolation coverage for the alias update |
Test evidence — the PR's own CI at 0b7ab7c2
All checks on this head are complete with zero failures — the Linux unit suite, the no-AK integration suite, the real-daemon E2E, desktop shell, web-shell smoke, the Java SDK matrix, and the security scans are all green. One correction to the previous stage comment here: the macOS/Windows unit jobs and the CLI integration job being skipped is by CI design, not pending approval — on pull_request events these lanes are structurally skipped (test_macos/test_windows run on merge_group/schedule/dispatch; the secrets-carrying CLI integration job runs on merge_group only). Platform coverage therefore lands in the merge-queue lane after approval. The Linux suite covers the alias-symlink logic as run here; Windows symlink behavior (where symlink fails without privilege) is exactly what the bounded-retry path protects, and its tests mock the failure — but the Windows lane itself does not exercise this head before merge.
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Test (macos-latest, Node 22.x) | skipped (merge_group lane by design) |
| Test (windows-latest, Node 22.x) | skipped (merge_group lane by design) |
| Integration Tests (CLI, No Sandbox) | skipped (merge_group lane by design) |
| Integration Tests (no-AK, No Sandbox) | success |
| Real daemon E2E / Java 11 | success |
| Desktop Shell (ubuntu-22.04 / windows-2022) | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success |
| SDK Java matrix (ubuntu/macOS/windows, Java 11–21) | success |
| Secret scan (TruffleHog) | success |
| Dependency CVE audit | success |
The included unit tests genuinely pin the change — each entry point asserts the bound context, config.test.ts asserts real appendFile routing to the bootstrap file, and the alias tests pin retry, streak reset, the sticky cap, and stale-failure isolation. But the end-to-end daemon behavior (a late-firing callback physically landing in its owning session's file on a live multi-session daemon) rests on the author's filesystem probe — their claim, not independently re-run in this static review. Sandboxed verification would settle it: @qwen-code /verify — that the budget-callback re-binding actually routes a real late write in a live multi-session daemon. The author has read-only access, so this would be a sponsored run: a maintainer's @qwen-code /verify approves the head it was written against, and that run carries a pre-execution risk screen plus a full workspace wipe — read the resulting report with the same skepticism as the fork's own CI logs.
Real-scenario testing
N/A — unchanged from the prior pass and still correct: the changed behavior has no TUI or single-session CLI surface (single-session routing is unchanged by design, so a before/after capture would be identical and prove nothing). It only manifests in which file receives a line inside a multi-session ACP daemon, which would require executing the PR's code in a daemon harness — not permitted here. The pinned unit tests, green CI above, and the named /verify lane are the substitutes.
中文说明
代码审查（针对 0b7ab7c2）：我读 diff 前的独立方案与实现依然逐点一致。本次复跑重点核验了上次之后新增的全部内容：生成 id 的占用检查跳过是安全的（内部标志，仅在无调用方 id 时置位，有测试双向钉住），并消除了扫描瞬态错误导致误报 session_id_conflict 的失败模式；别名重试有正确上限——readlink 校验与 updateSymlink 写相对目标一致（两边实现都核对过），连续失败上限 3 次后标记保持粘性，任一成功跨会话重置计数，代际计数防止陈旧失败回调误清新会话标记（均有测试钉住）；housekeeping 用的 sessionIdContext.exit() 是原生 AsyncLocalStorage 语义，continue→return 改写等价；transcript replay 判别式在全部四个调用点上精确成立，且会话模型/项目目录映射以 this.sessionId 为键、不受异步上下文污染；SESSION_ID_RE 提升为模块级后语义不变，同一正则同时用于新分发守卫与既有全部校验点；setDebugLogSession 的两处生产调用点均已加守卫（grep 确认无其他调用）；budget 回调绑定轮换后的当前 id、通知载荷保留稳定 ACP id；main 合并采用的 skipLlmInitialization 重命名与本头一致。非阻塞遗留：今天 /review 第 4 轮在同一提交上的两条探针延后项（轮换侧回退占用无测试钉住、drain 侧 exit 包裹对变异不可达），属覆盖观察而非缺陷。
测试证据：该提交上所有检查完成且零失败——Linux 单测、无 AK 集成、真实 daemon E2E、桌面壳、web-shell 冒烟、Java SDK 矩阵、安全扫描全绿。更正上次评论：macOS/Windows 单测与 CLI 集成任务被跳过是 CI 设计使然（pull_request 事件下这些通道只在 merge queue/定时/手动触发），并非等待批准；平台覆盖在批准后的 merge queue 通道完成。
沙箱验证：PR 自带单测确实钉住改动，但端到端 daemon 行为仍依赖作者的探针声明，未独立复跑。可用维护者赞助的 @qwen-code /verify 补齐（触发方式与注意事项见英文正文）。
真实场景测试：不适用。改动没有 TUI 或单会话 CLI 表现；多会话 daemon 场景需执行 PR 代码，静态审查不允许，以上述证据替代。
— Qwen Code · qwen3.8-max
Reviewed at 0b7ab7c232b894f483ece59b15e6b06776fc4816 · re-run with @qwen-code /triage
|
Confidence: 4/5 — same read as the prior pass, now on the final head: a clean, minimal follow-up that closes exactly the documented residuals, hardened by the review loop since (bounded retry, occupancy skip, mutation-pinning tests); held back from 5 only by the live multi-session daemon probe I cannot re-run and the two small coverage gaps the probes flagged. The commits since the last review are the review working as intended: round 1's concerns became the occupancy-check skip for generated ids and the bounded alias retry, round 4 landed zero findings on this head. Stepping back over the whole arc — #9535 opened for the privacy edge, #9538 closed the first half and recorded these entry points as a known deferral, and this PR closes exactly that list, no more — this is still what a good follow-up looks like. The guards fail toward the single-session status quo, dead-session binding is validated before it can ever name a file, and every load-bearing claim in the newer commits checked out against the code, not just the description. With a maintainer approval already standing on this exact commit and CI complete with zero failures, the remaining reservations are named in the Stage 2 comment and none of them block. CI is settled (all checks complete on this head, zero failures), so no deferred approval this time. 中文说明置信度：4/5 —— 与上次结论一致，现已针对最终提交：干净、克制的后续修复，恰好关闭已记录的残留入口，且此后经 review 循环进一步加固（重试加上限、生成 id 跳过占用检查、钉住变异的测试）；未给 5 分仅因无法独立复跑多会话 daemon 端到端探针，以及探针标记出的两处小覆盖缺口。 上次以来的提交正是 review 机制应有的样子：第 1 轮的顾虑变成了生成 id 占用检查跳过与有上限的别名重试，第 4 轮在该提交上零发现。纵观整条线——#9535 因隐私边界而开、#9538 关闭前半并把这些入口记为已知遗留、本 PR 恰好关闭这份清单——依然是后续 PR 应有的样子。守卫全部偏向单会话现状，dead-session 绑定先验证后命名文件，新提交中的每个关键论断都对照代码核验过而非只看描述。维护者已在同一提交上批准、CI 全部完成且零失败，剩余保留意见已在 Stage 2 写明，均不阻塞。 CI 已全部完成（该提交上零失败），因此本次直接批准，不再走延迟批准。 — Qwen Code · qwen3.8-max Reviewed at |
…e mock newSessionConfig now binds the session debug-log context, so the worktree suite's hand-rolled @qwen-code/qwen-code-core mock needs the actual sessionIdContext export; also export SessionIdConflictError from the config.js mock so errors in the try block don't surface as mock access errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CI on Fixed in Local run after the fix: |
…-routing-residuals
|
Merged Checked the semantic overlap with main's Config-ownership refactor (#8100): both Local re-run after merge: |
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 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): chunk 1: none — all checks I planned completed within budget; core-package claims in the design doc were deliberately left to the chunks that own those files..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 1：none — all checks I planned completed within budget; core-package claims in the design doc were deliberately left to the chunks that own those files.。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const effectiveSessionId = | ||
| sessionId ?? (preserveIdlessSession ? undefined : randomUUID()); |
There was a problem hiding this comment.
[Critical] Pre-generating the session UUID here (to bind the new sessionIdContext debug context) routes an internally generated ID through argv.sessionId into loadCliConfig's caller-supplied-ID occupancy check (findSessionIdIgnoringCase) — a branch that previously never ran for ID-less session/new requests. Two consequences, both confirmed by probing the real loadCliConfig + SessionService (no mocks) at this commit. (1) Every ID-less session creation now performs two readdirs (active + archive chats dirs) plus a per-entry scan on the session-creation hot path, scaling with accumulated session files on a long-lived daemon. (2) On any non-ENOENT readdir error — a transient EMFILE under fd pressure, an EACCES hitting only chats/archive while chats stays writable, EIO — the check fails closed (occupied = true), and since the daemon passes throwOnSessionIdConflict = true, newSession rejects with RequestError session_id_conflict and the message "Session Id already exists (active or archived). Delete or unarchive it first." — pointing operators at deleting a session that does not exist. Pre-diff, sessionId: undefined skipped the branch entirely, so the same FS error was harmless to ID-less creation. The fail-closed policy exists to protect caller-chosen IDs from case-only twins; a freshly generated UUIDv4 has no twin to protect, and the design doc lists "Changing session management behavior beyond debug-log ownership" as a non-goal.
Witness (probe through the real loadCliConfig + SessionService, isolated QWEN_HOME):
PR arm, chats readdir blocked, generated UUID:
thrown: SessionIdConflictError | Error: Session Id 2fe3ea58-08d5-4065-a9d1-14fd61542f7c already exists (active or archived). Delete or unarchive it first.
BASE arm, same blocked FS, argv.sessionId undefined (pre-diff shape):
Config constructed successfully
Flip (generated ids skip the occupancy check):
Config constructed; the 4 existing caller-id occupancy tests still pass
Fix direction: distinguish internally generated IDs from caller-supplied ones — e.g. mark argvForSession when the ID was generated here so loadCliConfig skips the occupancy check for it (keeping argv.sessionId semantics for caller-supplied IDs), or bind the context to the generated UUID while restoring sessionId: undefined in the argv handed to loadCliConfig.
中文说明
在此处预生成会话 UUID（用于绑定新的 sessionIdContext 调试上下文）会把内部生成的 ID 经由 argv.sessionId 送入 loadCliConfig 的"调用方提供 ID"占用检查（findSessionIdIgnoringCase）——该分支此前在无 ID 的 session/new 请求中从不执行。在本提交上用真实的 loadCliConfig + SessionService（无 mock）探针验证，有两个后果：（1）每次无 ID 的会话创建现在都会在会话创建热路径上执行两次 readdir（active 与 archive 的 chats 目录）并逐条扫描，在长期运行的守护进程上成本随累积的会话文件数增长；（2）任何非 ENOENT 的 readdir 错误——fd 紧张时的瞬时 EMFILE、仅 chats/archive 触发 EACCES 而 chats 仍可写、EIO——都会使检查失败关闭（occupied = true），而守护进程传入 throwOnSessionIdConflict = true，于是 newSession 以 RequestError session_id_conflict 拒绝，并给出误导性信息 "Session Id <刚生成微秒级的 uuid> already exists (active or archived). Delete or unarchive it first."——让运维去删除一个并不存在的会话。改动前 sessionId: undefined 完全跳过该分支，同样的 FS 错误对无 ID 创建没有影响。失败关闭策略是为了保护调用方选定的 ID 免受仅大小写不同的孪生 ID 侵害；刚生成的 UUIDv4 没有需要保护的孪生，且设计文档明确把"超出调试日志归属的会话管理行为变更"列为非目标。
修复方向：区分内部生成的 ID 与调用方提供的 ID——例如在 ID 为内部生成时对 argvForSession 加标记，让 loadCliConfig 对其跳过占用检查（保留 argv.sessionId 对调用方提供 ID 的语义）；或将上下文绑定到生成的 UUID，同时在传给 loadCliConfig 的 argv 中恢复 sessionId: undefined。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const updated = await doUpdateLatestDebugLogAlias(sessionId); | ||
| if (!updated && aliasGeneration === generation) { | ||
| lastAliasedKey = null; |
There was a problem hiding this comment.
[Suggestion] The new retry clears the lastAliasedKey dedup marker on every failed alias update, but nothing bounds that retry under persistent symlink failure — Windows without Developer Mode/symlink privilege (the exact case updateSymlink's fallbackCopy option documents, and which the debug alias opts out of with fallbackCopy: false), or a mount that forbids symlinks. There the readlink verification fails on every attempt, the marker is cleared every time, and every subsequent debug-log write re-schedules a doomed unlink + symlink + readlink cycle on the serialized aliasChain. Pre-diff the sticky marker limited this to one doomed attempt per session (per key); post-diff it is per log line for as long as opt-in debug logging is enabled. The alias is best-effort and the trigger is opt-in, hence Suggestion rather than higher.
Witness (probe with persistent symlink/readlink rejection, QWEN_DEBUG_LOG_FILE=1, three extra writes): PR code goes {"symlink":1,"unlink":1,"readlink":1} → {"symlink":4,"unlink":4,"readlink":4}; restoring the pre-PR sticky marker keeps it at 1.
Fix direction: cap consecutive alias-update failures (stop clearing lastAliasedKey after N consecutive failures, reset on success), or distinguish permanent symlink failure (EPERM/EACCES) from a transient verification mismatch and never retry the former.
中文说明
新的重试逻辑在每次别名更新失败时清除 lastAliasedKey 去重标记，但在持续性符号链接失败的场景下该重试没有上限——未开启开发者模式/无符号链接权限的 Windows（正是 updateSymlink 的 fallbackCopy 选项所记录的场景，而调试别名已用 fallbackCopy: false 明确禁用）、或禁止符号链接的挂载点。在这些环境下 readlink 校验每次都失败，标记每次都被清除，之后每条调试日志写入都会在串行化的 aliasChain 上重新调度一次注定失败的 unlink + symlink + readlink。改动前粘性标记把代价限制为每会话（每 key）一次注定失败的尝试；改动后在开启调试日志期间变成每条日志一次。别名本身是尽力而为、触发条件又是可选开启，因此评为 Suggestion 而非更高。
修复方向：限制连续的别名更新失败次数（连续失败 N 次后不再清除 lastAliasedKey，成功时重置计数）；或区分永久性符号链接失败（EPERM/EACCES）与瞬时校验不一致，对前者永不重试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it('keeps process-scoped cleanup outside session contexts', async () => { | ||
| const firstDir = path.join(qwenHome, 'first'); | ||
| const secondDir = path.join(qwenHome, 'second'); |
There was a problem hiding this comment.
[Suggestion] The change spans two sites guarded with sessionIdContext.exit — startNonInteractiveOpenAILogHousekeeping (scheduler.ts:197) and drainNonInteractiveQueue — but this test observes the context only inside the mocked cleanupOldOpenAILogs, which runs solely in the drain path. Deleting the start-side exit keeps the suite green (verified: the mutant passes 13/13). If that exit were dropped by a future edit and getOpenAILogCleanupTarget then threw inside the spawning session's sessionIdContext.run (the production shape after this PR — housekeeping is started from newSessionConfigInRuntimeContext), the debugLogger.error('failed to start non-interactive OpenAI log cleanup; skipping') line would route into the spawning session's debug file instead of the bootstrap log — the exact misrouting class this PR eliminates — and no test would fail. Probe evidence: with the start-side mutant the error line physically lands in session-a.txt; real code creates no such file.
Fix direction: add a start-side assertion — force a start failure inside sessionIdContext.run('session-a', …) (e.g. a makeConfig whose getModelsConfig throws), capture sessionIdContext.getStore() in a spied debugLogger.error implementation, and expect undefined.
中文说明
本次改动有两处用 sessionIdContext.exit 保护的站点——startNonInteractiveOpenAILogHousekeeping（scheduler.ts:197）与 drainNonInteractiveQueue——但该测试只在 mocked 的 cleanupOldOpenAILogs 内部观察上下文，而它只在 drain 路径上运行。删除启动侧的 exit 后整套测试仍然全绿（已验证：变异体 13/13 通过）。如果未来某次编辑删掉了启动侧的 exit，且 getOpenAILogCleanupTarget 在派生会话的 sessionIdContext.run 内抛出（本 PR 之后的生产形态——housekeeping 从 newSessionConfigInRuntimeContext 中启动），debugLogger.error('failed to start non-interactive OpenAI log cleanup; skipping') 这行日志就会路由进派生会话的调试文件而非引导日志——正是本 PR 要消除的错误路由类别——而且没有任何测试会失败。探针证据：启动侧变异体下该错误行真实落入 session-a.txt；真实代码不会产生该文件。
修复方向：补充启动侧断言——在 sessionIdContext.run('session-a', …) 内强制启动失败（例如让 makeConfig 的 getModelsConfig 抛错），在 spy 的 debugLogger.error 实现中捕获 sessionIdContext.getStore()，并断言其为 undefined。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(fs.copyFile).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('retries the latest alias after a failed update', async () => { |
There was a problem hiding this comment.
[Suggestion] Two mutations of the new marker-clear logic ship green: (a) dropping the aliasGeneration === generation guard lets a stale failed update clear a newer session's dedup marker; (b) clearing lastAliasedKey unconditionally after a completed update — even a successful one — passes every test, because no test writes to the same session after a successful (re)try and asserts the alias is NOT re-run (this retry test stops at toHaveBeenCalledTimes(2)). The production logic itself is correct — interleaved A/B and reset-race sequences were traced and hold — it is just unpinned: both mutants pass the existing suite 37/37, and the extension tests sketched below fail both mutants ("expected "spy" to be called 2 times, but got 3 times") while passing on the real code.
Fix direction: extend this retry test — after the successful retry, emit one more createDebugLogger().info(...) + vi.runAllTimersAsync() and expect fs.symlink still called exactly twice; add an interleaved case where session B's update is enqueued before A's deferred failure resolves, then assert B's next write does not re-run symlink (marker preserved by the generation guard).
中文说明
新的标记清除逻辑有两个变异可以在测试全绿的情况下存活：（a）去掉 aliasGeneration === generation 保护后，一个迟到的失败更新可以清除更新会话的去重标记；（b）在一次已完成的更新（哪怕是成功的更新）之后无条件清除 lastAliasedKey，也能通过所有测试——因为没有测试在成功（重）试之后再次写入同一会话并断言别名没有被重新执行（下面这个重试测试停在 toHaveBeenCalledTimes(2)）。生产逻辑本身是正确的——交错 A/B 与 reset 竞态序列都已推演验证成立——只是没有被测试钉住：两个变异体在现有测试套件下 37/37 全绿，而下面概述的扩展测试能让两个变异体都失败（"expected "spy" to be called 2 times, but got 3 times"），在真实代码上则通过。
修复方向：扩展该重试测试——在成功重试之后再发一条 createDebugLogger().info(...) + vi.runAllTimersAsync()，断言 fs.symlink 仍只被调用两次；再补一个交错场景：在 A 的延迟失败解决之前先把 B 的更新入队，然后断言 B 的下一次写入不会重新执行 symlink（去重标记由 generation 保护）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…bounded alias retry, mutation-pinning tests - R1-1 (critical): a daemon-generated session UUID no longer routes through loadCliConfig's caller-id occupancy check. The new CliArgs.sessionIdGenerated flag skips the check for ids minted only to bind the debug-log context, so id-less session creation loses the two per-request readdirs and can no longer fail closed into a spurious session_id_conflict on transient FS errors. Caller-supplied ids keep the check (both directions pinned by tests). - R1-2: latest-alias retries are bounded by a consecutive-failure streak (cap 3, reset on success). Hosts where symlinks never succeed fall back to the pre-retry one-attempt-per-session-change behavior instead of re-running a doomed unlink/symlink cycle per debug line. - R1-3: added a start-side housekeeping test that forces a target-resolution failure inside sessionIdContext.run and asserts the start body observes no session context (mutation-verified: deleting the start-side exit fails it). - R1-4: extended the alias retry test past the successful retry (marker must survive success) and added a stale-failure interleaving test (generation guard must protect a newer session's marker). All three mutants named in the review now fail their targeted test. Verification: debugLogger 39/39, core config 559/559, cli config 351/351, acpAgent 467/467, worktree 3/3, scheduler 14/14; mutation matrix re-run locally (4 mutants, each killed by its named test); typecheck/ESLint/Prettier clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Round-1 findings addressed in
Local mutation matrix re-run: all four mutants (occupancy-flip excluded as it's now the shipped behavior; generation-guard drop, unconditional clear, streak-cap drop, start-side exit drop) each fail their named test and the suites return green on the real code. Full runs: debugLogger 39/39, core config 559/559, cli config 351/351, acpAgent 467/467, worktree 3/3, scheduler 14/14. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit jobs were skipped in CI; the alias symlink logic is platform-sensitive and was only tested on Linux.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/config/config.ts:2272 — [review] Fallback-claim guard duplicated across the constructor and rotation pathspackages/core/src/utils/debugLogger.ts:193 — [test] readlink mismatch branch untested — return-true mutant survives
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit jobs were skipped in CI; the alias symlink logic is platform-sensitive and was only tested on Linux。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Best-effort; don't degrade overall logging | ||
| } | ||
| if (updated) { | ||
| aliasFailureStreak = 0; |
There was a problem hiding this comment.
[Suggestion] R2-1: The documented "a single success resets the streak" behaviour is not pinned by any test — deleting aliasFailureStreak = 0; survives the whole suite (mutation probe: 39/39 tests still pass with the line deleted). Every added alias test isolates failures from successes, so the reset is never observed. Concrete sequence no test runs: session A fails once (streak 1), A's retry succeeds, then a new session B fails twice — with the reset, B's failures land at streak 1 and 2 (< 3) and B gets a third attempt; with the line deleted, B's second failure reaches streak 3 and the marker goes sticky one failure early, so on a host with intermittent symlink failures the latest alias stops being retried sooner than the design doc and the code comment promise.
Witness (mutation probe in a scratch tree, vitest on debugLogger.test.ts):
delete `aliasFailureStreak = 0;` → 39/39 tests still pass (mutant survives)
suggested sequence added, under the mutant → AssertionError: expected "spy" to be called 5 times, but got 4 times
mutant reverted → 40/40 pass
Fix direction: extend the retry test — fail → succeed → session change → fail twice, and assert the marker still clears (a third fs.symlink attempt is scheduled), pinning the streak reset.
中文说明
文档中声明的「一次成功即重置连续失败计数」行为没有任何测试钉住——删除 aliasFailureStreak = 0; 后整个测试套件仍然全绿（变异探针：删除该行后 39/39 测试依旧通过）。所有新增的别名测试都把失败与成功隔离开，因此重置行为从未被观察到。没有任何测试覆盖的具体序列：会话 A 失败一次（streak 1），A 的重试成功，随后新会话 B 连续失败两次——有重置时，B 的两次失败落在 streak 1 和 2（< 3），B 能获得第三次尝试；删除该行后，B 的第二次失败就达到 streak 3，去重标记提前一次变为粘性，于是在符号链接间歇性失败的宿主上，latest 别名停止重试的时机会早于设计文档和代码注释的承诺。
证据（在临时树中用变异探针、对 debugLogger.test.ts 运行 vitest）：
删除 `aliasFailureStreak = 0;` → 39/39 测试仍全绿（变异体存活）
补上建议的序列后，在变异体下 → AssertionError: expected "spy" to be called 5 times, but got 4 times
恢复被删除的行 → 40/40 通过
修复方向：扩展重试测试——失败 → 成功 → 切换会话 → 连续失败两次，断言去重标记仍被清除（第三次 fs.symlink 尝试被调度），从而钉住成功重置计数的行为。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…branch Round-2 R2-1: the documented "a single success resets the streak" behavior had no observing test — deleting the reset survived the suite. The new sequence (A fails, A's retry succeeds, B fails twice, B retries) asserts the fifth symlink attempt that only happens with the reset in place. B's first failure arrives via a readlink that resolves to the wrong target, so the same test also kills the return-true mutant of the verification branch (the round-2 deferred note). Mutation-verified: streak-reset deletion and readlink return-true each fail this test; real code passes 40/40. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
R2-1 addressed in Two birds: B's first failure arrives via a readlink that resolves to the wrong target, so the same test pins the verification mismatch branch — the round-2 deferred Mutation-verified locally: deleting On the other deferred note (fallback-claim guard duplicated across constructor and rotation): left as-is deliberately — the two sites carry different context comments and a shared helper for two one-line guards felt like abstraction without payoff. Happy to extract it if you'd prefer. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/debugLogger.test.ts:570 — [review] Streak-cap recovery untested — circuit-breaker mutant survives all 40 tests
中文说明
无阻断问题。LGTM！✅
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Gentle ping: this has been sitting with the bot's approval (round 3, zero fresh findings, pinned to 温和提醒：本 PR 自 8 月 25 日起已获 bot 批准（第 3 轮零新发现）且 CI 全绿，内容是 #9538 评审记录在案的遗留项收尾。有空时恳请第二位 reviewer 过目，谢谢！ |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
…ve acpAgent.test.ts conflict: keep both the sessionIdContext binding test and main's standalone-restore tests)
|
Merged Post-merge verification: @wenshao conflict is resolved — ready for another look. 谢谢! |
|
@qwen-code /resolve |
…the workspace-mcp-discovery sessionIdContext wrapper, adopt main's skipLlmInitialization rename)
|
Merged Post-merge verification: core @wenshao sorry for the churn — main is moving fast under this one. If it helps, I'm watching the PR and will re-resolve within hours if it conflicts again. 谢谢！ |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
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 reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit jobs were skipped in CI; the alias symlink logic is platform-sensitive and was only tested on Linux.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/config/config.test.ts:644 — [probe] Rotation fallback branch untested — deletion mutant survives all 579 testspackages/cli/src/services/housekeeping/non-interactive-scheduler.test.ts:175 — [probe] Drain-side sessionIdContext.exit wrapper mutation-unreachable
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit jobs were skipped in CI; the alias symlink logic is platform-sensitive and was only tested on Linux。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Note on ordering: the 时间线说明：上面那条 /resolve 失败消息是在冲突已被手动合并（ |
|
@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: 60 passed · 0 failed · 60 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：60 通过 · 0 失败 · 60 总计 抖动门： Verification reportPR 9930 Deep Verification — fix(core): stop daemon Configs and background work from hijacking the debug-log sessionVerdict: 中文摘要结论：
Central claim + A/BCentral claim: in a multi-session daemon process, one session's debug lines no longer land in another session's debug file — Config construction/rotation no longer hijack the process-wide fallback, background callbacks re-bind their owning session, and the Every cell below drives the real compiled production modules (no stubs of code under test) against a real on-disk debug dir under a scratch
A/B harnesses: Cell 5 (acpAgent-level wrapping) — targeted fallback, justified: CorrectionsNone needed for prior review rounds. One factual note for readers of the description: the observed suite counts at the merged head are higher than the numbers quoted in the PR body (debugLogger 40 vs 37/39; core config 579 vs 552/559; acpAgent 513 vs 467; cli config 359 vs 351; scheduler+worktree 17 vs 13/14) — the final merge ( FindingsNone blocking. Two worried-shapes were probed and do not hold:
Also verified and clean: Mutation matrix (vacuity / pinning)Each row: revert the guard in source, run the suite that should catch it at HEAD, observe red, restore. Witness for M2 and M5:
No survivors. Every guard the PR introduces is pinned by a test that fails with the expected behavioral assertion (not an import/compile break — failure messages quote expected-vs-actual values), and the unmutated suites are green (below), so neither side of the matrix is vacuous. Targeted gates (all proven live first)Liveness: ESLint caught a planted
Repo-wide test/lint/build gates were not run (targeted scope; the PR's own CI covers them). Not covered
MethodologyOne container (the CI verify lane's own Flakiness gate logEvidence images
Harness 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. ✅
4 participants
What this PR does
Follow-up to #9538 (relates to #9535). That PR made
debugLoggerconsultsessionIdContextbefore the process-wide session, but only where a context was already bound. This PR closes the remaining entry points that ran without one, so a multi-session daemon no longer leaks one session's debug lines into another session's file:rotateSessionIdonly claim the process-wide debug-log fallback when nosessionIdContextis activeconfig.tssessionIdContext; transcript replay inherits the target session's validated contextacpAgent.tsacpAgent.tsdeleteSession/renameSessiondispatch binds the validated target session id even without a liveSessionobject in this processacpAgent.tssessionIdContext— its queue is process-scoped (deduped by log dir), so its logs stay in the bootstrap logscheduler.tslatest-alias dedup marker is cleared when an alias update fails, so the next write retries instead of leavinglateststale; stale docstrings updateddebugLogger.tsWhy it's needed
#9538's round-6 review recorded dead-session dispatch as a known deferral, and the same review noted the remaining fallback-hijack window: any Config construction still overwrote the process-wide debug session, so a background callback (MCP budget events, housekeeping timers) firing between session B's Config creation and its first bound turn wrote session A's lines into B's file — the same privacy edge #9535 was opened for (debug logs can carry prompt snippets, tool arguments, and file paths).
Reproduction (fix #1/#3, real filesystem, production modules)
A probe using the unmodified production
debugLogger+Storageagainst a real on-disk debug dir, replayingmain's exact callsite shapes:setDebugLogSession(A)— session A's Config is constructed (config.tsdoes this unconditionally onmain).debugLogger.debug(...)with no context wrapper — the literal shape ofmain's MCP budget callback inacpAgent.ts.setDebugLogSession(B)— session B's Config is constructed and overwrites the process-wide session.Result on
main's shape: the marker (text sayssession=A) physically lands in B's debug file, zero lines in A's. Wrapped insessionIdContext.run(A, ...)— this PR's shape — the same marker lands in A's own file with zero leakage into B's.Risk & Scope
sessionIdContextbound, Config claims the process-wide fallback exactly as before.docs/design/2026-08-24-debug-log-session-routing-residuals.mdVerification
debugLogger.test.ts37/37,config.test.ts552/552,acpAgent.test.ts467/467,non-interactive-scheduler.test.ts13/13 — each fix has a dedicated regression testmain's callsite shape, fix routes correctlytsc --noEmitclean on the changed files; ESLint + Prettier clean; pre-commit hooks green中文摘要
#9538 的后续修复（关联 #9535）。上一个 PR 让
debugLogger在回退到进程级会话前优先使用sessionIdContext，但只覆盖了已绑定上下文的路径。本 PR 关闭其余未绑定上下文的入口：rotateSessionId仅在无sessionIdContext时才占用进程级回退，创建/轮换 session B 不再劫持 session A 在途后台日志；sessionIdContext内运行；deleteSession/renameSession分发绑定已验证的目标会话 id；sessionIdContext（队列按日志目录去重，属进程级）；latest别名更新失败后清除去重标记以便重试，并更新过时注释。验证：四个测试文件全绿（37/552/467/13），真实文件系统探针复现 bug 并确认修复路由正确，typecheck/ESLint/Prettier 干净。
🤖 Generated with Claude Code