feat(channels): bound session lifetime with sessionRotation - #8927
feat(channels): bound session lifetime with sessionRotation#8927qwen-code-dev-bot wants to merge 13 commits into
Conversation
A channel route reuses its session forever, so a long-lived route grows until it passes the model's context window — after which every message on that route fails while the rest of the channel keeps working. Add a per-channel `sessionRotation` option with `maxTurns` and `maxAgeHours` bounds. When a route's session is past a bound, the next message starts a fresh session on it. Counters persist alongside the routes so a daemon restart cannot reset them, and channels without a bound configured skip the bookkeeping entirely, keeping their on-disk route shape and per-message write behavior unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
review/self-reported
|
Thanks for the quick iteration! Template looks good ✓ Problem: observed, not theoretical. Linked issue #8926 documents the incident — a Direction: aligned. Chat-channel routes have no natural end, an opt-in per-channel bound is the right knob, and a token-based bound stays out of scope honestly (it would need a bridge capability that doesn't exist today). Size: cross-package ( Approach: the previous blocker got fixed the better of the two ways: instead of adding caller-side wiring, rotation registration moved into the Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢快速迭代！ 模板完整 ✓ 问题：真实观测，不是理论假设。关联 issue #8926 记录了事故—— 方向：对齐。聊天频道路由没有自然终点，按频道可选配置限度是正确的开关；基于 token 的限度诚实地留在范围外（需要当前不存在的 bridge 能力）。 规模：跨包改动（ 方案：上一个阻塞项用了更好的方式修复：注册不是加在调用方，而是移进了 风险：无升级风险信号——改动文件均不命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-review at the new head. Before reading the diff I'd have fixed last run's blocker with one line of caller-side wiring; the PR did the structurally better thing instead — rotation registration now lives in the The machinery added since the last review reads clean under tracing:
Non-blocking notes:
TestingUnattended CI run — per policy I don't build or execute PR code; the evidence below is the PR's own CI on the reviewed commit, fetched via API, plus the maintainer's real-stack verification report. No TUI surface in this PR, so no tmux lane.
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
Beyond the suite, @wenshao ran a two-sided real-stack verification at exactly this commit (his report above): a real extension-loaded channel against a recording model server plus an ACP wire tap, standalone and daemon legs — all twelve behavioral claims held, including the ones this review traces statically (rotation at the bound, per-route isolation, restart-persisted counters, in-place upgrade of pre-rotation stores, deferral under a running turn, discard of the retired session). That also closes last run's open gap: the standalone leg drove 中文说明代码审查（按新 head 复审）：读 diff 之前，我本来打算用一行调用方接线修复上次的阻塞项；PR 选了结构上更好的做法——轮换注册移入 上次审查之后新增的机制经追踪是干净的：
非阻塞提醒：
测试：无人值守 CI 运行——按策略不构建、不执行 PR 代码；以上证据来自 PR 自身在受审 commit 上的 CI（经 API 获取）及维护者的真实链路验证报告。本 PR 无 TUI 面，因此没有 tmux 环节。 套件之外，@wenshao 恰在此 commit 上做了双向真实链路验证（见其报告）：扩展加载的真实频道 + 记录型模型服务 + ACP 线协议探针，覆盖独立腿与守护进程腿——十二条行为声明全部成立，包括本次静态审查追踪到的各项（到限轮换、按路由隔离、计数跨重启持久化、旧存储原地升级、回合中推迟、退役会话回收）。这也关闭了上次运行的缺口：独立腿实际驱动了 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — approve. The blocker from the last round is fixed the structurally better way, the new retirement machinery traces clean, CI is green, and a maintainer verified the behavior end to end on this exact commit. Stepping back: my independent sketch of this fix was caller-side wiring; the PR's answer — move registration into What keeps this at 4 rather than 5 is the residue, all non-blocking: the restart-defers-age-only-bound gap @wenshao measured (his suggested one-line On verification: the suite pins the feature at both layers (router semantics and channel wiring), and the maintainer's two-sided real-stack run on this commit — recording model server, ACP tap, standalone and daemon legs — held all twelve behavioral claims, including the standalone launch mode that sank the previous revision. CI is fully settled on this commit (no pending runs), and @wenshao's approval already stands on it; the approval below is pinned to the reviewed commit and supersedes my earlier change request. 中文说明置信度：4/5 —— 批准。上一轮的阻塞项以结构上更好的方式修复，新的退役机制经追踪无问题，CI 全绿，且维护者已在此 commit 上端到端验证了行为。 退一步看：我对这个修复的独立设想是调用方接线；PR 的答案——把注册移进 停在 4 而不是 5 的原因是遗留项，均不阻塞：@wenshao 实测出的"重启可推迟纯年龄限度"缺口（他建议的一行 验证方面：套件在路由器语义与频道接线两个层面钉住了功能；维护者在此 commit 上的双向真实链路运行——记录型模型服务、ACP 探针、独立腿与守护进程腿——十二条行为声明全部成立，包括曾让上一版折戟的独立启动模式。CI 在此 commit 上已完全收敛（无 pending 运行），@wenshao 的批准已在该 commit 上；下方的批准锚定在受审 commit，并取代我此前的修改请求。 — 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.
@qwen-code-dev-bot One fix needed before this can land: sessionRotation is never registered in single-channel mode — startSingle in packages/cli/src/commands/channel/start.ts passes a router it never calls setChannelRotation on, so the bound silently does not apply there (and the ChannelBase self-registration skips that path because a router is present). One line plus a test; full details in my review comment above. 🙏
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8927Feedback points and dispositions1. [CHANGES_REQUESTED] Confirmed:
2. Failed check: Reproduced the unit suite locally (clean
The full verification stack ( Conflict notesNone — VerificationCommands actually run on the updated tree (clean CI-equivalent environment: fresh
中文说明Autofix 审查轮次 — PR #8927反馈点及处理1. [CHANGES_REQUESTED] 单频道模式下 已确认：
2. 失败检查： 在本地复现了单测套件（先干净
完整校验栈（ 冲突说明无 — Verification（验证记录）在更新后的代码树上实际执行的命令（干净的 CI 等价环境：全新
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: AutoFix blocked: cannot attribute red
|
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): This PR adds a per-channel sessionRotation config optio...: none — all checks I started completed within budget.; This PR adds a per-channel sessionRotation config optio...: none — all checks I started were completed. Note: I did not fetch the PR's existing comment thread (not in scope of my reads); if an unresolved Critical exists …; This PR adds a per-channel sessionRotation config optio...: none — all checks I started completed within budget..
Test Plan (not a blocker): 158 tests passed — this review observed 1023, 290, 19069, 297, 266, 205, 59, 17, 134, 71 passed.
中文说明
未探索到全部深度（达到工具调用预算）：This PR adds a per-channel sessionRotation config optio...：none — all checks I started completed within budget.；This PR adds a per-channel sessionRotation config optio...：none — all checks I started were completed. Note: I did not fetch the PR's existing comment thread (not in scope of my reads); if an unresolved Critical exists …；This PR adds a per-channel sessionRotation config optio...：none — all checks I started completed within budget.。
Test Plan（非阻断）：158 tests passed — this review observed 1023, 290, 19069, 297, 266, 205, 59, 17, 134, 71 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| private rotateRoute( | ||
| key: string, | ||
| sessionId: string, | ||
| channelName: string, | ||
| ): void { | ||
| this.invalidateRouteOperation(key); | ||
| this.deleteByKey(key); | ||
| this.persist(); |
There was a problem hiding this comment.
[Critical] Rotation retires a live session without this codebase's retirement protocol: rotateRoute drops the route but never calls bridge.discardSession() and never notifies ChannelBase, so every rotation permanently leaks one live session. Every other retirement path discards: /clear and timed-out loops call discardRetiredSession (ChannelBase.ts:2962, :2107-2112), and the router's own invalidated-creation path calls scheduleDiscardInvalidatedSession → bridge.discardSession (~:1052-1058). Nothing reclaims a rotated session: the daemon's idle reaper is structurally disabled while the channel bridge keeps its SSE pump subscribed (DaemonChannelBridge.attachSession → pumpEvents), and on the ACP path only discardSession closes a child session. Probe-confirmed on unmodified code: discardSession calls after rotation = [].
Failure scenario: a busy route with maxTurns: 200 on a long-running gateway (the deployment this feature targets) leaks one full agent session holding up to 200 turns of context per 200 messages, plus orphaned ChannelBase state (instructedSessions, unattendedMemorySessions, pending permissions) → unbounded memory growth until the daemon OOMs and every channel on it goes down. Fractional bounds (maxTurns: 0.5 passes both parsers) rotate every message.
Suggested fix: retire through the existing machinery — best-effort bridge.discardSession(sessionId) from rotateRoute (mirroring scheduleDiscardInvalidatedSession), and surface the rotated ID to ChannelBase so it runs the same per-session purge onSessionDied performs. Sequence with the companion in-flight-turn finding: cancel/drain first, the way /clear does.
中文说明
严重：轮换在没有走本代码库退役协议的情况下退役了一个存活会话：rotateRoute 删除了路由，但从未调用 bridge.discardSession()，也从未通知 ChannelBase，因此每次轮换都会永久泄漏一个存活会话。所有其他退役路径都会 discard：/clear 与超时 loop 调用 discardRetiredSession（ChannelBase.ts:2962、:2107-2112），路由器自己作废创建的路径也会调用 scheduleDiscardInvalidatedSession → bridge.discardSession（~:1052-1058）。被轮换掉的会话没有任何回收者：daemon 的空闲回收器在 channel bridge 保持 SSE 事件泵订阅期间被结构性禁用（DaemonChannelBridge.attachSession → pumpEvents）；ACP 通路上只有 discardSession 会关闭子进程会话。已在未修改代码上用探针确认：轮换后 discardSession 调用数为 []。
失败场景：繁忙路由配置 maxTurns: 200、网关长期运行（正是本功能的目标部署形态）→ 每 200 条消息泄漏一个保存着最多 200 轮上下文的完整 agent 会话，外加 ChannelBase 的孤儿状态（instructedSessions、unattendedMemorySessions、待处理权限请求）→ 内存无界增长直到 daemon OOM，拖垮其上所有频道。小数限度（maxTurns: 0.5 能通过两层解析器）会导致每条消息都轮换。
建议修复：走现有机制退役——在 rotateRoute 中尽力调用 bridge.discardSession(sessionId)（与 scheduleDiscardInvalidatedSession 一致），并把被轮换的会话 ID 上报给 ChannelBase，让它执行与 onSessionDied 相同的按会话清理。注意与配套的「回合进行中轮换」发现配合：先取消/排空，如同 /clear 的做法。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if ( | ||
| existing && | ||
| !this.creatingSessions.has(key) && | ||
| this.shouldRotate(channelName, existing) | ||
| ) { | ||
| this.rotateRoute(key, existing, channelName); | ||
| existing = undefined; | ||
| } |
There was a problem hiding this comment.
[Critical] Rotation can fire while the outgoing session still has an active turn, and deleteByKey removes the route out from under it. The creatingSessions guard only covers session creation; turn concurrency lives in ChannelBase's sessionQueues/activePrompts, keyed by session ID, which resolve() never consults. Once the route is gone: the running turn's tool-permission requests auto-cancel (permissionTargetForEvent → getTarget undefined → respondToPermission(..., 'cancelled'), ChannelBase.ts:516-528; daemon path: registerPermissionRelay cancels with "No route for session"), dispatchBackgroundResponse silently drops late output, and channelLoopToolHandler stops handling its tool calls. Meanwhile the successor message resolves to a fresh session whose queue is empty, so the new turn starts immediately, running concurrently with the still-running predecessor in the same chat and cwd.
Failure scenario: non-yolo channel at its bound — turn N is running when the next message arrives on the busy route → rotation → the running turn's next approval prompt is auto-cancelled without the user ever seeing it, and late output is dropped. With maxTurns: 1 (legal — the parser rejects only <= 0) every message rotates, so an allowed sender rapid-firing N messages gets N concurrent agent turns executing tools in one working directory — the per-session queue serialization the code documents is defeated.
Suggested fix: defer rotation while the outgoing session still has an active or queued turn (the same deferral already applied to in-flight creations), enforcing the bound on the next message — or route rotation through /clear's retirement protocol (cancel → purge → discard). At minimum, keep toTarget for a session that may still emit events.
中文说明
严重：轮换可能发生在旧会话仍有回合在执行时，而 deleteByKey 会把正在运行回合脚下的路由删掉。creatingSessions 守卫只覆盖会话「创建」；回合并发由 ChannelBase 的 sessionQueues/activePrompts 管理，且按「会话 ID」建键，而 resolve() 从不查询它们。路由被删除后：运行中回合的工具权限请求会被自动取消（permissionTargetForEvent → getTarget 返回 undefined → ChannelBase.ts:516-528 处 respondToPermission(..., 'cancelled')；daemon 通路：registerPermissionRelay 以 "No route for session" 取消）；dispatchBackgroundResponse 静默丢弃迟到的输出；channelLoopToolHandler 不再处理该会话的工具调用。同时，后一条消息解析到一个队列为空的新会话，其回合立即启动，与仍在运行的前一回合在同一聊天、同一 cwd 下并发执行。
失败场景：非 yolo 频道到达限度时——第 N 轮还在运行，繁忙路由上又来了一条消息 → 触发轮换 → 运行中回合的下一次审批提示在用户根本看不到的情况下被自动取消，迟到输出被丢弃。而且 maxTurns: 1 是合法配置（解析器只拒绝 <= 0），每条消息都会轮换，被允许的发送者连发 N 条就会得到 N 个在同一工作目录中并发执行工具的 agent 回合——代码文档中承诺的按会话队列串行被破坏。
建议修复：当旧会话仍有活动或排队中的回合时推迟轮换（与对进行中创建已有的推迟一致），在下一条消息上落实限度——或者让轮换走 /clear 的退役协议（取消 → 清理 → discard）。至少，不要删除可能仍在产生事件的会话的 toTarget。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const raw = rawConfig['sessionRotation']; | ||
| if (raw === undefined) return undefined; |
There was a problem hiding this comment.
[Suggestion] sessionRotation: null throws "must be an object" while every sibling optional-field parser in this file treats null as unset — parseWebhookConfig, parseApprovalModeConfig, and parseObjectStringFields (identity/memoryScope) all check === undefined || === null.
Failure scenario: a user writes "sessionRotation": null in settings.json — the natural way to disable an inherited/merged key in commentless JSON — and qwen channel start fails with a config error, while the four sibling parsers accept the same shape.
| const raw = rawConfig['sessionRotation']; | |
| if (raw === undefined) return undefined; | |
| const raw = rawConfig['sessionRotation']; | |
| if (raw === undefined || raw === null) return undefined; |
中文说明
建议：sessionRotation: null 会抛出 "must be an object"，而本文件中所有同级的可选字段解析器都把 null 视为「未设置」——parseWebhookConfig、parseApprovalModeConfig、parseObjectStringFields（identity/memoryScope）都检查 === undefined || === null。
失败场景：用户在 settings.json 中写 "sessionRotation": null（在没有注释的 JSON 中禁用继承/合并来的键的自然写法），qwen channel start 会因配置错误启动失败，而四个同级解析器都接受同样的写法。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (!options?.router) { | ||
| this.router.setChannelRotation(this.name, config.sessionRotation); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The invariant "whoever creates the router registers rotation" is enforced by conditional checks at five sites, with this conditional pasted in two of them (here and QQChannel) and coupled only by cross-referencing comments. QQChannel needed its mirror hunk inside this very PR because it always passes a router to super. Registration is idempotent and name-keyed, so one unconditional site is safe.
Failure scenario: the next channel subclass that constructs its own SessionRouter pre-super (the proven QQChannel pattern, e.g. for a custom sessionsPath) silently never rotates — the config parses, validates with a friendly error, appears in docs, and is ignored with no warning, reproducing exactly the context-window failure this feature targets. This bug class already blocked the PR once (the startSingle gap, fixed in this diff).
Suggested fix: register unconditionally in the ChannelBase constructor right after this.router = options?.router || new SessionRouter(...), and delete the QQChannel mirror (gateway re-registration sets the same parsed value), leaving one owner of the invariant.
中文说明
建议：「谁创建 router 谁注册轮换」这一不变量目前由五处条件检查共同维持，其中两处（此处与 QQChannel）是粘贴的条件分支，仅靠互相引用的注释耦合。QQChannel 之所以需要在本 PR 中加上镜像补丁，正是因为它总是把 router 传给 super。注册是按频道名幂等的，因此单处无条件注册是安全的。
失败场景：下一个在 super 之前自建 SessionRouter 的频道子类（QQChannel 已验证过的模式，例如为了自定义 sessionsPath）会静默地永不轮换——配置能解析、有友好的校验报错、文档里也有，却被无声忽略，最终复现的正是本功能要解决的上下文窗口故障。这类 bug 已经阻塞过本 PR 一次（startSingle 缺口，已在本 diff 中修复）。
建议修复：在 ChannelBase 构造函数中 this.router = options?.router || new SessionRouter(...) 之后无条件注册，并删除 QQChannel 的镜像分支（网关侧重复注册写入的是同一份解析后的值），让该不变量只有一个归属。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (!options?.router) { | ||
| router.setChannelRotation(name, config.sessionRotation); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Mirror of the ChannelBase self-registration conditional (see the companion comment on ChannelBase.ts). QQChannel always passes a router to super, so the base-class branch never fires for it — this duplicate exists purely to patch the split invariant, and it had to be added inside this very PR.
Failure scenario: same as the companion comment — the next self-router subclass that does not know to mirror this block silently never rotates, and every future change to registration semantics must be applied in both copies.
Suggested fix: consolidate to one unconditional registration in ChannelBase's constructor and delete this mirror.
中文说明
建议：这是 ChannelBase 自注册条件分支的镜像（参见 ChannelBase.ts 上的配套评论）。QQChannel 总是把 router 传给 super，因此基类分支对它永远不会触发——这份重复代码纯粹是为了修补被拆开的不变量，而且它是在本 PR 内才不得不加上的。
失败场景：与配套评论相同——下一个不知道该镜像此代码块的自建 router 子类会静默地永不轮换；未来任何注册语义的变更都必须同时改两处。
建议修复：合并为 ChannelBase 构造函数中的一处无条件注册，删除此镜像。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| this.toTurns.delete(sessionId); | ||
| this.toStartedAt.delete(sessionId); | ||
| this.liveSessionIds.delete(sessionId); | ||
| return sessionId; |
There was a problem hiding this comment.
[Suggestion] [test] Test-efficacy probe (harness validated): the toTurns/toStartedAt cleanup statements in this teardown path (deleteByKey — used by handleSessionDied, route eviction, and rotateRoute itself) survived a deletion mutant — no test fails when they are removed. The same gap exists in removeSessionId and dispose() — see the companion comments.
Failure scenario: with these deletes dropped, rotated/dead/evicted sessions leak their counter entries forever — self-reinforcing here, since this is the cleanup rotation itself uses: rotating more leaks more — and a re-registered session ID inherits a stale turn count or start time.
Suggested fix: add a test: register a session under a rotation bound, remove its route via this path, re-register the same ID and assert fresh counters (no immediate rotation).
中文说明
建议：[test] 测试有效性探针（harness 已验证）：此清理路径（deleteByKey——被 handleSessionDied、路由驱逐以及 rotateRoute 自身使用）中的 toTurns/toStartedAt 删除语句在删除变异下存活——移除它们后没有任何测试失败。同样的缺口存在于 removeSessionId 与 dispose()——见配套评论。
失败场景：删除语句被移除后，被轮换/死亡/驱逐的会话的计数器条目永久泄漏——在此处是自我强化的，因为这正是轮换自身使用的清理路径：轮换越多泄漏越多——被重新注册的会话 ID 还会继承陈旧的轮次或起始时间。
建议修复：补一个测试：在轮换限度下注册会话，经此路径删除其路由，再注册同一 ID，断言计数器从零开始（不会立即轮换）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| this.toTurns.clear(); | ||
| this.toStartedAt.clear(); |
There was a problem hiding this comment.
[Suggestion] [test] Test-efficacy probe (harness validated): the two clear() calls in dispose() survived a deletion mutant — no test asserts the rotation maps are emptied with the rest of the router state. The same gap exists in removeSessionId and deleteByKey — see the companion comments.
Failure scenario: with these clears dropped, every other map is emptied at teardown while toTurns/toStartedAt keep all entries, so any code that inspects or reuses the router after dispose sees pre-dispose counters — stale rotation state survives teardown.
Suggested fix: add a test asserting dispose() leaves rotation state empty (e.g. re-registration after dispose starts fresh counters), mirroring the existing cleanup assertions for the other maps.
中文说明
建议：[test] 测试有效性探针（harness 已验证）：dispose() 中的两个 clear() 调用在删除变异下存活——没有测试断言轮换 map 与路由器其余状态一起被清空。同样的缺口存在于 removeSessionId 与 deleteByKey——见配套评论。
失败场景：清空调用被移除后，销毁时其他所有 map 都被清空，而 toTurns/toStartedAt 保留全部条目，任何在 dispose 之后检查或复用该路由器的代码都会看到销毁前的计数器——陈旧的轮换状态在销毁后存活。
建议修复：补一个测试断言 dispose() 后轮换状态为空（如 dispose 后重新注册从零开始计数），与其他 map 现有的清理断言保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| this.promoteTargetToGroup(sessionId, isGroup); | ||
| this.countTurn(channelName, sessionId); | ||
| return sessionId; | ||
| } catch (error) { | ||
| if (creating.invalidationError) { |
There was a problem hiding this comment.
[Suggestion] [test] Test-efficacy hunk probe (harness validated): reverting this countTurn call on the concurrent-creation wait branch leaves every test green — this call site, unlike the other two, is ungated.
Failure scenario: if a future change drops this call, a message that resolves by awaiting an in-flight creation on the same route is never counted: under concurrent messages on one route (racing senders, group-chat bursts) the session is under-counted and keeps being reused past its bound — the limit silently becomes "N plus however many messages took the wait path".
Suggested fix: add a test with a small maxTurns that drives two concurrent resolve calls onto one route (one creates, one waits on the in-flight creation) and asserts the counter reflects both, so the next message rotates.
中文说明
建议：[test] 测试有效性 hunk 探针（harness 已验证）：还原并发创建等待分支上的这个 countTurn 调用后所有测试仍为绿——与另外两处不同，这个调用点没有被测试钉住。
失败场景：若未来变更删掉此调用，通过等待同路由进行中创建而完成解析的消息将永远不被计数：在同路由并发消息下（发送者竞速、群聊突发），会话计数偏低，复用会越过限度——限度静默变成「N 加上走了等待路径的消息数」。
建议修复：补一个小 maxTurns 的测试：对同一路由发起两个并发 resolve（一个创建、一个等待进行中的创建），断言计数器两者都计入，使下一条消息触发轮换。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| (rawConfig['sessionScope'] as ChannelConfig['sessionScope']) || | ||
| plugin?.defaultSessionScope || | ||
| 'user', | ||
| sessionRotation: parseSessionRotationConfig(name, rawConfig), |
There was a problem hiding this comment.
[Suggestion] sessionRotation was never registered in the daemon-managed channel settings validation (channel-settings-store.ts assertSharedField), unlike every sibling shared option. Probe-reproduced on unmodified code: the daemon upsert route rejects it with HTTP 400 channel_settings_invalid_config — Channel field "sessionRotation" is not manageable. — unless byte-identical to the stored value; adding the implied assertSharedField branch flips the probe.
Failure scenario: an operator adding or changing sessionRotation through the workspace channel management API (the Web Shell editor round-trips stored config) gets a hard 400 and must hand-edit settings.json instead, while the file-based paths (qwen channel start, daemon-worker) parse it fine — the feature works only on the surface this PR touched.
Suggested fix: add a sessionRotation branch to assertSharedField validating the object shape (positive-finite maxTurns/maxAgeHours, mirroring parseSessionRotationConfig), or declare it in the shared management descriptors.
中文说明
建议：sessionRotation 从未登记进 daemon 托管频道配置的校验（channel-settings-store.ts 的 assertSharedField），而所有同级共享选项都已登记。已在未修改代码上用探针复现：daemon upsert 路由会以 HTTP 400 channel_settings_invalid_config 拒绝——Channel field "sessionRotation" is not manageable.——除非与已存储值逐字节相同；加上相应的 assertSharedField 分支后探针翻转。
失败场景：运维通过工作区频道管理 API（Web Shell 编辑器会原样往返已存储配置）新增或修改 sessionRotation 时会得到硬性 400，只能手工编辑 settings.json；而文件路径（qwen channel start、daemon-worker）解析正常——该功能只在本 PR 触及的入口可用。
建议修复：在 assertSharedField 中增加 sessionRotation 分支，校验对象结构（正且有限的 maxTurns/maxAgeHours，与 parseSessionRotationConfig 一致），或在共享管理描述符中声明它。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| export interface SessionRotationConfig { | ||
| /** Route to a new session once this many messages have used the current one. */ | ||
| maxTurns?: number; |
There was a problem hiding this comment.
[Suggestion] SessionRotationConfig is referenced by the exported ChannelConfig and is the parameter type of the new public SessionRouter.setChannelRotation, but it is not re-exported from packages/channels/base/src/index.ts while its siblings are (ChannelConfig, SessionScope, BlockStreamingCoalesceConfig). In-repo consumers are unaffected; the cost is confined to external consumers, against the package's established export pattern. One-line fix.
Failure scenario: an extension/plugin author calling the public router.setChannelRotation(name, rotation) (extensions can supply their own router via ChannelBaseOptions.router) cannot name or import the bound's type from @qwen-code/channel-base and must redeclare it structurally.
// packages/channels/base/src/index.ts — add to the type re-export block:
SessionRotationConfig,中文说明
建议：SessionRotationConfig 被导出的 ChannelConfig 引用，也是新的公开方法 SessionRouter.setChannelRotation 的参数类型，却没有在 packages/channels/base/src/index.ts 中再导出，而其同级类型（ChannelConfig、SessionScope、BlockStreamingCoalesceConfig）都有。仓库内使用不受影响；代价只落在外部使用者身上，且违背该包既有的导出惯例。一行即可修复。
失败场景：扩展/插件作者调用公开的 router.setChannelRotation(name, rotation)（扩展可通过 ChannelBaseOptions.router 提供自己的 router）时，无法从 @qwen-code/channel-base 命名或导入该限度的类型，只能按结构重新声明。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
🤖 Addressed the latest review feedback (round 3/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/10 轮）。改动内容与我反驳保留之处如下： PR #8927 review feedback — round summaryAll 20 findings (2 Critical, 18 Suggestion) from the automated review were Critical findingsR1-1 — Rotation leaked every retired session (SessionRouter.ts)Fixed.
R1-2 — Rotation fired while the outgoing session had an active turn (SessionRouter.ts)Fixed. Rotation now defers while the outgoing session still has a turn
Suggestions
Conflict
VerificationCommands actually run (after the fix commit):
中文说明PR #8927 评审反馈 — 本轮总结自动评审的 20 条发现（2 条严重、18 条建议）均已对照代码核实，并在一个提交 严重发现R1-1 — 轮换泄漏了每一个被退役的会话（SessionRouter.ts）已修复。
R1-2 — 轮换在旧会话仍有活动回合时触发（SessionRouter.ts）已修复。 当旧会话仍有回合在运行或排队时，轮换推迟到下一条消息落实限度
建议
冲突
验证实际执行的命令（修复提交之后）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review triage — no action required (PR #8927)Triaged all feedback newer than the last evaluation (2026-08-11T16:21:48Z). This round contains nothing actionable:
No changes were made; the branch stays at its current head commit. 中文说明Autofix 评审分诊 — 无需处理（PR #8927）已对上次评估（2026-08-11T16:21:48Z）之后新增的全部反馈进行分诊。本轮没有任何需要处理的内容：
未做任何修改；分支保持在当前 head 提交。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Verification report — real-stack run of
|
| commit | |
|---|---|
| PR head | 6dbca59 (fix(channels): retire rotated sessions safely and harden rotation config) |
| merge-base | 7425e42 |
Around each build:
- A real channel. A
ChannelPluginloaded the documented way — an extension inQWEN_HOME/extensionswith achannelsentry inqwen-extension.json. It subclasses the tree's ownChannelBase, so the routing, gating and rotation code under test is the real thing; the only fake part is the transport (WebSocket to a local fake chat platform). It reports the session ID the router handed it for every turn, which is what makes rotation observable from the chat side. - A real launcher.
qwen channel start <name>/qwen channel start(all channels) for the standalone legs, andqwen serve --workspace … --channel probe-botfor the daemon legs — the deployment shape the reported DingTalk bug came from. - A recording model server. A local OpenAI-compatible server that logs every request's full
messagesarray and answers deterministically with what it can see:CONTEXT_USER_MSGS=<n> | SECRET=<value|NONE>. So the bot's own reply is the assertion — after a rotation it literally cannot seeSECRET-ALPHA1any more. Host-issued side queries (next-turn suggestions) are tagged and excluded from turn counts. - A tap on the ACP wire. A shim at
argv[1]re-spawns the real bundle for the--acpchild and tees the JSON-RPC in both directions, soqwen/control/session/closefor a retired session is visible as a raw protocol frame rather than inferred. - Isolated
QWEN_HOME+ workspace per leg; macOS 26.6, Node v24.18.1.
Results
| # | Claim | Result | Evidence |
|---|---|---|---|
| 1 | Unconfigured channels behave exactly as today | ✅ | merge-base: 6 messages, one session, prompt grows every turn. PR build, channel with no bound: 5 messages, one session, and its sessions.json entry has no turns/startedAt — byte-identical shape to before |
| 2 | maxTurns rotates at the bound |
✅ | maxTurns: 3 → msgs 1-3 on session 9c8e2f9f, msg 4 on 10d92d2f; the fresh session answers SECRET=NONE |
| 3 | Only the route that hit the bound rotates | ✅ | alice rotated while bob's route on the same channel kept 842e969a |
| 4 | A channel without a bound is unaffected by one that has it | ✅ | plain-bot on the same router, same process: 5 messages, one session, no counter fields |
| 5 | maxAgeHours rotates on elapsed time |
✅ | maxAgeHours: 0.0084 (~30 s): +5 s reused, +37 s rotated |
| 6 | An age-only bound costs no per-message write | ✅ | across 4 messages the store's mtime only moves at session creation and at rotation, never per message, and no turns key is ever written |
| 7 | Counters persist; a daemon restart cannot reset the bound | ✅ | daemon leg: turns=2 on disk → daemon killed → reboot restores the route (Restored 1 dormant route(s)) and the same session with its history (CONTEXT_USER_MSGS=3) → msg 4 rotates. Counter resumed at 3, not 1 |
| 8 | Stores written before this change load cleanly and start their clock at the next message | ✅ | in-place upgrade: merge-base daemon wrote a pre-PR entry (3 msgs), PR daemon restored it, kept serving the same session with full context, counted from 1 and rotated on the 3rd message after upgrade |
| 9 | Rotation is announced in the chat | ✅ | notice is delivered before the answer from the fresh session in every rotation observed |
| 10 | The retired session is actually discarded | ✅ | ACP frame qwen/control/session/close {sessionId: 9c8e2f9f-…} right after the rotation log line |
| 11 | Rotation defers while a turn is running | ✅ | maxTurns: 2, msg 2's turn held open for 24 s; msg 3 arrived mid-turn at the bound and reused the session, msg 4 rotated |
| 12 | Bad bounds are rejected at parse time | ✅ | maxTurns: 0, maxTurns: -5, maxAgeHours: "daily", sessionRotation: "daily" all exit 1 with a field-accurate message; sessionRotation: null and {maxTurns: 3} start normally |
Both suites named in the description also pass on the head tree here: packages/channels/base SessionRouter.test.ts + ChannelBase.test.ts → 707 passed; packages/cli config-utils + start + daemon-worker + channel-settings-store → 232 passed.
Evidence
Before / after on the same harness — the bot's reply is the assertion: after rotation it no longer knows SECRET.
Daemon-managed channel — restart persistence and an in-place upgrade over a pre-PR route store.
The age bound, the mid-turn guard, and config validation.
Observations (non-blocking)
1. The age clock of a route that predates the upgrade is memory-only, so restarts can defer an age-only bound indefinitely.
shouldRotate() stamps toStartedAt for a restored entry that has none, but does not persist it; with an age-only bound countTurn() returns early, so nothing else writes the store either. Every daemon boot therefore re-arms the clock for that route.
Measured: a route created by the merge-base build, then given maxAgeHours ≈ 30 s. Three boots spanning 74 s of wall clock — each boot shorter than the bound — never rotated, and startedAt never appeared on disk. The control boot that stayed up 35 s rotated as designed.
Scope is narrow — pre-PR route entries only, age-only bounds, and only until the first rotation (after which the new session's startedAt is seeded and persisted) — and a channel with more than one active route gets the value written out incidentally by another route's persist. But it is exactly the shape of the reported case: one long-lived thread, a daemon that restarts on deploys. A this.persist() next to the this.toStartedAt.set(sessionId, Date.now()) in shouldRotate() closes it, at the cost of one write per session, once.
2. Context, not a defect: in qwen channel start the route store is write-only.
startSingle/startAll never restore it at boot (restoreSessions() is reached only from bridge crash recovery), and clearAll() on SIGINT deletes the file outright. I confirmed this is identical on the merge-base build, so it is not from this PR — but it does mean the persistence guarantee applies to daemon-managed channels specifically, which is where I tested it (row 7). Worth keeping in mind if anyone reads "a daemon restart cannot reset a bound" as covering channel start too.
3. Not covered here. Carrying counters across a reload that returns a new session ID (the daemon's reload returned the same ID in every run I got), rotation skipped while a session creation is in flight, and the qwen serve channel-settings validation path — all three are covered by the unit tests, just not by this run.
中文版
验证报告 — sessionRotation 真实环境跑通
我把 PR 两侧都构建出来，各自跑了一条真实的频道链路，而不是只读测试。描述里的每一条行为声明都成立，包括第三个 commit 新增的部分（聊天内提示、退役会话回收、回合进行中推迟轮换）。文末两点观察值得看一眼，但都不阻塞合入。
结论：行为与描述一致，可以合入。 唯一建议先修的是观察 (1)，一行 persist() 即可。
怎么验的
两棵干净的树，各自 npm ci && npm run build && npm run bundle：PR head 6dbca59，merge-base 7425e42。
- 真实频道：按官方文档的方式，用扩展（
QWEN_HOME/extensions里带channels字段的qwen-extension.json）加载一个ChannelPlugin。它继承所在树自己的ChannelBase，所以被测的路由、门禁、轮换逻辑都是真的，只有传输层（连本地假聊天平台的 WebSocket）是假的。它会把路由器为每一轮分配的 session ID 一并上报，轮换因此在"聊天侧"可见。 - 真实启动方式：独立腿用
qwen channel start，守护进程腿用qwen serve --workspace … --channel probe-bot——也就是这个 bug 最初被发现的部署形态。 - 记录型模型服务：本地 OpenAI 兼容服务，把每次请求完整的
messages落盘，并按它实际看到的内容确定性作答：CONTEXT_USER_MSGS=<n> | SECRET=<值|NONE>。于是机器人自己的回复就是断言——轮换之后它确实看不见SECRET-ALPHA1了。ACP host 发起的旁路请求（下一句建议）会被标记并排除在回合统计外。 - ACP 线协议探针：在
argv[1]放一个 shim，--acp子进程由它转发真实 bundle 并双向抓取 JSON-RPC，因此退役会话的qwen/control/session/close是原始协议帧，而非推断。 - 每条腿独立的
QWEN_HOME与 workspace；macOS 26.6，Node v24.18.1。
结果
| # | 声明 | 结果 | 证据 |
|---|---|---|---|
| 1 | 不配置时行为与今天完全一致 | ✅ | merge-base：6 条消息、一个会话、prompt 逐轮增长。PR 构建下未配置限度的频道：5 条消息一个会话，sessions.json 条目没有 turns/startedAt，磁盘结构与改动前一致 |
| 2 | maxTurns 在限度处轮换 |
✅ | maxTurns: 3 → 第 1-3 条在 9c8e2f9f，第 4 条换到 10d92d2f，新会话回答 SECRET=NONE |
| 3 | 只有触达限度的路由轮换 | ✅ | alice 轮换时，同频道 bob 的路由仍是 842e969a |
| 4 | 未配置限度的频道不受影响 | ✅ | 同一路由器、同一进程里的 plain-bot：5 条消息一个会话，无计数字段 |
| 5 | maxAgeHours 按时间轮换 |
✅ | maxAgeHours: 0.0084（约 30 秒）：+5 秒复用，+37 秒轮换 |
| 6 | 纯年龄限度没有每条消息的写盘 | ✅ | 4 条消息期间 store 的 mtime 只在会话创建和轮换时变动，逐条消息不写，且从不写 turns |
| 7 | 计数持久化，守护进程重启不能重置限度 | ✅ | 守护进程腿：磁盘 turns=2 → 杀掉进程 → 重启后恢复路由（Restored 1 dormant route(s)）并带着历史复用同一会话（CONTEXT_USER_MSGS=3）→ 第 4 条轮换。计数从 3 继续，不是从 1 |
| 8 | 旧版本写的存储能干净加载，从下一条消息开始计时 | ✅ | 原地升级：merge-base 守护进程写下 pre-PR 条目（3 条消息），PR 守护进程恢复它、带完整上下文继续服务、从 1 开始计数，并在升级后第 3 条消息轮换 |
| 9 | 轮换会在聊天里发提示 | ✅ | 观察到的每次轮换，提示都先于新会话的回答送达 |
| 10 | 退役会话真的被回收 | ✅ | 轮换日志之后紧跟 ACP 帧 qwen/control/session/close {sessionId: 9c8e2f9f-…} |
| 11 | 回合进行中不轮换 | ✅ | maxTurns: 2，第 2 条的回合被挂住 24 秒；第 3 条在回合进行中到达且已达限度，复用了会话，第 4 条才轮换 |
| 12 | 非法限度在解析期报错 | ✅ | maxTurns: 0、maxTurns: -5、maxAgeHours: "daily"、sessionRotation: "daily" 均以 1 退出并给出字段级信息；sessionRotation: null 与 {maxTurns: 3} 正常启动 |
描述里点名的两个套件在本机 head 树上也全绿：packages/channels/base 的 SessionRouter.test.ts + ChannelBase.test.ts 共 707 条通过；packages/cli 的 config-utils + start + daemon-worker + channel-settings-store 共 232 条通过。
观察（不阻塞）
1. 升级前就存在的路由，其年龄时钟只存在内存里，反复重启可以无限期推迟纯年龄限度。
shouldRotate() 会给没有起始时间的恢复条目盖上 toStartedAt，但不持久化；而纯年龄限度下 countTurn() 直接返回，也没有别的地方写盘。于是每次守护进程启动都会把这个路由的时钟重新归零。
实测：先用 merge-base 构建产生一个路由，再配上 maxAgeHours ≈ 30 秒。三次启动横跨 74 秒真实时间（每次在线时长都短于限度），从未轮换，startedAt 也始终没落盘；作为对照，一次在线 35 秒的启动按预期轮换了。
适用范围有限——只影响改动前写下的路由条目、只在纯年龄限度下、且只到第一次轮换为止（之后新会话的 startedAt 会被写入）；另外，频道里若有多个活跃路由，这个值会被别的路由的写盘顺带带出去。但这恰好就是报告场景的形状：一个长期存在的 thread，加上会随发布重启的守护进程。在 shouldRotate() 里 this.toStartedAt.set(sessionId, Date.now()) 旁边补一次 this.persist() 即可，代价是每个会话多写一次盘。
2. 背景说明，不是缺陷：qwen channel start 的路由存储实际上只写不读。
startSingle/startAll 启动时从不恢复它（restoreSessions() 只在 bridge 崩溃恢复路径上被调用），并且 SIGINT 时 clearAll() 会直接删除该文件。我在 merge-base 构建上确认行为完全相同，所以这不是本 PR 引入的——但这意味着持久化保证具体是针对守护进程托管的频道，我也正是在那里验证的（第 7 行）。如果有人把"守护进程重启不会重置限度"理解成也覆盖 channel start，需要注意这一点。
3. 本次未覆盖：重载返回新 session ID 时计数的迁移（几次运行里守护进程重载都返回了同一个 ID）、会话创建在途时跳过轮换检查、以及 qwen serve 的频道设置校验路径——这三点单测有覆盖，只是本次真实链路没跑到。
|
@qwen-code /triage |
|
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: 59 passed · 1 failed · 60 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：59 通过 · 1 失败 · 60 总计 Verification reportPR #8927 Deep Verification —
|
| Cell | Scenario | Head | Base (control) |
|---|---|---|---|
| A | 5 msgs, one route, maxTurns: 2 |
[1,1,2,2,3] — 3 sessions, rotates at msg 3 and 5 ✅ |
1 session forever; no rotation API exists ✅ (expected-fail control) |
| B | sibling route under its bound (maxTurns: 3, 2 msgs) |
only the bounded route rotates; sibling keeps its session and is not discarded ✅ | — |
| C | unbounded channel next to bounded | unbounded: 1 session across 5 msgs; bounded rotates ✅ | — |
| D | maxAgeHours: 2, fake Date.now |
no rotate at 1h59m; rotates at 2h1m ✅ | — |
| E | retirement machinery | listener fired once with retired id + target; discardSession('sess-1'); sanitized stderr log emitted ✅ |
— |
| F | bounds 0 / -3 / NaN |
dropped defensively, no per-message rotation ✅ | — |
Count: head 15/15, base 2/2. Witness: 01-ab-rotation-head-vs-base.png.
Secondary claim 1 — persistence and store shape (harness/persistence.mjs): head 13/14, base 2/2. Witness: 02-persistence-restart-legacy-head-vs-base.png.
| Cell | Result (head) |
|---|---|
| G restart survival | turns persist; after restoreSessions() the 4th message rotates exactly at the bound; no duplicate creation ✅ |
| H store shape | unbounded-channel entries carry no turns/startedAt — byte-same shape as base (A/A shape control, passes on both arms); bounded entry gets turns: 1 at creation, no startedAt without maxAgeHours ✅ |
| I legacy store | restores, first message reuses the legacy session (no rotate-on-sight), rotates after post-upgrade turns reach the bound ✅ |
| J fresh age clock | startedAt persisted at creation → rotation fires across a restart ✅ |
| K legacy age clock | FAIL — Finding 1 below ❌ |
| L1/L2/L3 hand-edited stores | turns: 1e308 → at most one rotation then normal cadence; turns: "lots" → entry rejected, store rewritten, fresh session; turns: -5 → defers, never per-message-rotates ✅ |
Secondary claim 2 — safe retirement paths (harness/lazy-and-defer.mjs): head 10/10, base 2/2. Witness: 03-lazy-reload-defer-head-vs-base.png.
- M: evicted (non-live) route at its bound rotates without a
loadSessionattempt — the bound cannot be dodged by memory eviction. - N: reload that returns a new ID carries
turns/startedAtover; rotation then lands exactly at the bound. - O: rotation defers while the activity checker reports the session active, enforces on the next message once settled; clearing the checker re-enables.
- P: two concurrent messages share an in-flight creation (no invalidation); the bound applies to the next message.
- Q (ad-hoc probe,
logs/03b-lazy-restoreRoutes-probe.txt): the lazyrestoreRoutes()path used bydaemon-worker(recoveryMode: 'lazy') also carries restored turn counts — m3 reuses the restored session, m4 rotates (LAZY-RESTART-BOUND: PASS).
Corrections (to the PR description, not code requests)
- "One wiring line in each of
start.tsanddaemon-worker.ts… exactly three call sites" (Risk & Scope) — stale for the final head. At6dbca5908fthe registration is centralized in theChannelBaseconstructor (setChannelRotationhas exactly one production call site,ChannelBase.ts:855);start.ts/daemon-worker.tscontain nosessionRotationreferences. Verified this is behaviorally equivalent or better: all three launch paths (start.ts:369,start.ts:498single-channel,daemon-worker.ts:539) construct channels viacreateChannel→ChannelBaseconstructor, and the M8 mutation proves the registration is load-bearing (removing it kills 4 tests). The consolidation happened in commit53a6777("register sessionRotation bounds in every launch mode"); the scope note describes the earlier per-site wiring. - "No user-facing notice is posted to the chat when a rotation happens — the reset is silent" (Risk & Scope, both languages) — contradicted by the final code:
handleSessionRotatedsends"This conversation reached its configured limit and was rotated; starting a fresh session."to the affected chat/thread, and the updated docs say the same. The notice is pinned by test (M6 mutation killsannounces rotation and discards the retired session). The docs and code agree with each other; only the description lags.
Findings
Finding 1 — Low — legacy sessions under maxAgeHours-only lose their age clock on daemon restart
Repro (preserved harness): node harness/persistence.mjs packages/channels/base/dist HEAD <scratch> — cell K. A pre-rotation store entry (sessionId only, no startedAt) on a channel configured with { maxAgeHours: 2 } alone: the first post-upgrade message seeds startedAt in memory inside shouldRotate() and returns without persisting; countTurn is a no-op without maxTurns, and nothing else on the reuse path writes. The on-disk entry after the message:
{"sessionId":"legacy-aged","target":{...},"cwd":"/cwd"} // startedAt absentConsequence: each daemon restart re-seeds the clock at the first message, so age rotation for this cohort requires maxAgeHours of continuous uptime. This deviates from the description ("Turn counts and start times persist alongside the routes, so a daemon restart cannot reset a bound") and the docs ("Counters … survive a daemon restart"). Bounds established: (a) fresh sessions are unaffected — their startedAt persists at creation (cell J passes); (b) turn-bound channels are unaffected — countTurn persists every message; (c) the window closes on any unrelated persist — an unrelated bounded route's write flushed the seeded clock in the same run (cell K.control passes). Blast radius is the one-time migration cohort on age-only configs. Note the PR's own test suite is green on both sides of this axis — nothing pins it (see mutation note below).
Suggested fix (measured, preserves commit intent):
if (startedAt === undefined) {
this.toStartedAt.set(sessionId, Date.now());
+ this.persist();
return false;
}Applied in a scratch rebuild: the persistence harness flips to 14/14 (hostile fixture clean), the full SessionRouter + ChannelBase suite stays 707/707 (benign fixtures byte-identical — in particular does not write per message when only maxAgeHours is configured still passes, because the seed branch fires only for legacy sessions, not per message), logs/06-kfix-persistence.txt, logs/06-kfix-suite.txt. Since the suite is green with and without the patch, the fix should ship with its pinning fixture, e.g. "legacy store + maxAgeHours: after the first routed message the persisted store contains startedAt, and the age bound survives a restart".
Finding 2 — Nit — description-vs-code drift (see Corrections)
The two description statements above contradict the final head. No code change requested; flagged so the next reader does not rely on "silent rotation" or the three-call-site topology.
Vacuity check and mutation matrix
Baseline SessionRouter.test.ts + ChannelBase.test.ts: 707/707 green. Positive control: M1's off-by-one turned exactly the rotation block red with behavioral assertions (expected 'session-2' to be 'session-1'-style), proving the harness can fail the suite. Witness: 04-mutation-matrix-11-of-11-killed.png, raw logs under logs/mutations/.
| # | Mutant (guard removed/broken) | Result | Killed by |
|---|---|---|---|
| M1 | >= → > in shouldRotate |
KILLED (9) | all maxTurns rotation tests + announces rotation and discards the retired session |
| M2 | drop persist() in countTurn |
KILLED (2) | persists turn counts so a restart cannot reset the bound, persists once per routed message instead of stacking writes |
| M3 | drop isSessionActive defer condition |
KILLED (2) | both defers rotation… tests (router + ChannelBase) |
| M4 | drop counter migration on ID-changing reload | KILLED (1) | carries counters over an ID-changing reload |
| M5 | rotation check disabled (false) |
KILLED (12) | entire rotation block, both packages |
| M6 | ChannelBase: no onSessionRotated subscription |
KILLED (1) | announces rotation and discards the retired session |
| M7 | ChannelBase: activity checker always false | KILLED (1) | defers rotation while the outgoing turn is still running |
| M8 | ChannelBase: no setChannelRotation registration |
KILLED (4) | both registration tests + announce + defer |
| M9 | cli: sessionRotation parse dropped |
KILLED (3) | parses sessionRotation bounds, both throw-tests |
| M10 | settings-store validation disabled | KILLED (1) | accepts env-resolvable descriptor fields and typed shared fields |
| M11 | legacy seed branch rotates on sight | KILLED (1) | accepts route stores written before rotation existed |
11/11 killed, zero survivors. Every guard the PR introduces is pinned by a behavioral assertion, and each failure quoted the expected-vs-actual mismatch (no import/compile-break reds). The reverse mutation (Finding 1's fix) left the suite green on both sides — the unpinned axis is exactly where Finding 1 lives; the fixture that would go red is named there.
Config-parse surface (harness/config-parse.mjs against the real cli dist/): 15/15 — accepts maxTurns-only / maxAgeHours-only / both / fractional / {}→unset / null→unset / omitted→unset; rejects 0, -1, NaN, Infinity, "5", maxAgeHours: 0, non-object, and arrays, each with a sessionRotation-named error. The two validation sites are equivalent: isValidRotationBound (channel-base) vs the settings-store inline expression agree on all 15 ladder values. Witness: 05-config-parse-reject-accept-matrix.png.
Targeted gates
| Gate | Result |
|---|---|
packages/channels/base full vitest suite |
1039/1039 passed (19 files) |
packages/cli channel + settings-store suites (config-utils, start, daemon-worker, channel-settings-store) |
232/232 passed (4 files) |
cli workspace typecheck (tsc --noEmit) |
0 errors |
| ESLint on the 6 changed production files | clean, liveness-proven: a planted const unusedPlantedVar = 1; in types.ts was reported (no-unused-vars), then removed |
No pre-existing failures encountered on either arm; no repo-wide gate was run (see Not covered).
Not covered
- Per-commit attribution: the checkout is depth-2 (
is-shallow-repository: true);git rev-list HEAD^1..HEAD^2yields 1 commit locally while the metadata snapshot lists 5. The aggregateHEAD^1..HEADdiff is what was verified; the intermediate states (e.g. the per-site wiring of53a6777) were not individually exercised. - End-to-end with a real daemon and real chat platform: rotation's announcement/discard wiring was exercised at the
ChannelBaseunit level (its own tests, mutation-pinned) and at the router level with a fake bridge — this reproduces the shape of the wire flow, not delivery through a live platform adapter. Thestart.ts/daemon-worker.tsruntime paths were verified by their suites + code trace, not by booting a daemon. - Repo-wide gates: only the two affected workspaces were run (per scope). No repo-wide
npm run test, fullnpm run lint, or integration suites. - Token-based bound: explicitly out of scope per the PR description; not probed.
- Base-side
HEAD^1differs from the metadatabaseRefOid(7425e42f…): the merge ref was rebuilt against a newer main tip (962dc8eadc, including fix(serve): Keep restore request shapes distinct #8933). Per the CI contract the merge ref is authoritative; the diff between the two main tips is outside this PR. - A/B for announcement ordering UX (notice posts before the successor's reply) — code-read only.
Methodology
Environment: the CI verify container (node v22.23.2, Linux, $RUNNER_TEMP=/__w/_temp), merge-ref checkout at c82232f6; npm ci + npm run build pre-run. Harnesses (harness/*.mjs) import the compiled dist/ of each arm by absolute path and drive it with a fake bridge peer encoding the real peer semantics (fresh-ID newSession, echo loadSession, recorded discardSession) — no stubbing of the unit under test; time was controlled by patching Date.now around the aged cells. Base control: scratch worktree at HEAD^1, only packages/channels/base rebuilt against the root node_modules (no internal workspace links in this package's closure, verified). Cell K's probe encodes the PR's own stated invariant, so its red counts as a fail. Mutations were applied with point edits to source, run under vitest against source, and restored with git checkout (verified git status --porcelain empty after each). Raw per-arm logs: logs/01-…–logs/06-… and logs/mutations/.
Evidence 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. ✅
|
🤖 Addressed the latest review feedback (round 4/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/10 轮）。改动内容与我反驳保留之处如下： Autofix review round for PR #8927This round triaged the real-stack verification report from @wenshao (issue-level comment; no inline comments, no failed checks, no base conflicts). One observation was addressed with a code fix; the other two were context-only and are declined with reasons below. Feedback dispositions1. Observation (1) — age clock of a pre-upgrade route is memory-only, restarts can defer an age-only bound indefinitely → Addressed. New regression test: 2. Observation (2) — 3. Observation (3) — paths not exercised by the real-stack run → No action needed. Changes
Commit: Verification
Integration tests were not run: the touched behavior is exercised directly by the package's unit tests, not only through the bundled CLI or integration harness. 中文说明PR #8927 的 Autofix 评审轮次本轮分诊了 @wenshao 的真实环境验证报告（issue 级评论；没有行内评论、没有失败的检查、没有与 base 的冲突）。其中一条观察以代码修复处理；另外两条属于背景说明，附理由予以婉拒。 反馈处理结果1. 观察 (1) —— 升级前就存在的路由，其年龄时钟只在内存里，反复重启可以无限期推迟纯年龄限度 → 已修复。 新增回归测试： 2. 观察 (2) —— 3. 观察 (3) —— 本次真实链路未覆盖的路径 → 无需处理。 改动内容
提交： 验证
未运行集成测试：本次触及的行为由包的单测直接覆盖，并非只能通过打包后的 CLI 或集成测试框架验证。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
e4f78f3
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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): PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all planned checks completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all planned checks completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks I started were completed within budget (~15 tool calls used)., and 3 more.
Test Plan (not a blocker): 1023 tests passed — this review observed 1056, 17, 19080, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1056, 17, 19080, 297, 266, 205, 59, 289, 134, 71 passed.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all planned checks completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all planned checks completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks I started were completed within budget (~15 tool calls used).，另有 3 条。
Test Plan（非阻断）：1023 tests passed — this review observed 1056, 17, 19080, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1056, 17, 19080, 297, 266, 205, 59, 289, 134, 71 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (this.shouldRotate(channelName, sessionId)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R4-I: Two messages concurrently awaiting the same eager restore reservation, with the restored session already at its rotation bound: the first waiter's re-entry rotates (this R3-13 fix re-check + continue), rotateRoute → invalidateRouteOperation destroys the route token the second waiter's reservation still references, and the second message is dropped with Session route operation was invalidated (the catch below rethrows creating.invalidationError without retry) instead of being routed.
Failure scenario: probe-reproduced with deterministic ordering — daemon restart + persisted session at its maxTurns bound + two concurrent same-route messages during the restore window (adapters dispatch handleInbound fire-and-forget): waiter 1 resolves to the successor session, waiter 2 rejects with Session route operation was invalidated. Pre-diff, both waiters were served the restored session — this diff changes that race from "both delivered" to "one lost". Side effect: a duplicate best-effort discardSession (once via discardRotatedSession, once via waiter 2's scheduleDiscardInvalidatedSession).
Suggested fix: make the invalidation retryable on this path instead of terminal — e.g. in the reservation-wait catch, when creating.invalidationError is set and a successor operation has taken over the key, failedWaits++; continue; (the re-entered loop sees the successor route and resolves normally). Note: always-retry is NOT correct — it flips the intentional-rejection test 'invalidates a restore waiter when removeSession runs after reservation resolution'. At minimum, cover the two-waiter at-bound case with a test.
中文说明
严重：两条消息并发等待同一个 eager 恢复预留（restore reservation），且恢复出的会话已达轮换限度时：第一个等待者的重入触发轮换（即 R3-13 修复加入的复查 + continue），rotateRoute → invalidateRouteOperation 摧毁了第二个等待者的预留仍引用的路由令牌，第二条消息以 Session route operation was invalidated 被丢弃（下方 catch 直接重抛 creating.invalidationError、不重试），而不是被路由。
失败场景：已用探针按确定性顺序复现——守护进程重启 + 持久化会话已达 maxTurns 限度 + 恢复窗口内同一路线上两条并发消息（各 adapter 以 fire-and-forget 方式派发 handleInbound）：等待者 1 解析到后继会话，等待者 2 以 Session route operation was invalidated 被拒绝。改动前两个等待者都会被交给恢复出的会话——本 diff 把这个竞争的结果从「都送达」变成「丢一条」。副作用：重复的尽力 discardSession（discardRotatedSession 一次、等待者 2 的 scheduleDiscardInvalidatedSession 又一次）。
建议修复：让这条路径上的作废可重试而不是终结——例如在预留等待的 catch 中，当 creating.invalidationError 已设置且有后继操作接管该 key 时，failedWaits++; continue;（重入循环会看到后继路由并正常解析）。注意：无条件重试不可取——会使故意作废的测试 'invalidates a restore waiter when removeSession runs after reservation resolution' 翻转。至少应补一个「双等待者 + 已达限度」的测试。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| private countTurn(channelName: string, sessionId: string): void { | ||
| const rotation = this.channelRotations.get(channelName); | ||
| if (rotation?.maxTurns === undefined) return; |
There was a problem hiding this comment.
[Critical] R4-K: The diff's new per-message persist sites (countTurn here, plus rotateRoute and the shouldRotate stamp write) can fire from reservation waiters while eager restoreSessions() is mid-flight, when toSession holds only the prefix of keys restored so far — persist() then writes a truncated route store to disk, and the final if (changed && restoreGeneration === ...) persist is skipped when every session restores under the same ID (the common case — AcpBridge.loadSession returns the input ID; DaemonChannelBridge.loadSession returns session.sessionId), so the truncated file becomes durable.
Failure scenario: probe-reproduced — eager router, two-key store, rotation configured; a waiter released at the first key's restore persists while the second key is still out of toSession: the file holds only the restored prefix mid-restore and remains truncated after restore completes. (1) Any restart/crash/redeploy before the next persist silently loses routes K2..Kn — those users get fresh sessions (context loss) and the old sessions are orphaned; (2) on a later bridge crash, restoreSessions reserves only the surviving keys, and the lost keys still in memory route to dead bridge IDs (eager isLive() is unconditional). Reachable in production: QQ cold-start dispatches messages ungated (handleC2C has no _ready check). Pre-diff the only waiter-reachable persist was the rare promoteTargetToGroup; on age-only channels nothing self-heals the file afterwards.
Suggested fix: make persist() a no-op while restoreSessions() runs (e.g. a persistSuspended flag set around the restore loop, checked at the top of persist()), remember that a persist was requested during the window, and perform one unconditional persist after the loop ends (subject to the existing generation check).
中文说明
严重：本 diff 新增的每消息持久化点（此处 countTurn，以及 rotateRoute 与 shouldRotate 的打时间戳写入）可能在 eager restoreSessions() 进行中被预留等待者触发——此时 toSession 只包含已恢复的前若干个 key——persist() 会把被截断的路由存储写到磁盘；而当所有会话以原 ID 恢复时（常见情形——AcpBridge.loadSession 原样返回入参 ID，DaemonChannelBridge.loadSession 返回 session.sessionId），末尾的 if (changed && restoreGeneration === ...) 持久化会被跳过，截断文件从此固化。
失败场景：已用探针复现——eager 路由器、双 key 存储、配置了轮换；第一个 key 恢复完成时放行的等待者在第二个 key 尚未回到 toSession 时执行持久化：恢复过程中文件只含已恢复前缀，恢复结束后仍是截断的。(1) 下一次持久化之前的任何重启/崩溃/重新部署都会静默丢失 K2..Kn 路由——这些用户得到全新会话（上下文丢失），旧会话成为孤儿；(2) 之后 bridge 崩溃时，restoreSessions 只为幸存 key 建预留，而内存中丢失的 key 仍映射到死 bridge ID（eager 的 isLive() 无条件为真）。生产可达：QQ 冷启动在 READY 时未加门控地派发消息（handleC2C 没有 _ready 检查）。改动前等待者唯一可触达的持久化是罕见的 promoteTargetToGroup；纯年龄限度频道之后没有任何写盘能自愈该文件。
建议修复：restoreSessions() 运行期间让 persist() 空操作（例如在恢复循环前后设置 persistSuspended 标志，并在 persist() 顶部检查），记住窗口内有持久化请求，循环结束后执行一次无条件持久化（仍受现有 generation 检查约束）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| }); | ||
| }); | ||
|
|
||
| describe('session rotation', () => { |
There was a problem hiding this comment.
[Suggestion] R4-A: uncountTurn's turns <= 1 floor edge is pinned by no test — deleting the floor guard leaves all 724 SessionRouter+ChannelBase tests green (probe-verified mutant). A loop firing dropped as the session's FIRST message leaves the phantom seed count (creation seeds turns=1 without countTurn; the drop's uncountTurn at turns=1 is a floor no-op), so the bound consumes one slot for a firing that never prompted. The primary uncount behavior IS integration-pinned by two tests; the gap is the floor edge itself plus uncountTurn as a unit (zero references in this file).
Failure scenario: probe-reproduced — maxTurns: 2, dropped first firing → the second real message rotates a one-turn session. The JSDoc documents the floor as deliberate ('Never drops below the seed value of one'), so the gap is that this decided behavior is pinned by no test; a refactor dropping the guard ships silently.
Suggested fix: add a test pinning the floor edge deliberately — maxTurns: 2, drop the session's first loop firing, then assert which session the next two messages prompt (the probe shape flips under the floor-removed mutant).
中文说明
建议：uncountTurn 的 turns <= 1 下限边界没有任何测试钉住——删除该下限守卫后全部 724 个 SessionRouter+ChannelBase 测试仍为绿（已用探针验证变异体）。被丢弃的 loop 触发若是会话的第一条消息，会留下幽灵种子计数（创建时种子 turns=1 不走 countTurn；丢弃时 turns=1 的 uncountTurn 因下限而空操作），限度为一个从未 prompt 的触发白白消耗一格。主要的 uncount 行为确有两个集成测试钉住；缺口在下限边界本身以及 uncountTurn 作为单元（本文件中零引用）。
失败场景：已用探针复现——maxTurns: 2、丢弃首次触发 → 第二条真实消息会轮换只有一轮的会话。JSDoc 明确该下限是有意设计（'Never drops below the seed value of one'），因此缺口是这一已决定的行为没有测试钉住；重构删掉守卫会静默通过。
建议修复：补一个有意钉住下限边界的测试——maxTurns: 2，丢弃会话的首次 loop 触发，然后断言接下来两条消息各自 prompt 的会话（该探针形态在删除下限的变异体下会翻转）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (target?.channelName !== this.name) return; | ||
| this.purgeSessionState(sessionId); |
There was a problem hiding this comment.
[Suggestion] R4-B: The rotation path's per-session state purge (purgeSessionState here — pending permissions, instructed/unattended-memory markers) is asserted by no test: deleting this call leaves all 597 ChannelBase tests green (probe-verified); rotation tests only assert the notice text, the successor session ID, and bridge.discardSession.
Failure scenario: probe-reproduced — seed a pending permission for the outgoing session, rotate via maxTurns: 1, then send an approval: with the purge deleted, respondToPermission is called with the stale request ID — the user's approval matches a stale request on an already-discarded session, and the channel's pending-permission/instructed-session maps accumulate stale entries keyed by dead session IDs.
Suggested fix: add a ChannelBase rotation test that seeds a pending permission for the outgoing session (the existing permission-relay test plumbing can do this), rotates via maxTurns: 1, and asserts an approval message afterwards does NOT reach respondToPermission.
中文说明
建议：轮换路径的按会话状态清理（此处 purgeSessionState——待处理权限、instructed/unattended-memory 标记）没有任何测试断言：删除该调用后全部 597 个 ChannelBase 测试仍为绿（已用探针验证）；轮换测试只断言提示文案、后继会话 ID 与 bridge.discardSession。
失败场景：已用探针复现——为即将退役的会话预置一个待处理权限请求，以 maxTurns: 1 触发轮换，然后发送批准消息：删除清理后，respondToPermission 会以过期请求 ID 被调用——用户的批准命中了已被 discard 会话上的过期请求；频道的待处理权限/instructed 会话映射会累积以死会话 ID 为键的过期条目。
建议修复：补一个 ChannelBase 轮换测试：为旧会话预置待处理权限（可复用现有 permission-relay 测试管线），以 maxTurns: 1 轮换，断言此后的批准消息不会到达 respondToPermission。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| this.toTurns.delete(sessionId); | ||
| this.toStartedAt.delete(sessionId); | ||
| this.sessionRoutingLeases.delete(sessionId); |
There was a problem hiding this comment.
[Suggestion] R4-D: Test-efficacy probe (harness validated): deleting the added safety statement this.sessionRoutingLeases.delete(sessionId); in this forget path (removeSessionId) leaves every affected test green — the lease-map cleanup is ungated, while the sibling toTurns/toStartedAt deletes on the lines above were killed.
Failure scenario: measured — with that one statement deleted, zero test failures. If a future change drops it, sessionRoutingLeases entries for forgotten sessions are never reclaimed (a slow map leak in the router), and a stale lease count would skew acquireRoutingLease/releaseRoutingLease accounting if a session id is reused; the second cleanup site (deleteByKey) keeps the existing tests green as a mask.
Suggested fix: add a SessionRouter test that acquires a routing lease for a session, runs this forget path, and asserts the lease map no longer holds the id.
中文说明
建议：测试有效性探针（harness 已验证）：删除该遗忘路径（removeSessionId）中新增的安全语句 this.sessionRoutingLeases.delete(sessionId); 后所有受影响测试仍为绿——lease 映射的清理没有被门控，而上方紧邻的 toTurns/toStartedAt 删除均被杀死。
失败场景：实测——删除该语句后零测试失败。若未来改动将其删掉，被遗忘会话的 sessionRoutingLeases 条目永远不会回收（路由器中的缓慢映射泄漏），且会话 ID 复用时过期的 lease 计数会扭曲 acquireRoutingLease/releaseRoutingLease 记账；第二个清理点（deleteByKey）会让现有测试继续为绿，形成掩护。
建议修复：补一个 SessionRouter 测试：为某会话取得路由 lease，执行该遗忘路径，断言 lease 映射不再持有该 ID。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Default (eager) recovery: channel start uses restoreSessions(). | ||
| const router = new SessionRouter(bridge, '/tmp', 'user', persistPath); | ||
| router.setChannelRotation('ch', { maxTurns: 3 }); |
There was a problem hiding this comment.
[Suggestion] R4-H: Eager-recovery (restoreSessions) counter carry-over is only tested for same-ID loads — both eager tests' loadSession mocks resolve to the persisted ID. Probe-verified mutant: keying restoreRotationState by entry.sessionId (the persisted ID) instead of the awaited loadSession result leaves 127/127 SessionRouter tests green. The lazy path's ID-changing equivalent IS pinned ('carries counters over an ID-changing reload'); eager restore uses separate carry code.
Failure scenario: a regression to persisted-ID keying in the eager path orphans turns/stamp under the dead ID: shouldRotate reads no counters for the live session, so an at-bound restored route is reused past its bound — with no test failing.
Suggested fix: add an eager-recovery case where bridge.loadSession resolves to a DIFFERENT id (as the lazy reload tests do) with persisted turns at/under the bound, asserting rotationCounters is keyed by the new ID and rotation fires exactly at the carried bound.
中文说明
建议：eager 恢复（restoreSessions）的计数迁移只测了同 ID 加载——两个 eager 测试的 loadSession mock 都原样返回持久化 ID。已用探针验证的变异体：把 restoreRotationState 改为按 entry.sessionId（持久化 ID）而非等待到的 loadSession 结果建键后，127/127 个 SessionRouter 测试仍全绿。lazy 路径的换 ID 等价场景有测试钉住（'carries counters over an ID-changing reload'）；eager 恢复用的是另一段迁移代码。
失败场景：eager 路径退化为按持久化 ID 建键时，turns/时间戳会被孤儿挂在死 ID 下：shouldRotate 读不到存活会话的计数，已达限度的恢复路由会被超限复用——且没有任何测试失败。
建议修复：补一个 eager 恢复用例：bridge.loadSession 解析为不同 ID（参照 lazy 重载测试），持久化 turns 在限度处或限度内，断言 rotationCounters 以新 ID 为键、且轮换恰好按迁移过来的限度触发。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(ch.sent.some((m) => m.text.includes('rotated'))).toBe(true); | ||
| expect(bridge.discardSession).toHaveBeenCalledWith(retiredId); |
There was a problem hiding this comment.
[Suggestion] R4-L: No rotation test exercises a threaded route — this block has zero threadId references (SessionRouter's rotation block has exactly one threadId: undefined). Probe-verified mutant: replacing target.threadId with undefined in handleSessionRotated leaves all tests green — the announcement's thread routing is gated by nothing.
Failure scenario: on a thread-scoped channel the 'reached its configured limit and was rotated' notice posts to the parent chat instead of the thread/topic — in forum-style chats (Telegram topics) participants of the rotated conversation never see why their context reset; an adapter that requires a topic id could also fail the send outright (error only logged to stderr).
Suggested fix: add one rotation test with envelope({ text: 'first', threadId: 'topic-1' }) asserting the announcement was delivered with that threadId. Harness note: TestChannel inherits the base sendThreadMessage, which discards _threadId — the test needs a sendThreadMessage spy (the pattern already used elsewhere in this file).
中文说明
建议：没有任何轮换测试走带 thread 的路由——本测试块零 threadId 引用（SessionRouter 的轮换块恰好有一个 threadId: undefined）。已用探针验证的变异体：把 handleSessionRotated 中的 target.threadId 替换为 undefined 后所有测试仍为绿——通告的 thread 路由没有任何门控。
失败场景：thread 作用域频道上，「已达配置限度并已轮换」的通告会发到父聊天而不是 thread/topic——在论坛式聊天（Telegram topics）中，被轮换会话的参与者完全看不到上下文为何重置；要求 topic id 的 adapter 甚至可能直接发送失败（错误只写 stderr）。
建议修复：补一个轮换测试：envelope({ text: 'first', threadId: 'topic-1' })，断言通告以该 threadId 送达。Harness 提示：TestChannel 继承的基类 sendThreadMessage 会丢弃 _threadId——测试需要用 sendThreadMessage spy（本文件其他地方已有该写法）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| router.releaseRoutingLease(second); | ||
| router.releaseRoutingLease(second); | ||
| expect(await routed(router, 'ch', 'alice', 'chat1')).not.toBe(second); |
There was a problem hiding this comment.
[Suggestion] R4-N: The lease map's reference-COUNTING semantics are pinned by no test — this test holds two overlapping leases on second but releases both back-to-back before any assertion, so the intermediate one-lease-of-two state is never observed; ChannelBase.test.ts never calls the lease API. Probe-verified mutants: leaseSession → set(sessionId, 1) (flag instead of increment) and releaseRoutingLease → unconditional delete both leave the whole suite green.
Failure scenario: under flag semantics the lease no longer prevents the race it exists for: message A resolves an at-bound session S and is still in the pre-turn-registration window (real awaits exist there — channel-memory recall, bridge-recovery wait); message B reuses S taking lease 2; A settles and releases; the lease is now 0, so message C's gate rotates and discards S while B has not registered its turn — B's turn lands on a discarded session (auto-cancelled approvals, dropped late output).
Suggested fix: split the double release — release one lease, then assert the next router.resolve(...) still returns second (rotation deferred with one lease outstanding), then release the remainder and assert rotation fires. Under either flag-semantics mutant the intermediate assertion fails (probe-verified).
中文说明
建议：lease 映射的引用计数语义没有任何测试钉住——本测试让 second 持有两个重叠 lease，但在任何断言前连续释放两次，中间「二剩一」的状态从未被观察；ChannelBase.test.ts 从不调用 lease API。已用探针验证的变异体：leaseSession → set(sessionId, 1)（布尔标记代替自增）与 releaseRoutingLease → 无条件 delete，两者都让整个套件保持绿色。
失败场景：布尔标记语义下，lease 不再能阻止它本要防的竞争：消息 A 解析了已达限度的会话 S，仍处于回合登记前的窗口（那里有真实的 await——频道记忆读取、bridge 恢复等待）；消息 B 复用 S、取得 lease 2；A 结束并释放；此时 lease 归 0，消息 C 的门控触发轮换并 discard S——而 B 尚未登记回合：B 的回合落在已被 discard 的会话上（审批被自动取消、迟到输出被丢弃）。
建议修复：拆开双释放——先释放一个 lease，断言下一次 router.resolve(...) 仍返回 second（仍有一个 lease 未释放时轮换被推迟），再释放余下的并断言轮换触发。两种布尔标记变异体下中间断言都会失败（已用探针验证）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(await routed(router, 'ch', 'alice', 'chat1')).not.toBe(first); | ||
| await drainMicrotasks(); | ||
|
|
||
| expect(bridge.discardSession).toHaveBeenCalledWith(first); |
There was a problem hiding this comment.
[Suggestion] R4-O: discardRotatedSession's failure isolation ('Best-effort cleanup must not fail the incoming message') is asserted by no test — both bridge mocks resolve undefined and no test anywhere overrides discardSession to reject or throw. Probe-verified mutant: stripping BOTH isolation layers (the .catch(() => undefined) and the surrounding try/catch) leaves the entire suite green. Test 13 (listener throws) pins listener isolation only; the discard leg has no equivalent.
Failure scenario: AcpBridge.discardSession ends in an unguarded await conn.extMethod('qwen/control/session/close', ...) — an RPC that rejects when the ACP connection is down or times out, and rotation fires precisely on busy routes. A future edit dropping the catch (invisible to CI) turns a rejected discard during rotation into an unhandled promise rejection — Node's default policy throws, taking down the channel process mid-message after the retirement was already persisted.
Suggested fix: add a rotation test with bridge.discardSession rejecting once, asserting resolve still returns the successor. Harness caveat (probe-verified): vitest here swallows unhandled rejections originating from vi.fn() mocks, so 'no unhandled rejection observed' is NOT a working oracle — spy on the catch path instead (assert the fire-and-forget chain is attached / the catch handler ran).
中文说明
建议：discardRotatedSession 的失败隔离（「尽力清理不得让进入的消息失败」）没有任何测试断言——两个 bridge mock 都 resolve undefined，全仓库没有任何测试把 discardSession 覆盖为 reject 或 throw。已用探针验证的变异体：把两层隔离（.catch(() => undefined) 与外层 try/catch）都剥掉后整个套件仍为绿。测试 13（listener 抛错）只钉住 listener 隔离；discard 这一支没有等价测试。
失败场景：AcpBridge.discardSession 末尾是无保护的 await conn.extMethod('qwen/control/session/close', ...)——ACP 连接断开或超时时该 RPC 会 reject，而轮换恰好在繁忙路由上触发。未来某次删掉 catch 的改动（CI 不可见）会把轮换中被拒绝的 discard 变成未处理 rejection——Node 默认策略抛出异常，在退役已持久化之后于消息处理中途击垮频道进程。
建议修复：补一个轮换测试：bridge.discardSession reject 一次，断言 resolve 仍返回后继会话。Harness 注意（已用探针验证）：本处 vitest 会吞掉源自 vi.fn() mock 的未处理 rejection，因此「未观察到未处理 rejection」不是可用的判据——应改为 spy catch 路径（断言 fire-and-forget 链已挂接 / catch 处理器已执行）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| router: { | ||
| setChannelRotation: () => {}, | ||
| setSessionActivityChecker: () => {}, | ||
| onSessionRotated: () => () => {}, |
There was a problem hiding this comment.
[Suggestion] R2-18: Still-standing from round 2 — the telegram rotation coverage gates nothing. Whole-file revert probe (harness validated, re-run at the current head): all 17 telegram tests PASS with the paired source change reverted — this edit only pads the fake router with no-op rotation stubs to satisfy the widened interface.
Failure scenario: measured by the revert probe — because the suite is green whether or not the feature exists, a regression that drops or mis-wires per-channel sessionRotation delivery into the adapter's router (e.g. a telegram route configured with maxTurns but rotation never engaged) ships with no telegram-level test failing. Core rotation behavior remains gated by the channels/base suites; the gap is the adapter-integration surface specifically.
Suggested fix: add one telegram-level test asserting the channel wires configured sessionRotation into the router (setChannelRotation invoked with the configured maxTurns/maxAgeHours).
中文说明
建议：第 2 轮遗留至今——telegram 的轮换覆盖什么都没门控。整文件回退探针（harness 已验证，并在当前 head 重跑）：把配套源码改动回退后全部 17 个 telegram 测试仍然通过——本编辑只是给 fake router 补了空操作的轮换桩以满足加宽的接口。
失败场景：回退探针实测——无论特性是否存在套件都是绿色，因此一个把按频道 sessionRotation 下发到 adapter 路由器删掉或接错的回归（例如 telegram 路由配置了 maxTurns 但轮换从未生效）会在没有任何 telegram 层测试失败的情况下合入。核心轮换行为仍由 channels/base 套件门控；缺口专指 adapter 集成面。
建议修复：补一个 telegram 层测试，断言频道把配置的 sessionRotation 接入了路由器（setChannelRotation 以配置的 maxTurns/maxAgeHours 被调用）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
…sts (#8927) Reservation waiters that find their restored session at the rotation bound can rotate it while sibling waiters still reference the old route token; the invalidated waiters now re-route against the successor operation instead of failing, while deliberate rejections (removeSession) stay terminal. Eager restore now suspends persistence for its duration and flushes once the store is whole: per-message persists released mid-restore previously wrote the restored prefix only, and the common same-ID restore skipped the final write, making the truncation durable. Adds mutation-pinned tests for the uncount floor, lease reclamation and reference counting, combined turn+age bounds, ID-changing eager counter carry-over, rotation discard failure isolation, rotation permission purge, threaded rotation announcements, and the telegram rotation wiring.
|
🤖 Addressed the latest review feedback (round 7/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/10 轮）。改动内容与我反驳保留之处如下： Round 6 summary — PR #8927Two Critical race findings fixed in production code; nine test-gap findings Critical findings (fixed)
Suggestion findings (implemented as tests)All implemented; each was verified load-bearing by temporarily applying the
Review-level CHANGES_REQUESTED note (integration tests)The automated reviewer's CHANGES_REQUESTED review records that VerificationCommands actually run and their results:
中文说明第 6 轮总结 — PR #8927修复了两个 Critical 竞态问题（生产代码）；以变异钉住（mutation-pinned）测试回应了 9 个测试缺口建议。单个提交： Critical 问题（已修复）
建议类问题（以测试实现）全部实现；每一项都通过临时套用评审者的探针变异体、确认新测试失败、再恢复修复的方式验证了门控作用（共探测 13 个变异体，全部被杀死）：
评审层 CHANGES_REQUESTED 说明（集成测试）自动评审者的 CHANGES_REQUESTED 评审记录了 验证实际执行的命令及结果：
Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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 — did not converge within the reverse-audit round cap of 5.
Not explored to full depth (tool budget reached): PR #8927 adds an opt-in per-channel sessionRotation opt...: did not execute ChannelBase.test.ts to confirm the loop-drop test passes as written (read-only review).; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above were completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above completed within budget., and 3 more.
Test Plan (not a blocker): 1023 tests passed — this review observed 1066, 18, 19080, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1066, 18, 19080, 297, 266, 205, 59, 289, 134, 71 passed.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未探索到全部深度（达到工具调用预算）：PR #8927 adds an opt-in per-channel sessionRotation opt...：did not execute ChannelBase.test.ts to confirm the loop-drop test passes as written (read-only review).；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above were completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above completed within budget.，另有 3 条。
Test Plan（非阻断）：1023 tests passed — this review observed 1066, 18, 19080, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1066, 18, 19080, 297, 266, 205, 59, 289, 134, 71 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Released waiters route (and persist) while this loop still restores | ||
| // later keys; suspend persistence so a store holding only the restored | ||
| // prefix cannot become durable. The flush after the loop writes the | ||
| // whole store instead. | ||
| this.persistSuspended = true; |
There was a problem hiding this comment.
[Critical] R5-3: persistSuspended is a non-reentrant boolean and restoreSessions() has no reentrancy guard — a second concurrent restore makes this finally lift the suspension early, and the first restore's final flush persists the second restore's truncated mid-restore store: the exact "truncated store becomes durable" failure this suspension mechanism exists to prevent.
Reachable trigger (traced at this commit): QQChannel fires this.router.restoreSessions() untracked on every cold-start READY, and coldStart stays true until the restore settles — so any abnormal WS close during a restore reconnects and the next READY starts a second restore while the first still runs. Inbound messages keep routing during the overlap (their persists set persistRequestedWhileSuspended), which forces restore A's post-loop flush to write restore B's partial store. B's own final flush is then skipped (changed false, flag already consumed), so the truncated file stays durable: after the next restart, the lost routes' users get fresh sessions (context loss) and the old sessions are orphaned.
Failure scenario: probe-reproduced on unmodified code — a 3-route store with overlapping restores ends with a durable routes.json holding only 2 of 3 routes after BOTH restores complete cleanly; patching the boolean to a depth counter flips the probe (single write, whole store).
Suggested fix: make the suspension reentrancy-safe — a depth counter (persistSuspendDepth++ on entry, -- in finally, suspend while > 0, and gate the post-loop flush on no other restore being in flight), or serialize restoreSessions() by returning the in-flight promise to a second caller.
中文说明
严重：R5-3：persistSuspended 是不可重入的布尔量，且 restoreSessions() 没有重入守卫——第二个并发恢复会让这个 finally 提前解除挂起，第一个恢复的末尾 flush 会把第二个恢复进行到一半的存储固化到磁盘：这正是该挂起机制要防止的「截断存储变得持久」故障。
可达触发路径（已在本 commit 追踪）：QQChannel 在每次冷启动 READY 时以未跟踪方式调用 this.router.restoreSessions()，且 coldStart 在恢复结束前一直为 true——因此恢复期间的任何异常 WS 断开都会重连，下一个 READY 会在第一个恢复仍在运行时启动第二个恢复。重叠期间消息继续路由（其持久化会设置 persistRequestedWhileSuspended），迫使恢复 A 的循环后 flush 写入恢复 B 的不完整存储。随后 B 自己的末尾 flush 被跳过（changed 为 false、标志已被消费），截断文件从此固化：下次重启后，丢失路由的用户得到全新会话（上下文丢失），旧会话成为孤儿。
失败场景：已在未修改代码上用探针复现——3 路由存储、重叠恢复：两个恢复都干净完成后，持久的 routes.json 只剩 3 条中的 2 条；把布尔量改成深度计数后探针翻转（单次写入、完整存储）。
建议修复：让挂起机制可重入安全——改用深度计数（进入时 persistSuspendDepth++，finally 中 --，> 0 时保持挂起，循环后 flush 以无其他恢复在途为前提），或让 restoreSessions() 串行化（把在途 promise 返回给第二个调用者）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| existing && | ||
| !this.creatingSessions.has(key) && | ||
| !this.isSessionActive(channelName, existing) && | ||
| !this.hasRoutingLease(existing) && | ||
| this.shouldRotate(channelName, existing) | ||
| ) { |
There was a problem hiding this comment.
[Critical] R5-6: The rotation gate's activity deferral is self-extending under sustained traffic. trackSessionTurn increments sessionPendingTurns at ENQUEUE time and decrements only after the turn fully settles, so this gate sees pending > 0 not just while a turn runs but while any turn is QUEUED — and every message the gate defers falls through to live reuse, is counted, and enqueues its own turn, extending the very window that deferred it. On a route whose messages arrive at or above turn throughput, sessionPendingTurns never reaches 0, shouldRotate is never consulted again, and NEITHER bound ever fires — the session grows past the model context window until every later message fails: the exact failure sessionRotation exists to prevent.
Failure scenario: probe-reproduced — with maxTurns: 1 and every message arriving before the prior turn settles, 4 messages all past the bound all routed to the SAME session with zero rotations; the moment one message arrives with no turn pending, rotation fires (quiescent flip); a settled-traffic control arm rotates at the bound normally. Production shapes: a webhook channel receiving events faster than turns complete, an overrunning scheduled loop, or a busy group thread — precisely the unattended, steady-arrival shapes this PR adds deferral tests for (webhook/loop turn paths).
The pinned deferral tests and the new docs ("rotates on the next message after it settles") cover the TRANSIENT state only; permanent starvation is unargued, untested, and undocumented. Collect mode escapes (buffered messages register no pending turn), but followup AND steer modes are susceptible — and steer is the default dispatch mode.
Suggested fix (author's choice of direction): re-check the bound at turn-dequeue time before bridge.prompt and bounce the message back through routing; or narrow the deferral to RUNNING turns with explicit handling of queued ones (must not reintroduce the mid-turn rotation hazards the deferral was added to fix); or — if the idle-window limitation is an accepted tradeoff — say so in this gate comment and in the Session Rotation docs so operators of webhook/loop channels know the bound is only enforced during idle gaps.
中文说明
严重：R5-6：轮换门控的活动推迟在持续流量下会自我延长。trackSessionTurn 在入队时递增 sessionPendingTurns、只在回合完全结束后才递减，因此该门控不仅在回合运行时、在回合排队期间也会看到 pending > 0——而每条被门控推迟的消息都会走存活复用路径、被计数并入队自己的回合，从而延长了推迟它的那个窗口。当消息到达速率不低于回合吞吐时，sessionPendingTurns 永远不为 0，shouldRotate 不再被查询，两个限度都不会触发——会话增长超过模型上下文窗口，此后每条消息都失败：这正是 sessionRotation 要防止的故障。
失败场景：已用探针复现——maxTurns: 1、每条消息都在上一回合结束前到达：4 条全部超限的消息都路由到同一会话，零轮换；一旦某条消息到达时没有 pending 回合，轮换立即触发（静息翻转）；沉降流量对照组按限度正常轮换。生产形态：事件到达快于回合完成的 webhook 频道、执行超时的定时 loop、繁忙群线程——正是本 PR 为其添加推迟测试的无人值守、稳定到达形态（webhook/loop 回合路径）。
已有的推迟测试与新文档（「回合结束后下一条消息才轮换」）只覆盖瞬时状态；永久饿死既无论证、也无测试、也未写入文档。collect 模式可幸免（缓冲消息不登记 pending 回合），但 followup 与 steer 模式都会中招——而 steer 是默认派发模式。
建议修复（方向由作者选择）：在回合出队时（bridge.prompt 之前）复查限度并把消息弹回路由；或把推迟收窄为仅针对运行中的回合并显式处理排队中的回合（不得重新引入推迟机制要修复的回合中轮换危害）；或者——若空闲窗口限制是可接受的权衡——请在该门控注释与 Session Rotation 文档中写明，让 webhook/loop 频道的运维者知道限度只在空闲间隙执行。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| this.rotateRoute(key, existing, channelName, { | ||
| channelName: input.channelName, | ||
| senderId: input.senderId, | ||
| chatId: input.chatId, | ||
| threadId: input.threadId, | ||
| isGroup: input.isGroup, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R5-8: Under sessionScope: single every chat on the channel shares one route (the ${channelName}:__single__ key), but the rotation notice target is built solely from the message that tripped the bound — so exactly one chat is notified of a rotation that silently resets EVERY participating chat's context. toTarget cannot help: it records only the session-creating chat, so the router knows at most one of the participating chats, and no single-target choice can cover the siblings (probe flip: the creation-time target notifies chat A only; the current input-derived target notifies chat B only). This is the mirror residual of the round-2 R2-8 fix. — Failure scenario: probe-reproduced — single-scope channel with maxTurns: 2; chat A sends turns 1–2, chat B trips the bound → exactly one notice, delivered to chat B only; chat A (which contributed 2 of the 3 turns on the rotated session) receives nothing, and its next message meets an unexplained context reset — the exact unexplained reset the notice exists to prevent ("rotation is automatic, so participants get no other signal"). No data impact; the cost is incomplete signaling in a configuration the PR's own docs name as a rotation use case. — Suggested fix: cheapest is one sentence in the docs' single-scope / Session Rotation section noting that only the chat whose message triggered the rotation receives the notice (other chats sharing the session see the reset without a notice); the full fix tracks participating targets per single-scope route and fans the notice out across them.
中文说明
建议：R5-8：在 sessionScope: single 下，频道内所有聊天共享同一路由（${channelName}:__single__ 键），但轮换提示的目标只由触发限度的那条消息构造——因此一次静默重置所有参与聊天上下文的轮换，恰好只通知其中一个聊天。toTarget 帮不上忙：它只记录创建会话的聊天，路由器至多知道参与聊天中的一个，任何单一目标都无法覆盖兄弟聊天（探针翻转：用创建时目标只通知 chat A；当前的输入派生目标只通知 chat B）。这是第 2 轮 R2-8 修复留下的镜像残余。——失败场景：已用探针复现——single 作用域频道、maxTurns: 2；chat A 发送第 1、2 轮，chat B 触发限度 → 恰好一条提示，只发给 chat B；chat A（在被轮换会话的 3 轮中贡献了 2 轮）收不到任何提示，它的下一条消息将遇到无解释的上下文重置——正是提示要避免的无解释重置（「轮换是自动的，参与者没有其他信号」）。无数据影响；代价是在 PR 文档自己点名的轮换使用场景下信号传递不完整。——建议修复：最便宜的是在文档 single 作用域 / Session Rotation 小节加一句：只有触发轮换的消息所在聊天会收到提示（共享会话的其他聊天只会看到重置而无提示）；完整做法是按单作用域路由记录参与目标集合并向其全部发送提示。
— qwen3.8-max via Qwen Code /review (v0.21.10)
) A reconnect READY can start a second restoreSessions() while the cold-start one still runs; the first finisher lifted the single persist-suspension flag and flushed while the second restore was mid-flight, making a truncated store durable. Suspend by depth and let only the last restore to finish flush, gated on a completed loop. Also document the rotation deferral's idle-window semantics (gate comment and Session Rotation docs) and that single-scope rotation notices only reach the triggering chat.
|
🤖 Addressed the latest review feedback (round 8/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 8/10 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #8927 (round on R5 feedback)Commit: Review-level CHANGES_REQUESTED (coverage notes, no code finding)The review recorded that its own coverage was incomplete: "Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally", and "did not execute ChannelBase.test.ts to confirm the loop-drop test passes as written". Response — the named suites were run this round:
No code change was required for this item. [rc:3766095214] R5-3 — Critical: overlapping restores can persist a truncated route storeVerified against this commit: Fixed (the reviewer's depth-counter direction):
The reviewer's alternative (serialize by returning the in-flight promise) was not taken: it would change caller semantics on the crash-recovery path (bridge swap + timed-out restores), where a fresh restore against the new bridge is the intended behavior. The depth counter fixes the durable corruption without changing who runs which restore. Regression test added: [rc:3766095229] R5-6 — Critical: rotation deferral is self-extending under sustained trafficVerified: the gate defers while a turn is running or queued, and each deferred message enqueues its own turn, so a route whose messages arrive continuously never consults Addressed via the reviewer's third (author's-choice) direction — make the idle-window semantics explicit instead of a behavioral redesign:
Why not the behavioral directions this round: both require rotating a route while turns are still pending for it and then handling those turns (bounce each dequeued turn back through routing, or narrow the deferral to running turns with dequeue-time re-routing of queued ones). That is a redesign of the exact mid-turn deferral machinery earlier rounds deliberately added ( [rc:3766095241] R5-8 — Suggestion: single-scope rotation notifies only the triggering chatAddressed with the reviewer's cheapest fix: the Session Rotation docs now state that with VerificationAll commands actually run, in order:
中文说明处理评审总结 — PR #8927（针对 R5 反馈的一轮）提交： 评审级 CHANGES_REQUESTED（覆盖度说明，非代码问题）评审记录了其自身覆盖不完整：「Integration Tests (CLI, No Sandbox) 在 CI 中被跳过且该套件未在本地运行」，以及「未执行 ChannelBase.test.ts 以确认 loop-drop 测试按所写方式通过」。 回应 — 本轮已运行上述点名的套件：
该条目无需代码改动。 [rc:3766095214] R5-3 — 严重：重叠恢复可能持久化截断的路由存储已在本 commit 核实： 已修复（采用评审建议的深度计数方向）：
评审的另一个方案（把在途 promise 返回给第二个调用者以实现串行化）未被采用：它会改变崩溃恢复路径上的调用者语义（bridge 更换 + 超时恢复的场景下，针对新 bridge 的全新恢复才是预期行为）。深度计数在持久化层面修复了截断问题，且不改变「谁执行哪次恢复」。 新增回归测试： [rc:3766095229] R5-6 — 严重：持续流量下轮换推迟会自我延长已核实：门控在回合运行或排队期间推迟轮换，而每条被推迟的消息都会入队自己的回合，因此消息持续到达的路由在流量停歇前不会再查询 按评审提供的第三个方向（作者可选）处理——把空闲窗口语义写明，而非做行为重构：
为何本轮不采取行为类方向：两者都需要在路由仍有在途回合时就轮换、随后再处理这些回合（把每个出队的回合弹回重新路由，或把推迟收窄为仅运行中回合并对排队回合在出队时重新路由）。这是对前几轮刻意加入的回合中推迟机制（ [rc:3766095241] R5-8 — 建议：single 作用域轮换只通知触发消息所在聊天采用评审给出的最便宜修复：Session Rotation 文档现写明， 验证以下命令均为实际运行，按顺序列出：
Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5 (round 5 still reported findings).
Not explored to full depth (tool budget reached): PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above completed within budget.; chunk 7: none — all planned checks completed (read full source, helpers, type definitions, test plugin setup; ran build + both test files).; PR #8927 adds an opt-in per-channel sessionRotation opt...: none — all checks above completed within budget.; chunk 3: none — all checks I started completed within budget.; PR #8927 adds an opt-in per-channel sessionRotation opt...: nothing materializing — but if I must name an unfinished check: did not run the mutation probes myself (relied on static tracing); also did not exhaustively tra…, and 3 more.
Test Plan (not a blocker): 1023 tests passed — this review observed 1067, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1067, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed.
中文说明
未审查：reverse audit — did not converge within the reverse-audit round cap of 5 (round 5 still reported findings)。
未探索到全部深度（达到工具调用预算）：PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above completed within budget.；chunk 7：none — all planned checks completed (read full source, helpers, type definitions, test plugin setup; ran build + both test files).；PR #8927 adds an opt-in per-channel sessionRotation opt...：none — all checks above completed within budget.；chunk 3：none — all checks I started completed within budget.；PR #8927 adds an opt-in per-channel sessionRotation opt...：nothing materializing — but if I must name an unfinished check: did not run the mutation probes myself (relied on static tracing); also did not exhaustively tra…，另有 3 条。
Test Plan（非阻断）：1023 tests passed — this review observed 1067, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1067, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (this.persistSuspendDepth > 0) { | ||
| // Mid-restore the store holds only the restored prefix; let | ||
| // restoreSessions() flush once the store is whole again. | ||
| this.persistRequestedWhileSuspended = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Critical] R6-1: The persist suspension opened by restoreSessions() is released only in that call's finally; if a restore never settles, persistSuspendDepth stays > 0 for the router's lifetime and every later persist() is silently dropped. Reachable at this commit: QQChannel fires restoreSessions() on cold-start READY with no timeout wrapper (coldStart resets to true on INVALID_SESSION / abnormal close, so a second cold-start READY can arrive while the first restore still runs); an ACP child death mid-restore leaves the awaited conn.loadSession(...) unsettled — the ACP SDK never rejects #pendingResponses on stream end. Crash recovery's nested restore on the same router takes depth 2→1, never 0; neither setBridge nor dispose() resets the depth. — Failure scenario: probe-reproduced — hung restore + completed recovery restore, then a new route mutation: the created route never reaches disk (all 12 persist() call sites funnel into the suspended branch); the routes file is frozen until process exit, and the next restart resurrects stale/rotated/deleted routes, re-firing rotation notices and discards. Pre-diff a hung restore left persistence working — this diff's suspension converts it into a permanent, silent persistence outage.
Suggested direction (hypothesis, untested beyond the probe): make the suspension robust against an abandoned restore — reject in-flight loadSession/newSession promises in AcpBridge's child-exit handler (the root cause), and/or key each suspension to the restore that opened it so setBridge/dispose() can drop stale ones; a bare depth reset must also handle a late-finishing stuck restore decrementing below zero.
中文说明
严重：restoreSessions() 打开的持久化挂起只在该调用的 finally 中释放；若恢复永不结束，persistSuspendDepth 将在路由器的整个生命周期内保持 > 0，此后每次 persist() 都被静默丢弃。本 commit 可达：QQChannel 在冷启动 READY 时以无超时包装调用 restoreSessions()（coldStart 在 INVALID_SESSION / 异常关闭时重置为 true，第一个恢复仍在运行时第二个冷启动 READY 可能到来）；恢复期间 ACP 子进程死亡会让被等待的 conn.loadSession(...) 永不落定——ACP SDK 在流结束时从不拒绝 #pendingResponses。崩溃恢复在同一路由器上再次恢复，深度 2→1 永不到 0；setBridge 与 dispose() 均不重置深度。——失败场景：已用探针复现——挂起的恢复 + 完成的崩溃恢复之后，一次新路由变更：创建的路由永不上盘（全部 12 个 persist() 调用点都汇入挂起分支）；routes 文件冻结至进程退出，下次重启复活过期/已轮换/已删除的路由，再次触发轮换提示与 discard。改动前恢复挂起不影响持久化；本 diff 的挂起机制把它变成永久的静默持久化中断。
建议方向（假设，仅在探针上验证过）：让挂起机制对「被抛弃的恢复」健壮——在 AcpBridge 子进程退出处理器中拒绝在途的 loadSession/newSession promise（根因），和/或把每次挂起关联到打开它的恢复，使 setBridge/dispose() 能丢弃过期挂起；直接重置深度还需处理卡住的恢复迟到结束时减到负数的问题。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| this.toTarget.set(sessionId, entry.target); | ||
| this.toCwd.set(sessionId, entry.cwd); | ||
| this.restoreRotationState(sessionId, entry); |
There was a problem hiding this comment.
[Critical] R6-2: Overlapping restores rewind LIVE rotation counters to the stale persisted snapshot: the second restore's reservation pass deleteByKeys already-restored-AND-routed keys (wiping toTurns/toStartedAt/sessionRoutingLeases), and restoreRotationState then unconditionally reseeds from the stale disk read; the last finisher's flush persists the regressed value, so the lost counts are permanent. The same wipe resets toStartedAt, extending a maxAgeHours bound. Overlaps are reachable: QQChannel fires restoreSessions() on every cold-start READY, and coldStart resets on INVALID_SESSION / abnormal close. The existing overlap test pins only whole-store key presence, so it stays green; this contradicts the diff's own 'overlapping restores are safe' comment. — Failure scenario: probe-reproduced deterministically — persisted turns:2 with maxTurns:4; cold-start restore #1 restores the key while another key still loads; a message routes onto the restored session mid-restore (countTurn → 3 in memory, persist suspended, disk still 2); a reconnect READY fires restore #2 — its reservation pass wipes the live counter and the reseed restores turns=2 from the stale disk read; both restores complete cleanly and the final flush durably writes turns:2. The session then carried 5 messages against a bound of 4 — the unbounded-growth failure rotation exists to prevent — and every later racing overlap adds more slack.
Suggested fix: carry live rotation state across the reservation pass — before each deleteByKey(key) capture the mapped session's toTurns/toStartedAt/sessionRoutingLeases (when present) and re-apply them after the load instead of letting restoreRotationState overwrite (in-memory values are always newer than a snapshot read while persistence was suspended); add an overlap test that routes a message mid-restore and pins that the counters survive.
中文说明
严重：重叠恢复会把存活的轮换计数器回退到过期的持久化快照：第二个恢复的预留阶段对「已恢复且已被路由」的键执行 deleteByKey（抹掉 toTurns/toStartedAt/sessionRoutingLeases），随后 restoreRotationState 无条件地用过期的磁盘读取重新播种；最后结束一方的 flush 把回退后的值固化，丢失的计数永久无法找回。同样的抹除会重置 toStartedAt，变相延长 maxAgeHours 限度。重叠可达：QQChannel 每次冷启动 READY 都触发 restoreSessions()，且 coldStart 在 INVALID_SESSION / 异常关闭时重置。现有重叠测试只钉住整库键存在性，因此保持绿色；这与本 diff 自己写的「重叠恢复是安全的」注释相矛盾。——失败场景：已用探针确定性复现——持久化 turns:2、maxTurns:4；冷启动恢复 #1 恢复该键时另一键仍在加载；恢复期间一条消息路由到该会话（内存 countTurn → 3，持久化被挂起，磁盘仍为 2）；重连 READY 触发恢复 #2——其预留阶段抹掉存活计数器并以过期磁盘值重新播种 turns=2；两个恢复都干净完成，最终 flush 把 turns:2 固化。该会话随后在限度 4 下承载了 5 条消息——正是轮换要防止的无界增长故障——且此后每次重叠竞争都会再放宽一格。
建议修复：让存活轮换状态跨越预留阶段——在每次 deleteByKey(key) 之前捕获该键映射会话的 toTurns/toStartedAt/sessionRoutingLeases（若存在），在加载完成后重新应用，而不是让 restoreRotationState 覆盖（持久化挂起期间内存值永远比快照新）；补一个在恢复中途路由消息并钉住计数器存活的重叠测试。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (this.creatingSessions.has(key) || this.toSession.has(key)) { | ||
| failedWaits++; | ||
| if (failedWaits > 3) throw creating.invalidationError; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R6-3: The waiter-retry branch and its failedWaits > 3 cap are only ever exercised with a single retry; no test parks a waiter through multiple consecutive invalidations or asserts the eventual give-up. Mutation-verified: changing the cap to > 1 or deleting it entirely (infinite retry) leaves all 735 SessionRouter+ChannelBase rotation tests green. — Failure scenario: a maxTurns: 1 route under sustained traffic while an eager restore has parked waiters can re-invalidate one message repeatedly; today it routes after ≤3 retries and throws invalidationError (dropping the message) on the 4th — neither half of that behaviour is pinned, so a refactor of the cap or the retry loop ships silently. (Production reach is bounded — creation-in-flight skips the rotation gate — which is why this is a test-gap Suggestion.)
Suggested fix: add a SessionRouter test that parks a waiter on a restore reservation and churns the route repeatedly while the waiter is parked: assert it still routes after two consecutive invalidations, and assert it rejects with the invalidation error once the retry budget is exhausted.
中文说明
建议：等待者重试分支及其 failedWaits > 3 上限只在单次重试情形下被执行；没有测试让等待者经历连续多次作废、也没有测试断言最终放弃。已用变异验证：把上限改为 > 1 或整体删除（无限重试），全部 735 个 SessionRouter+ChannelBase 轮换测试仍为绿。——失败场景：maxTurns: 1 的路由在 eager 恢复有等待者停靠时持续来消息，同一条消息可能被反复作废；当前行为是 ≤3 次重试内完成路由、第 4 次抛出 invalidationError（丢弃消息）——这两半行为都没有测试钉住，因此对上限或重试循环的重构会静默通过。（生产可达性有限——创建在途时轮换门控被跳过——故定级为测试缺口建议。）
建议修复：补一个 SessionRouter 测试：让等待者停靠在恢复预留上，在其停靠期间反复扰动该路由——断言连续两次作废后仍能路由，并断言重试预算耗尽时以作废错误拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Persist the retirement now: if the successor creation fails before its | ||
| // own persist, a restart must not restore the stale route and re-fire the | ||
| // whole rotation (a second notice and discard for what was one rotation). | ||
| this.persist(); |
There was a problem hiding this comment.
[Suggestion] R6-5: rotateRoute's immediate retirement persist is silently a no-op while persistence is suspended mid-restoreSessions() — the user-visible effects (chat notice + session discard) fire immediately, but durability waits for the restore's end flush. Probe-reproduced end-to-end, including the crash-restart double rotation. — Failure scenario: a channel restarts with a persisted session already at its bound; an inbound message arrives during restoreSessions() and rotates it — notice posted, session discarded — but this persist() hits the persistSuspendDepth > 0 early return. If the process dies before the restore's final flush (crash/SIGKILL in the recovery window; the flush is also skipped when restoreGeneration !== lifecycleGeneration), the on-disk store still maps the key to the retired session; next startup restores it and rotates it again — a second chat announcement and a second discard for what was one rotation, exactly what this comment exists to prevent.
Suggested fix: make the retirement durable before the user-visible effects fire, or exempt the retirement write from suspension (the post-rotation store is whole for the rotated key — the route is deleted, not partial); at minimum document in the suspend contract that a crash between a mid-restore rotation and the end flush re-fires the rotation, and pin the chosen behaviour with a test.
中文说明
建议：rotateRoute 的立即退役持久化在 restoreSessions() 挂起期间是静默空操作——用户可见的效果（聊天提示 + 会话 discard）立即发生，但持久化要等恢复结束时的 flush。已用探针端到端复现，包括崩溃重启后的二次轮换。——失败场景：频道重启时持久化会话已达限度；restoreSessions() 期间一条消息到达并触发轮换——提示已发出、会话已 discard——但这里的 persist() 命中 persistSuspendDepth > 0 提前返回。若进程在恢复末尾 flush 之前死亡（恢复窗口内崩溃/SIGKILL；restoreGeneration !== lifecycleGeneration 时 flush 也会被跳过），磁盘存储仍把该键映射到已退役会话；下次启动恢复该路由并再次轮换——同一次轮换产生第二次聊天提示与第二次 discard，正是本注释要防止的结果。
建议修复：让退役在用户可见效果发生之前先固化，或让退役写盘豁免挂起（轮换后的存储对被关闭的键是完整的——路由是被删除而非部分状态）；至少在挂起契约中写明「恢复中途的轮换与末尾 flush 之间崩溃会重放轮换」，并用测试钉住所选行为。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const firstId = await first; | ||
| const secondId = await second; | ||
| router.releaseRoutingLease(firstId); | ||
| router.releaseRoutingLease(secondId); |
There was a problem hiding this comment.
[Suggestion] R6-6: The only test routing a message through the waiter invalidation-retry path asserts identity only — it never pins that the retried message is counted. Probe-verified: a faithful short-circuit that skips countTurn + leaseSession for invalidation-retried messages keeps all 39 rotation tests green while the successor's toTurns drops 2→1; reverting flips the probe. (Distinct from R6-3, which covers the give-up cap.) — Failure scenario: a refactor returning the successor directly instead of continue-ing through the loop skips countTurn for every message routed via a retry: the session carries one more turn than maxTurns allows before rotating — the bound silently weakened for exactly the messages arriving during restore/rotation churn — and this test still passes (nothing reads the counter; releaseRoutingLease on a never-leased session is a silent no-op).
Suggested fix: after the two lease releases, assert the carried count — expect(rotationCounters(router).toTurns.get(firstId)).toBe(2); — and add one routed(...) reuse/final-rotation pair so the retried message's turn is observable in the bound.
中文说明
建议：唯一把消息经由「等待者作废重试」路径路由的测试只断言了会话身份——从未钉住重试消息被计数。已用探针验证：一个忠实的短路实现（对作废重试的消息跳过 countTurn + leaseSession）让全部 39 个轮换测试保持绿色，而后继会话的 toTurns 从 2 掉到 1；回退后探针翻转。（与 R6-3 不同，那条覆盖的是放弃上限。）——失败场景：若重构直接返回后继会话而不再 continue 走循环，每条经重试路由的消息都会跳过 countTurn：会话在轮换前会比 maxTurns 多承载一轮——恰恰是恢复/轮换扰动期间到达的消息被静默放宽——而本测试仍然通过（没有任何断言读计数器；对从未取得租约的会话调用 releaseRoutingLease 是静默空操作）。
建议修复：在两次释放租约之后断言携带的计数——expect(rotationCounters(router).toTurns.get(firstId)).toBe(2);——并追加一组 routed(...) 复用/最终轮换断言，使重试消息的轮次在限度中可观察。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Creator and waiter both counted: the next message hits the bound. | ||
| expect(await routed(router, 'ch', 'alice', 'chat1')).not.toBe( | ||
| 'busy-session', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R6-7: The leaseSession call on resolve()'s waiter-success path is unpinned: mutation-verified, deleting only that line leaves all 735 tests green, while the adjacent countTurn IS pinned by this test (mutating the comparator fails it — the harness is alive). Distinct from R6-3/R6-6 (different branch, different mutant). — Failure scenario: with maxTurns: 1, message A creates session S and message B waits on the same creation (both counted); while B is in the async gap between resolve() returning and trackSessionTurn(), message C arrives: without the lease the rotation gate sees no lease and no pending turns, rotates, and discards S out from under B's imminent prompt — auto-cancelled approvals and dropped late output, the exact hazard the lease machinery exists to prevent.
Suggested fix: pin the waiter's lease — after expect(await waiter).toBe('busy-session'); assert expect(routingLeases(router).get('busy-session')).toBe(1); (creator lease already released), or behaviorally hold the waiter lease unreleased and assert an at-bound routed(...) still defers.
中文说明
建议：resolve() 等待者成功路径上的 leaseSession 调用没有测试钉住：已用变异验证，仅删除该行全部 735 个测试仍为绿，而相邻的 countTurn 被本测试钉住（变异对照会失败——测试装置是有效的）。与 R6-3/R6-6 不同（不同分支、不同变异体）。——失败场景：maxTurns: 1 时，消息 A 创建会话 S，消息 B 等待同一创建（两者都被计数）；当 B 处于 resolve() 返回与 trackSessionTurn() 之间的异步间隙时，消息 C 到达：若没有租约，轮换门控看不到租约也看不到待处理回合，触发轮换并把 S 从 B 即将发起的 prompt 脚下 discard——审批被自动取消、迟到输出被丢弃，正是租约机制要防止的危害。
建议修复：钉住等待者的租约——在 expect(await waiter).toBe('busy-session'); 之后断言 expect(routingLeases(router).get('busy-session')).toBe(1);（创建者租约已释放），或行为化地不释放等待者租约并断言到限的 routed(...) 仍被推迟。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| completed && | ||
| this.persistSuspendDepth === 0 && | ||
| (changed || this.persistRequestedWhileSuspended) && | ||
| restoreGeneration === this.lifecycleGeneration | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R6-8: Overlapping restores can drop an earlier finisher's changed signal: this flush decision reads only the LAST finisher's local changed plus the persist-request flag, so a key another restore failed to load (and dropped from memory) can be left behind on disk. Probe-reproduced deterministically; one-line flip verified (setting persistRequestedWhileSuspended = true in the per-key failure catch makes the last finisher flush the whole store). — Failure scenario: restore A (slow) running; a reconnect READY fires restore B; B's reservation pass runs after A restored key K; B's loadSession(K) fails (B drops K, changed=true, correctly defers the flush); A finishes its remaining keys with no drops (changed=false) and no persist was requested mid-window, so A's finally skips the flush: memory no longer maps K but disk still carries K's stale entry. Self-heals on the next unrelated persist or restart — a bounded, recoverable divergence, but it contradicts the 'the last restore to finish flushes the whole store' contract this finally block documents.
// one possible fix (probe-verified):
} catch (err) {
this.persistRequestedWhileSuspended = true; // carry the change signal router-level中文说明
建议：重叠恢复可能丢弃先结束一方的 changed 信号：这个 flush 判定只读最后结束一方的局部 changed 与持久化请求标志，因此另一个恢复加载失败（并已从内存中删除）的键可能一直被留在磁盘上。已用探针确定性复现；单行翻转已验证（在按键失败 catch 中设置 persistRequestedWhileSuspended = true 可让最后结束方固化完整存储）。——失败场景：恢复 A（慢）运行中；重连 READY 触发恢复 B；B 的预留阶段在 A 已恢复键 K 之后执行；B 的 loadSession(K) 失败（B 删除 K、changed=true、正确推迟 flush）；A 无丢弃地完成其余键（changed=false）且窗口内无持久化请求，于是 A 的 finally 跳过 flush：内存已不再映射 K，但磁盘仍留着 K 的过期条目。会在下一次无关持久化或重启时自愈——有限且可恢复的偏差，但与该 finally 块所记载的「最后结束的恢复固化完整存储」契约相矛盾。
// 一种可行修复（已用探针验证）：
} catch (err) {
this.persistRequestedWhileSuspended = true; // 把变更信号提升到路由器级— qwen3.8-max via Qwen Code /review (v0.21.10)
…es honest (#8927) The ACP SDK never settles requests awaiting a response once the stream ends, so a child death mid-restore left restoreSessions() hung and its persist suspension lifted never. Reject in-flight newSession/loadSession when the child exits. Overlapping restores rewound live rotation counters to the stale persisted snapshot and could strand an earlier finisher's dropped key on disk: carry live rotation state across the reservation pass and let a per-key load failure carry its flush signal to the last finisher.
|
🤖 Addressed the latest review feedback (round 9/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 9/10 轮）。改动内容与我反驳保留之处如下： Round 7 summary — PR #8927 review feedback addressedAll seven inline findings from the round-6 review were verified against the exact code at HEAD and addressed in one commit ( Critical findings
Suggestions
Dispositions
No finding was declined, deferred, or escalated. Verification
中文说明第 7 轮总结 — PR #8927 评审反馈处理第 6 轮评审的全部 7 条内联发现均已在 HEAD 代码上逐条核实，并在一个提交（ Critical 发现
建议类
处置一览
没有发现被拒绝、延后或升级。 验证
Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (round 4 reported findings; round 5 dry).
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): You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget.; chunk 1: could not run AcpBridge.test.ts / ChannelBase.test.ts — no node_modules in the worktree or the parent checkout. Static verification only.; chunk 1: could not execute AcpBridge.test.ts or ChannelBase.test.ts — node_modules is missing in the worktree and the parent checkout, so verification was static only.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; chunk 7: executing the three test files in this chunk (review worktree has no node_modules; verified statically instead — every assertion maps 1:1 to implementation beha….
Test Plan (not a blocker): 1023 tests passed — this review observed 1073, 18, 19078, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1073, 18, 19078, 297, 266, 205, 59, 289, 134, 71 passed.
中文说明
未审查：reverse audit — reached the 5-round cap without two consecutive dry rounds (round 4 reported findings; round 5 dry)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.；chunk 1：could not run AcpBridge.test.ts / ChannelBase.test.ts — no node_modules in the worktree or the parent checkout. Static verification only.；chunk 1：could not execute AcpBridge.test.ts or ChannelBase.test.ts — node_modules is missing in the worktree and the parent checkout, so verification was static only.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；chunk 7：executing the three test files in this chunk (review worktree has no node_modules; verified statically instead — every assertion maps 1:1 to implementation beha…。
Test Plan（非阻断）：1023 tests passed — this review observed 1073, 18, 19078, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1073, 18, 19078, 297, 266, 205, 59, 289, 134, 71 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (liveRotation.leases) { | ||
| this.sessionRoutingLeases.set( | ||
| sessionId, | ||
| (this.sessionRoutingLeases.get(sessionId) ?? 0) + liveRotation.leases, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Critical] R7-1: Overlapping/crash restore permanently leaks routing leases. The reservation pass captures the live lease count, deleteByKey wipes the entry (turning the owner's later releaseRoutingLease into a no-op), and this carry then re-adds the captured count — a phantom lease that never drains. hasRoutingLease stays true in the rotation gate forever, so rotation is permanently deferred for that route and the configured bound is silently never enforced again until restart. — Failure scenario: channel with maxTurns; a user runs a ! shell command (the bang path deliberately holds resolve()'s lease across await bridgeShellCommand, releasing only in finally). While it runs, bridge crash recovery calls restoreSessions(): the pass captures leases=1 and deleteByKey deletes the entry; the shell settles and its finally release is a no-op (entry gone); the load completes and the carry re-adds lease=1. Probe-reproduced at this commit: 5 messages past the bound on one session, zero rotations; deleting the leases block from the carry flips the probe (rotation fires). Reported independently by three review agents. — Suggested fix: make the balance signed — in releaseRoutingLease, decrement even when the entry is absent (keep hasRoutingLease as > 0) so releases landing in the wipe→carry window net against the carry; or drop leases from the carry entirely — the pending-turn count already covers registered turns.
中文说明
严重：重叠/崩溃恢复会永久泄漏路由租约。预留阶段捕获存活租约数，deleteByKey 抹掉条目（使持有者随后的 releaseRoutingLease 变成空操作），这里的 carry 又把捕获的数量加回去——形成一个永远不会排空的幽灵租约。hasRoutingLease 在轮换门控中永远为真，该路由的轮换被永久推迟，配置的限度在重启之前被静默地不再执行。——失败场景：配置 maxTurns 的频道；用户执行 ! shell 命令（bang 路径故意在 await bridgeShellCommand 期间持有 resolve() 的租约，只在 finally 中释放）。命令运行期间 bridge 崩溃恢复调用 restoreSessions()：预留阶段捕获 leases=1，deleteByKey 删除条目；shell 结束后其 finally 的释放成为空操作（条目已不存在）；加载完成后 carry 把 lease=1 加回。已在本 commit 用探针复现：超出限度 5 条消息全部路由到同一会话、零轮换；删除 carry 中的 leases 块后探针翻转（轮换正常触发）。三个评审代理独立报告了该问题。——建议修复：让租约余额带符号——releaseRoutingLease 在条目不存在时也做减法（hasRoutingLease 保持 > 0 判断），使落在「抹除→carry」窗口内的释放能与 carry 相抵；或者干脆不 carry leases——已登记的回合由 pending-turn 计数覆盖。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const liveSessionId = this.toSession.get(key); | ||
| // A route already back in memory can have routed messages newer than | ||
| // the persisted snapshot (persists stay suspended across the restore | ||
| // window); carry its rotation state across the wipe so the reload | ||
| // below cannot rewind it. |
There was a problem hiding this comment.
[Critical] R7-14: A route deleted mid-restore is silently resurrected by an overlapping restore — the live-state carry can only represent presence. /clear (or a removeSessionId retirement / timed-out-loop eviction) landing while a restore is open has its persist() suspended, so the removal never reaches disk; an overlapping restore then reads the pre-deletion snapshot, finds no live route for the key (nothing to capture), reloads the stale entry's session, re-adds the route, and the last finisher's flush durably re-persists it. — Failure scenario: QQ cold-start restore #1 in flight; the user sends /clear (slash commands dispatch before routing, so it never waits on the reservation) and is told "the next message starts a fresh conversation"; a WS drop re-arms coldStart and the reconnect READY fires restore #2 while #1 still runs — the cleared route is resurrected and the next message continues the "cleared" conversation (privacy-relevant for shared sessions). Death-retirements likewise resurrect pointing at discarded sessions. Probe-reproduced and flipped at this commit; pre-diff the deletion persisted immediately, so this resurrection is a new failure mode introduced by this diff. — Suggested fix: track deletions across the suspension window — a tombstone set of keys removed while persistSuspendDepth > 0; skip tombstoned keys in the reservation/load pass; clear the set in the last finisher's flush.
中文说明
严重：恢复窗口中途被删除的路由会被重叠恢复静默复活——存活状态 carry 只能表达「存在」。恢复进行中落下的 /clear（或 removeSessionId 退役、超时 loop 驱逐）的 persist() 被挂起，删除永不上盘；重叠恢复随后读到删除前的快照，发现该键没有存活路由（无可捕获），重新加载旧条目里的会话、重新添加路由，最后结束者的 flush 把它持久化。——失败场景：QQ 冷启动恢复 #1 进行中；用户发送 /clear（slash 命令在路由之前派发，不会等待预留），并看到「下一条消息将开启全新对话」；WS 断开使 coldStart 重新置位，重连 READY 在 #1 仍未结束时触发恢复 #2——被清空的路由复活，下一条消息继续「已被清空」的对话（对共享会话涉及隐私）。死亡退役同样会复活并指向已 discard 的会话。已在本 commit 用探针复现并翻转；改动前删除会立即落盘，因此复活是本 diff 引入的新故障形态。——建议修复：跨挂起窗口追踪删除——记录挂起期间被移除键的墓碑集合；预留/加载阶段跳过墓碑键；在最后结束者的 flush 中清空该集合。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| stop(): void { | ||
| this.rejectPendingSessionRequests(); |
There was a problem hiding this comment.
[Suggestion] R7-5: The stop() rejection path for in-flight session requests added in this diff has no test; the only new test exercises the child-exit path. — Failure scenario: mutation-verified at this commit — deleting this rejectPendingSessionRequests() call leaves all 37 AcpBridge tests green, so if a later change drops it, a newSession/loadSession racing channel shutdown hangs its caller forever (the exact hang this PR fixes) with no red test. — Suggested fix: extend the new test (or add a sibling): hold a never-resolving newSession, call bridge.stop(), and assert rejection with the same reason string.
中文说明
建议：本 diff 新增的 stop() 对在途会话请求的拒绝路径没有测试；唯一的新测试只覆盖了子进程 exit 路径。——失败场景：已在本 commit 做变异验证——删除这行 rejectPendingSessionRequests() 后全部 37 个 AcpBridge 测试仍为绿，因此后续改动若删掉它，与频道关闭竞争的 newSession/loadSession 会永远挂起调用方（正是本 PR 修复的挂起），且没有任何测试变红。——建议修复：扩展新测试（或补一个姊妹测试）：持有一个永不落定的 newSession，调用 bridge.stop()，断言以相同原因串被拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| (result) => { | ||
| this.pendingSessionRequests.delete(pending); | ||
| resolve(result); | ||
| }, |
There was a problem hiding this comment.
[Suggestion] R7-6: Test-efficacy probe (harness validated): no test fails when this success-path pendingSessionRequests.delete(pending) is deleted. — Failure scenario: if a future change drops this delete, every successfully settled session request leaves its { reject } record in pendingSessionRequests for the connection's lifetime; the Set grows with traffic and the next child exit iterates all stale records and re-rejects already-settled promises. — Suggested fix: add a test that settles a session request successfully and then asserts the pending set is empty (e.g. a subsequent child exit rejects nothing).
中文说明
建议：测试有效性探针（harness 已验证）：删除成功路径上的这个 pendingSessionRequests.delete(pending) 时没有任何测试失败。——失败场景：若后续改动删除该 delete，每个成功落定的会话请求都会在连接生命周期内留下 { reject } 记录；Set 随流量增长，下一次子进程退出会遍历所有过期记录并重复拒绝已落定的 promise。——建议修复：补一个测试——让一个会话请求成功落定，随后断言 pending 集合为空（例如此后的子进程退出不再拒绝任何东西）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| (error: unknown) => { | ||
| this.pendingSessionRequests.delete(pending); | ||
| reject(error); | ||
| }, |
There was a problem hiding this comment.
[Suggestion] R7-7: Test-efficacy probe (harness validated): no test fails when this error-path pendingSessionRequests.delete(pending) is deleted. — Failure scenario: if this delete is removed, every failed session request keeps its record in pendingSessionRequests; the next child exit calls reject again on already-rejected promises and retains the records indefinitely — the same unbounded-growth mechanism as the success path, exercised by request failures. — Suggested fix: add a test that fails a session request and then asserts a subsequent child-exit settlement finds no pending records.
中文说明
建议：测试有效性探针（harness 已验证）：删除错误路径上的这个 pendingSessionRequests.delete(pending) 时没有任何测试失败。——失败场景：若删除该 delete，每个失败的会话请求都会保留其记录；下一次子进程退出会对已拒绝的 promise 再次调用 reject 并无限期保留记录——与成功路径相同的无界增长机制，由请求失败触发。——建议修复：补一个测试——让一个会话请求失败，随后断言子进程退出结算时不再有 pending 记录。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| private purgeSessionState(sessionId: string): void { | ||
| this.instructedSessions.delete(sessionId); | ||
| this.unattendedMemorySessions.delete(sessionId); |
There was a problem hiding this comment.
[Suggestion] R7-9: Rotation retires a session ID permanently, but handleSessionRotated/purgeSessionState never reclaim sessionQueues[sessionId] (and sessionGenerations[sessionId] when /clear-bumped) — every rotation leaks both entries for the gateway's lifetime. — Failure scenario: probe-verified at this commit — after a rotation, sessionQueues still holds the retired ID. The comment's justifications (a queued turn may still hold the captured chain; lazy recovery may re-attach the same ID) hold for the death path but not rotation: the rotation gate defers while pending turns or leases exist, so the chain is drained at rotation time, and a rotated ID is never re-attached. Growth is unbounded in the number of rotations; the router side reclaims everything it owns via deleteByKey. This re-states round-2 R2-14's observation with the rotation-vs-death distinction and a verified fix. — Suggested fix: in handleSessionRotated only (keep purgeSessionState's death-path semantics), also delete the sessionQueues and sessionGenerations entries — verified safe: all 57 rotation tests still pass with that fix applied.
中文说明
建议：轮换会永久退役一个会话 ID，但 handleSessionRotated/purgeSessionState 从不回收 sessionQueues[sessionId]（以及被 /clear bump 过的 sessionGenerations[sessionId]）——每次轮换都会在网关生命周期内泄漏这两个条目。——失败场景：已在本 commit 用探针验证——轮换之后 sessionQueues 仍持有已退役的 ID。注释里的理由（排队的回合可能仍持有捕获的链；lazy 恢复可能重新挂载同一 ID）对死亡路径成立，但对轮换不成立：轮换门控会在有 pending 回合或租约时推迟，轮换时链已排空，且被轮换的 ID 不会被重新挂载。泄漏随轮换次数无界增长；路由器一侧已通过 deleteByKey 回收其全部自有状态。这是对第二轮 R2-14 观察的重述，补充了轮换与死亡的区分和已验证的修复。——建议修复：仅在 handleSessionRotated 中（保留 purgeSessionState 的死亡路径语义）同时删除 sessionQueues 与 sessionGenerations 条目——已验证安全：应用该修复后全部 57 个轮换测试仍然通过。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const turns = this.toTurns.get(sessionId); | ||
| if (turns === undefined || turns <= 1) return; | ||
| this.toTurns.set(sessionId, turns - 1); |
There was a problem hiding this comment.
[Suggestion] R7-10: The "never below one" floor silently swallows the give-back when the uncounted message was the session's first counted message — that session then rotates one turn early for its entire lifetime. — Failure scenario: probe-verified at this commit: a loop firing on a route with no session creates it (seed turns=1), is queued, then dropped via shouldContinue (the scheduler's async findJob is the drop window); this floor sees turns===1 and returns without decrementing, so the counter stays at 1 although zero turns ran — with maxTurns: 2 the session rotated after ONE real turn. Applying the fix below flips the probe to rotate exactly on time. The floor's actual function is papering over a persistence-validation gap (a persisted turns: 0 would fail isPersistedEntry and drop the whole route on restore). Flip the seed-floor test (companion comment on SessionRouter.test.ts) in the same change.
| const turns = this.toTurns.get(sessionId); | |
| if (turns === undefined || turns <= 1) return; | |
| this.toTurns.set(sessionId, turns - 1); | |
| const turns = this.toTurns.get(sessionId); | |
| if (turns === undefined) return; | |
| if (turns <= 1) { | |
| this.toTurns.delete(sessionId); | |
| } else { | |
| this.toTurns.set(sessionId, turns - 1); | |
| } |
中文说明
建议：「不低于 1」的下限会在被回退的消息是会话第一个计数消息时静默吞掉这次回退——该会话此后整个生命周期都会提前一轮轮换。——失败场景：已在本 commit 用探针验证：没有会话的路由上 loop 触发创建会话（种子 turns=1）、入队后被 shouldContinue 丢弃（调度器的异步 findJob 即丢弃窗口）；该下限看到 turns===1 直接返回不减，零回合运行过计数器却停留在 1——maxTurns: 2 时会话在仅 1 个真实回合后就轮换。应用下面的修复后探针翻转为按时轮换。该下限的实际作用是掩盖持久化校验缺口（持久化的 turns: 0 过不了 isPersistedEntry，会在恢复时丢掉整条路由）。请在同一改动中翻转 SessionRouter.test.ts 上的配套种子下限测试。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (liveRotation.turns !== undefined) { | ||
| this.toTurns.set(sessionId, liveRotation.turns); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R7-11: A turn-count decrement (uncountTurn) that lands while the restore reservation pass has wiped the route is silently dropped, and this carry then overwrites with the captured pre-decrement count — one persisted phantom count. — Failure scenario: probe-verified with a discriminator arm (same inputs, only timing differs): a maxTurns channel in collect mode — a message resolves (countTurn N→N+1), the documented overlapping restore captures N+1 and deleteByKey removes toTurns, the buffered message's uncountTurn no-ops (entry gone), the carry writes N+1 back, and the drain re-counts the coalesced message → N+2 for one real message, persisted, so the bound fires one message early. The loop-path variant (ChannelBase.ts:1530) loses its give-back with no re-count ever arriving. Same wipe-window hazard class as the confirmed lease leak (R7-1), applied to the counter. — Suggested fix: make the wipe/carry pair decrement-aware — capture the counter at reservation time as a baseline and apply the delta at carry time, or defer uncountTurn/releaseRoutingLease writes against a wiped-but-reserved session (queue them on the reservation).
中文说明
建议：落在恢复预留阶段抹除路由窗口内的回合计数减少（uncountTurn）会被静默丢弃，随后这里的 carry 用捕获的减少前的值覆盖——产生一个被持久化的幽灵计数。——失败场景：已用带对照的探针验证（相同输入、仅时序不同）：collect 模式的 maxTurns 频道——一条消息 resolve（countTurn N→N+1），文档化的重叠恢复捕获 N+1 并由 deleteByKey 移除 toTurns，被缓冲消息的 uncountTurn 成为空操作（条目已不存在），carry 写回 N+1，drain 重新计数合并消息 → 一条真实消息对应 N+2 并被持久化，限度提前一条消息触发。loop 路径变体（ChannelBase.ts:1530）的回退彻底丢失且不会再有重计。与已确认的租约泄漏（R7-1）同属抹除窗口危害类，作用于计数器。——建议修复：让抹除/carry 对减少敏感——预留时捕获计数器作为基线，carry 时应用差值；或把针对「已抹除但有预留」会话的 uncountTurn/releaseRoutingLease 写入推迟（挂到预留上）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| } finally { | ||
| this.router.releaseRoutingLease(sessionId); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R7-15: The ! shell-command path is a third "no turn will start for this message" routing, but unlike the other two it refunds only the routing lease, not the resolve-time turn count — every shell command consumes one maxTurns unit without ever prompting a turn. — Failure scenario: probe-verified at this commit: with maxTurns: 3, after 1 real turn + 2 shell messages the next resolve rotated (adding an uncountTurn flips the probe). The collect-buffer and loop-drop paths both refund via uncountTurn, and this path's own comment says "No turn will start for this message". A 1:1 route used purely for shell commands rotates after N commands with zero prompted turns and a misleading "reached its configured limit" notice; mixed shell/chat routes lose context sooner than maxTurns intends. — Suggested fix: if the bound counts prompted turns, mirror the buffered path in this finally (this.router.uncountTurn(this.name, sessionId); before the release). If it deliberately counts routed messages (docs: "messages have used the current session"), align the docs/comment rationale and reconsider the buffered/loop refunds — the three no-turn paths currently disagree.
中文说明
建议：! shell 命令路径是第三条「本消息不会开始回合」的路由，但与前两条不同，它只归还路由租约、不归还 resolve 时取得的回合计数——每条 shell 命令都会消耗一个 maxTurns 单位却从未发起回合。——失败场景：已在本 commit 用探针验证：maxTurns: 3 时，1 个真实回合 + 2 条 shell 消息后下一次 resolve 触发轮换（补上 uncountTurn 后探针翻转）。collect 缓冲与 loop 丢弃路径都通过 uncountTurn 回退，且本路径自己的注释写着「本消息不会开始回合」。纯 shell 使用的 1:1 路由会在 N 条命令后轮换（零回合）并显示误导性的「已达配置限度」提示；shell/聊天混合路由会比 maxTurns 预期更早丢失上下文。——建议修复：若限度统计的是发起的回合，在这个 finally 中仿照缓冲路径（release 之前加 this.router.uncountTurn(this.name, sessionId);）；若有意统计路由消息数（文档：「消息使用了当前会话」），请统一文档/注释口径并重新考虑缓冲/loop 的回退——三条无回合路径目前互相矛盾。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| router.uncountTurn('ch', first); | ||
| expect(rotationCounters(router).toTurns.get(first)).toBe(1); |
There was a problem hiding this comment.
[Suggestion] R7-16: This new test ('never uncounts a session below its seed turn') pins exactly the floor semantics the companion finding (R7-10) identifies as defective — it blesses the swallowed give-back (rotation one turn early) and fails the verified R7-10 fix. — Failure scenario: mutation-verified at this commit: applying R7-10's fix (represent zero as an absent counter) makes exactly 1 of 141 SessionRouter tests fail — this one (expected undefined to be 1). Until R7-10 is fixed the suite certifies the defect; a future fixer flipping the floor gets a red test that defends the bug. The test's comment is backwards: it is the floor, not the uncount, that leaves "the bound consuming a slot nothing ever used" — the loop-path give-back comment at ChannelBase.ts:1528-1530 states the opposite principle. — Suggested fix: update in the same change as the R7-10 fix — assert toTurns.has(first) is false after the uncount, then expect two full reuses before rotation. If the maintainer instead rules the floor intended, R7-10 evaporates but that loop-path comment must be rewritten.
中文说明
建议：这个新测试（'never uncounts a session below its seed turn'）钉住的正是配套发现（R7-10）认定为缺陷的下限语义——它认可了被吞掉的回退（提前一轮轮换），并会使已验证的 R7-10 修复失败。——失败场景：已在本 commit 做变异验证：应用 R7-10 的修复（以「无计数器」表示零）后，141 个 SessionRouter 测试中恰好这一个失败（expected undefined to be 1）。在 R7-10 修复之前，测试套件在为该缺陷背书；未来翻转下限的修复者会得到一个为 bug 辩护的红色测试。测试注释说反了：正是下限（而不是 uncount）在「让限度消耗一个从未被使用的槽位」——ChannelBase.ts:1528-1530 的 loop 路径回退注释陈述了相反的原则。——建议修复：与 R7-10 的修复同一改动中更新——uncount 后断言 toTurns.has(first) 为 false，然后期望轮换前完整复用两次。若维护者裁定下限是有意设计，R7-10 消失，但那条 loop 路径注释必须重写。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Addressed the latest review feedback (round 10/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 10/10 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — round summary (PR #8927)All 11 inline findings from the round-7 reverse audit are resolved in the code (commit Critical findings
Suggestions implemented
Notes
VerificationCommands actually run in this checkout (all after the fix commit):
中文说明已处理的评审反馈 — 轮次总结（PR #8927）第 7 轮 reverse audit 的全部 11 条内联发现均已在代码中解决（提交 Critical 发现
已实现的建议
说明
验证本次 checkout 中实际执行的命令（均在修复提交之后）：
Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ AutoFix paused: this PR reached its automatic round cap (10/10) and the loop will not manage it further — new feedback and base conflicts stay unhandled. Comment 中文说明⏸️ AutoFix 已暂停：本 PR 达到自动轮次上限（10/10），循环不再管理——新反馈与 base 冲突将无人处理。评论 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 4 and 5 both reported findings).
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): PR #8927 adds a per-channel sessionRotation option (max...: none — all checks above were completed within budget.; PR #8927 adds a per-channel sessionRotation option (max...: none — finished within the tool budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above ran to a conclusion within budget.; You are review agent reverse-audit — Reverse audit agen...: none — the full assigned range was read to completion and every check above was finished., and 4 more.
Test Plan (not a blocker): 1023 tests passed — this review observed 1082, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1082, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed.
[Critical] R6-1 (re-check: still stands, narrowed): the persist suspension opened by restoreSessions() still has no timeout/lifecycle cleanup at this head (SessionRouter.ts ~1036). The child-death trigger is fixed (AcpBridge now rejects in-flight session requests on child exit; 5-minute stall watchdog), but the QQChannel cold-start READY restore remains fire-and-forget with no timeout race on the long-lived shared router (coldStart re-arms on non-1000 WS close / INVALID_SESSION, so restores overlap), and a wedged-but-alive ACP child whose session/load never responds — an async I/O hang, no exit, no event-loop stall, so neither guard fires — pins persistSuspendDepth ≥ 1 for the router's lifetime: every later persist() is silently dropped and the on-disk route store goes permanently stale until restart (probe-traced mechanism; trigger likelihood is the low-confidence part).
[Critical] R8-10: the new waiter retry heuristic (SessionRouter.ts:468-472) classifies any invalidation with a successor as a rotation/reload handoff, but removeSession (/clear) followed immediately by a new message produces the same shape — so a message parked on the restore reservation at clear time retries and is delivered into the fresh post-clear session instead of being dropped (pre-PR it threw invalidationError unconditionally). Probe-reproduced at this commit: removedIds at /clear [], parked waiter resolved to the post-clear session; reverting to the unconditional throw flips it. ChannelBase's generation guard cannot catch it (generation snapshot at enqueue, after resolve returns; /clear never saw the new session ID). Relocated to the body because its anchor line overlaps the existing R6-3 test-coverage thread at :472 — this is a distinct Critical, not a re-report of R6-3.
中文说明
未审查：reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 4 and 5 both reported findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #8927 adds a per-channel sessionRotation option (max...：none — all checks above were completed within budget.；PR #8927 adds a per-channel sessionRotation option (max...：none — finished within the tool budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above ran to a conclusion within budget.；You are review agent reverse-audit — Reverse audit agen...：none — the full assigned range was read to completion and every check above was finished.，另有 4 条。
Test Plan（非阻断）：1023 tests passed — this review observed 1082, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed; 158 tests passed — this review observed 1082, 18, 19084, 297, 266, 205, 59, 289, 134, 71 passed。
[Critical] R6-1 (re-check: still stands, narrowed): the persist suspension opened by restoreSessions() still has no timeout/lifecycle cleanup at this head (SessionRouter.ts ~1036). The child-death trigger is fixed (AcpBridge now rejects in-flight session requests on child exit; 5-minute stall watchdog), but the QQChannel cold-start READY restore remains fire-and-forget with no timeout race on the long-lived shared router (coldStart re-arms on non-1000 WS close / INVALID_SESSION, so restores overlap), and a wedged-but-alive ACP child whose session/load never responds — an async I/O hang, no exit, no event-loop stall, so neither guard fires — pins persistSuspendDepth ≥ 1 for the router's lifetime: every later persist() is silently dropped and the on-disk route store goes permanently stale until restart (probe-traced mechanism; trigger likelihood is the low-confidence part).
[Critical] R8-10: the new waiter retry heuristic (SessionRouter.ts:468-472) classifies any invalidation with a successor as a rotation/reload handoff, but removeSession (/clear) followed immediately by a new message produces the same shape — so a message parked on the restore reservation at clear time retries and is delivered into the fresh post-clear session instead of being dropped (pre-PR it threw invalidationError unconditionally). Probe-reproduced at this commit: removedIds at /clear [], parked waiter resolved to the post-clear session; reverting to the unconditional throw flips it. ChannelBase's generation guard cannot catch it (generation snapshot at enqueue, after resolve returns; /clear never saw the new session ID). Relocated to the body because its anchor line overlaps the existing R6-3 test-coverage thread at :472 — this is a distinct Critical, not a re-report of R6-3.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const liveRotation = liveSessionId | ||
| ? { | ||
| turns: this.toTurns.get(liveSessionId), |
There was a problem hiding this comment.
[Critical] R8-1: Overlapping restoreSessions() can rewind LIVE rotation counters to the stale persisted snapshot — a later restore whose reservation pass lands inside an earlier restore's wipe window captures no live state (toSession.get(key) is empty here, so liveRotation is undefined), and its later restoreRotationState unconditionally seeds toTurns/toStartedAt from the stale disk read, clobbering the counters the earlier restore carried; the last finisher's flush persists the regressed value. This is the sibling ordering the R6-2 fix (liveRotation capture + carryLiveRotationState) does not cover — the existing overlap test resolves the first restore's load before starting the second. — Failure scenario: probe-reproduced at this commit: maxTurns channel; a route live at turns=3 with its persists suspended (disk still 1) while restore C starts holding the key wiped; two more messages route (turns=5 in memory); C's load settles and seeds turns=3 from its stale snapshot — observed toTurns === 3 where 5 was expected; the final flush persists the regressed counter, so the session runs past its configured bound. Guarding restoreRotationState not to overwrite a counter already live in memory flips the probe.
// in the load loop, skip seeding rotation state that is already live
// (carried by an overlapping restore):
if (!this.toTurns.has(sessionId)) {
this.restoreRotationState(sessionId, entry);
}中文说明
严重：重叠的 restoreSessions() 会把存活轮换计数器回退到过期持久化快照——当后一个恢复的预留阶段落在前一个恢复的抹除窗口内时，它捕获不到任何存活状态（此时 toSession.get(key) 为空，这里的 liveRotation 为 undefined），随后其 restoreRotationState 无条件用过期的磁盘读取播种 toTurns/toStartedAt，覆盖前一个恢复 carry 回来的计数；最后结束者的 flush 把回退后的值固化。这是 R6-2 修复（liveRotation 捕获 + carryLiveRotationState）未覆盖的兄弟时序——现有重叠测试在启动第二个恢复之前就先解析了第一个恢复的 load。——失败场景：已在本 commit 用探针复现：maxTurns 频道；路由存活 turns=3（持久化被挂起，磁盘仍为 1）时恢复 C 启动并持有被抹除的键；再路由两条消息（内存 turns=5）；C 的 load 落定后以过期快照播种 turns=3——观察到 toTurns === 3（期望 5）；最终 flush 固化回退后的计数器，会话因此超出配置限度继续运行。为 restoreRotationState 增加「不覆盖内存中已存活计数器」的守卫后探针翻转。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (loadWindow.delete(sessionId)) { | ||
| throw new Error('Restored session died before routing completed'); | ||
| } | ||
| this.toSession.set(key, sessionId); |
There was a problem hiding this comment.
[Critical] R8-4: The tombstone mechanism (suspendedDeletionKeys) added as the R7-14 fix guards only the reservation pass. A /clear landing after BOTH reservation passes invalidates only the LATER restore's operation (invalidateRouteOperation reaches only creatingSessions.get(key)), so the EARLIER restore's in-flight load loop passes assertOperationCurrent and re-adds the cleared route at this toSession.set, and the last finisher's flush persists it — the user's /clear is silently undone. Probe-reproduced at this commit: /clear during two overlapping restores returns [] ('No active session to clear'), the earlier restore's load settles un-invalidated, the parked waiter resolves to the cleared session, and the final flush persists it; inserting a suspendedDeletionKeys.has(key) check here flips the probe. — Failure scenario: QQ cold-start restore in flight; the user clears the route; the reconnect READY already fired a second restore; after both reservation passes the cleared session is resurrected and the next message continues the 'cleared' conversation (privacy-relevant for shared sessions).
| if (loadWindow.delete(sessionId)) { | |
| throw new Error('Restored session died before routing completed'); | |
| } | |
| this.toSession.set(key, sessionId); | |
| if (loadWindow.delete(sessionId)) { | |
| throw new Error('Restored session died before routing completed'); | |
| } | |
| if (this.suspendedDeletionKeys.has(key)) { | |
| throw new Error('Restored route was removed while its restore was in flight'); | |
| } | |
| this.toSession.set(key, sessionId); |
中文说明
严重：作为 R7-14 修复加入的墓碑机制（suspendedDeletionKeys）只守卫预留阶段。落在两个预留阶段都通过之后的 /clear 只会作废后一个恢复的操作（invalidateRouteOperation 只能触达 creatingSessions.get(key)），因此前一个恢复在途的加载循环会通过 assertOperationCurrent，并经这行 toSession.set 重新添加被清空的路由，最后结束者的 flush 将其固化——用户的 /clear 被静默撤销。已在本 commit 用探针复现：两个重叠恢复期间执行 /clear 返回 []（「没有可清空的活跃会话」），前一个恢复的 load 未被作废便落定，停靠的等待者解析到被清空的会话，最终 flush 将其固化；在此处加入 suspendedDeletionKeys.has(key) 检查后探针翻转。——失败场景：QQ 冷启动恢复在途；用户清空该路由；重连 READY 已触发第二个恢复；两个预留阶段均通过后，被清空的会话复活，下一条消息继续「已被清空」的对话（对共享会话涉及隐私）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| this.toTarget.set(sessionId, entry.target); | ||
| this.toCwd.set(sessionId, entry.cwd); |
There was a problem hiding this comment.
[Critical] R8-11: The overlap-restore carry protects only ROTATION state (liveRotation = turns/startedAt/leases); a live toTarget mutation — promoteTargetToGroup's monotonic isGroup promotion — made during the suspension window is wiped by the later restore's reservation pass (deleteByKey) and reseeded from the stale snapshot here, then made durable by the last finisher's flush. The diff's own safety comment names rotation state only. — Failure scenario: probe-reproduced at this commit: promote isGroup false→true mid-suspension (its persist is swallowed); the overlapping restore reseeds toTarget from the pre-promotion snapshot — observed isGroup: false in memory and in the persisted store after both restores complete. Consumers read the regressed value: FeishuAdapter.pushProactiveDelivery selects the wrong receive-id type for a group chat; loopToolTarget copies it into stored loop records which normalizeLoopTarget then treats as one-to-one permanently (loop firings mis-addressed to a DM instead of the group); permissionTargetForEvent falls back to it. Reactive routing self-heals on the next group message, but stored loop records never do. Capturing the live target in the reservation pass and re-applying it flips the probe.
// capture alongside liveRotation in the reservation pass:
liveTarget: liveSessionId ? this.toTarget.get(liveSessionId) : undefined,
// ...and re-apply after restoreRotationState in the load loop:
if (reserved.liveTarget) this.toTarget.set(sessionId, reserved.liveTarget);中文说明
严重：重叠恢复的 carry 只保护轮换状态（liveRotation = turns/startedAt/leases）；挂起窗口内发生的存活 toTarget 变更——promoteTargetToGroup 对 isGroup 的单调提升——会被后一个恢复的预留阶段经 deleteByKey 抹掉，并在这里被过期快照重新播种，最后结束者的 flush 使其固化。diff 自己的安全性注释只提及了轮换状态。——失败场景：已在本 commit 用探针复现：挂起期间将 isGroup false→true（其 persist 被吞掉）；重叠恢复用提升前的快照重新播种 toTarget——两个恢复都完成后，内存与持久化存储中均观察到 isGroup: false。消费方读到回退值：FeishuAdapter.pushProactiveDelivery 为群聊选错 receive-id 类型；loopToolTarget 把它复制进存储的 loop 记录、normalizeLoopTarget 将其永久当作单聊（loop 触发被错投到私聊而非群）；permissionTargetForEvent 回退到它。被动路由会在下一条群消息时自愈，但存储的 loop 记录不会。在预留阶段同时捕获存活 target 并在加载后重新应用，探针翻转。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| for (const key of persisted.droppedKeys) { | ||
| this.deleteByKey(key); | ||
| } |
There was a problem hiding this comment.
[Critical] R8-17: The tombstone mechanism covers user/rotation deletions, but this validation-drop path runs BEFORE persistSuspendDepth++ and never tombstones — so an overlapping restore reading the stale snapshot re-applies the drop and wipes a live replacement route created mid-window (deleteByKey never calls discardSession, so the replacement session also leaks on the bridge). This is the exact stale-snapshot re-application the tombstones exist to prevent, in the deletion direction (mirror of the R8-8 resurrection direction, different code path). The trigger is rare — persist() only writes gate-valid entries atomically, so an entry failing isPersistedEntry requires external corruption/hand-editing — flagged for maintainer weighing. — Failure scenario: probe-reproduced at this commit: a store entry with turns: 0 (fails the gate) is dropped by restore A; a mid-window resolve creates live route session-1 (creation persist suspended, stale entry still on disk); overlapping restore B reads the stale file and its droppedKeys pass wipes the live route — observed the route undefined and discardSession calls []; the last finisher flushes the wiped state. Skipping deleteByKey when toSession.has(key) flips the probe.
| for (const key of persisted.droppedKeys) { | |
| this.deleteByKey(key); | |
| } | |
| for (const key of persisted.droppedKeys) { | |
| this.deleteByKey(key); | |
| this.tombstoneSuspendedKey(key); | |
| } |
中文说明
严重：墓碑机制覆盖用户/轮换删除，但此校验丢弃路径在 persistSuspendDepth++ 之前运行、且从不打墓碑——重叠恢复读到过期快照时会重新应用该丢弃，抹掉窗口中途创建的存活替换路由（deleteByKey 从不调用 discardSession，替换会话同时泄漏在 bridge 上）。这正是墓碑机制要防止的「过期快照再应用」，只是发生在删除方向（与 R8-8 的复活方向互为镜像、代码路径不同）。触发罕见——persist() 只原子地写入通过门控的条目，能未过 isPersistedEntry 的条目需要外部损坏/手工编辑——提请维护者权衡。——失败场景：已在本 commit 用探针复现：含 turns: 0（未过门控）的存储条目被恢复 A 丢弃；窗口中途的 resolve 创建存活路由 session-1（创建持久化被挂起，磁盘仍是过期条目）；重叠恢复 B 读到过期文件，其 droppedKeys 阶段抹掉存活路由——观察到路由变为 undefined、discardSession 调用为 []；最后结束者固化被抹掉的状态。当 toSession.has(key) 时跳过 deleteByKey，探针翻转。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Reject unknown keys loudly: a typo'd bound (say "maxTurn") must not | ||
| // silently disable rotation — the exact failure mode rotation prevents. | ||
| for (const key of Object.keys(parsed)) { |
There was a problem hiding this comment.
[Suggestion] R8-12: The loud unknown-key strictness this PR adds guards only NESTED keys; a typo'd or renamed top-level sessionRotation key (sessionRotations, session_rotation) is silently ignored by parseChannelConfig — the channel starts cleanly with rotation unset and no diagnostic, the exact failure mode this comment says it prevents. The management API path rejects unknown top-level keys (assertPreservedUnknownField), so only the hand-edited settings.json path is silent. — Concrete cost: probe-reproduced — parseChannelConfig('bot', { type: 'telegram', token: 't', sessionRotations: { maxTurns: 200 } }) resolves cleanly with sessionRotation: undefined and the typo'd key passed through; a long-lived route then accumulates context without bound until it exceeds the provider window and every later message on it fails. A near-miss top-level guard flips the probe. Wholesale unknown-top-level-key rejection is structurally blocked (plugin pass-through fields are not enumerable), so a sessionRotation-specific near-miss check is the narrow fix:
// in parseChannelConfig, reject near-misses of this key:
if (key !== 'sessionRotation' &&
key.toLowerCase().replace(/[_-]/g, '') === 'sessionrotation') {
throw new Error(`Channel "${name}" field "${key}" is not a valid field. Did you mean "sessionRotation"?`);
}中文说明
建议：本 PR 新增的「未知键大声报错」只守卫嵌套键；顶层 sessionRotation 键拼错或改名（sessionRotations、session_rotation）会被 parseChannelConfig 静默忽略——频道以未配置轮换的状态正常启动、没有任何诊断，正是该注释声称要防止的失效形态。管理 API 路径会拒绝未知顶层键（assertPreservedUnknownField），因此只有手工编辑 settings.json 的路径是静默的。——具体成本：已用探针复现——parseChannelConfig('bot', { type: 'telegram', token: 't', sessionRotations: { maxTurns: 200 } }) 干净地解析为 sessionRotation: undefined、拼错的键原样透传；长期存在的路由将无界累积上下文，直到超出提供方窗口、此后每条消息都失败。顶层近似键守卫可使探针翻转。整批拒绝未知顶层键在结构上不可行（插件透传字段不可枚举），针对 sessionRotation 的近似匹配检查是窄修复。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (options.shouldContinue && !(await options.shouldContinue())) { | ||
| // The firing was routed and counted but never prompted: give the | ||
| // count back so a dropped firing cannot consume the session's bound. | ||
| this.router.uncountTurn(this.name, sessionId); |
There was a problem hiding this comment.
[Suggestion] R8-16: This refund covers only shouldContinue() resolving false; a REJECTING shouldContinue() still aborts the firing before any prompt while keeping the resolve-time count, violating the invariant the added comment states. ChannelLoopScheduler's shouldContinue awaits findJob → ChannelLoopStore.readJobs, which throws on non-ENOENT read errors, malformed JSON, or a non-array payload — so rejection is reachable in production. countTurn is new in this diff, so this asymmetry is newly introduced (pre-PR there was no count to lose). — Concrete cost: a loop firing dequeues behind a busy session while the loop store is transiently unreadable/corrupt: shouldContinue rejects, the queued turn throws having never prompted, and the count survives — the session rotates one turn earlier than its configured bound, and repeated transient store errors silently consume the bound with no diagnostic. The existing drop test covers only shouldContinue: async () => false. Suggested fix: refund on the rejection path too — wrap the await in try/catch and call this.router.uncountTurn(this.name, sessionId) before re-throwing, mirroring the false branch.
中文说明
建议：此回退只覆盖 shouldContinue() 解析为 false 的情形；shouldContinue() 拒绝时同样会在 prompt 之前中止触发，却保留 resolve 时取得的计数，违反所加注释陈述的不变量。ChannelLoopScheduler 的 shouldContinue 等待 findJob → ChannelLoopStore.readJobs，后者在非 ENOENT 读取错误、JSON 损坏或 payload 非数组时抛出——拒绝在生产可达。countTurn 为本 diff 新增，该不对称因此也是新引入的（改动前没有计数可丢）。——具体成本：排队中的触发遇到 loop 存储瞬时不可读/损坏：shouldContinue 拒绝、排队回合未 prompt 即抛出、计数保留——会话提前一轮轮换，重复的瞬时存储错误静默消耗限度且无诊断。现有丢弃测试只覆盖 shouldContinue: async () => false。建议修复：在拒绝路径同样回退——用 try/catch 包裹该 await，在重新抛出前调用 this.router.uncountTurn(this.name, sessionId)，与 false 分支对齐。
— qwen3.8-max via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 1-4 reported findings; round 5 dry).
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): You are review agent reverse-audit — Reverse audit agen...: none — all checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; chunk 8: could not run channel-settings-store.test.ts to confirm green — the review worktree has no node_modules installed ( ERR_MODULE_NOT_FOUND: vitest ), and inst…, and 6 more.
Test Plan (not a blocker): 1023 tests passed — this review observed 1095, 18, 19369, 297, 266, 207, 59, 291, 134, 71 passed; 158 tests passed — this review observed 1095, 18, 19369, 297, 266, 207, 59, 291, 134, 71 passed.
[Critical] R8-1: Overlapping restoreSessions() rewind LIVE rotation counters to the stale persisted snapshot when the later restore's reservation pass captures no live state (its settle unconditionally re-seeds toTurns/toStartedAt). Probe-reproduced at this head: toTurns settles [10, 7] with live 10; a one-line reservation guard flips it. Covers the same mechanism as earlier-round R6-2; the existing overlap test resolves the first restore's load before starting the second, so the interleaving stays untested.
[Critical] R8-4: The tombstone mechanism (suspendedDeletionKeys) guards only the reservation pass; a /clear landing after both reservation passes invalidates only the current creatingSessions entry, so an earlier restore's orphaned operation settle passes assertOperationCurrent and re-adds the cleared route (the settle path never re-checks suspendedDeletionKeys). Anchored region unchanged since round 8.
[Critical] R8-10: The waiter retry heuristic (SessionRouter.ts ~463-472) classifies any invalidation with a successor as a rotation/reload handoff; /clear (removeSession) followed immediately by a new message produces the same shape, so a parked pre-clear waiter retries into the fresh post-clear session instead of being dropped — /clear defeated for that message, and ChannelBase's generation guard cannot catch it. Independently re-derived at this head by four review agents this round.
[Critical] R8-11: The overlap-restore carry protects only ROTATION state (turns/startedAt/leases); a live toTarget mutation (promoteTargetToGroup's monotonic isGroup promotion) made during the suspension window is wiped by the later restore's reservation pass and re-seeded from the stale snapshot at settle, then made durable by the last finisher's flush. Anchored region unchanged since round 8.
[Critical] R8-17: The persisted-entry validation-drop path (deleteByKey over persisted.droppedKeys, SessionRouter.ts ~912-914) runs BEFORE persistSuspendDepth++ and never tombstones — an overlapping restore reading the same stale snapshot re-applies the drop and wipes a live replacement route created mid-window (deleteByKey never calls discardSession, so the replacement session also leaks on the bridge). Anchored region unchanged since round 8.
[Critical] R6-1 (re-check, narrowed): the persist suspension opened by restoreSessions() still has no timeout/lifecycle cleanup at this head — the QQChannel cold-start READY restore is fire-and-forget on the long-lived shared router, and a wedged-but-alive ACP child whose session/load never responds (async I/O hang, no exit, no event-loop stall, so neither the exit-reject nor the stall watchdog fires) pins persistSuspendDepth >= 1 for the router's lifetime: every later persist() is silently dropped and the on-disk route store goes permanently stale until restart.
中文说明
未审查：reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 1-4 reported findings; round 5 dry)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；chunk 8：could not run channel-settings-store.test.ts to confirm green — the review worktree has no node_modules installed ( ERR_MODULE_NOT_FOUND: vitest ), and inst…，另有 6 条。
Test Plan（非阻断）：1023 tests passed — this review observed 1095, 18, 19369, 297, 266, 207, 59, 291, 134, 71 passed; 158 tests passed — this review observed 1095, 18, 19369, 297, 266, 207, 59, 291, 134, 71 passed。
[Critical] R8-1: Overlapping restoreSessions() rewind LIVE rotation counters to the stale persisted snapshot when the later restore's reservation pass captures no live state (its settle unconditionally re-seeds toTurns/toStartedAt). Probe-reproduced at this head: toTurns settles [10, 7] with live 10; a one-line reservation guard flips it. Covers the same mechanism as earlier-round R6-2; the existing overlap test resolves the first restore's load before starting the second, so the interleaving stays untested.
[Critical] R8-4: The tombstone mechanism (suspendedDeletionKeys) guards only the reservation pass; a /clear landing after both reservation passes invalidates only the current creatingSessions entry, so an earlier restore's orphaned operation settle passes assertOperationCurrent and re-adds the cleared route (the settle path never re-checks suspendedDeletionKeys). Anchored region unchanged since round 8.
[Critical] R8-10: The waiter retry heuristic (SessionRouter.ts ~463-472) classifies any invalidation with a successor as a rotation/reload handoff; /clear (removeSession) followed immediately by a new message produces the same shape, so a parked pre-clear waiter retries into the fresh post-clear session instead of being dropped — /clear defeated for that message, and ChannelBase's generation guard cannot catch it. Independently re-derived at this head by four review agents this round.
[Critical] R8-11: The overlap-restore carry protects only ROTATION state (turns/startedAt/leases); a live toTarget mutation (promoteTargetToGroup's monotonic isGroup promotion) made during the suspension window is wiped by the later restore's reservation pass and re-seeded from the stale snapshot at settle, then made durable by the last finisher's flush. Anchored region unchanged since round 8.
[Critical] R8-17: The persisted-entry validation-drop path (deleteByKey over persisted.droppedKeys, SessionRouter.ts ~912-914) runs BEFORE persistSuspendDepth++ and never tombstones — an overlapping restore reading the same stale snapshot re-applies the drop and wipes a live replacement route created mid-window (deleteByKey never calls discardSession, so the replacement session also leaks on the bridge). Anchored region unchanged since round 8.
[Critical] R6-1 (re-check, narrowed): the persist suspension opened by restoreSessions() still has no timeout/lifecycle cleanup at this head — the QQChannel cold-start READY restore is fire-and-forget on the long-lived shared router, and a wedged-but-alive ACP child whose session/load never responds (async I/O hang, no exit, no event-loop stall, so neither the exit-reject nor the stall watchdog fires) pins persistSuspendDepth >= 1 for the router's lifetime: every later persist() is silently dropped and the on-disk route store goes permanently stale until restart.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // channel start crash recovery, which reloads the persisted sessions. | ||
| this.rejectPendingSessionRequests(); | ||
| this.resolvePendingPermissions(); |
There was a problem hiding this comment.
[Critical] Rejecting in-flight loadSession calls on child exit turns an interrupted restoreSessions() into a fast-completed one whose end-of-restore flush permanently prunes every route the restore never reached from the persisted store — the crash-recovery restore that runs seconds later has nothing to reload for those routes. — Failure scenario: an eager restore of N routes is loading when the ACP child dies (crash, or the stall watchdog's SIGKILL): the in-flight request rejects, every later key fast-fails in ensureConnection() ("Not connected to ACP agent"), each catch sets changed/persistRequestedWhileSuspended without re-adding the key (the reservation pass already deleteByKey'd it), and the finally-flush persists only the restored prefix. Probe-reproduced at this commit: a 3-route store ends with 1 key on disk and crash recovery restores 1 of 3; removing the reject flips it (the restore hangs pre-flush, store intact — the pre-PR behavior). — Suggested fix: in restoreSessions(), treat bridge-death load failures differently from genuine load failures — re-seed the persisted entry instead of leaving the key dropped (and don't set changed/persistRequestedWhileSuspended for those keys), or skip the end-flush when the restore was interrupted by a bridge exit.
中文说明
严重：子进程退出时拒绝在途 loadSession 调用，会把被打断的 restoreSessions() 变成一次快速完成的恢复，其结束 flush 会把恢复未触及的所有路由从持久化存储中永久剪掉——几秒后 crash recovery 读到的就是这份被裁剪的文件，那些路由无从重载。失败场景：eager 恢复 N 条路由途中 ACP 子进程死亡（崩溃或事件循环卡死看门狗 SIGKILL）：在途请求被拒绝，其余每个 key 在 ensureConnection() 中快速失败（"Not connected to ACP agent"），每个 catch 设置 changed/persistRequestedWhileSuspended 但不重新放回该 key（预留阶段已 deleteByKey），最终 flush 只持久化已恢复的前缀。已在本 commit 用探针复现：3 路由存储结束后磁盘只剩 1 个 key，crash recovery 只恢复 1/3；去掉该 reject 后翻转（恢复在 flush 前挂起、存储保持完整——即改动前行为）。建议修复：在 restoreSessions() 中把「bridge 死亡导致的加载失败」与真正的加载失败区别对待——重新种入持久化条目而不是丢弃该 key（且不为这些 key 设置 changed/persistRequestedWhileSuspended），或在恢复被 bridge 退出打断时跳过结束 flush。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // sessionQueues is deliberately NOT purged: a queued turn may still hold | ||
| // the captured chain, and deleting the entry would let the next message | ||
| // (which lazy recovery can re-attach to this same session ID) seed a | ||
| // fresh chain and run concurrently with the stale queued turn. /clear is | ||
| // the only path that may delete it, after the chain drains. |
There was a problem hiding this comment.
[Suggestion] This new comment asserts /clear is the ONLY path that may delete sessionQueues, but this same PR adds a second deletion site in handleSessionRotated (~line 2337, asserted by 'reclaims the queue and generation of a rotated session'), and the rotation comment ten lines below ("it reclaims what the death path must keep") contradicts it directly. The two sites rest on different safety arguments — /clear deletes under a drain-capture + generation guard; rotation defers until no turn is running or queued and retires the ID permanently — and the wording now conflates them. — Concrete cost: a maintainer debugging a sessionQueues leak, or reviewing a future change against this invariant, concludes the rotation-path deletion is a bug and removes or gates it, reintroducing per-session map leaks for every rotated session in a long-running gateway.
| // sessionQueues is deliberately NOT purged: a queued turn may still hold | |
| // the captured chain, and deleting the entry would let the next message | |
| // (which lazy recovery can re-attach to this same session ID) seed a | |
| // fresh chain and run concurrently with the stale queued turn. /clear is | |
| // the only path that may delete it, after the chain drains. | |
| // sessionQueues is deliberately NOT purged: a queued turn may still hold | |
| // the captured chain, and deleting the entry would let the next message | |
| // (which lazy recovery can re-attach to this same session ID) seed a | |
| // fresh chain and run concurrently with the stale queued turn. /clear | |
| // (after the chain drains) and rotation (which defers until no turn is | |
| // running or queued and retires the ID permanently) are the only paths | |
| // that may delete it. |
中文说明
建议：这段新注释声称 /clear 是唯一可以删除 sessionQueues 的路径，但本 PR 同时在 handleSessionRotated（约 2337 行，由测试 'reclaims the queue and generation of a rotated session' 断言）新增了第二个删除点，且十行下方的轮换注释（"it reclaims what the death path must keep"）与之直接矛盾。两处删除依赖不同的安全论证——/clear 在排空捕获 + generation 守卫下删除；轮换则推迟到没有任何回合在运行或排队、且该 ID 被永久退役——现有措辞把两者混为一谈。具体代价：维护者在排查 sessionQueues 泄漏、或依据该不变量评审后续改动时，会误以为轮换路径的删除是 bug 而将其移除或加守卫，从而在长期运行的网关中为每个被轮换的会话重新引入按会话的 Map 泄漏。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| this.sessionQueues.delete(sessionId); | ||
| this.sessionGenerations.delete(sessionId); |
There was a problem hiding this comment.
[Suggestion] The new sessionPendingTurns map has no purge site anywhere — not purgeSessionState (death), not handleSessionRotated (rotation), not doClear (whose comment claims it purges "every per-session map"). trackSessionTurn's finish is the only decrement and runs only when the turn promise settles; bridge.prompt is not wrapped in settleOnChildExit, and the ACP SDK never settles requests after the stream ends. — Failure scenario: on the production lazy daemon path, a turn hangs in await bridge.prompt(...) (ACP child death mid-prompt) on a route at its bound; hasPendingTurns stays true forever, so every later resolve() skips the rotation gate — maxTurns/maxAgeHours are silently never enforced for that route until /clear or restart, and rotation (this PR's own auto-recovery for wedged routes) cannot un-wedge it; each occurrence also leaks one Map entry. Probe at this commit: the flag survives session death and an at-bound resolve() returns the wedged session; deleting the stuck flag flips it to rotate + discard. — Suggested fix: purge the entry where the ID is permanently retired — alongside the deletes here in handleSessionRotated and in doClear's purge block (finish's (get ?? 1) - 1 tolerates a missing entry); leave the death path untouched, and consider whether a never-settling turn should be force-settled so the wedge itself clears.
中文说明
建议：新增的 sessionPendingTurns Map 没有任何清理点——purgeSessionState（死亡）、handleSessionRotated（轮换）、doClear（其注释声称清理"每一个按会话的 Map"）都不删它。trackSessionTurn 的 finish 是唯一的递减点，且只在回合 promise 落定时运行；bridge.prompt 没有被 settleOnChildExit 包裹，而 ACP SDK 在流结束后永远不会落定仍在等待的请求。失败场景：生产 daemon 的 lazy 路径下，某条已触达限度的路由上有一个回合卡在 await bridge.prompt(...)（prompt 途中子进程死亡），hasPendingTurns 永远为真，此后该路由的每次 resolve() 都跳过轮换门控——maxTurns/maxAgeHours 在 /clear 或重启之前被静默地不再执行，轮换（本 PR 自带的卡死路由自救机制）也无法解救该路由；每次发生还泄漏一个 Map 条目。已在本 commit 用探针验证：该标志在会话死亡后依然存活，触达限度的 resolve() 仍返回卡死的会话；删除该卡死的标志后翻转为轮换 + discard。建议修复：在 ID 被永久退役的位置清理该条目——此处 handleSessionRotated 的删除旁边以及 doClear 的清理块中（finish 的 (get ?? 1) - 1 容忍条目缺失）；死亡路径保持不变，并可考虑是否应强制落定永不落定的回合以解除卡死本身。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // second restore against the stale pre-clear snapshot. | ||
| router.removeSession('ch', 'alice', 'chat1'); |
There was a problem hiding this comment.
[Suggestion] The rotation suite's only /clear-vs-overlapping-restore test clears BEFORE the second restore's reservation pass; the post-reservation window — /clear lands after the overlap's reservation wiped the route but before that key's loadSession settles — has zero coverage, even though its correctness at this commit rests on three cooperating invariants: removeSession's unconditional invalidateRouteOperation(key) (deleteByKey returns null in this window, so a future guard keyed on "route still mapped" would skip it), the settle's post-await assertOperationCurrent re-check, and the tombstone for later restores. The settle path itself never re-checks suspendedDeletionKeys. — Failure scenario: refactoring any one invariant away lets the in-flight settle re-add the cleared route via toSession.set and the end-of-restore flush persist it — the user's /clear silently resurrects across restart with the whole suite green. Probe at this commit: guarding the invalidation on deleteByKey's result resurrects the route in memory AND on disk; HEAD wins the clear. — Suggested fix: add a variant of 'does not resurrect a route cleared before an overlapping restore' that calls router.removeSession AFTER the second restore's reservation pass (second restore started, its load still unresolved), then asserts the route stays gone in memory and in the persisted store.
中文说明
建议：轮换测试套件中唯一的「/clear 对重叠恢复」测试是在第二次恢复的预留阶段之前执行 clear 的；预留之后的窗口——/clear 落在重叠恢复的预留已抹除路由、但该 key 的 loadSession 尚未落定之间——零覆盖，尽管当前 commit 下其正确性依赖三个协同不变量：removeSession 无条件调用 invalidateRouteOperation(key)（此窗口内 deleteByKey 返回 null，未来任何以"路由仍在映射中"为前提的守卫都会跳过它）、settle 在 await 之后的 assertOperationCurrent 复查、以及面向更晚恢复的 tombstone。settle 路径本身从不复查 suspendedDeletionKeys。失败场景：重构掉其中任何一个不变量，在途 settle 就会通过 toSession.set 重新加回被清除的路由，并由恢复结束 flush 持久化——用户的 /clear 在重启后静默复活，而整个测试套件全绿。已在本 commit 用探针验证：把失效化守卫改为依赖 deleteByKey 的返回值后，被清除的路由在内存和磁盘双双复活；当前 HEAD 则正确赢得 clear。建议修复：新增 'does not resurrect a route cleared before an overlapping restore' 的变体，在第二次恢复的预留阶段之后（第二次恢复已启动、其加载尚未落定）调用 router.removeSession，然后断言该路由在内存与持久化存储中均保持消失。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
|
||
| Set either, both, or neither; whichever bound is hit first rotates. `maxTurns` must be a positive integer and `maxAgeHours` a positive number. Omitting `sessionRotation` keeps the previous behavior of never rotating. In `collect` dispatch mode, messages buffered while a turn runs are coalesced into one turn and count once against `maxTurns`. | ||
|
|
||
| Rotation is a context reset, not a cleanup: the new session starts empty, so the bot no longer remembers the earlier conversation on that route. The channel posts a short notice in the chat or thread whose message triggered the rotation, and the daemon logs the rotated route. With `sessionScope: single`, only the chat whose message triggered the rotation is notified; other chats sharing the session see the reset without a notice. Counters are stored alongside the routes and survive a daemon restart. Sessions that were already routed before you enabled rotation start their clock at the first message after the upgrade. A route that still has a turn running or queued rotates on the next message after it settles instead of mid-turn; under sustained traffic, where every message arrives while a turn is still running or queued, the bound waits for the first pause in traffic. Each message routed during that window extends it, so a continuously saturated route — a webhook receiving events faster than turns complete, or an overrunning loop — rotates only once traffic stops. |
There was a problem hiding this comment.
[Suggestion] The documented sessionScope: single rotation semantics in this sentence (only the triggering chat is notified; other chats sharing the session see a silent reset) have zero test coverage — every rotation test in ChannelBase.test.ts uses the default user scope or threads, and no test anywhere in packages/ combines single scope with rotation. This sentence was added as the R5-8 fix, and the announce target comes from the triggering message's input (the R2-8 fix) — nothing pins either in-tree. — Failure scenario: a future refactor that fetches the announce target from the stored route target (reverting R2-8), or that broadcasts the notice to every chat on a single-scope route, would silently change this user-visible documented behaviour with the entire suite green. — Suggested fix: add a ChannelBase rotation test with sessionScope: 'single': two chats share the route, hit maxTurns from chat A, assert chat A receives the rotation notice and chat B receives none while its next prompt lands on the fresh session.
中文说明
建议：本句所记载的 sessionScope: single 轮换语义（只通知触发轮换的那个聊天；共享该会话的其他聊天静默重置）没有任何测试覆盖——ChannelBase.test.ts 中的全部轮换测试都使用默认 user 作用域或 thread，packages/ 下也没有任何测试把 single 作用域与轮换组合起来。这句话是作为 R5-8 的修复加入的，通知目标取自触发消息的输入（R2-8 的修复）——两者在仓库中都没有测试钉住。失败场景：未来若有重构把通知目标改为取自存储的路由 target（回退 R2-8），或把通知广播给 single 作用域路由上的所有聊天，都会静默改变这一用户可见的已记载行为，而整个测试套件全绿。建议修复：新增一个 sessionScope: 'single' 的 ChannelBase 轮换测试：两个聊天共享同一路由，从聊天 A 触达 maxTurns，断言聊天 A 收到轮换通知、聊天 B 收不到通知且其下一条消息落在新会话上。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🐑 Merge conflict with main detected — dispatched the autofix loop to resolve it. / 检测到与 main 的合并冲突，已触发 autofix 处理。 |
|
⏸️ Dispatch refused: this PR has exhausted its automatic round cap (10/10), so the loop will not touch it — whatever triggered this dispatch (a merge conflict, new feedback) stays unhandled. Comment 中文说明⏸️ 已拒绝本次调度：本 PR 的自动轮次上限已用完（10/10），循环不会介入——触发本次调度的事项（合并冲突、新反馈）仍未处理。评论 |
qwen-code-dev-bot
added
the
autofix/needs-human
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): chunk 3: None — I read all relevant source files and diff sections within my territory..
Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds — the posted findings were ruled on, and the misses the rest of the review left were hunted, if at all, without the briefs this skill certifies against.
[Critical] Critical: runLoopPrompt missing trackSessionTurn call — runLoopPrompt in ChannelBase.ts calls this.router.resolve(...) which takes a routing lease via leaseSession(), but never calls trackSessionTurn() (which releases the lease and registers the pending-turn count). The lease leaks for the lifetime of the session, and sessionPendingTurns never carries the loop turn. A session driven entirely by loop firings will accumulate a routing lease that is never released, permanently blocking rotation — the exact scenario sessionRotation was designed to prevent.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）：chunk 3：None — I read all relevant source files and diff sections within my territory.。
未审查：验证与反向审计——验证 agent 与反向审计 agent 都没有用本 skill 构建的 prompt 启动——发布的发现即便被裁定过、评审其余部分遗漏的问题即便被搜寻过，也都缺失了本 skill 用以认证的 brief。
[Critical] Critical: runLoopPrompt missing trackSessionTurn call — runLoopPrompt in ChannelBase.ts calls this.router.resolve(...) which takes a routing lease via leaseSession(), but never calls trackSessionTurn() (which releases the lease and registers the pending-turn count). The lease leaks for the lifetime of the session, and sessionPendingTurns never carries the loop turn. A session driven entirely by loop firings will accumulate a routing lease that is never released, permanently blocking rotation — the exact scenario sessionRotation was designed to prevent.
— qwen-max via Qwen Code /review (v0.21.10)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "You are review agent invariant-b — Invariant agent B:…": 无。在分配的约 65 次工具调用预算内完成了所有检查。如有必要，可以进一步验证 restoreSessions 中 rotationDeltas 的边界情况，但证据已经足够： carryLiveRotationState 在 reservation.resolve() 之后、恢复的 finally 块….
Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："You are review agent invariant-b — Invariant agent B:…"：无。在分配的约 65 次工具调用预算内完成了所有检查。如有必要，可以进一步验证 restoreSessions 中 rotationDeltas 的边界情况，但证据已经足够： carryLiveRotationState 在 reservation.resolve() 之后、恢复的 finally 块…。
未审查：反向审计——没有审计 agent 是用本 skill 构建的 prompt 启动的——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法。
— qwen-max via Qwen Code /review (v0.21.10)
ReviewReviewed the full diff at Medium1. The routing lease has no
The 2.
The cost scales with total route count, so the deployment this PR is written for — a 3. Sessions created during
Low4.
5. An ID-changing reload carries At 6.
Description / code mismatchThe Risk & Scope section says "No user-facing notice is posted to the chat when a rotation happens — the reset is silent, matching Checked and clean
|
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
5 participants
What this PR does
Adds a per-channel
sessionRotationoption that bounds how long a route keeps the same session. When the current session on a route is past its bound, the next message on that route starts a fresh session instead of reusing it. Two bounds are supported —maxTurns(messages routed to the session) andmaxAgeHours(wall-clock age) — and either may be set on its own; whichever is hit first rotates.The bound is checked before a message reuses a session, so it caps what the session carries into a turn rather than what it is left holding after one. It is checked on both the live-reuse and the lazy-reload path, so a route cannot dodge its bound by having been evicted from memory, and it is skipped while a session creation is already in flight on that key — invalidating that operation would fail the concurrent message instead of rotating it, and the next message enforces the bound just as well.
Turn counts and start times persist alongside the routes, so a daemon restart cannot reset a bound, and they carry across a session ID change when a reload returns a new ID. Channels with no bound configured skip the bookkeeping entirely: no counters are tracked, the on-disk route shape is unchanged, and there is no extra persist per message. Non-positive or non-finite bounds are rejected at config-parse time, and defensively normalized in the router so a hand-edited store cannot make a channel rotate on every single message.
Omitting
sessionRotationpreserves today's behavior exactly.Why it's needed
SessionRoutermaps a routing key to a session ID and reuses that session for every later message on the key, with nothing bounding how large it gets. A long-lived route grows monotonically until it passes the model's context window; from that point on every message on that route fails while the rest of the channel keeps working, and the only recovery is hand-editing the daemon'sroutes.json.I hit this on a DingTalk Q&A bot with
sessionScope: "thread". One group thread had been accumulating since July 27 — 8577 entries / 21 MB in the session JSONL, peakpromptTokenCount849,748. At ~327k prompt tokens every turn began failing upstream, retrying 7 times per turn before giving up, while other threads on the same channel, model, and credentials answered normally. Replaying the failing thread's own tail through the provider API succeeded, confirming the failure was specific to that accumulated session rather than the channel or the model. The operator-visible symptom is "the bot is down" when one route is wedged.Auto-compaction does not cover this: it is driven by the client's configured context window, so when that is larger than what the endpoint actually serves for the session, the wall arrives before compaction ever triggers.
sessionScopealready decides how routes are partitioned; there was no knob for how long a partition lives. Chat channels are the case that needs one — a group thread has no natural end, unlike a CLI session a user closes.Reviewer Test Plan
How to verify
Unit tests cover the behavior end to end. From the repo root:
The added
session rotationblock asserts: no rotation when unconfigured; a new session oncemaxTurnsis reached; only the route that hit the bound rotates while a sibling route keeps its session; a channel without a bound is unaffected when another channel has one;maxAgeHoursrotates on elapsed time (fake timers); non-positive bounds are ignored rather than rotating every message; turn counts survive a restore so a restart cannot reset the bound; and a route store written before this change restores cleanly and starts its clock at the next message instead of rotating on sight.Config parsing tests assert the bounds round-trip, stay
undefinedwhen omitted, and that a non-positive bound is rejected with a clear message.To confirm manually, configure a channel with
"sessionRotation": { "maxTurns": 2 }, send three messages, and observe the third get a new session ID — the router logs[SessionRouter] Rotated session for <channel>: <id> reached its configured limit; starting a new session.and the bot no longer recalls the first two messages.Evidence (Before & After)
N/A — no TUI surface. Behavior change is in routing and is covered by the unit tests above.
Full suites run locally:
Tested on
Environment (optional)
Unit tests only, via
npx vitest runper package on Linux / Node 22.Risk & Scope
sessionRotationchanges nothing. When a bound is configured, each message costs one extra smallroutes.jsonwrite to persist the turn counter; channels without a bound are exempt from that write.get_context_usageexists only on the SDK control path), so a token bound would need a new bridge capability. Turn count and age are coarser but keep a route from growing without limit, and a token bound can be added later behind the same config key. No user-facing notice is posted to the chat when a rotation happens — the reset is silent, matching/clearsemantics.feat, not arefactor. It touchespackages/channels/base(router, types, one wiring line inChannelBase) andpackages/cli/src/commands/channel(config parsing plus one wiring line in each ofstart.tsanddaemon-worker.ts). The new router method has exactly three call sites, all listed above.Linked Issues
Closes #8926
中文说明
这个 PR 做了什么
为频道新增
sessionRotation配置，用于限制一个路由复用同一会话的时长。当路由上的当前会话超出配置的限度时，下一条消息会开一个全新会话，而不是继续复用。支持两个限度——maxTurns（路由到该会话的消息数）和maxAgeHours（自然时间年龄），二者可单独设置，先达到的那个触发轮换。限度在消息复用会话之前检查，因此它约束的是会话带入本轮的上下文量，而不是本轮结束后残留的量。检查同时覆盖存活复用和惰性重载两条路径，避免路由因为被逐出内存而绕过限度；如果该 key 上已有创建操作在途则跳过本次检查——此时作废该操作会让并发的那条消息失败而不是完成轮换，而下一条消息同样能落实限度。
轮次计数和起始时间与路由一起持久化，因此守护进程重启不会重置限度；当重载返回新的会话 ID 时，这些计数会随之迁移。未配置限度的频道完全跳过这套记账：不跟踪计数器，磁盘上的路由结构不变，也没有每条消息的额外写盘。非正数和非有限值在配置解析阶段就会报错，路由层还会再做一次防御性归一化，避免手工改坏的存储导致频道对每条消息都轮换。
不填
sessionRotation时行为与当前完全一致。为什么需要
SessionRouter把路由键映射到会话 ID，之后该键上的每条消息都复用这个会话，没有任何机制限制它增长到多大。长期存在的路由会单调增长，直到超过模型上下文窗口；从那一刻起该路由上的每条消息都会失败，而频道其余部分一切正常，唯一的恢复手段是手工编辑守护进程的routes.json。我在一个
sessionScope: "thread"的钉钉答疑机器人上遇到了这个问题。某个群 thread 从 7 月 27 日起持续累积——会话 JSONL 已有 8577 条 / 21 MB，promptTokenCount峰值 849748。在约 32.7 万 prompt token 时，每一轮都开始在上游失败，每轮重试 7 次后放弃，而同一频道、同一模型、同一凭证下的其他 thread 回答完全正常。把失败 thread 自己的上下文尾部通过 provider API 回放是成功的，这确认了故障绑定在那个累积起来的会话上，而非频道或模型。运维视角看到的现象是「机器人挂了」，实际只是一个路由卡死。自动压缩覆盖不了这种情况：它由客户端配置的上下文窗口驱动，当该配置大于端上实际为会话提供的窗口时，硬墙会在压缩触发之前就到来。
sessionScope已经决定了路由如何划分，但没有任何开关决定一个划分能活多久。聊天频道正是需要这个开关的场景——群 thread 没有自然终点，不像用户会主动关闭的 CLI 会话。审阅者验证方案
如何验证
单元测试完整覆盖了该行为。在仓库根目录执行：
新增的
session rotation测试块断言了：未配置时不轮换；达到maxTurns后开新会话；只有触达限度的那个路由轮换、同级路由保持原会话；某个频道配置了限度时其他频道不受影响；maxAgeHours按流逝时间触发轮换（使用 fake timers）；非正数限度被忽略而不是每条消息都轮换；轮次计数在恢复后仍然有效，重启无法重置限度；本次改动之前写入的路由存储能正常恢复，并从下一条消息开始计时而不是立刻轮换。配置解析测试断言了限度能正确往返、省略时保持
undefined、以及非正数限度会带清晰信息报错。手工确认方式：给某个频道配置
"sessionRotation": { "maxTurns": 2 }，发三条消息，观察第三条拿到新的会话 ID——路由层会输出[SessionRouter] Rotated session for <channel>: <id> reached its configured limit; starting a new session.，且机器人不再记得前两条消息。证据（前后对比）
N/A——没有 TUI 界面改动。行为变更在路由层，由上述单元测试覆盖。
本地跑过的完整套件：
测试平台
运行环境（可选）
仅单元测试，在 Linux / Node 22 上按包执行
npx vitest run。风险与范围
sessionRotation则什么都不变。配置了限度后，每条消息会多一次很小的routes.json写入以持久化轮次计数；未配置限度的频道不承担这次写入。get_context_usage只存在于 SDK 控制通路），因此 token 限度需要新增 bridge 能力。轮次和年龄更粗糙，但足以防止路由无限增长，后续可以在同一个配置键下补充 token 限度。轮换发生时不会向聊天里发送提示——重置是静默的，与/clear的语义一致。feat而非refactor。改动涉及packages/channels/base（路由器、类型、ChannelBase中一行接线）和packages/cli/src/commands/channel（配置解析，以及start.ts和daemon-worker.ts各一行接线）。新增的路由器方法恰好有三个调用点，均已在上文列出。关联 Issue
Closes #8926