fix(web-shell): surface loop detection turn errors - #8853
Conversation
E2E test report
|
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. The per-turn loop-protection stop is a real runtime behavior, and the PR shows before/after screenshots of what Web Shell renders today (the internal skipped-tool error as the only explanation, and the turn error lost on page refresh), plus a macOS E2E walk-through in this thread. That is enough evidence that the problem exists. Direction: aligned. The tool-call safety caps are an existing mechanism; turning their stops into a localized, non-retryable turn error instead of a silent Size: cross-package change ( Approach: the scope feels right. Reusing the existing Risk: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，不是理论性问题。单轮循环保护停止是真实存在的运行时行为，PR 提供了 before/after 截图（Web Shell 目前只展示内部的"工具被跳过"错误、刷新后轮次错误丢失），并在本帖中附上了 macOS 手工 E2E 验证记录。问题存在的证据充分。 方向：对齐。工具调用安全上限是既有机制；把它的停止从静默 规模：跨包改动（ 方案：范围合理。复用既有 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 2 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewThe layering matches what this fix needs, and it follows the flow I would have proposed independently: Correctness checks I ran against the diff and base code:
Two non-blocking questions:
Nothing blocking found. sequenceDiagram
participant P1 as Session (cli)
participant P2 as Bridge (acp-bridge)
participant P3 as Web Shell
P1->>P1: loop cap trips, unstarted tools marked failed
P1->>P2: prompt rejects with RequestError (LOOP_DETECTED, loopType)
P2->>P3: turn_error event with errorKind loop_detected
P3->>P3: localized guidance, no retry, composer stays enabled
P3->>P2: page refresh reloads the live session
P2->>P3: persisted page plus appended in-memory turn_error
Files changed (14 of 14 shown)
Testing evidenceThis is an unattended CI run — no PR code was built or executed by the reviewer; the evidence below is the PR's own CI on the reviewed commit, fetched via the API. As of this review there are no red checks: the main unit suite and the web-shell visual capture are still running, and the completed checks are green (desktop shell builds on ubuntu/windows, live host on macOS, SDK Java across Java 11/17/21). The author's macOS E2E walk-through in this thread is their claim, not reviewer evidence. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: browser-side rendering of the localized error and the refresh-retention behavior — the unit suite pins the wiring (the new tests fail without the change), but no automated check drives a real Web Shell session. Sandboxed verification would settle this: 中文说明代码审查改动分层与本修复所需完全一致，也与我独立设想的方案一致： 对照 diff 与基线代码完成的正确性检查：结构化 两个非阻塞问题：其一，从共享的 测试证据本次为无人值守 CI 运行——审查方未构建或执行任何 PR 代码，以下证据为 PR 自身 CI 在被审 commit 上的结果（经 API 获取）。截至目前无红色检查：主单元测试与 web-shell 视觉截图仍在运行，已完成检查均为绿色。作者在帖中的 macOS 手工验证是其声明，不构成审查证据。未验证项：浏览器侧的本地化错误渲染与刷新保留行为——单元测试固定了接线（新测试在缺少改动时会失败），但没有自动化检查驱动真实 Web Shell 会话。维护者可用 — Qwen Code · qwen3.8-max Reviewed at |
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. |
ytahdn
added
the
autofix/takeover
|
Confidence: 4/5 — solid, well-layered fix with tests that pin the change; two non-blocking questions (the cron/background stop-message removal, and the refresh-append dedup edge) are worth an author reply but don't block. Stepping back: this is exactly the change I would have proposed. The loop-protection stop already preserves the failed tool cards; the missing piece was a terminal that tells the user the turn (not the session) stopped, in their language, without offering a pointless retry. Reusing the existing Reservations, named plainly: the shared-helper change quietly drops the transcript explanation on cron/background stops, so the PR description's "cron/background behavior is unchanged" is not strictly accurate. I suspect the honest answer is "intentional, cosmetic for non-interactive paths" — but the author should say so, and the design doc could carry one line about it. The refresh-append dedup question is a confirmation, not a demonstrated defect. CI is still running on the reviewed commit (main unit suite pending, no red checks so far), so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 扎实、分层合理的修复，新测试将改动固定下来；两个非阻塞问题（cron/后台停止消息的移除、刷新追加的去重边界）值得作者回应，但不阻塞合入。 回顾整体：这正是我会提出的方案。循环保护停止本就保留失败工具卡片，缺的是一个用用户语言说明"停止的是本轮而非会话"、且不提供无意义重试的终止事件。复用既有 保留意见直说：共享 helper 的改动顺带移除了 cron/后台停止时的转录解释文案，因此 PR 描述中"cron/后台行为不变"并不严格准确。我倾向于答案是"有意为之，对非交互路径仅是展示层面"——但应由作者确认，设计文档也可以补一句。刷新追加的去重问题属于待确认项，不是已证实的缺陷。 被审 commit 的 CI 仍在运行（主单元测试未完成，目前无红色检查），因此批准推迟至 CI 在该 commit 上全绿后执行——finalize 任务会在全部检查通过后提交绑定 commit 的批准；若有检查变红或 head 移动，则标记状态评论。 — Qwen Code · qwen3.8-max Reviewed at |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review summary — PR #8853Feedback points and dispositions1. Failed check:
|
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): You are review agent reverse-audit — Reverse audit agen...: (none); This PR converts foreground tool-loop-protection stops in...: did not trace selectBackwardPageUuids → replayTranscriptRecordPage end-to-end to prove backward pages replay in chronological order (assumed when validating…; This PR converts foreground tool-loop-protection stops in...: did not run the web-shell/acp-bridge/Session test suites to confirm the new tests pass (code audit only).; You are review agent 8a — a diff-specialized finder for...: did not trace the daemon-side transcript writer to independently prove terminal events are never persisted (relied on the structural argument that turn_error is…; This PR converts foreground tool-loop-protection stops in...: end-to-end runtime verification of the cron channel-delivery skip in finding 1 (no cron E2E executed; mechanism verified by code path only)., and 1 more.
中文说明
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：(none)；This PR converts foreground tool-loop-protection stops in...：did not trace selectBackwardPageUuids → replayTranscriptRecordPage end-to-end to prove backward pages replay in chronological order (assumed when validating…；This PR converts foreground tool-loop-protection stops in...：did not run the web-shell/acp-bridge/Session test suites to confirm the new tests pass (code audit only).；You are review agent 8a — a diff-specialized finder for...：did not trace the daemon-side transcript writer to independently prove terminal events are never persisted (relied on the structural argument that turn_error is…；This PR converts foreground tool-loop-protection stops in...：end-to-end runtime verification of the cron channel-delivery skip in finding 1 (no cron E2E executed; mechanism verified by code path only).，另有 1 条。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (toolRun.loopDetected) { | ||
| this.todoStopGuard.suspend(); | ||
| await this.#preserveStoppedToolRun(toolRun, pendingSend.signal); |
There was a problem hiding this comment.
[Critical] The new unconditional throw createLoopDetectedTurnError(toolLoopState) inside #runStopContinuation is shared by cron and background-notification callers via #handleStopHookLoop (Session.ts:6380-6389 and 6990-6997; none of the three #runStopContinuation call sites is wrapped in try/catch). The continuation's loop state is created with createDaemonToolLoopState('off') (Session.ts:4328), which disables only the repeated-failure guard — the per-turn tool-call cap and invalid-params stagnation detectors stay active, so a Stop continuation can still trip loop detection. Pre-diff this path returned {kind:'terminal', stopReason:'end_turn'}; now the throw propagates into the cron catch-all (Session.ts:6397-6407): cronHadError = true, completion status flips to 'error', and cronCompleted stays false so #scheduleChannelDelivery (Session.ts:6432-6446) is silently skipped. Background-notification turns degrade into the [notification error] path (~7004). This contradicts the design doc added by this PR ("Cron and background-notification work keep their existing non-interactive handling") — true for the main loops (Session.ts:6372-6378, 6974-6981), not for this shared Stop-continuation path. — Failure scenario: a cron/background-notification turn with pending todos enters a Stop continuation whose tool calls trip loop detection (e.g. three invalid-params attempts for one tool, or the per-turn cap) → the turn is classified 'error', emits [cron error] Tool-call loop protection stopped this turn…, and the configured channel delivery is silently skipped — the scheduled task's output never reaches its target. Suggested fix: gate the rejection on the caller (e.g. a rejectOnLoopDetected option only the foreground chain sets), or catch the LOOP_DETECTED RequestError in the cron/notification handlers and restore the graceful end-turn handling:
// #runStopContinuation — foreground chain passes rejectOnLoopDetected: true
if (toolRun.loopDetected) {
this.todoStopGuard.suspend();
await this.#preserveStoppedToolRun(toolRun, pendingSend.signal);
if (pendingSend.signal.aborted) { /* cancelled return, as today */ }
if (!options.rejectOnLoopDetected) {
return { kind: 'terminal', stopReason: 'end_turn' }; // pre-PR behavior for automatic turns
}
throw createLoopDetectedTurnError(toolLoopState);
}中文说明
[Critical] #runStopContinuation 中新增的无条件 throw createLoopDetectedTurnError(toolLoopState) 会被 cron 和后台通知调用方通过 #handleStopHookLoop 共享（调用点见 Session.ts:6380-6389 与 6990-6997；#runStopContinuation 的三个调用点均未包裹 try/catch）。续跑（continuation）的循环状态以 createDaemonToolLoopState('off') 创建（Session.ts:4328），'off' 仅关闭重复失败守卫——单轮工具调用上限与无效参数停滞检测仍然有效，因此 Stop 续跑仍可能触发循环检测。改动前该路径返回 {kind:'terminal', stopReason:'end_turn'}；现在 throw 会传入 cron 的兜底 catch（Session.ts:6397-6407）：cronHadError = true、完成状态变为 'error'，且 cronCompleted 保持 false 导致 #scheduleChannelDelivery（Session.ts:6432-6446）被静默跳过。后台通知轮次同样退化为 [notification error] 路径（约 7004 行）。这与本 PR 新增设计文档中“Cron 与后台通知保持既有非交互处理”的承诺矛盾——该承诺仅对主循环（Session.ts:6372-6378、6974-6981）成立，对这条共享的 Stop 续跑路径不成立。 — 失败场景：带有待办 todos 的 cron/后台通知轮次进入 Stop 续跑，续跑中的工具调用触发循环检测（例如同一工具连续 3 次无效参数，或达到单轮上限）→ 该轮被标记为 'error'，输出 [cron error] Tool-call loop protection stopped this turn…，且配置的频道投递被静默跳过——定时任务的输出永远无法送达目标。建议修复：按调用方门控该拒绝（例如仅前台链路设置 rejectOnLoopDetected 选项），或在 cron/通知处理器中捕获 LOOP_DETECTED RequestError 并恢复原有的正常结束轮次处理（见上方代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const replay = entry.events.snapshotReplay(); | ||
| const turnError = entry.turnError | ||
| ? [ |
There was a problem hiding this comment.
[Suggestion] snapshotReplay() is computed unconditionally on every bounded refresh, but is only read inside the entry.turnError branch below it. It runs flattenReplaySegments() (slice + flatMap over all replay segments) plus liveJournal.slice() — O(replay window) — and the result is discarded whenever entry.turnError is unset, which is the overwhelming common case (set only on active-turn failure, deleted at the next dispatch). Pre-PR this branch returned page.events directly with no snapshot; snapshot() has no side effects, so deferring it is safe. — Concrete cost: every Web Shell refresh of a live session without a turn error pays the full replay-window flatten + copy and discards it; long sessions (replay window up to the configured caps) pay proportionally more on this user-facing path. Suggested fix: hoist the snapshot into the branch that uses it:
const turnError = entry.turnError
? (() => {
const replay = entry.events.snapshotReplay();
return [
...(replay?.compactedTurns ?? []),
...(replay?.liveJournal ?? []),
]
.reverse()
.find((event) => event.type === 'turn_error');
})()
: undefined;中文说明
[Suggestion] snapshotReplay() 在每次 bounded refresh 上都会被无条件计算，但只有下方 entry.turnError 分支会读取它。该调用会执行 flattenReplaySegments()（对全部 replay 段做 slice + flatMap）外加 liveJournal.slice()——开销为 O(replay 窗口)——而 entry.turnError 未设置时（绝大多数情况：它只在活跃轮次失败时设置、在下次 dispatch 时删除）结果会被直接丢弃。改动前该分支直接返回 page.events，完全不做快照；snapshot() 没有副作用，因此延迟执行是安全的。 — 具体代价：每次没有 turn error 的 live session 刷新（Web Shell 的常见场景）都要为整个 replay 窗口付出一次 flatten + 拷贝再丢弃；长会话（replay 窗口可达配置上限）在这条用户可见路径上的开销按比例放大。建议修复：将快照移入使用它的分支内（见上方代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (block.errorKind !== 'loop_detected') { | ||
| retryableTurnErrorId = block.id; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The "loop_detected is not retryable" policy is encoded as two independent string checks — here and in transcriptToMessages.ts:782-783 (retryable: errorBlock.source === 'turn_error' && errorKind !== 'loop_detected') — with no shared predicate; grep confirms these are the only two loop_detected policy sites in web-shell, and this diff itself extends the DAEMON_ERROR_KINDS taxonomy they key off. — Failure scenario: when a second non-retryable kind is added, both sites must be edited; updating only the adapter leaves the composer retry affordance offering to resubmit the exact prompt the daemon just terminated (for loop_detected: re-running the prompt that tripped loop protection, likely re-looping and burning another turn), and updating only App.tsx leaves the per-message affordance inconsistent with the hint. Suggested fix: extract one predicate in the transcript adapter and consume it from both sites:
export function isRetryableTurnErrorKind(errorKind: string | undefined): boolean {
return errorKind !== 'loop_detected';
}中文说明
[Suggestion] “loop_detected 不可重试”这一策略被写成两处相互独立的字符串比较——此处与 transcriptToMessages.ts:782-783（retryable: errorBlock.source === 'turn_error' && errorKind !== 'loop_detected'）——没有共享谓词；grep 确认这是 web-shell 中仅有的两处 loop_detected 策略点，而本 PR 本身还在扩展它们所依赖的 DAEMON_ERROR_KINDS 分类。 — 失败场景：将来新增第二种不可重试的 kind 时，两处都必须修改；只改 adapter 会让输入框上方的重试入口仍然提议重新提交刚被 daemon 终止的 prompt（对 loop_detected 而言就是重跑触发循环保护的 prompt，很可能再次循环并再烧一轮）；只改 App.tsx 则会让单条消息上的重试入口与提示不一致。建议修复：在 transcript adapter 中提取一个共享谓词（见上方代码），两处都消费它。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const code = | ||
| structuredErrorKind === 'loop_detected' | ||
| ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) | ||
| : extractErrorCode(err); |
There was a problem hiding this comment.
[Suggestion] The preference for the structured data.code over the JSON-RPC numeric code is gated on structuredErrorKind === 'loop_detected' instead of on "this error is structured" (structuredErrorKind !== undefined). This PR establishes new RequestError(-32603, msg, { code: 'X', errorKind: 'x_kind' }) as the pattern for structured turn errors, and the DAEMON_ERROR_KINDS taxonomy it extends is explicitly growing. No observable misbehavior today — the other structured producers (session_writer_* kinds) carry no data.code and their per-kind rpcCodes are meaningful — but the gate is written for exactly one kind. — Failure scenario: the next error kind thrown with this PR's RequestError pattern passes through broadcastTurnError unchanged and ships code: '-32603' (a stringified JSON-RPC number) in its turn_error payload until someone edits this function again.
| const code = | |
| structuredErrorKind === 'loop_detected' | |
| ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) | |
| : extractErrorCode(err); | |
| const code = | |
| structuredErrorKind !== undefined | |
| ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) | |
| : extractErrorCode(err); |
中文说明
[Suggestion] 结构化 data.code 优先于 JSON-RPC 数字 code 的取值逻辑被门控在 structuredErrorKind === 'loop_detected' 上，而不是门控在“这是一个结构化错误”（structuredErrorKind !== undefined）上。本 PR 已把 new RequestError(-32603, msg, { code: 'X', errorKind: 'x_kind' }) 确立为结构化轮次错误的模式，而它所扩展的 DAEMON_ERROR_KINDS 分类明确还在增长。当前没有可观察的错误行为——其他结构化来源（session_writer_* 系列）不携带 data.code，且各自的 rpcCode 本身有意义——但这个门控只为一种 kind 而写。 — 失败场景：下一个按本 PR 的 RequestError 模式抛出的错误 kind 会原样通过 broadcastTurnError，在其 turn_error 负载中携带 code: '-32603'（被字符串化的 JSON-RPC 数字），直到有人再次修改这个函数。建议按上方 suggestion 修改（对现有行为逐字节等价，未来的结构化 kind 自动获得正确的取值优先级）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (nextAfterTools.stoppedByRepeatedToolFailure) { | ||
| return { | ||
| stopReason: getAbortAwareEndTurnStopReason( | ||
| pendingSend.signal, | ||
| ), | ||
| }; | ||
| if (pendingSend.signal.aborted) { | ||
| return { stopReason: 'cancelled' }; | ||
| } | ||
| throw createLoopDetectedTurnError(toolLoopState); | ||
| } |
There was a problem hiding this comment.
[Suggestion] This abort-precedence-then-throw block is pasted four times in this diff — here and at Session.ts:3947-3951 (plain { stopReason: 'cancelled' } shape), plus 4883-4892 and 4902-4911 ({ kind: 'terminal', stopReason: 'cancelled', supersededAutomaticContinuation? } shape) — replacing two call sites of the file's existing precedence helper getAbortAwareEndTurnStopReason (Session.ts:402) with inlined copies. The PR's stated invariant "cancellation takes precedence when it races the loop stop" is now encoded in four separate places in two already-divergent shapes. — Failure scenario: any future change to loop-stop terminal semantics — a new structured field on the error, a different stop reason, superseded-continuation handling in the guard-loop copies — must be applied to all four sites; missing one makes one loop type (tool-call cap vs repeated-failure vs continuation) resolve to a different stopReason/outcome than the others, silently per detector. Suggested fix: extract one helper next to createLoopDetectedTurnError and call it from all four sites, building each site's return object around the returned 'cancelled':
function cancelledOrThrowLoopDetected(
signal: AbortSignal,
loopState: DaemonToolLoopState,
): 'cancelled' {
if (signal.aborted) return 'cancelled';
throw createLoopDetectedTurnError(loopState);
}中文说明
[Suggestion] 这个“先检查 abort 再 throw”的块在本 diff 中被粘贴了四次——此处与 Session.ts:3947-3951（普通 { stopReason: 'cancelled' } 形状），以及 4883-4892 与 4902-4911（{ kind: 'terminal', stopReason: 'cancelled', supersededAutomaticContinuation? } 形状）——把该文件已有的优先级辅助函数 getAbortAwareEndTurnStopReason（Session.ts:402）的两处调用替换成了内联副本。PR 声明的不变量“取消与循环停止竞争时以取消为准”现在被编码在四个位置、且已是两种相互分歧的形状。 — 失败场景：未来任何对循环停止终止语义的修改——错误上新增结构化字段、更换 stop reason、在 guard-loop 副本中处理 superseded-continuation——都必须同时应用到四处；漏掉任何一处都会让某种循环类型（工具调用上限 / 重复失败 / 续跑）解析出与其他类型不同的 stopReason/结果，且按检测器静默分歧。建议修复：在 createLoopDetectedTurnError 旁提取一个辅助函数（见上方代码），四处调用它，各调用点围绕返回的 'cancelled' 构造自己的返回对象。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(refreshed.compactedReplay).toContainEqual( | ||
| expect.objectContaining({ | ||
| type: 'turn_error', |
There was a problem hiding this comment.
[Suggestion] This test mocks the persisted transcript page with events: [] and asserts with toContainEqual, so the position of the appended turn_error is unpinned. Probe-verified on this commit: mutating refreshedReplayFieldsFor from [...page.events, turnError] to [turnError, ...page.events] survives the test; strengthening the mocked page with one event and asserting the turn_error is the last element fails under the same mutation (and passes against the PR as written). — Failure scenario: a future refactor of the bounded-refresh path prepends (or otherwise misorders) the in-memory turn_error; with a non-empty persisted page, Web Shell would render the loop-detected error block above the replayed history of the turn it terminates, and this test stays green because toContainEqual over a single element plus an empty array cannot distinguish append from prepend. Suggested fix: return at least one event from the mocked sessionTranscript response and pin the position:
expect(
refreshed.compactedReplay[refreshed.compactedReplay.length - 1],
).toMatchObject({ type: 'turn_error', promptId: 'prompt-loop' });中文说明
[Suggestion] 该测试把持久化转录分页 mock 成 events: [] 并用 toContainEqual 断言，因此追加的 turn_error 的位置没有被钉住。已在本提交上用探针验证：把 refreshedReplayFieldsFor 从 [...page.events, turnError] 变异为 [turnError, ...page.events]，测试仍然通过；把 mock 分页改为返回一个事件并断言 turn_error 是最后一个元素后，同一变异会使测试失败（对未变异的 PR 代码则通过）。 — 失败场景：未来对 bounded-refresh 路径的重构把内存中的 turn_error 前置（或以其他方式错序）；在持久化分页非空时，Web Shell 会把循环检测错误块渲染在它终止的那轮重放历史上方，而本测试仍为绿色——因为 toContainEqual 作用于单元素加空数组时无法区分追加与前置。建议修复：让 mock 的 sessionTranscript 响应至少返回一个事件，并钉住位置（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (nextAfterTools.stoppedByRepeatedToolFailure) { | ||
| if (pendingSend.signal.aborted) { | ||
| return { | ||
| kind: 'terminal', |
There was a problem hiding this comment.
[Suggestion] The diff deletes the only test covering cancellation racing a repeated-tool-failure loop stop (returns cancelled when cancellation arrives while the stop message is emitted) and adds no replacement for that path. Verified: the replacement test (lets cancellation win while a loop-detected Stop continuation is preserved) reaches only the tool-call-cap toolRun.loopDetected branch of #runStopContinuation — #buildNextMessageAfterToolRun is never called on that path. The pre-existing drain test does not cover these branches either: its cancellation fires #buildNextMessageAfterToolRun's abort early-return (Session.ts:5370-5382) BEFORE the stop decision is computed, so stoppedByRepeatedToolFailure is never set (its own not.toHaveBeenCalledWith(... REPEATED_TOOL_EXECUTION_FAILURE) assertion proves it). The new abort sub-branches at Session.ts:3937-3939 and 4902-4910 therefore have zero cancellation coverage. — Failure scenario: if the if (pendingSend.signal.aborted) check were dropped or reordered during a future refactor of the four-way duplicated block, a user cancellation landing during the post-failure drain would surface a LOOP_DETECTED turn error instead of stopReason: 'cancelled' — violating the PR's own "cancellation still wins if it races loop termination" promise — and no test would fail. Suggested fix: add a cancellation test for the repeated-tool-failure path (failing tool + failure streak in enforce mode, cancel during the post-failure drain window — the sibling test's blocking-rewriter pattern reaches that window — expect { stopReason: 'cancelled' }), or parameterize the existing cancellation test over both loop triggers.
中文说明
[Suggestion] 本 diff 删除了唯一覆盖“取消与重复工具失败循环停止竞争”的测试（returns cancelled when cancellation arrives while the stop message is emitted），且没有为该路径补充替代测试。已核实：替代测试（lets cancellation win while a loop-detected Stop continuation is preserved）只到达 #runStopContinuation 中工具调用上限的 toolRun.loopDetected 分支——该路径根本不会调用 #buildNextMessageAfterToolRun。既有的 drain 测试也未覆盖这些分支：它的取消触发的是 #buildNextMessageAfterToolRun 的 abort 提前返回（Session.ts:5370-5382），发生在停止决定计算之前，因此 stoppedByRepeatedToolFailure 从未被置位（该测试自己的 not.toHaveBeenCalledWith(... REPEATED_TOOL_EXECUTION_FAILURE) 断言恰好证明了这一点）。于是 Session.ts:3937-3939 与 4902-4910 新增的 abort 子分支完全没有取消场景覆盖。 — 失败场景：如果未来重构这个四处复制的块时删除或调换了 if (pendingSend.signal.aborted) 检查，用户在失败后 drain 期间发起的取消会表现为 LOOP_DETECTED 轮次错误而不是 stopReason: 'cancelled'——违背 PR 自己承诺的“取消与循环终止竞争时仍以取消为准”——且没有任何测试会失败。建议修复：为重复工具失败路径补一个取消测试（enforce 模式下的失败工具 + 失败连击，在失败后 drain 窗口内取消——姊妹测试的阻塞 rewriter 模式可以到达该窗口——期望 { stopReason: 'cancelled' }），或把现有取消测试参数化到两种循环触发器。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| compactedReplay: turnError | ||
| ? [...page.events, turnError] | ||
| : page.events, |
There was a problem hiding this comment.
[Suggestion] Probe-verified on this commit (outOfOrder=true on the unmodified PR; flipped by the guard below): cron/background-notification turns execute inside the CLI session and bypass bridge prompt admission — the only delete entry.turnError site (bridge.ts:6778) — while their session updates still flow through the bridge fan-in (bridgeClient.ts:908-927) into the journal with newer ids and into persisted records. This append then places the stale loop turn_error after page.events that already end with the newer automatic turn's content, so the refreshed transcript renders the loop error after content that actually happened later — and the stale entry.turnError keeps re-appending it on every refresh until the next interactive prompt admission. — Failure scenario: an interactive prompt fails with loop detection; before the user refreshes, a cron tick or background-notification turn runs on the same session → the refreshed Web Shell transcript shows the loop error after the automatic turn's output, reading as if the error belongs to the automatic turn. Suggested fix: only append when the found turn_error is still the last journaled event (or store the published event on the entry at broadcast time and append it only while it is newest):
| compactedReplay: turnError | |
| ? [...page.events, turnError] | |
| : page.events, | |
| compactedReplay: | |
| turnError && | |
| [...(replay?.compactedTurns ?? []), ...(replay?.liveJournal ?? [])].at(-1)?.id === turnError.id | |
| ? [...page.events, turnError] | |
| : page.events, |
中文说明
[Suggestion] 已在本提交上用探针验证（未修改的 PR 代码上观察到 outOfOrder=true；加上下方守卫后翻转）：cron/后台通知轮次在 CLI 会话内部执行，绕过 bridge 的 prompt 准入——而那是唯一 delete entry.turnError 的位置（bridge.ts:6778）——但它们的 session update 仍会经由 bridge 扇入（bridgeClient.ts:908-927）以更小的新 id 进入日志并写入持久化记录。于是这里的追加会把过期的循环 turn_error 放在已经以更新的自动轮次内容结尾的 page.events 之后，刷新后的转录会把循环错误渲染在实际发生得更晚的内容之后——且过期的 entry.turnError 会在每次刷新时重复追加，直到下一个交互 prompt 准入。 — 失败场景：交互 prompt 因循环检测失败；用户刷新之前，同一会话上运行了一次 cron 或后台通知轮次 → 刷新后的 Web Shell 转录在自动轮次的输出之后显示循环错误，读起来像是该错误属于那个自动轮次。建议修复：仅当找到的 turn_error 仍是日志中最后一个事件时才追加（见上方 suggestion；或在广播时把发布的事件存到 entry 上，仅当它仍最新时追加）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| throw createLoopDetectedTurnError(toolLoopState); | ||
| } | ||
| if (nextAfterTools.hadMidTurnUserInput) { |
There was a problem hiding this comment.
[Suggestion] This stoppedByRepeatedToolFailure branch added to #runStopContinuation is unreachable: the function creates its only loop state via createDaemonToolLoopState('off') (Session.ts:4328, never re-created or mutated), and reduceRepeatedToolFailureGuard short-circuits to {kind:'none'} for mode 'off' (repeated-tool-failure-guard.ts:186-188) — so #buildNextMessageAfterToolRun can never return stoppedByRepeatedToolFailure: true here. The sibling toolRun.loopDetected branch above IS reachable (cap detection is mode-independent); only this branch is dead. — Concrete cost: 12 lines of dead code tell readers a Stop-hook continuation can terminate with a repeated-failure loop error, which it cannot, and add a fourth divergent site to the pasted abort-throw block flagged above; if the mode at Session.ts:4328 is ever changed to make this reachable, the throw would also land in the cron/background-notification callers of #handleStopHookLoop — the exact regression the design doc rules out for them. Suggested fix: drop the branch from #runStopContinuation (keep it only at the foreground site, Session.ts:3936), or add a comment stating it is defensive for a future mode change.
中文说明
[Suggestion] 新增到 #runStopContinuation 中的这个 stoppedByRepeatedToolFailure 分支不可达：该函数唯一的循环状态由 createDaemonToolLoopState('off') 创建（Session.ts:4328，之后从不重建或变更模式），而 reduceRepeatedToolFailureGuard 对 mode 'off' 直接短路返回 {kind:'none'}（repeated-tool-failure-guard.ts:186-188）——因此 #buildNextMessageAfterToolRun 在这里永远不可能返回 stoppedByRepeatedToolFailure: true。上方姊妹分支 toolRun.loopDetected 是可达的（上限检测与 mode 无关）；只有这个分支是死代码。 — 具体代价：12 行死代码会让读者以为 Stop-hook 续跑可能以重复失败循环错误终止，而实际不可能，并且为上文标记的粘贴式 abort-throw 块增加了第四个分歧位置；如果 Session.ts:4328 的 mode 未来被改为使该分支可达，这个 throw 还会落入 #handleStopHookLoop 的 cron/后台通知调用方——正是设计文档明确排除的回归。建议修复：从 #runStopContinuation 中删除该分支（仅保留前台位置 Session.ts:3936 的那份），或加注释说明它是为未来 mode 变更预留的防御代码。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 2/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31389561905 🧠 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: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31394772225 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 4/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31399630023 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. 3 of those were silent-sandbox (idle) timeouts that no budget increase can cure — investigate the sandbox image and runner docker daemon for those. A human should investigate the sandbox image and runner docker daemon, then comment What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31404260822 🧠 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 explored to full depth (tool budget reached): PR #8853: converts foreground tool-loop-protection stops ...: none — all checks I started were completed within budget.; PR #8853: converts foreground tool-loop-protection stops ...: did not run build/tests; did not exhaustively trace the bridge's queued-prompt deadline flow beyond reading the broadcastTurnError comment; did not verify the d…; PR #8853: converts foreground tool-loop-protection stops ...: did not run build/unit tests to confirm the new tests pass (read-only review).; PR #8853: converts foreground tool-loop-protection stops ...: did not exhaustively trace the bridge queued-prompt deadline/teardown-flush flows beyond broadcastTurnError 's documented mutateTurnState gating.; PR #8853: converts foreground tool-loop-protection stops ...: did not read compactionEngine.ts deeply enough to rule out that compaction always retains terminal events (bears only on Finding 2's mode 1 likelihood)., and 1 more.
中文说明
未探索到全部深度（达到工具调用预算）：PR #8853: converts foreground tool-loop-protection stops ...：none — all checks I started were completed within budget.；PR #8853: converts foreground tool-loop-protection stops ...：did not run build/tests; did not exhaustively trace the bridge's queued-prompt deadline flow beyond reading the broadcastTurnError comment; did not verify the d…；PR #8853: converts foreground tool-loop-protection stops ...：did not run build/unit tests to confirm the new tests pass (read-only review).；PR #8853: converts foreground tool-loop-protection stops ...：did not exhaustively trace the bridge queued-prompt deadline/teardown-flush flows beyond broadcastTurnError 's documented mutateTurnState gating.；PR #8853: converts foreground tool-loop-protection stops ...：did not read compactionEngine.ts deeply enough to rule out that compaction always retains terminal events (bears only on Finding 2's mode 1 likelihood).，另有 1 条。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| throw createLoopDetectedTurnError(toolLoopState); | ||
| } | ||
| const nextAfterTools = await this.#buildNextMessageAfterToolRun( |
There was a problem hiding this comment.
[Critical] R1-1: Round 2 — still stands at the reviewed commit 54cfa0f (this file is unchanged since round 1; HEAD moved only via a merge of main). The new unconditional throw createLoopDetectedTurnError(toolLoopState) inside #runStopContinuation is shared by cron and background-notification callers via #handleStopHookLoop (Session.ts:6380-6389 and 6990-6997; none of the three #runStopContinuation call sites is wrapped in try/catch). The continuation's loop state is created with createDaemonToolLoopState('off') (Session.ts:4328), which disables only the repeated-failure guard — the per-turn tool-call cap and invalid-params stagnation detectors stay active, so a Stop continuation can still trip loop detection. Pre-diff this path returned {kind:'terminal', stopReason:'end_turn'}; now the throw propagates into the cron catch-all (Session.ts:6397-6407): cronHadError = true, completion status flips to 'error', and cronCompleted stays false so #scheduleChannelDelivery (Session.ts:6432-6446) is silently skipped. Background-notification turns degrade into the [notification error] path (~7004). This contradicts the design doc added by this PR ("Cron and background-notification work keep their existing non-interactive handling") — true for the main loops (Session.ts:6372-6378, 6974-6981), not for this shared Stop-continuation path. — Failure scenario: a cron/background-notification turn with pending todos enters a Stop continuation whose tool calls trip loop detection (e.g. three invalid-params attempts for one tool, or the per-turn cap) → the turn is classified 'error', emits [cron error] Tool-call loop protection stopped this turn…, and the configured channel delivery is silently skipped — the scheduled task's output never reaches its target. Suggested fix: gate the rejection on the caller (e.g. a rejectOnLoopDetected option only the foreground chain sets), or catch the LOOP_DETECTED RequestError in the cron/notification handlers and restore the graceful end-turn handling:
// #runStopContinuation — foreground chain passes rejectOnLoopDetected: true
if (toolRun.loopDetected) {
this.todoStopGuard.suspend();
await this.#preserveStoppedToolRun(toolRun, pendingSend.signal);
if (pendingSend.signal.aborted) { /* cancelled return, as today */ }
if (!options.rejectOnLoopDetected) {
return { kind: 'terminal', stopReason: 'end_turn' }; // pre-PR behavior for automatic turns
}
throw createLoopDetectedTurnError(toolLoopState);
}中文说明
[Critical] #runStopContinuation 中新增的无条件 throw createLoopDetectedTurnError(toolLoopState) 会被 cron 和后台通知调用方通过 #handleStopHookLoop 共享（调用点见 Session.ts:6380-6389 与 6990-6997；#runStopContinuation 的三个调用点均未包裹 try/catch）。续跑（continuation）的循环状态以 createDaemonToolLoopState('off') 创建（Session.ts:4328），'off' 仅关闭重复失败守卫——单轮工具调用上限与无效参数停滞检测仍然有效，因此 Stop 续跑仍可能触发循环检测。改动前该路径返回 {kind:'terminal', stopReason:'end_turn'}；现在 throw 会传入 cron 的兜底 catch（Session.ts:6397-6407）：cronHadError = true、完成状态变为 'error'，且 cronCompleted 保持 false 导致 #scheduleChannelDelivery（Session.ts:6432-6446）被静默跳过。后台通知轮次同样退化为 [notification error] 路径（约 7004 行）。这与本 PR 新增设计文档中“Cron 与后台通知保持既有非交互处理”的承诺矛盾——该承诺仅对主循环（Session.ts:6372-6378、6974-6981）成立，对这条共享的 Stop 续跑路径不成立。 — 失败场景：带有待办 todos 的 cron/后台通知轮次进入 Stop 续跑，续跑中的工具调用触发循环检测（例如同一工具连续 3 次无效参数，或达到单轮上限）→ 该轮被标记为 'error'，输出 [cron error] Tool-call loop protection stopped this turn…，且配置的频道投递被静默跳过——定时任务的输出永远无法送达目标。建议修复：按调用方门控该拒绝（例如仅前台链路设置 rejectOnLoopDetected 选项），或在 cron/通知处理器中捕获 LOOP_DETECTED RequestError 并恢复原有的正常结束轮次处理（见上方代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const replay = entry.events.snapshotReplay(); | ||
| const turnError = entry.turnError |
There was a problem hiding this comment.
[Suggestion] R1-2: Round 2 — still stands at the reviewed commit (unchanged since round 1). snapshotReplay() is computed unconditionally on every bounded refresh, but is only read inside the entry.turnError branch below it. It runs flattenReplaySegments() (slice + flatMap over all replay segments) plus liveJournal.slice() — O(replay window) — and the result is discarded whenever entry.turnError is unset, which is the overwhelming common case (set only on active-turn failure, deleted at the next dispatch). Pre-PR this branch returned page.events directly with no snapshot; snapshot() has no side effects, so deferring it is safe. — Concrete cost: every Web Shell refresh of a live session without a turn error pays the full replay-window flatten + copy and discards it; long sessions (replay window up to the configured caps) pay proportionally more on this user-facing path. Suggested fix: hoist the snapshot into the branch that uses it:
const turnError = entry.turnError
? (() => {
const replay = entry.events.snapshotReplay();
return [
...(replay?.compactedTurns ?? []),
...(replay?.liveJournal ?? []),
]
.reverse()
.find((event) => event.type === 'turn_error');
})()
: undefined;中文说明
[Suggestion] snapshotReplay() 在每次 bounded refresh 上都会被无条件计算，但只有下方 entry.turnError 分支会读取它。该调用会执行 flattenReplaySegments()（对全部 replay 段做 slice + flatMap）外加 liveJournal.slice()——开销为 O(replay 窗口)——而 entry.turnError 未设置时（绝大多数情况：它只在活跃轮次失败时设置、在下次 dispatch 时删除）结果会被直接丢弃。改动前该分支直接返回 page.events，完全不做快照；snapshot() 没有副作用，因此延迟执行是安全的。 — 具体代价：每次没有 turn error 的 live session 刷新（Web Shell 的常见场景）都要为整个 replay 窗口付出一次 flatten + 拷贝再丢弃；长会话（replay 窗口可达配置上限）在这条用户可见路径上的开销按比例放大。建议修复：将快照移入使用它的分支内（见上方代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (block.errorKind !== 'loop_detected') { | ||
| retryableTurnErrorId = block.id; |
There was a problem hiding this comment.
[Suggestion] R1-3: Round 2 — still stands at the reviewed commit (unchanged since round 1). The "loop_detected is not retryable" policy is encoded as two independent string checks — here and in transcriptToMessages.ts:782-783 (retryable: errorBlock.source === 'turn_error' && errorKind !== 'loop_detected') — with no shared predicate; grep confirms these are the only two loop_detected policy sites in web-shell, and this diff itself extends the DAEMON_ERROR_KINDS taxonomy they key off. — Failure scenario: when a second non-retryable kind is added, both sites must be edited; updating only the adapter leaves the composer retry affordance offering to resubmit the exact prompt the daemon just terminated (for loop_detected: re-running the prompt that tripped loop protection, likely re-looping and burning another turn), and updating only App.tsx leaves the per-message affordance inconsistent with the hint. Suggested fix: extract one predicate in the transcript adapter and consume it from both sites:
export function isRetryableTurnErrorKind(errorKind: string | undefined): boolean {
return errorKind !== 'loop_detected';
}中文说明
[Suggestion] “loop_detected 不可重试”这一策略被写成两处相互独立的字符串比较——此处与 transcriptToMessages.ts:782-783（retryable: errorBlock.source === 'turn_error' && errorKind !== 'loop_detected'）——没有共享谓词；grep 确认这是 web-shell 中仅有的两处 loop_detected 策略点，而本 PR 本身还在扩展它们所依赖的 DAEMON_ERROR_KINDS 分类。 — 失败场景：将来新增第二种不可重试的 kind 时，两处都必须修改；只改 adapter 会让输入框上方的重试入口仍然提议重新提交刚被 daemon 终止的 prompt（对 loop_detected 而言就是重跑触发循环保护的 prompt，很可能再次循环并再烧一轮）；只改 App.tsx 则会让单条消息上的重试入口与提示不一致。建议修复：在 transcript adapter 中提取一个共享谓词（见上方代码），两处都消费它。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const code = | ||
| structuredErrorKind === 'loop_detected' | ||
| ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) |
There was a problem hiding this comment.
[Suggestion] R1-4: Round 2 — still stands at the reviewed commit (unchanged since round 1). The preference for the structured data.code over the JSON-RPC numeric code is gated on structuredErrorKind === 'loop_detected' instead of on "this error is structured" (structuredErrorKind !== undefined). This PR establishes new RequestError(-32603, msg, { code: 'X', errorKind: 'x_kind' }) as the pattern for structured turn errors, and the DAEMON_ERROR_KINDS taxonomy it extends is explicitly growing. No observable misbehavior today — the other structured producers (session_writer_* kinds) carry no data.code and their per-kind rpcCodes are meaningful — but the gate is written for exactly one kind. — Failure scenario: the next error kind thrown with this PR's RequestError pattern passes through broadcastTurnError unchanged and ships code: '-32603' (a stringified JSON-RPC number) in its turn_error payload until someone edits this function again.
| const code = | |
| structuredErrorKind === 'loop_detected' | |
| ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) | |
| const code = | |
| structuredErrorKind !== undefined | |
| ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) | |
| : extractErrorCode(err); |
中文说明
[Suggestion] 结构化 data.code 优先于 JSON-RPC 数字 code 的取值逻辑被门控在 structuredErrorKind === 'loop_detected' 上，而不是门控在“这是一个结构化错误”（structuredErrorKind !== undefined）上。本 PR 已把 new RequestError(-32603, msg, { code: 'X', errorKind: 'x_kind' }) 确立为结构化轮次错误的模式，而它所扩展的 DAEMON_ERROR_KINDS 分类明确还在增长。当前没有可观察的错误行为——其他结构化来源（session_writer_* 系列）不携带 data.code，且各自的 rpcCode 本身有意义——但这个门控只为一种 kind 而写。 — 失败场景：下一个按本 PR 的 RequestError 模式抛出的错误 kind 会原样通过 broadcastTurnError，在其 turn_error 负载中携带 code: '-32603'（被字符串化的 JSON-RPC 数字），直到有人再次修改这个函数。建议按上方 suggestion 修改（对现有行为逐字节等价，未来的结构化 kind 自动获得正确的取值优先级）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (nextAfterTools.stoppedByRepeatedToolFailure) { | ||
| return { | ||
| stopReason: getAbortAwareEndTurnStopReason( | ||
| pendingSend.signal, | ||
| ), | ||
| }; | ||
| if (pendingSend.signal.aborted) { | ||
| return { stopReason: 'cancelled' }; | ||
| } | ||
| throw createLoopDetectedTurnError(toolLoopState); |
There was a problem hiding this comment.
[Suggestion] R1-5: Round 2 — still stands at the reviewed commit (unchanged since round 1). This abort-precedence-then-throw block is pasted four times in this diff — here and at Session.ts:3947-3951 (plain { stopReason: 'cancelled' } shape), plus 4883-4892 and 4902-4911 ({ kind: 'terminal', stopReason: 'cancelled', supersededAutomaticContinuation? } shape) — replacing two call sites of the file's existing precedence helper getAbortAwareEndTurnStopReason (Session.ts:402) with inlined copies. The PR's stated invariant "cancellation takes precedence when it races the loop stop" is now encoded in four separate places in two already-divergent shapes. — Failure scenario: any future change to loop-stop terminal semantics — a new structured field on the error, a different stop reason, superseded-continuation handling in the guard-loop copies — must be applied to all four sites; missing one makes one loop type (tool-call cap vs repeated-failure vs continuation) resolve to a different stopReason/outcome than the others, silently per detector. Suggested fix: extract one helper next to createLoopDetectedTurnError and call it from all four sites, building each site's return object around the returned 'cancelled':
function cancelledOrThrowLoopDetected(
signal: AbortSignal,
loopState: DaemonToolLoopState,
): 'cancelled' {
if (signal.aborted) return 'cancelled';
throw createLoopDetectedTurnError(loopState);
}中文说明
[Suggestion] 这个“先检查 abort 再 throw”的块在本 diff 中被粘贴了四次——此处与 Session.ts:3947-3951（普通 { stopReason: 'cancelled' } 形状），以及 4883-4892 与 4902-4911（{ kind: 'terminal', stopReason: 'cancelled', supersededAutomaticContinuation? } 形状）——把该文件已有的优先级辅助函数 getAbortAwareEndTurnStopReason（Session.ts:402）的两处调用替换成了内联副本。PR 声明的不变量“取消与循环停止竞争时以取消为准”现在被编码在四个位置、且已是两种相互分歧的形状。 — 失败场景：未来任何对循环停止终止语义的修改——错误上新增结构化字段、更换 stop reason、在 guard-loop 副本中处理 superseded-continuation——都必须同时应用到四处；漏掉任何一处都会让某种循环类型（工具调用上限 / 重复失败 / 续跑）解析出与其他类型不同的 stopReason/结果，且按检测器静默分歧。建议修复：在 createLoopDetectedTurnError 旁提取一个辅助函数（见上方代码），四处调用它，各调用点围绕返回的 'cancelled' 构造自己的返回对象。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(refreshed.compactedReplay).toContainEqual( | ||
| expect.objectContaining({ |
There was a problem hiding this comment.
[Suggestion] R1-6: Round 2 — still stands at the reviewed commit (unchanged since round 1). This test mocks the persisted transcript page with events: [] and asserts with toContainEqual, so the position of the appended turn_error is unpinned. Probe-verified in round 1 on this code: mutating refreshedReplayFieldsFor from [...page.events, turnError] to [turnError, ...page.events] survives the test; strengthening the mocked page with one event and asserting the turn_error is the last element fails under the same mutation (and passes against the PR as written). — Failure scenario: a future refactor of the bounded-refresh path prepends (or otherwise misorders) the in-memory turn_error; with a non-empty persisted page, Web Shell would render the loop-detected error block above the replayed history of the turn it terminates, and this test stays green because toContainEqual over a single element plus an empty array cannot distinguish append from prepend. Suggested fix: return at least one event from the mocked sessionTranscript response and pin the position:
expect(
refreshed.compactedReplay[refreshed.compactedReplay.length - 1],
).toMatchObject({ type: 'turn_error', promptId: 'prompt-loop' });中文说明
[Suggestion] 该测试把持久化转录分页 mock 成 events: [] 并用 toContainEqual 断言，因此追加的 turn_error 的位置没有被钉住。已在第 1 轮用探针验证：把 refreshedReplayFieldsFor 从 [...page.events, turnError] 变异为 [turnError, ...page.events]，测试仍然通过；把 mock 分页改为返回一个事件并断言 turn_error 是最后一个元素后，同一变异会使测试失败（对未变异的 PR 代码则通过）。 — 失败场景：未来对 bounded-refresh 路径的重构把内存中的 turn_error 前置（或以其他方式错序）；在持久化分页非空时，Web Shell 会把循环检测错误块渲染在它终止的那轮重放历史上方，而本测试仍为绿色——因为 toContainEqual 作用于单元素加空数组时无法区分追加与前置。建议修复：让 mock 的 sessionTranscript 响应至少返回一个事件，并钉住位置（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (nextAfterTools.stoppedByRepeatedToolFailure) { | ||
| if (pendingSend.signal.aborted) { | ||
| return { |
There was a problem hiding this comment.
[Suggestion] R1-7: Round 2 — still stands at the reviewed commit (unchanged since round 1). The diff deletes the only test covering cancellation racing a repeated-tool-failure loop stop (returns cancelled when cancellation arrives while the stop message is emitted) and adds no replacement for that path. Verified again this round: the replacement test (lets cancellation win while a loop-detected Stop continuation is preserved) reaches only the tool-call-cap toolRun.loopDetected branch of #runStopContinuation — #buildNextMessageAfterToolRun is never called on that path. The pre-existing drain test does not cover these branches either: its cancellation fires #buildNextMessageAfterToolRun's abort early-return before the stop decision is computed. The new abort sub-branches at Session.ts:3937-3939 and here therefore have zero cancellation coverage (round-2 v8 coverage measurement: 0 hits). — Failure scenario: if the if (pendingSend.signal.aborted) check were dropped or reordered during a future refactor of the four-way duplicated block, a user cancellation landing during the post-failure drain would surface a LOOP_DETECTED turn error instead of stopReason: 'cancelled' — violating the PR's own "cancellation still wins if it races loop termination" promise — and no test would fail. Suggested fix: add a cancellation test for the repeated-tool-failure path (failing tool + failure streak in enforce mode, cancel during the post-failure drain window, expect { stopReason: 'cancelled' }), or parameterize the existing cancellation test over both loop triggers.
中文说明
[Suggestion] 本 diff 删除了唯一覆盖“取消与重复工具失败循环停止竞争”的测试（returns cancelled when cancellation arrives while the stop message is emitted），且没有为该路径补充替代测试。本轮再次核实：替代测试（lets cancellation win while a loop-detected Stop continuation is preserved）只到达 #runStopContinuation 中工具调用上限的 toolRun.loopDetected 分支——该路径根本不会调用 #buildNextMessageAfterToolRun。既有的 drain 测试也未覆盖这些分支：它的取消触发的是 #buildNextMessageAfterToolRun 的 abort 提前返回，发生在停止决定计算之前。于是 Session.ts:3937-3939 与此处新增的 abort 子分支完全没有取消场景覆盖（第 2 轮 v8 覆盖率实测：0 次命中）。 — 失败场景：如果未来重构这个四处复制的块时删除或调换了 if (pendingSend.signal.aborted) 检查，用户在失败后 drain 期间发起的取消会表现为 LOOP_DETECTED 轮次错误而不是 stopReason: 'cancelled'——违背 PR 自己承诺的“取消与循环终止竞争时仍以取消为准”——且没有任何测试会失败。建议修复：为重复工具失败路径补一个取消测试（enforce 模式下的失败工具 + 失败连击，在失败后 drain 窗口内取消，期望 { stopReason: 'cancelled' }），或把现有取消测试参数化到两种循环触发器。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| compactedReplay: turnError | ||
| ? [...page.events, turnError] |
There was a problem hiding this comment.
[Suggestion] R1-8: Round 2 — still stands at the reviewed commit (unchanged since round 1). Probe-verified in round 1 on this code (outOfOrder=true on the unmodified PR; flipped by the guard below): cron/background-notification turns execute inside the CLI session and bypass bridge prompt admission — the only delete entry.turnError site (bridge.ts:6778) — while their session updates still flow through the bridge fan-in (bridgeClient.ts:908-927) into the journal with newer ids and into persisted records. This append then places the stale loop turn_error after page.events that already end with the newer automatic turn's content, so the refreshed transcript renders the loop error after content that actually happened later — and the stale entry.turnError keeps re-appending it on every refresh until the next interactive prompt admission. — Failure scenario: an interactive prompt fails with loop detection; before the user refreshes, a cron tick or background-notification turn runs on the same session → the refreshed Web Shell transcript shows the loop error after the automatic turn's output, reading as if the error belongs to the automatic turn. Suggested fix: only append when the found turn_error is still the last journaled event (or store the published event on the entry at broadcast time and append it only while it is newest):
| compactedReplay: turnError | |
| ? [...page.events, turnError] | |
| compactedReplay: | |
| turnError && | |
| [...(replay?.compactedTurns ?? []), ...(replay?.liveJournal ?? [])].at(-1)?.id === turnError.id | |
| ? [...page.events, turnError] | |
| : page.events, |
中文说明
[Suggestion] 已在第 1 轮用探针验证（未修改的 PR 代码上观察到 outOfOrder=true；加上下方守卫后翻转）：cron/后台通知轮次在 CLI 会话内部执行，绕过 bridge 的 prompt 准入——而那是唯一 delete entry.turnError 的位置（bridge.ts:6778）——但它们的 session update 仍会经由 bridge 扇入（bridgeClient.ts:908-927）以更小的新 id 进入日志并写入持久化记录。于是这里的追加会把过期的循环 turn_error 放在已经以更新的自动轮次内容结尾的 page.events 之后，刷新后的转录会把循环错误渲染在实际发生得更晚的内容之后——且过期的 entry.turnError 会在每次刷新时重复追加，直到下一个交互 prompt 准入。 — 失败场景：交互 prompt 因循环检测失败；用户刷新之前，同一会话上运行了一次 cron 或后台通知轮次 → 刷新后的 Web Shell 转录在自动轮次的输出之后显示循环错误，读起来像是该错误属于那个自动轮次。建议修复：仅当找到的 turn_error 仍是日志中最后一个事件时才追加（见上方 suggestion；或在广播时把发布的事件存到 entry 上，仅当它仍最新时追加）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| throw createLoopDetectedTurnError(toolLoopState); | ||
| } | ||
| if (toolRun.loopDetected) { |
There was a problem hiding this comment.
[Suggestion] R2-2: Loop-detected rejections route through the bridge's pre-existing "forward failed" compensation path (broadcastPromptCancelled[Once] in bridge.ts ~6864-6900), newly reachable because this PR is what makes loop stops reject. The compensation handler treats any non-deadline, non-abort rejection as a failed forward: it logs sendPrompt: forward failed for session … to daemon stderr and publishes prompt_cancelled{reason:'forward_failed'} before the formal turn_error terminal. Probe-verified on this commit: running this PR's own new bridge test prints sendPrompt: forward failed for session sess:/work/a: Loop protection stopped this turn. The SDK transcript reducer already suppresses the prompt.cancelled UI block for the forward_failed reason (sdk-typescript transcript.ts:372-382), so Web Shell renders nothing extra — the practical harm is a phantom forward-failure line on daemon stderr for every loop-detected turn (oncall noise) plus a misleading wire event for non-SDK consumers. — Failure scenario: oncall investigating a loop-stopped turn sees sendPrompt: forward failed in stderr and chases a forward/transport problem that does not exist; non-SDK event consumers receive prompt_cancelled{forward_failed} for a turn that was neither cancelled nor a forward failure. Suggested fix: in the compensation handler, skip the stderr line and the prompt_cancelled{forward_failed} broadcast for structured turn errors carrying data.errorKind (the formal turn_error terminal already covers them).
中文说明
[Suggestion] loop 检测导致的 prompt 拒绝会走进 bridge 既有的 “forward failed” 补偿路径（bridge.ts 约 6864-6900 的 broadcastPromptCancelled[Once]）——这条路径是因为本 PR 让循环停止变为拒绝才变得可达。补偿处理器把任何非超时、非取消的拒绝都当作转发失败：向日志 stderr 打印 sendPrompt: forward failed for session …，并在正式的 turn_error 终止事件之前发布 prompt_cancelled{reason:'forward_failed'}。已在本提交上用探针验证：运行本 PR 自己新增的 bridge 测试会打印 sendPrompt: forward failed for session sess:/work/a: Loop protection stopped this turn。SDK 的转录 reducer 已对 forward_failed 原因抑制 prompt.cancelled UI 块（sdk-typescript transcript.ts:372-382），因此 Web Shell 不会多渲染内容——实际危害是每次循环检测停止都会在 daemon stderr 留下一条虚假的转发失败日志（误导 oncall），以及非 SDK 事件消费方收到一条语义错误的 wire 事件。 — 失败场景：排查循环停止轮次的 oncall 在 stderr 看到 sendPrompt: forward failed，会去追查一个并不存在的转发/传输问题；非 SDK 事件消费方会收到 prompt_cancelled{forward_failed}，而该轮既不是用户取消也不是转发失败。建议修复：在补偿处理器中，对携带 data.errorKind 的结构化轮次错误跳过 stderr 打印与 prompt_cancelled{forward_failed} 广播（正式的 turn_error 终止事件已覆盖）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (block?.kind === 'error' && block.source === 'turn_error') { | ||
| retryableTurnErrorId = block.id; | ||
| if (block.errorKind !== 'loop_detected') { |
There was a problem hiding this comment.
[Suggestion] R2-5: Suppressing retryability for loop_detected also silences the public onSessionChange turn_complete error signal, because that signal is derived from the same retryableTurnErrorIdRef. The streaming→idle effect (App.tsx:6092-6104) builds the public dispatch as error: retryableTurnErrorIdRef.current != null ? new Error(...) : undefined — so a loop-stopped turn, which is now a genuine failure everywhere else (prompt rejected with -32603, turn_error terminal event, summary.turnError set, localized error block rendered), dispatches turn_complete with error: undefined. Probe-verified on this commit: a loop-detected turn dispatches turn_complete with error: undefined while a turn_error of any other kind carries the error; reverting this gate restores it. Not a regression (pre-diff loop turns also dispatched error: undefined), but a new undocumented asymmetry among error kinds introduced by reusing the retryability ref. — Failure scenario: an embedder of the web-shell package keying failure notifications/badges on turn_complete.error cannot distinguish a loop-detected failure from a successful turn. Suggested fix: track turn-error detection separately from retryability — keep a lastTurnErrorIdRef set for every turn_error block (including loop_detected) and use it for the turn_complete error flag, while retryableTurnErrorIdRef stays gated on errorKind !== 'loop_detected' for the retry hint.
中文说明
[Suggestion] 对 loop_detected 抑制重试的同时，也让公开的 onSessionChange turn_complete 错误信号失声了——因为该信号派生自同一个 retryableTurnErrorIdRef。streaming→idle 副作用（App.tsx:6092-6104）按 error: retryableTurnErrorIdRef.current != null ? new Error(...) : undefined 构造公开事件——于是被循环保护停止的轮次（在其他所有层面都是真正的失败：prompt 以 -32603 拒绝、turn_error 终止事件、summary.turnError 已设置、本地化错误块已渲染）却会以 error: undefined 派发 turn_complete。已在本提交上用探针验证：loop_detected 轮次派发的 turn_complete 的 error 为 undefined，而其他任何 kind 的 turn_error 都携带 error；还原这个门控后信号恢复。这不是回归（改动前循环停止轮次同样派发 error: undefined），而是复用重试引用带来的、未记录在案的新 kind 间不对称。 — 失败场景：基于 turn_complete.error 做失败通知/角标的 web-shell 嵌入方无法区分“循环检测停止”与“轮次成功”。建议修复：把轮次错误检测与可重试性分开追踪——为每个 turn_error 块（含 loop_detected）设置一个 lastTurnErrorIdRef 并用于 turn_complete 的 error 标志，retryableTurnErrorIdRef 继续按 errorKind !== 'loop_detected' 门控重试提示。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
|
@qwen-code /takeover |
|
🔄 Takeover re-armed: the round counter starts a fresh window (previous rounds no longer count toward the cap); management continues. 中文说明🔄 已重新武装：轮次计数开启新窗口（此前轮次不再计入上限），托管继续。 |
Stop-hook continuations are shared with cron and background-notification turns, which must keep their pre-loop-error graceful end-turn handling; only the foreground prompt chain now rejects a loop-detected stop. Also folds in review feedback: drop the unreachable repeated-failure branch from the continuation, extract the cancellation-precedence helper, defer and freshness-guard the bounded-refresh turn-error replay, skip the phantom forward-failed compensation for structured turn errors, key the structured code gate on structuredness, share the retryability predicate in Web Shell, and keep the turn_complete error signal alive for loop-detected turns.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — PR #8853 (round 3)All 11 unique findings (2 Critical rounds of R1-1 + 9 Suggestions) were verified against the reviewed commit and resolved in code in commit Findings and resolutions
Files changed
VerificationCommands actually run (this round, after the final state of the commit):
中文说明已处理的评审反馈 — PR #8853（第 3 轮）全部 11 个独立发现（R1-1 的两轮 Critical + 9 条 Suggestion）均已在被评审提交上逐条核实，并在提交 发现与处理
变更文件
验证本轮在提交最终状态上实际执行的命令：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 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 explored to full depth (tool budget reached): PR #8853 converts foreground tool-loop-protection stops i...: did not trace the out-of-repo channel gateway's handling of a rejected prompt RPC (blocks full confirmation of finding 1's end-to-end outcome).; PR #8853 converts foreground tool-loop-protection stops i...: did not exhaustively enumerate every future producer of data.errorKind prompt rejections beyond Session.ts/acpAgent.ts writer and trust paths (finding 2's lat…; PR #8853 converts foreground tool-loop-protection stops i...: did not verify whether webui/SDK-projection clients have an independent idle-time tool-spinner cleanup that would mask Finding 1's residual-spinner scenario (th…; PR #8853 converts foreground tool-loop-protection stops i...: none — all checks above were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget., and 1 more.
中文说明
未探索到全部深度（达到工具调用预算）：PR #8853 converts foreground tool-loop-protection stops i...：did not trace the out-of-repo channel gateway's handling of a rejected prompt RPC (blocks full confirmation of finding 1's end-to-end outcome).；PR #8853 converts foreground tool-loop-protection stops i...：did not exhaustively enumerate every future producer of data.errorKind prompt rejections beyond Session.ts/acpAgent.ts writer and trust paths (finding 2's lat…；PR #8853 converts foreground tool-loop-protection stops i...：did not verify whether webui/SDK-projection clients have an independent idle-time tool-spinner cleanup that would mask Finding 1's residual-spinner scenario (th…；PR #8853 converts foreground tool-loop-protection stops i...：none — all checks above were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.，另有 1 条。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (turnError && journal.at(-1)?.id === turnError.id) { | ||
| compactedReplay = [...page.events, turnError]; | ||
| } |
There was a problem hiding this comment.
[Critical] R3-1: The refresh-append guard only appends the in-memory turn_error while it is still the LAST journaled event of any type — but turn_error is a turn-boundary event (folded into compactedTurns, journal reset at publish), so any later non-transient event defeats the guard. Two concrete triggers: (a) pending_prompt_completed — published by result.finally for every prompt that was queued at admission — lands in liveJournal after the loop turn_error of a queued-then-promoted prompt; (b) model_switched / approval_mode_changed publishes. — Failure scenario: prompt B queues while A runs, B is promoted and trips loop protection → turn_error published → result.finally publishes pending_prompt_completed → page refresh: journal.at(-1) is pending_prompt_completed, the guard fails, the error is not appended → the reloaded transcript shows B's user message and failed/skipped tools with no terminal error — the exact "error vanishes on refresh" failure this PR exists to fix. Probe-verified on the unmodified PR: the refreshed replay contained zero turn_error events for the queued shape; a boundary-based guard restored it. Suggested fix: compare against the last turn-boundary event (turn_error/turn_complete) rather than the last event of any kind, or walk backwards skipping queue-bookkeeping events (pending_prompt_*) before comparing ids.
中文说明
[Critical] R3-1：刷新追加守卫仅在内存中的 turn_error 仍是日志中“任意类型的最后一个事件”时才追加——但 turn_error 是轮次边界事件（发布时即被折叠进 compactedTurns、日志被重置），因此其后出现的任何非瞬态事件都会使守卫失效。两个具体触发：(a) pending_prompt_completed——对每个入队时被标记为 queued 的 prompt，result.finally 都会发布它——它会落在“排队后晋升”的 prompt 的 loop turn_error 之后进入 liveJournal；(b) model_switched / approval_mode_changed 的发布同样如此。— 失败场景：prompt A 运行时 B 排队，B 晋升后触发循环保护 → 发布 turn_error → result.finally 发布 pending_prompt_completed → 刷新页面：journal.at(-1) 是 pending_prompt_completed，守卫失效，错误未被追加 → 重新加载的转录只显示 B 的用户消息与失败/跳过的工具，没有任何终止错误——这正是本 PR 要修复的“刷新后错误消失”问题。已在未修改的 PR 上用探针验证：排队场景下刷新后的 replay 中 turn_error 事件数为 0；改为基于轮次边界的守卫后恢复。建议修复：与最后一个轮次边界事件（turn_error/turn_complete）比较，而不是与任意类型的最后一个事件比较；或在比较 id 前从尾部向前跳过队列簿记事件（pending_prompt_*）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| channelDeliveryCapture, | ||
| true, // rejectOnLoopDetected |
There was a problem hiding this comment.
[Critical] R3-3: Channel turns that trip loop detection lose their channel delivery entirely. Channel prompts run through Session.prompt() — the foreground path that now throws LOOP_DETECTED (this call passes rejectOnLoopDetected: true) — but #scheduleChannelDelivery is gated on channelDelivery && result.stopReason === 'end_turn' (Session.ts:3077), which only runs when #executePrompt resolves; the new throw propagates through prompt()'s catch (only SessionWriterError is converted) and skips the gate. The design doc preserves "existing non-interactive handling" for cron/background turns only — channels are a class of non-interactive turn it does not mention. — Failure scenario: a webhook turn trips the per-turn tool-call cap → prompt rejects with LOOP_DETECTED → delivery is never scheduled → DaemonChannelBridge rejects on the turn_error terminal, runWebhookTask's catch emits a failed lifecycle event and never calls pushProactive → the remote channel user gets no terminal reply where pre-PR the collected text was delivered. Suggested fix: catch the loop RequestError for channel turns and schedule the delivery (with the error text) before rethrowing, or extend the rejectOnLoopDetected gating so channel turns keep the graceful end_turn.
中文说明
[Critical] R3-3：触发循环检测的频道轮次会完全丢失频道投递。频道 prompt 经由 Session.prompt() 运行——即现在会抛出 LOOP_DETECTED 的前台路径（此调用传入 rejectOnLoopDetected: true）——但 #scheduleChannelDelivery 被 channelDelivery && result.stopReason === 'end_turn'（Session.ts:3077）门控，仅在 #executePrompt 正常返回时执行；新的 throw 穿过 prompt() 的 catch（只转换 SessionWriterError）并跳过该门。设计文档只为 cron/后台通知轮次保留“既有非交互处理”——频道同样是一类非交互轮次，却未被提及。— 失败场景：webhook 轮次触发单轮工具调用上限 → prompt 以 LOOP_DETECTED 拒绝 → 投递永远不会被调度 → DaemonChannelBridge 在 turn_error 终止事件上拒绝，runWebhookTask 的 catch 发出 failed 生命周期事件且从不调用 pushProactive → 远端频道用户收不到任何终止回复，而 PR 前已收集的文本本会被投递。建议修复：为频道轮次捕获 loop RequestError，在重新抛出前调度投递（附带错误文本）；或扩展 rejectOnLoopDetected 门控，使频道轮次保持优雅的 end_turn。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| @@ -3921,8 +3944,9 @@ export class Session implements SessionContext { | |||
| nextMessage = nextAfterTools.message; | |||
| if (nextAfterTools.stoppedByRepeatedToolFailure) { | |||
There was a problem hiding this comment.
[Critical] R3-12: Loop-detected rejections strand queued cron/notification work. prompt() drains #drainCronQueue/#drainNotificationQueue only on the resolve path (Session.ts:3074-3075); the finally drain is gated on five TodoStopGuard flags that are all false in steady state (and the loop stop calls todoStopGuard.suspend() first, forcing blocksUnrelatedAutomaticTurns false); the mid-turn scheduler drain no-ops on if (this.pendingPrompt) return; BEFORE reaching #deferAutomaticQueueDrainUntilTurnsSettle, so nothing re-arms. Loop-detected turns previously always resolved end_turn and drained; this diff routes them into the rejection class, breaking the invariant documented at Session.ts:5923-5924. — Failure scenario: cron enabled, no active TodoStopGuard work chain; a cron job fires mid-foreground-turn and is queued; the turn trips loop detection and rejects → the cron queue is never drained; a one-shot/@wakeup job waits indefinitely for the next unrelated drain trigger — in an idle session the wakeup chain dies silently. Probe-verified: the reject arm left cronQueue=1 (stranded) while the resolve control arm drained to 0; reverting this diff hunk flips the result. Note: an unconditional finally-drain fix breaks the pinned test Session.test.ts:24292 — the fix must target the loop-detected rejection specifically (e.g. drain in a LOOP_DETECTED catch path).
中文说明
[Critical] R3-12：循环检测拒绝会使已排队的 cron/通知任务滞留。prompt() 仅在正常返回路径上排空 #drainCronQueue/#drainNotificationQueue（Session.ts:3074-3075）；finally 中的排空被五个 TodoStopGuard 标志门控，稳态下它们全为 false（且循环停止会先调用 todoStopGuard.suspend()，强制 blocksUnrelatedAutomaticTurns 为 false）；轮中的调度器排空在到达 #deferAutomaticQueueDrainUntilTurnsSettle 之前就因 if (this.pendingPrompt) return; 提前返回，因此没有任何重新武装。循环检测停止的轮次此前总是以 end_turn 正常返回并排空队列；本 diff 把它们路由进拒绝类，破坏了 Session.ts:5923-5924 处文档化的不变量。— 失败场景：启用 cron、无活动 TodoStopGuard 工作链；前台轮次运行中 cron 任务触发并入队；该轮触发循环检测并拒绝 → cron 队列永远不会被排空；一次性/@wakeup 任务无限期等待下一个无关的排空触发——空闲会话中唤醒链会静默死亡。探针验证：拒绝分支下 cronQueue=1（滞留），正常返回对照分支排空为 0；还原该 diff hunk 后结果翻转。注意：无条件的 finally 排空修复会破坏既有钉住的测试 Session.test.ts:24292——修复必须只针对循环检测拒绝（例如在 LOOP_DETECTED 的 catch 路径中排空）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const replay = entry.events.snapshotReplay(); | ||
| const journal = [ | ||
| ...(replay?.compactedTurns ?? []), | ||
| ...(replay?.liveJournal ?? []), | ||
| ]; |
There was a problem hiding this comment.
[Suggestion] R3-4: This rebuilds and scans the entire replay journal when the outcome depends only on the last event: snapshotReplay() flattens up to the full compacted replay window (default 4 MB, configurable to 256 MB) and maps the live journal (default 10,000 events), then the code copies it twice ([...spread], [...journal].reverse()) and .find()s it — while the trailing guard only ever uses journal.at(-1). Concrete cost: every loadSession attach/page-refresh of a live session while entry.turnError is pending (set for every running-prompt failure, cleared only at the next prompt dispatch) pays an O(replay-window) allocation + scan on the transcript-render latency path, growing with maxReplayBytes; and when an automatic turn ran after the error the whole computation is discarded. The backward-scan shape also misleads maintainers into thinking history is searched, inviting removal of the trailing-id guard that actually protects against stale re-append. Suggested fix: read the tail directly (liveJournal.at(-1) ?? compactedTurns.at(-1)), or store the published turn_error event (or its id) on entry.turnError at broadcast time and compare against the lastEventId getter with no snapshot at all.
中文说明
[Suggestion] R3-4：当结果只取决于最后一个事件时，这里却重建并扫描了整个回放日志：snapshotReplay() 会展平至多整个压缩回放窗口（默认 4 MB，可配置到 256 MB）并 map 整个 live journal（默认 10,000 个事件），随后代码又复制两次（[...spread]、[...journal].reverse()）并 .find()——而尾部守卫实际只使用 journal.at(-1)。具体代价：只要 entry.turnError 悬置（任何运行中 prompt 的失败都会设置，直到下一次 prompt 分发才清除），每次 live session 的 loadSession 附加/页面刷新都会在转录渲染的延迟路径上付出 O(回放窗口) 的分配 + 扫描，且随 maxReplayBytes 增长；若错误之后还运行过自动轮次，整个计算结果还会被丢弃。反向扫描的形态还会误导维护者以为在搜索历史，诱导其删除真正防止陈旧错误被重新追加的尾部 id 守卫。建议修复：直接读取尾部（liveJournal.at(-1) ?? compactedTurns.at(-1)）；或在广播时把已发布的 turn_error 事件（或其 id）存到 entry.turnError，与 lastEventId getter 比较，完全不做快照。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (turnError && journal.at(-1)?.id === turnError.id) { | ||
| compactedReplay = [...page.events, turnError]; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-6: The no-append (stale-error) branch of this guard has no test. The only refresh test ('keeps the current turn error when refreshing from persisted history') has the turn_error as the newest journaled event, so the reject path — a stale error after newer events, the exact scenario this comment describes — is never exercised. Mutation probe: replacing journal.at(-1)?.id === turnError.id with true (always append) leaves the full bridge suite green (524/524). A future regression of the staleness guard would ship uncaught. Suggested fix: add a bridge test — produce a loop turn_error, publish a subsequent turn's events without an interactive dispatch (simulate the automatic turn), refresh via loadSession, and assert compactedReplay does NOT contain the stale turn_error.
中文说明
[Suggestion] R3-6：该守卫的“不追加”（陈旧错误）分支没有任何测试。唯一的刷新测试（'keeps the current turn error when refreshing from persisted history'）让 turn_error 恰好是日志中最新的事件，因此拒绝路径——即更新事件出现后的陈旧错误，正是此注释描述的场景——从未被执行。变异探针：把 journal.at(-1)?.id === turnError.id 替换为 true（总是追加）后，完整 bridge 测试套件仍然全绿（524/524）。陈旧守卫未来的回归将不被发现地被合入。建议修复：新增 bridge 测试——制造一个 loop turn_error，在没有交互式分发的情况下发布后续轮次的事件（模拟自动轮次），通过 loadSession 刷新，并断言 compactedReplay 不包含陈旧的 turn_error。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (extractJsonRpcErrorField(err, 'errorKind')) { | ||
| // Structured turn error (e.g. loop_detected): the |
There was a problem hiding this comment.
[Suggestion] R3-7: The suppression this branch implements has no test asserting its effect. Both new bridge tests assert only the presence of turn_error, never the absence of prompt_cancelled. Mutation probe: deleting this entire branch (restoring the stderr line + prompt_cancelled{forward_failed} broadcast for structured errors) leaves the full bridge suite green (524/524) — under natural event ordering the extra event lands before turn_error, so even the journal-tail assertion still passes. The misreport this branch exists to prevent would silently return. Suggested fix: in 'preserves structured loop detection details on turn_error', collect all emitted events and assert none has type === 'prompt_cancelled' (mirroring the existing C3 forward-failure test's collection pattern).
中文说明
[Suggestion] R3-7：该分支实现的抑制行为没有任何测试断言其效果。两个新增 bridge 测试都只断言 turn_error 的存在，从不断言 prompt_cancelled 的缺席。变异探针：删除整个分支（为结构化错误恢复 stderr 行 + prompt_cancelled{forward_failed} 广播）后，完整 bridge 套件仍全绿（524/524）——在自然事件顺序下多余事件落在 turn_error 之前，连日志尾部断言也依然通过。该分支要防止的误报会悄无声息地回归。建议修复：在 'preserves structured loop detection details on turn_error' 中收集所有发出的事件，并断言没有 type === 'prompt_cancelled' 的事件（仿照既有 C3 forward-failure 测试的收集模式）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| stopReason: options.rejectOnLoopDetected | ||
| ? cancelledOrThrowLoopDetected(pendingSend.signal, toolLoopState) | ||
| : getAbortAwareEndTurnStopReason(pendingSend.signal), |
There was a problem hiding this comment.
[Suggestion] R3-8: The rejectOnLoopDetected threading is pinned by tests at only one of its three call sites (the cron graceful test). This foreground branch is never exercised as a rejection: no test makes a foreground Stop-continuation loop trip actually REJECT — the cancellation test reaches only the 'cancelled' side of cancelledOrThrowLoopDetected, and the cron test covers only the graceful false side. Mutation probe: flipping true → false at the foreground #handleStopHookLoop call site leaves Session.test.ts green (563/563). If that happens in a future edit, a foreground turn whose Stop-hook continuation batch trips loop protection resolves as a successful end_turn — precisely the misreport this PR exists to fix, confined to the stop-hook path. (The sibling gap at the notification call site is noted in the comment on the rejectOnLoopDetected = false default.) Suggested fix: add a Session test reusing the cron test's setup (explicit 1-call cap, Stop hook blocking, 2-call continuation batch) but without cancelling, asserting session.prompt(...) rejects with data.code === 'LOOP_DETECTED'.
中文说明
[Suggestion] R3-8：rejectOnLoopDetected 的传递只在三个调用点中的一个（cron 优雅测试）被测试钉住。这个前台分支从未以“拒绝”形态被执行：没有任何测试让前台 Stop 续跑的循环触发真正 REJECT——取消测试只到达 cancelledOrThrowLoopDetected 的 'cancelled' 侧，cron 测试只覆盖优雅的 false 侧。变异探针：把前台 #handleStopHookLoop 调用点的 true → false 后，Session.test.ts 仍全绿（563/563）。若未来编辑真的这样改，Stop-hook 续跑批次触发循环保护的前台轮次将以成功的 end_turn 返回——正是本 PR 要修复的误报，只是被限制在 stop-hook 路径。（通知调用点的姊妹缺口见 rejectOnLoopDetected = false 默认值处的评论。）建议修复：复用 cron 测试的装置（显式 1 次调用上限、Stop hook 阻塞、2 次调用的续跑批次）但不取消，断言 session.prompt(...) 以 data.code === 'LOOP_DETECTED' 拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| channelDeliveryCapture?: ChannelDeliveryCapture, | ||
| rejectOnLoopDetected = false, |
There was a problem hiding this comment.
[Suggestion] R3-8 (location 2 of 2): The background-notification graceful handling rests on this default at the notification call site (Session.ts:~7001 passes only 5 args), and no test pins it — the new graceful test covers only the cron call site. Mutation probe: passing true at the notification call site ships green (563/563), while the identical mutation at the pinned cron site fails the suite — the comparator is alive, the notification half is genuinely unpinned. A future true there makes a notification turn that trips loop protection throw past #emitBackgroundNotificationEndTurn into the notification catch, emitting [notification error] … and calling pauseForTrustedRetry() — misreporting a designed-graceful automatic turn as an error, silently violating the design doc's "cron and background-notification work keep their existing non-interactive handling" for half of that pair. Suggested fix: add a sibling of the cron graceful test for a background-notification turn whose Stop continuation trips the explicit cap, asserting no [notification error] chunk and end_turn telemetry.
中文说明
[Suggestion] R3-8（第 2 处，共 2 处）：后台通知的优雅处理依赖通知调用点（Session.ts:~7001 只传 5 个参数）处的这个默认值，且没有测试钉住它——新增的优雅测试只覆盖 cron 调用点。变异探针：在通知调用点传入 true 后套件全绿（563/563），而在被钉住的 cron 调用点做同样变异会使测试失败——比较器有效，通知这一半确实未被钉住。未来若在此传入 true，触发循环保护的通知轮次会抛过 #emitBackgroundNotificationEndTurn 进入通知 catch，输出 [notification error] … 并调用 pauseForTrustedRetry()——把设计上应优雅处理的自动轮次误报为错误，静默违背设计文档“cron 与后台通知保持既有非交互处理”承诺的一半。建议修复：为后台通知轮次补一个 cron 优雅测试的姊妹测试（其 Stop 续跑触发显式上限），断言没有 [notification error] 块且遥测为 end_turn。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| function createLoopDetectedTurnError( | ||
| loopState: DaemonToolLoopState, | ||
| ): RequestError { | ||
| return new RequestError(-32603, LOOP_DETECTED_TURN_ERROR_MESSAGE, { |
There was a problem hiding this comment.
[Suggestion] R3-9: LOOP_DETECTED_TURN_ERROR_MESSAGE is pinned by no test. The flipped Session tests match only the data envelope (code/errorKind/loopType), and the bridge tests inject their own synthetic message text that differs from the real constant. The design doc's compatibility section makes this message the surface older clients and the session summary show verbatim ("older clients show the daemon message"). Failure scenario: mutating or emptying the constant leaves every test green while older clients and the session summary silently display a wrong or empty explanation. Suggested fix: extend one flipped assertion with message: LOOP_DETECTED_TURN_ERROR_MESSAGE (import the constant), e.g.
await expect(session.prompt(...)).rejects.toMatchObject({
message: LOOP_DETECTED_TURN_ERROR_MESSAGE,
data: expect.objectContaining({ code: 'LOOP_DETECTED' }),
});中文说明
[Suggestion] R3-9：LOOP_DETECTED_TURN_ERROR_MESSAGE 没有任何测试钉住。翻转后的 Session 测试只匹配 data 信封（code/errorKind/loopType），bridge 测试注入的是与真实常量文本不同的合成消息。设计文档的兼容性章节使该消息成为旧客户端与会话摘要逐字展示的表面（“旧客户端展示 daemon 消息”）。失败场景：修改或清空该常量后所有测试仍为绿色，而旧客户端与会话摘要会静默展示错误甚至为空的说明。建议修复：在一个翻转断言中补上 message: LOOP_DETECTED_TURN_ERROR_MESSAGE（导入该常量），例如上文代码块所示。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Address round-3 review findings: - The bounded-refresh append guard no longer compares the in-memory turn_error against the last journaled event of any kind. The published event is stored on the entry at broadcast time; any newer turn terminal clears it, and only turn-content events journaled after it block the append (queue/config bookkeeping no longer hides the error on refresh). The staleness check reads the in-flight journal via a new liveJournalSnapshot() accessor instead of flattening the replay window. - Channel turns keep the graceful end-turn handling like cron and background-notification turns, so their collected text is still delivered when loop protection stops them. - Loop-detected rejections drain the cron/notification queues in prompt()'s finally, preserving the pre-rejection drain invariant for queued automatic work. - Tests pin each behavior: queued-bookkeeping refresh append, stale-error no-append, prompt_cancelled absence, foreground stop-hook rejection, background-notification graceful default, and the user-facing message constant.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review round 3 — PR #8853 address summaryAll 9 findings from the automated reviewer (3 Critical, 6 Suggestion) were verified against the code and addressed in one batch. No finding was declined. Critical findingsR3-1 — refresh-append guard defeated by queue bookkeeping (bridge.ts)Fixed. The guard compared the in-memory The new guard stores the published
A regression test reproduces the queued shape (prompt B queued behind A, promoted, trips loop protection, R3-3 — channel turns lose delivery entirely (Session.ts)Fixed. Channel turns now keep the graceful non-interactive handling: R3-12 — loop rejections strand queued cron/notification work (Session.ts)Fixed. Suggestions
Conflict notes
VerificationCommands actually run (all from the repository root unless noted), with results:
中文说明评审第 3 轮 — PR #8853 处理总结自动评审者的全部 9 条发现（3 条 Critical、6 条 Suggestion）均已对照代码核实，并在同一批提交中处理完毕。没有任何发现被拒绝。 Critical 发现R3-1 — 刷新追加守卫被队列簿记事件击穿（bridge.ts）已修复。 原守卫把内存中的 新守卫在广播时把已发布的
新增回归测试复现了排队场景（prompt B 排在 A 之后入队、晋升、触发循环保护， R3-3 — 频道轮次完全丢失投递（Session.ts）已修复。 频道轮次现在保持优雅的非交互处理： R3-12 — 循环拒绝使已排队的 cron/通知任务滞留（Session.ts）已修复。 Suggestion 发现
冲突说明
验证实际执行的命令（如无特别说明均在仓库根目录执行）及结果：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
…tection-error # Conflicts: # packages/acp-bridge/src/bridge.test.ts
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix address-review summary — PR #8853 (round with base-conflict resolution)Feedback pointsThe prepared feedback contained no review findings, no inline comments, no Changes this roundThe only work was the flagged base conflict ( Conflict notesExactly one content conflict:
The two additions are orthogonal. Resolution kept both accessor groups on All other overlaps ( Verification
中文说明Autofix address-review 总结 — PR #8853（含基分支冲突解决的一轮）反馈点准备好的反馈中没有任何新的评审发现、行内评论、issue 级评论、失败检查项，也没有上次评估（2026-08-13T08:32:34Z）之后仍然标红的检查项。本轮没有需要分诊处理的反馈。 本轮改动唯一的工作是处理被标记的基分支冲突（ 冲突说明仅有一处内容冲突：
两处新增互不相关。解决方式是在 其余所有重叠文件（ 验证
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 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.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": did not trace daemon-side sessionTranscript persistence semantics (whether a persisted page can itself already contain a turn-error record, which would make t…; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": could not inspect @qwen-code/webui 's useStreamingState (external package not in this worktree's node_modules), so "streaming returns to idle with zero block…, and 12 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：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 agent…"：did not trace daemon-side sessionTranscript persistence semantics (whether a persisted page can itself already contain a turn-error record, which would make t…；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：could not inspect @qwen-code/webui 's useStreamingState (external package not in this worktree's node_modules), so "streaming returns to idle with zero block…，另有 12 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // turn — that opts the turn out of loop-detected rejection and the | ||
| // repeated-failure guard. |
There was a problem hiding this comment.
[Critical] R9-1: The new trusted-parent gating covers only qwen.channel.prompt; the second input to the same channel classification, qwen.daemon.channelDelivery, still passes the ACP boundary unstripped from untrusted callers, so it can mark a turn as a channel turn — exactly what the comment above says an untrusted caller "must not be able to". — Failure scenario: a standalone qwen --acp agent initializes untrusted and its sanitize block deletes only the five reserved keys, letting unknown _meta keys through (probe-verified on this commit: a forged delivery payload reaches session.prompt). Session.prompt computes channelTurn = channelDelivery !== undefined || channelPromptTurn and uses it to set rejectOnLoopDetected = false and force the repeated-failure guard 'off' → the turn never rejects with the structured LOOP_DETECTED error no matter how badly it loops, the guard stays off even under enforce, and a bogus channel delivery is scheduled at end_turn — reintroducing through a sibling key the exact ungated opt-out this PR gates qwen.channel.prompt to close. (The daemon route is NOT exposed for this key — bridge.ts strips it and re-injects only the internally-set trusted context.channelDelivery.)
Suggested fix — mirror the channel-prompt pattern for the delivery key:
const suppliedChannelDelivery = meta[DAEMON_CHANNEL_DELIVERY_META_KEY];
delete meta[DAEMON_CHANNEL_DELIVERY_META_KEY];
// ...after the existing trusted re-injections:
if (this.privateParentState === 'trusted' && suppliedChannelDelivery !== undefined) {
meta[DAEMON_CHANNEL_DELIVERY_META_KEY] = suppliedChannelDelivery;
}and extend the new untrusted-caller test to assert a forged qwen.daemon.channelDelivery payload is also stripped.
中文说明
[Critical] R9-1：新的可信父级门控仅覆盖了 qwen.channel.prompt；同一 channel 分类的第二个输入 qwen.daemon.channelDelivery 仍会原样穿过不可信调用方的 ACP 边界，从而把轮次标记为 channel turn——正是上方注释声明不可信调用方"不得做到"的事。— 失败场景：独立 qwen --acp agent 以 untrusted 初始化，其 sanitize 块只删除五个保留键，未知 _meta 键会被放行（已在当前提交上探针验证：伪造的 delivery 载荷到达了 session.prompt）。Session.prompt 计算 channelTurn = channelDelivery !== undefined || channelPromptTurn，并据此把 rejectOnLoopDetected 置为 false、把重复失败守卫强制为 'off' → 轮次无论怎样循环都不会以结构化 LOOP_DETECTED 错误拒绝，enforce 配置下守卫也保持关闭，且 end_turn 时会调度一次伪造的频道投递——经由姊妹键重新引入了本 PR 正要堵住的无门控豁免。（daemon 路由不受此键影响——bridge.ts 会剥离该键，只重新注入内部设置的可信 context.channelDelivery。）建议按 channel-prompt 的模式处理 delivery 键（捕获→删除→仅可信时重注入），并扩展新的 untrusted 测试，断言伪造的 qwen.daemon.channelDelivery 同样被剥离。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if ( | ||
| this.privateParentState === 'trusted' && | ||
| suppliedChannelPrompt === true | ||
| ) { | ||
| meta[CHANNEL_PROMPT_META_KEY] = true; | ||
| } |
There was a problem hiding this comment.
[Critical] R9-2: The new trust gate is bypassed on the daemon-hosted path: no daemon-side layer strips qwen.channel.prompt from client-supplied _meta, and the daemon is a trusted parent — so a forged key survives end to end and is honored by this re-injection. — Failure scenario: probe-verified through the real bridge hop on this commit: any SDK/HTTP client that can prompt a daemon session sends _meta: {'qwen.channel.prompt': true}; the serve route copies _meta into the forwarded body deleting only two keys, and the bridge admission strip deletes seven reserved keys but not this one; the daemon initializes the child with the private-parent capability, so privateParentState === 'trusted' and this gate re-injects the forged key. Session.prompt then computes channelTurn = true: a loop-stopped turn resolves end_turn (Web Shell never gets the loop_detected turn error this PR ships) and the repeated-failure guard is forced 'off' even under enforce — exactly what the gate's comment says "must not" happen. The new test only proves the direct qwen --acp child path; DaemonChannelBridge legitimately sends the key over this same public route, so the legitimate sender is indistinguishable from a forger here. This diff doubles the key's privileges and asserts the boundary in new code/comments (Session.ts: "the ACP boundary strips the channel-prompt key from untrusted callers"), leaving the assertion false on the primary path.
Suggested fix: strip CHANNEL_PROMPT_META_KEY in the bridge admission strip list and the serve route's forwarded-meta cleanup, and re-inject it only for prompts dispatched through a trusted channel context (the way context.channelDelivery / display text are re-injected, bridge.ts:7669-7683) — or require the existing channel-worker authorization for that key. Add a daemon-path regression test: a forged qwen.channel.prompt must still reject on loop detection and keep the configured guard mode.
中文说明
[Critical] R9-2：新的信任门控在 daemon 托管路径上被绕过：daemon 侧没有任何一层剥离客户端 _meta 中的 qwen.channel.prompt，而 daemon 是可信父级——伪造的键全程存活，并被此处的重新注入逻辑承认。— 失败场景：已在当前提交上通过真实 bridge 链路探针验证：任何能对 daemon 会话发起 prompt 的 SDK/HTTP 客户端发送 _meta: {'qwen.channel.prompt': true}；serve 路由转发 _meta 时只删除两个键，bridge 准入剥离删除七个保留键但不包含此键；daemon 用 private-parent capability 初始化子进程，privateParentState === 'trusted'，此门控把伪造的键重新注入。Session.prompt 随即计算 channelTurn = true：循环停止的轮次以 end_turn 结束（Web Shell 永远收不到本 PR 提供的 loop_detected 轮次错误），重复失败守卫在 enforce 下也被强制关闭——正是门控注释声明"不得发生"的情形。新增测试只证明了直连 qwen --acp 子进程路径；DaemonChannelBridge 经由同一公共路由合法发送该键，合法发送者与伪造者在该路径上不可区分。本 diff 使该键的特权翻倍，并在新代码/注释中断言该边界（Session.ts："the ACP boundary strips the channel-prompt key from untrusted callers"），而该断言在主路径上不成立。建议修复：在 bridge 准入剥离列表与 serve 路由的转发元数据清理中剥离 CHANNEL_PROMPT_META_KEY，仅对经受信 channel 上下文分发的 prompt 重新注入（如同 context.channelDelivery / display text 的处理方式），或对该键要求现有的 channel-worker 授权；并补充 daemon 路径回归测试（伪造的 qwen.channel.prompt 必须仍在循环检测时拒绝、并保持配置的守卫模式）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // journal holds exactly the events published since the last | ||
| // turn boundary (the terminal itself folds into the replay | ||
| // window), so no history scan is needed. | ||
| const journal = entry.events.liveJournalSnapshot() ?? []; |
There was a problem hiding this comment.
[Critical] R9-3: The refresh-append guard treats the synthetic history_truncated live-journal marker as newer turn content — journal truncation silently drops the pending loop terminal from the refreshed transcript. — Failure scenario: probe-verified on the unmodified PR (small injected maxJournalBytes, idle shell output after a loop reject): the loop turn_error boundary resets the journal and journalTruncatedEvents; verbose idle user-shell output (each output chunk a separate journal entry — the very traffic isUserShellSessionUpdate exists to exempt) exceeds the caps; eviction sets journalTruncatedEvents > 0; on refresh liveJournalSnapshot() (this line) unshifts a synthetic history_truncated event that was never ingested as content; the predicate sees a type outside REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES that is not a user-shell update → hasNewerTurnContent = true → the append is skipped → the loop terminal disappears from the refreshed transcript while the summary still advertises it — the exact regression this PR fixes, smuggled back in by the truncation marker of the very shell traffic the exemption protects. Both journal caps are documented bridge options accepting any integer ≥ 1, and the growth pool is skipped precisely when the operator pins them, so small pinned caps hit this with a handful of events; none of the nine new tests induces truncation.
// exempt the size-accounting marker from the predicate, e.g.:
const hasNewerTurnContent = journal.some(
(event) =>
event.type !== 'history_truncated' &&
!REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES.has(event.type) &&
!isUserShellSessionUpdate(event),
);plus a regression test that pushes the post-terminal journal past a small injected cap before refreshing.
中文说明
[Critical] R9-3：刷新追加守卫把合成的 history_truncated 实时日志标记判为"更新的轮次内容"——日志截断会静默丢掉刷新转录中本应保留的循环终止事件。— 失败场景：已在未修改的 PR 上探针验证（注入较小的 maxJournalBytes，循环拒绝后运行空闲 shell）：loop turn_error 边界重置日志与 journalTruncatedEvents；冗长的空闲 user-shell 输出（每个输出块都是独立的日志条目——正是 isUserShellSessionUpdate 要豁免的流量）超出上限；逐出逻辑把 journalTruncatedEvents 置为非零；刷新时 liveJournalSnapshot()（本行）在头部插入一个从未被写入日志的合成 history_truncated 事件；守卫谓词看到一个既不在 REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES 中、又不是 user-shell 更新的类型 → hasNewerTurnContent = true → 跳过追加 → 刷新后的转录丢失循环终止事件，而摘要仍在宣告它——正是本 PR 要修复的回归，却由豁免机制所保护的 shell 流量自身的截断标记重新引入。两个日志上限都是有文档的 bridge 选项、接受任意 ≥ 1 的整数，且操作者固定上限时增长池恰好被跳过，因此较小的固定上限只需少量事件即可触发；九个新测试均未构造截断。建议在谓词中豁免该标记（如上方代码），并补充一个在刷新前把终止后日志推过较小注入上限的回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ) { | ||
| retriedTurnErrorIdRef.current = retryableTurnError.id; | ||
| } | ||
| lastTurnErrorIdRef.current = turnErrorId; |
There was a problem hiding this comment.
[Critical] R9-4: The new backward turnErrorId scan breaks on ANY user block, while getRetryableTurnError(blocks) — computed in the same effect for the retry decision — deliberately skips user blocks with meta.source === 'background_notification'; the two walks disagree about whether the current turn has a turn error. — Failure scenario: probe-verified on this commit with the transcript shape [turn_error, background_notification user block] that the existing test 'keeps a turn-error retry visible through background notifications' pins: the inline scan stops at the notification user block → lastTurnErrorIdRef.current = null → turn_complete fires with error: undefined while the retry path still sees the error → the host embedding Web Shell is told the turn succeeded while the UI shows the error and offers retry. Pre-PR this interleaving DID report the error (turn_complete read retryableTurnErrorIdRef derived from getRetryableTurnError semantics), and the PR's stated guarantee "Loop-detected turn errors still surface through turn_complete" is defeated in it. The new App.test.tsx case covers only the bare shape with no trailing notification.
| lastTurnErrorIdRef.current = turnErrorId; | |
| lastTurnErrorIdRef.current = getRetryableTurnError(blocks)?.id ?? null; |
(getRetryableTurnError does not filter by kind, so loop-detected errors still surface through turn_complete as intended — the duplicated inline scan above then has no remaining reader and can be removed.)
中文说明
[Critical] R9-4：新的从后向前 turnErrorId 扫描在遇到任何 user 块时终止，而同一 effect 中用于重试判定的 getRetryableTurnError(blocks) 会刻意跳过 meta.source === 'background_notification' 的 user 块——两次遍历对"当前轮次是否存在 turn error"给出不同答案。— 失败场景：已在当前提交上探针验证，使用既有测试 'keeps a turn-error retry visible through background notifications' 所钉住的转录形态 [turn_error, background_notification user 块]：内联扫描在通知 user 块处停止 → lastTurnErrorIdRef.current = null → turn_complete 以 error: undefined 触发，而重试路径仍能看到错误 → 嵌入 Web Shell 的宿主被告知轮次成功，UI 却同时显示错误并提供重试。PR 前该交错形态确实会上报错误（turn_complete 读取由 getRetryableTurnError 语义派生的 retryableTurnErrorIdRef），本 PR 声明的"Loop-detected turn errors still surface through turn_complete"保证在该交错下失效。新增的 App.test.tsx 用例只覆盖了没有尾随通知块的裸形态。建议改用 getRetryableTurnError(blocks)?.id ?? null（该函数不按 kind 过滤，loop-detected 错误仍会如预期经由 turn_complete 上报），上方的内联扫描随之失去读者、可一并删除。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| function isUserShellSessionUpdate(event: BridgeEvent): boolean { | ||
| if (event.type !== 'session_update') return false; |
There was a problem hiding this comment.
[Critical] R9-5: The refresh-append guard exempts only user-shell session_updates, but other idle-reachable bookkeeping session_update subtypes — concretely available_commands_update and the legacy dual-emit current_mode_update frame — are journaled while idle and defeat the append, so the loop terminal disappears after a page refresh. — Failure scenario: probe-verified on this commit: after a loop reject, a workspace skills/settings refresh fans refreshSkillsFromSettings over all active sessions and each session emits session_update{available_commands_update} (Session.ts sendAvailableCommandsUpdateOrThrow); BridgeClient.sessionUpdate forwards every agent session_update onto the session bus with no idle gating and ingest() journals it; on refresh hasNewerTurnContent sees the event — it is not in REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES, and isUserShellSessionUpdate returns false (its _meta carries availableSkills, not source: 'user-shell') → the append is skipped → the refreshed transcript loses the turn_error{loop_detected} terminal while the summary still advertises it. The set's own doc comment mandates auditing "any other idle-reachable session-bus publish", and the PR explicitly put sibling idle events (settings_changed, extensions_changed) in the set and tested them — available_commands_update is an unargued sibling state of the same shared gate.
Suggested fix: extend the skip to idle bookkeeping subtypes — e.g. treat the LATEST_WINS_UPDATES subtypes (available_commands_update, current_mode_update) the same as user-shell output in this predicate (renaming it to isIdleBookkeepingSessionUpdate), and add a matrix case publishing such a session_update after the terminal.
中文说明
[Critical] R9-5：刷新追加守卫只豁免 user-shell 的 session_update，但其他空闲可达的簿记类 session_update 子类型——具体是 available_commands_update 与遗留双发的 current_mode_update 帧——会在空闲期被写入日志并击穿追加，导致循环终止事件在刷新后消失。— 失败场景：已在当前提交上探针验证：循环拒绝之后，一次工作区技能/设置刷新会对所有活动会话执行 refreshSkillsFromSettings，每个会话发出 session_update{available_commands_update}（Session.ts sendAvailableCommandsUpdateOrThrow）；BridgeClient.sessionUpdate 把每个 agent session_update 无空闲门控地转发到会话总线，ingest() 将其写入日志；刷新时 hasNewerTurnContent 看到该事件——它不在 REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES 中，isUserShellSessionUpdate 也返回 false（其 _meta 携带 availableSkills 而非 source: 'user-shell'）→ 跳过追加 → 刷新后的转录丢失 turn_error{loop_detected} 终止事件，而摘要仍在宣告它。集合自身的注释要求审计"任何其他空闲可达的会话总线发布"，且本 PR 已把同族空闲事件（settings_changed、extensions_changed）加入集合并配了测试——available_commands_update 是同一共享门控下未经论证的兄弟状态。建议把空闲簿记子类型纳入豁免——例如在本谓词中把 LATEST_WINS_UPDATES 子类型（available_commands_update、current_mode_update）与 user-shell 输出同等对待（可更名为 isIdleBookkeepingSessionUpdate），并在矩阵测试中新增一个在终止事件之后发布此类 session_update 的用例。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| it('keeps the turn error on refresh when a queued deadline terminal lands after it', async () => { | ||
| // A queued prompt's terminal publishes the event alone without mutating | ||
| // turn state; it must not erase the refresh-replay record of the active |
There was a problem hiding this comment.
[Critical] R9-6: A queued prompt's terminal (deadline expiry / queued removal) is a turn_error/turn_complete boundary that resets the compaction journal while leaving entry.turnErrorEvent intact — newer automatic-turn content journaled before it is wiped from the guard's only evidence source, and the refresh-append then re-appends the stale loop error AFTER newer turn content: the exact misplacement the guard exists to prevent. — Failure scenario: probe-verified on this commit in both variants (held-cancel-ack deadline expiry, and removePendingPrompt while a cancel-forward drain is pending): loop reject sets turnErrorEvent; an automatic turn (e.g. a cron turn drained by the rejection itself) journals session_update content — correctly blocking the append, as the sibling drop test pins; the queued terminal then folds+resets the journal again without clearing turnErrorEvent; on refresh liveJournalSnapshot() is empty → hasNewerTurnContent = false → replay = persisted page + automatic-turn content + the stale loop turn_error. The refreshed transcript shows the newer automatic turn's content followed by the previous turn's stale loop_detected terminal, contradicting the guard's own comment ("re-appending the stale error after their newer content would misplace it") and the field doc ("any newer turn terminal clears it so a stale error is never re-appended"). No test covers content-then-queued-terminal; the two new queued-terminal tests pin only the empty-journal case.
Suggested fix: track the content evidence outside the resettable journal — e.g. set a flag on the entry when a non-bookkeeping, non-user-shell event is ingested while turnErrorEvent is set, cleared only on admission or a mutating terminal; have the guard require both an empty-of-content journal AND that flag. Add a regression test: loop terminal → inject an automatic-turn session_update → queued deadline/removal terminal → loadSession → assert the stale error is NOT appended.
中文说明
[Critical] R9-6：排队 prompt 的终止事件（截止期到期 / 排队中被移除）是 turn_error/turn_complete 边界，会重置压缩日志，却不触动 entry.turnErrorEvent——先于它写入日志的更新轮次内容被从守卫唯一的证据来源中抹掉，刷新追加于是把过期的循环错误追加到更新轮次内容之后：正是守卫要防止的错位。— 失败场景：已在当前提交上探针验证两种变体（挂起 cancel ack 时的截止期到期；cancel 转发 drain 挂起时 removePendingPrompt）：循环拒绝置位 turnErrorEvent；一个自动轮次（例如被该拒绝自身排空的 cron 轮次）把 session_update 内容写入日志——正确阻断追加（兄弟 drop 测试已钉住此行为）；随后排队终止事件再次折叠+重置日志，却不清除 turnErrorEvent；刷新时 liveJournalSnapshot() 为空 → hasNewerTurnContent = false → replay = 持久化页内容 + 自动轮次内容 + 过期的 loop turn_error。刷新后的转录显示较新自动轮次的内容之后跟着上一轮的过期 loop_detected 终止事件，与守卫自身注释（"把过期错误重新追加到更新内容之后会造成错位"）及字段文档（"任何更新的轮次终止都会清除它，过期错误永不被重新追加"）矛盾。没有测试覆盖"先有内容、后有排队终止"的序列；两个新的排队终止测试只钉住了日志为空的情形。建议修复：把内容证据记录在可重置的日志之外——例如当 turnErrorEvent 置位期间有非簿记、非 user-shell 事件写入时，在 entry 上置一个标志，仅在准入或变更性终止时清除；守卫要求"日志无内容"与该标志同时成立。并补充回归测试：循环终止 → 注入自动轮次 session_update → 排队截止/移除终止 → loadSession → 断言过期错误未被追加。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const compactedReplay = refreshed.compactedReplay ?? []; | ||
| expect(compactedReplay).toHaveLength(2); | ||
| expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ |
There was a problem hiding this comment.
[Suggestion] R9-7: This test (and the queued-deadline/queued-removed tests at ~5085/~5205) assert presence of the appended turn_error without anchoring compactedReplay[0] to the persisted page — they pass unchanged if the bounded-append branch regresses into the in-memory fallback: the exact regression shape the sibling tests' anchors were written to catch. — Concrete cost: probe-verified on this commit: running the identical shell scenario with the persisted-transcript fetch throwing (forcing the bounded path's catch → replayFieldsFor in-memory fallback) passes this test's exact assertions — the fallback's compactedReplay has the same length-2/last-is-terminal shape — so such a regression ships green through all three tests; only the sibling tests anchoring compactedReplay[0] to 'persisted turn content' would fire.
// add the sibling anchor:
expect(compactedReplay[0]).toMatchObject({
type: 'session_update',
data: expect.objectContaining({
content: { type: 'text', text: 'persisted turn content' },
}),
});中文说明
[Suggestion] R9-7：该测试（以及 ~5085/~5205 处的排队截止/排队移除测试）只断言追加的 turn_error 存在，未把 compactedReplay[0] 锚定到持久化页——若有界追加分支回归为内存回退，这些测试会原样通过：这正是兄弟测试的锚点专门要捕获的回归形态。— 具体代价：已在当前提交上探针验证：让持久化转录拉取抛错（强制有界路径走 catch → replayFieldsFor 内存回退）运行相同的 shell 场景，该测试的全部断言依然通过——回退的 compactedReplay 具有相同的"长度为 2、末位为终止事件"形态——因此这类回归会在三个测试全绿的情况下溜走；只有把 compactedReplay[0] 锚定到 'persisted turn content' 的兄弟测试才会变红。建议按上方代码补上同款锚点。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| it('keeps a Goal turn graceful when loop protection stops it', async () => { | ||
| // Goal continuations are non-interactive and bypass the bridge: a |
There was a problem hiding this comment.
[Suggestion] R9-8: The graceful repeated-failure-guard stop branch — the only stop path this PR newly conditioned on rejectOnLoopDetected — has zero test coverage: all three new graceful-stop tests trip TURN_TOOL_CALL_CAP, and goal turns are the one non-interactive path that keeps the configured guard mode (Session.ts: channelTurn ? 'off' : mode) while getting rejectOnLoopDetected = false. — Concrete cost: mutation-verified on this commit — deleting the gated emit block, or flipping the graceful stoppedByRepeatedToolFailure resolution to an unconditional rejection, leaves the full Session.test.ts suite green (606/606 under both mutants); the old positive emission assertion was inverted in this very diff. A future regression here would reject goal turns, settling them failed and pausing the goal with no turn_error published, and the only user-visible explanation of a silently-stopped autonomous goal turn disappears with no red signal.
Suggested fix: add a sibling to the goal test below driving the repeated-failure guard — recreate the session with guard mode enforce, run a goal turn through a failing-batch streak, and assert it resolves gracefully with logLoopDetectedSpy called for REPEATED_TOOL_EXECUTION_FAILURE and the stop message emitted to the client.
中文说明
[Suggestion] R9-8：优雅的重复失败守卫停止分支——本 PR 唯一新近以 rejectOnLoopDetected 为条件的停止路径——完全没有测试覆盖：三个新的优雅停止测试全部触发 TURN_TOOL_CALL_CAP，而 goal turn 是唯一既保留配置守卫模式（Session.ts：channelTurn ? 'off' : mode）又得到 rejectOnLoopDetected = false 的非交互路径。— 具体代价：已在当前提交上变异验证——删除受门控的发送块，或把优雅的 stoppedByRepeatedToolFailure 返回翻转为无条件拒绝，Session.test.ts 全套 606 个测试在两种变异下均保持全绿；旧的正向发送断言恰好在本 diff 中被反转。未来此处的回归会拒绝 goal turn、将其按失败结算并暂停 goal，且不发布任何 turn_error——被静默停止的自主 goal 轮次唯一的用户可见解释就此消失，且没有任何红色信号。建议在该 goal 测试旁补一个驱动重复失败守卫的兄弟测试：以 enforce 模式重建会话，让 goal 轮次经历失败批次连击，断言其优雅结束、logLoopDetectedSpy 以 REPEATED_TOOL_EXECUTION_FAILURE 被调用、且停止消息已发送给客户端。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
|
||
| The session remains alive and the per-turn loop state is recreated for the next prompt. Cron, background-notification, channel-delivery, and goal turns keep their existing non-interactive handling: only interactive foreground prompts reject. Goal turns bypass the bridge entirely, so rejecting one would settle it as failed and pause the goal without publishing any `turn_error`; they resolve `end_turn` like the other automatic turn types. A loop-detected rejection still drains the cron/notification queues, preserving the invariant that a loop-stopped turn never strands queued automatic work. | ||
|
|
||
| When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh without changing historical persistence. |
There was a problem hiding this comment.
[Suggestion] R9-9: This sentence claims unconditional refresh-survival, but the doc's own queue-drain invariant (previous paragraph) creates turns whose journaled content permanently disables the refresh-append. — Concrete cost: a cron job queued while a foreground turn loops is drained by prompt()'s finally on rejection (rejectedByLoopProtection); the drained in-daemon cron turn publishes ordinary session_updates that are not in the bookkeeping set, so hasNewerTurnContent is true and the append is skipped until the next interactive terminal (which clears turnErrorEvent anyway). The code behavior is deliberate — the PR's own stale-drop test pins it — but this sentence reads as if refresh-survival were unconditional, and the doc's own drain invariant makes the defeating condition likely immediately after a loop stop whenever work was queued.
| When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh without changing historical persistence. | |
| When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh while the session remains idle; newer turn content — including automatic turns the rejection itself drains — supersedes it by design. |
中文说明
[Suggestion] R9-9：该句声称刷新后无条件保留，但文档上一段自己的队列排空不变量会制造出"日志内容永久禁用刷新追加"的轮次。— 具体代价：前台轮次循环期间排队的 cron 任务会在拒绝时被 prompt() 的 finally（rejectedByLoopProtection）排空；被排空的 daemon 内 cron 轮次发布的是不在簿记集合中的普通 session_update，于是 hasNewerTurnContent 为真，追加被跳过，直到下一个交互轮次终止（而那时 turnErrorEvent 也已被清除）。代码行为是刻意的——本 PR 的 stale-drop 测试已钉住——但该句读起来像刷新保留是无条件的，且文档自身的排空不变量使得"只要有排队工作，循环停止后立刻触发击败条件"成为大概率事件。建议按上方文本限定该句。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| undefined, | ||
| { promptId: 'prompt-queued', deadlineMs: 120 }, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R9-10: This test's stated premise — the deadline expires while the prompt is still queued, AFTER the loop terminal has landed — is enforced only by a 120 ms real-time budget, and no assertion verifies that ordering, so the scenario silently inverts under load while the test stays green. — Concrete cost: the deadline timer is armed at sendPrompt time, before queueing; between that point and heldTurn.reject(...) the test performs two vi.waitFor polls (50 ms granularity) plus a held cancel-forward — on a loaded 2–4-core CI worker this can exceed 120 ms. Probe-verified: once the window is exceeded the order inverts (deadline terminal before loop terminal), and under the inversion every assertion in the test still passes (turnErrorEvent is set last by the loop terminal, the journal is empty at refresh, the summary is set last) — silently, unlike the sibling queue-bookkeeping test, which asserts its ordering with an explicit indexOf comparison. Regression coverage for exactly this PR's newest protection (a queued terminal erasing turnErrorEvent/turnError) degrades to luck without any red signal.
// pin the ordering like the sibling test, e.g.:
expect(seenTypes.indexOf('turn_error')).toBeLessThan(
seenTypes.lastIndexOf('prompt_deadline_exceeded'),
);中文说明
[Suggestion] R9-10：该测试声明的前提——截止期在 prompt 仍处于排队状态、且 loop 终止事件已经落地之后才到期——只由 120 ms 实时预算保证，没有任何断言验证这个顺序，因此负载下场景会静默反转而测试依旧全绿。— 具体代价：截止期定时器在 sendPrompt 时启动，早于入队；从那里到 heldTurn.reject(...) 之间，测试执行两次 vi.waitFor 轮询（50 ms 粒度）外加一次挂起的 cancel 转发——在负载较高的 2–4 核 CI runner 上可能超过 120 ms。探针验证：窗口一旦被超过，顺序反转（截止终止事件先于 loop 终止事件），而反转后测试的所有断言仍然通过（turnErrorEvent 最后由 loop 终止事件置位、刷新时日志为空、摘要最后置位）——全程无声，不像兄弟队列簿记测试那样用显式 indexOf 比较钉住顺序。针对本 PR 最新保护（排队终止事件不得抹掉 turnErrorEvent/turnError）的回归覆盖就此退化为碰运气，且无任何红色信号。建议按上方代码像兄弟测试一样钉住顺序。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…esh replay (#8853) Close two trust-gate gaps in the channel-turn classification introduced for loop-detected turn errors: the `qwen.daemon.channelDelivery` sibling key was not gated like `qwen.channel.prompt` at the standalone ACP boundary, and on the daemon-hosted path a client-forged `qwen.channel.prompt` survived the serve route and bridge admission strip to be re-injected for trusted parents. Both keys are now stripped from untrusted callers and honored only from trusted context (private parent state, or the channel-worker prompt authorization validated by the daemon prompt route). Also harden the refresh-append guard for pending turn errors: the synthetic `history_truncated` journal marker and idle latest-wins `session_update` snapshots no longer defeat the append, and a queued terminal that folds newer turn content supersedes the stale error before the fold erases the evidence. Web Shell now derives turn_complete's error from the same backward walk as the retry decision.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix review round 9 — summaryAll ten findings (6 Critical, 4 Suggestion) are resolved in the code. No finding was declined, deferred, or escalated; Critical
Suggestion
Files changed
VerificationCommands actually run (all passed):
中文说明Autofix 评审第 9 轮 — 总结全部 10 条发现（6 条 Critical、4 条 Suggestion）均已在代码中解决。没有拒绝、推迟或升级任何发现； Critical
Suggestion
变更文件
验证实际执行的命令（全部通过）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 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 address-review round: no action required (PR #8853)This round found no actionable work:
Critical-only mode is active because the PR diff grew beyond this counting window's net-growth budget (src +75 / test +765 net lines vs. the 400/400 budgets). This is a growth-only engagement, so the single deferred non-Critical item — the automated reviewer's summary comment (issuecomment-5284329652) — is an audit record, not work: no code changes, thread resolutions, or comment replies were made for it. It remains open for human follow-up. No commits were made; the PR head is unchanged. 中文说明Autofix 处理评审轮次：无需处理（PR #8853）本轮没有发现需要处理的工作：
由于本 PR 的 diff 净增长超出了当前计数窗口的预算（源码 +75 / 测试 +765 行净增长，预算为 400/400），目前已进入仅处理 Critical 的模式。这是一次纯增长（growth-only）触发，因此唯一被延后的非 Critical 条目 —— 自动评审机器人的总结评论（issuecomment-5284329652）—— 属于审计记录而非本轮工作：未针对它做任何代码改动、线程解决或评论回复。该条目保持开放，留待人工跟进。 本轮未产生任何提交；PR 的 head 保持不变。 Deferred non-Critical feedbackCritical-only mode is active: the PR's diff grew src 75 / test 765 net lines beyond this counting window's baseline (budgets: 400/400). The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback continues to flow unaffected during a growth-only engagement (the per-author batch budget applies only after 5 change-producing rounds). ( 中文说明已进入仅处理 Critical 的模式：本计数窗口内 diff 净增长已达 源码 75 / 测试 765 行（预算 400/400）。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。纯增长触发期间维护者反馈照常流动（按作者的批次预算仅在完成 5 个产生改动的轮次后生效）。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "PR #8853 converts foreground tool-loop-protection stops…": none — all planned checks completed within budget.; chunk 4: tests not executed — the review worktree has no node_modules , so verification was static (source reads + greps) rather than a vitest run.; chunk 4: cross-chunk consumers of the refreshed compactedReplay in web-shell not traced (structurally outside a chunk agent's duties).; "PR #8853 converts foreground tool-loop-protection stops…": none — all checks I started were completed within budget.; "PR #8853 converts foreground tool-loop-protection stops…": none — finished within budget., and 17 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："PR #8853 converts foreground tool-loop-protection stops…"：none — all planned checks completed within budget.；chunk 4：tests not executed — the review worktree has no node_modules , so verification was static (source reads + greps) rather than a vitest run.；chunk 4：cross-chunk consumers of the refreshed compactedReplay in web-shell not traced (structurally outside a chunk agent's duties).；"PR #8853 converts foreground tool-loop-protection stops…"：none — all checks I started were completed within budget.；"PR #8853 converts foreground tool-loop-protection stops…"：none — finished within budget.，另有 17 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // Channel classification opts the turn out of loop-detected | ||
| // rejection, so it rides the same worker authorization as the | ||
| // display projection; a forged key from any other caller is dropped | ||
| // here and again at the bridge admission strip. | ||
| const trustedChannelPrompt = | ||
| channelWorkerAuthorized && channelPrompt === true; |
There was a problem hiding this comment.
[Critical] R10-1: The route gates qwen.channel.prompt behind channel-worker authorization, but the sibling classification input — the first-class delivery body field (~line 3339) — is parsed and forwarded as context.channelDelivery with no worker-authorization check, and Session.prompt computes channelTurn = channelDelivery !== undefined || channelPromptTurn, treating both inputs identically — so the ungated field reproduces exactly the bypass this gate exists to prevent. parseChannelDelivery is structural-only validation; ChannelDeliveryAuthorizationStore.authorizePrompt records unconditionally (it is delivery-time bookkeeping, not an admission gate); the existing test 'validates delivery and forwards it only through trusted prompt context' proves context.channelDelivery is set for a request with no worker token. The route is non-strict mutate() — open on loopback in the no-token default, so any local client reaches it. — Failure scenario: any local client POSTs {prompt: [...], delivery: {kind:'channel', target:{...}}} with no authorization token → the bridge re-injects DAEMON_CHANNEL_DELIVERY_META_KEY from the trusted context → channelTurn=true → the turn never rejects with the structured LOOP_DETECTED error no matter how badly it loops, and the repeated-failure guard runs 'off' even under enforce — reintroducing through the sibling field the exact ungated opt-out this PR gates qwen.channel.prompt to close. Pre-PR, delivery-marked turns ran the configured guard mode; both bypass effects are introduced by this diff, and the diff's own acpAgent-strip rationale ("an untrusted client marking its own prompt as a channel turn must not reach the session") is contradicted by the sibling input staying open in the same handler. Bounding: the per-turn cap and global-duplicate detectors ignore guard mode, so a runaway self-marked turn stays bounded; effects are confined to the caller's own turn.
Suggested fix: gate the delivery context flag on the same worker authorization (spread channelDelivery into the sendPrompt context only when channelWorkerAuthorized), or narrow Session's channelTurn so only the authenticated channelPromptTurn input disables rejection/guard mode. If self-classification via the public delivery feature is deliberate product behavior, align the new comments/rationale with that decision instead.
中文说明
[Critical] R10-1：该路由把 qwen.channel.prompt 置于 channel-worker 授权之后，但同一分类的姊妹输入——一等公民的 delivery 请求体字段（约 3339 行）——在没有任何 worker 授权检查的情况下被解析并作为 context.channelDelivery 转发，而 Session.prompt 计算 channelTurn = channelDelivery !== undefined || channelPromptTurn，对两个输入一视同仁——因此这个未设防的字段恰好复现了本门控要防止的绕过。parseChannelDelivery 只做结构校验；ChannelDeliveryAuthorizationStore.authorizePrompt 无条件记录（它是投递时簿记，不是准入门控）；既有测试 'validates delivery and forwards it only through trusted prompt context' 证明无 worker token 的请求也会设置 context.channelDelivery。该路由是非严格 mutate()——无 token 默认配置下对 loopback 开放，任何本地客户端都可访问。— 失败场景：任何本地客户端不带授权 token POST {prompt: [...], delivery: {kind:'channel', target:{...}}} → bridge 从受信 context 重新注入 DAEMON_CHANNEL_DELIVERY_META_KEY → channelTurn=true → 无论该轮如何循环都不会以结构化 LOOP_DETECTED 错误拒绝，且重复失败守卫在 enforce 下也保持 'off'——经由姊妹字段重新引入了本 PR 正要堵住的无门控豁免。PR 前 delivery 标记的轮次运行配置的守卫模式；两个绕过效果都是本 diff 引入的，且 diff 自身的 acpAgent 剥离理由（"不可信客户端不得把自己的 prompt 标记为 channel turn"）与同一处理器中保持开放的姊妹输入相矛盾。边界说明：单轮上限与全局重复检测不受守卫模式影响，失控的自标记轮次仍然有界；效果限于调用者自己的轮次。
建议修复：把 delivery 上下文标志置于同样的 worker 授权之下（仅在 channelWorkerAuthorized 时才把 channelDelivery 放入 sendPrompt context），或收窄 Session 的 channelTurn，只让经过认证的 channelPromptTurn 输入豁免拒绝/守卫模式。如果公共 delivery 功能的自我分类是刻意的产品行为，请让新注释/理由与该决定保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const retryableTurnError = | ||
| lastTurnError && | ||
| lastTurnError.kind === 'error' && | ||
| isRetryableTurnErrorKind(lastTurnError.errorKind) | ||
| ? lastTurnError | ||
| : undefined; |
There was a problem hiding this comment.
[Critical] R10-2: The new isRetryableTurnErrorKind filter is applied here in the blocks effect but not at handleRetry's failure path (~line 9913), where rearmFailedTurnErrorRetry(nextTurnError, …) is still called with the unfiltered getRetryableTurnError result — so a retry whose own turn is loop-stopped arms retry state (retryableTurnErrorIdRef, identity refs, setShowRetryHint(true)) on the very error class this PR de-retries. Probe-verified in the App.test.tsx harness: after the loop-stopped retry lands, the retry button stays hidden (message.retryable is false for loop_detected) but the Ctrl+Y global shortcut calls handleRetry() directly, whose guards (showRetryHintRef, retryableTurnErrorIdRef, identity match, lastSubmittedPromptRef) all pass — sendPrompt was observed called a third time, resubmitting the loop-stopped prompt. With the filter applied at the re-arm site the probe flips to two calls. — Failure scenario: a turn fails with a retryable error → the user retries → the retried turn is loop-stopped (newly possible as a rejection) → the catch re-arms on the loop error → Ctrl+Y resubmits the loop-stopped prompt, which tends to re-loop and be stopped again — the exact outcome the added comment above ("no retry affordance is offered for them") says is prevented.
Suggested fix: mirror this blocks-effect filter at the handleRetry catch re-arm site —
const nextTurnError = getRetryableTurnError(store.getSnapshot().blocks);
if (
nextTurnError &&
nextTurnError.kind === 'error' &&
isRetryableTurnErrorKind(nextTurnError.errorKind)
) {
rearmFailedTurnErrorRetry(nextTurnError, store.getSnapshot().blocks);
}中文说明
[Critical] R10-2：新的 isRetryableTurnErrorKind 过滤器在 blocks effect 此处生效，但在 handleRetry 的失败路径（约 9913 行）没有生效——那里仍用未过滤的 getRetryableTurnError 结果调用 rearmFailedTurnErrorRetry(nextTurnError, …)——因此一次自身轮次被循环保护停止的重试，会在本 PR 明确取消重试的错误类别上重新武装重试状态（retryableTurnErrorIdRef、identity refs、setShowRetryHint(true)）。已在 App.test.tsx 测试装置中探针验证：循环停止的重试落地后，重试按钮保持隐藏（loop_detected 的 message.retryable 为 false），但 Ctrl+Y 全局快捷键直接调用 handleRetry()，其守卫（showRetryHintRef、retryableTurnErrorIdRef、identity 匹配、lastSubmittedPromptRef）全部通过——观察到 sendPrompt 第三次被调用，重新提交了被循环停止的 prompt。在重新武装处加上过滤器后探针翻转为两次调用。— 失败场景：某轮以可重试错误失败 → 用户重试 → 重试轮被循环保护停止（本 PR 使其可能以拒绝形式出现）→ catch 在循环错误上重新武装 → Ctrl+Y 重新提交被循环停止的 prompt，大概率再次循环并再次被停止——正是上方新增注释（"不会为其提供重试入口"）声明要防止的结果。
建议修复：在 handleRetry catch 的重新武装处镜像此 blocks effect 过滤器（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const retryableTurnError = | ||
| lastTurnError && | ||
| lastTurnError.kind === 'error' && | ||
| isRetryableTurnErrorKind(lastTurnError.errorKind) | ||
| ? lastTurnError | ||
| : undefined; |
There was a problem hiding this comment.
[Critical] R10-3: Stale failedTurnErrorRetryRef payload survives a filtered loop error and is later consumed by an unrelated retryable error. Probe-verified on the real DaemonSessionProvider: the prompt promise rejects BEFORE the loop turn_error block commits to the store — structural, not a race: settle rejects on a microtask while the transcript event flushes on a setTimeout(0) macrotask (blockAtCatch=false, true after the dispatch flush). So in handleRetry's catch the snapshot walk still returns the OLD error err1, rearmFailedTurnErrorRetry early-returns on retryableTurnError.id === failedRetry.errorId, and the stashed payload {errorId: err1.id, text: P1} survives. The blocks effect now filters the loop error (this hunk) and never calls rearm — and its only clearing branch requires retryableTurnError truthy — so the payload is never cleared. — Failure scenario: a later retryable turn error the backward walk reaches (e.g. a queued prompt's deadline terminal — turn_error with no errorKind, so isRetryableTurnErrorKind(undefined) is true, and no user block follows it) runs rearm first in the effect and consumes the stale payload: restores P1 into lastSubmittedPromptRef, arms retry on err3, and because previousIdentity is read after rearm overwrites the identity ref the mismatch cleanup is skipped → Ctrl+Y/button resubmits P1 — the loop-stopped prompt — misattributed to an error from a turn the user never submitted. The window is unbounded until a new submit or session change.
Suggested fix: gate the catch-time failedTurnErrorRetryRef save on the terminal being retryable (the thrown DaemonHttpError carries code: 'LOOP_DETECTED'), and/or have the blocks effect clear failedTurnErrorRetryRef when lastTurnError exists but is filtered out by isRetryableTurnErrorKind.
中文说明
[Critical] R10-3：过期的 failedTurnErrorRetryRef 载荷能在被过滤的循环错误之后存活，并被之后一个无关的可重试错误消费。已在真实 DaemonSessionProvider 上探针验证：prompt promise 会在 loop turn_error block 提交进 store 之前被拒绝——这是结构性的而非竞态：settle 在微任务中拒绝，而转录事件在 setTimeout(0) 宏任务中刷新（blockAtCatch=false，dispatch 刷新后为 true）。因此 handleRetry 的 catch 中快照遍历仍返回旧错误 err1，rearmFailedTurnErrorRetry 因 retryableTurnError.id === failedRetry.errorId 提前返回，暂存的载荷 {errorId: err1.id, text: P1} 得以存活。blocks effect 现在会过滤掉循环错误（本 hunk）且不再调用 rearm——而其唯一的清理分支要求 retryableTurnError 为真——因此载荷永远不会被清理。— 失败场景：之后出现一个反向遍历可达的可重试轮次错误（例如排队 prompt 的截止期终止事件——不带 errorKind 的 turn_error，isRetryableTurnErrorKind(undefined) 为 true，且其后没有用户 block）在 effect 中先运行 rearm 并消费过期载荷：把 P1 恢复进 lastSubmittedPromptRef，在 err3 上武装重试，且由于 previousIdentity 在 rearm 覆写 identity ref 之后才读取，失配清理被跳过 → Ctrl+Y/按钮重新提交 P1——被循环停止的 prompt——并被错误归因到一个用户从未提交的轮次的错误上。该窗口在新提交或会话切换之前是无限期的。
建议修复：把 catch 时 failedTurnErrorRetryRef 的暂存置于"终止事件可重试"的门控之下（抛出的 DaemonHttpError 携带 code: 'LOOP_DETECTED'），和/或让 blocks effect 在 lastTurnError 存在但被 isRetryableTurnErrorKind 过滤时清理 failedTurnErrorRetryRef。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // Same walk as the retry decision above, so turn_complete and the | ||
| // retry affordance never disagree about whether the current turn has | ||
| // a turn error (e.g. across a trailing background notification). | ||
| lastTurnErrorIdRef.current = lastTurnError?.id ?? null; |
There was a problem hiding this comment.
[Critical] R10-4: Widening turn_complete reporting from the canRetry-gated retryableTurnErrorIdRef to this ungated walk result drops the "already retried" suppression along with the loop-detected filter it was introduced to bypass. A/B-probe-verified against the built merge base with the identical test in both trees: BASE passes (a cancelled retry turn reports error: undefined); on this PR the same probe fails — the cancelled retry turn dispatches a second turn_complete carrying the PREVIOUS turn's error. — Failure scenario: turn 1 fails with retryable err1 → turn_complete(err1) fires, retry armed → user retries (retriedTurnErrorIdRef = err1.id; retry: true suppresses the user echo) → the retry turn is cancelled (Esc) before emitting content → streamingState transitions responding→idle while rendered blocks still end with err1 (settle precedes the transcript flush — the same microtask/setTimeout(0) ordering verified for R10-3) → the turn_complete effect reads lastTurnErrorIdRef = err1.id and dispatches turn_complete { error: err1 } for the cancelled retry turn — a duplicate, misattributed failure signal to the host (onSessionChange), contradicting this comment's "never disagree" claim (no retry affordance is shown while turn_complete still reports the error). Pre-PR reported no error here. The suggested fix flips the A/B probe green with all 438 App.test.tsx tests passing.
| lastTurnErrorIdRef.current = lastTurnError?.id ?? null; | |
| lastTurnErrorIdRef.current = | |
| lastTurnError && lastTurnError.id !== retriedTurnErrorIdRef.current | |
| ? lastTurnError.id | |
| : null; |
(loop-detected errors are unaffected — they are never retried, so the ref never matches them.)
中文说明
[Critical] R10-4：把 turn_complete 上报从受 canRetry 门控的 retryableTurnErrorIdRef 扩展为此处未门控的遍历结果，在绕过 loop-detected 过滤的同时也丢掉了"已重试过"的抑制。已用同一测试对构建后的 merge base 做 A/B 探针验证：BASE 通过（被取消的重试轮上报 error: undefined）；在本 PR 上同一探针失败——被取消的重试轮第二次派发了携带上一轮错误的 turn_complete。— 失败场景：第 1 轮以可重试错误 err1 失败 → turn_complete(err1) 触发、重试武装 → 用户重试（retriedTurnErrorIdRef = err1.id；retry: true 抑制用户回声）→ 重试轮在产出内容前被取消（Esc）→ streamingState 在渲染 blocks 仍以 err1 结尾时从 responding→idle（settle 先于转录刷新——与 R10-3 验证的微任务/setTimeout(0) 时序相同）→ turn_complete effect 读到 lastTurnErrorIdRef = err1.id，为被取消的重试轮派发 turn_complete { error: err1 }——向宿主（onSessionChange）发出重复且归因错误的失败信号，与本注释"永不分歧"的声明矛盾（界面不显示重试入口，turn_complete 却仍上报错误）。PR 前此处不上报错误。建议修复使 A/B 探针翻绿，且 App.test.tsx 全部 438 个测试通过。
（loop-detected 错误不受影响——它们从不被重试，ref 永远不会匹配到它们。）
— qwen3.8-max via Qwen Code /review (v0.21.11)
| stopReason: rejectOnLoopDetected | ||
| ? cancelledOrThrowLoopDetected( | ||
| pendingSend.signal, | ||
| toolLoopState, | ||
| ) | ||
| : getAbortAwareEndTurnStopReason(pendingSend.signal), |
There was a problem hiding this comment.
[Suggestion] R10-5: The cancellation-wins-over-loop-rejection invariant is tested at two of the three foreground cancelledOrThrowLoopDetected call sites, but not at this one (the per-turn-cap / duplicate-tool toolRun.loopDetected branch). Mutation-verified: replacing cancelledOrThrowLoopDetected(pendingSend.signal, toolLoopState) here with an unconditional throw keeps Session.test.ts 607/607 green. A follow-up audit additionally verified the existing cancel-race test never reaches cancelledOrThrowLoopDetected at all (the pre-existing abort short-circuit returns 'cancelled' first), so even the site that appears covered is not. The proposed race test discriminates: it passes on this PR and fails under the mutation. — Concrete cost: a future edit dropping the signal.aborted check at this branch rejects a user-cancelled turn with LOOP_DETECTED instead of resolving stopReason: 'cancelled' — Web Shell presents a loop error for a turn the user cancelled — and nothing in the suite catches the regression.
Suggested fix: add a Session.test.ts race test mirroring the existing two — gate the cap-tripping batch (held stream or messageRewriter stub), cancel after the cap trips, assert resolves.toEqual({ stopReason: 'cancelled' }) plus logLoopDetectedSpy still called.
中文说明
[Suggestion] R10-5：取消优先于循环拒绝的不变量在三个前台 cancelledOrThrowLoopDetected 调用点中的两个有测试，但此处（单轮上限/重复工具 toolRun.loopDetected 分支）没有。突变验证：把此处的 cancelledOrThrowLoopDetected(pendingSend.signal, toolLoopState) 替换为无条件 throw，Session.test.ts 仍 607/607 全绿。后续审计还验证了既有取消竞态测试根本到不了 cancelledOrThrowLoopDetected（既有的 abort 短路先返回 'cancelled'），因此看似有覆盖的调用点实际也没有。建议的竞态测试具有区分力：在本 PR 上通过，在该突变下失败。— 具体代价：未来若删除此分支的 signal.aborted 检查，用户已取消的轮次会以 LOOP_DETECTED 拒绝而不是返回 stopReason: 'cancelled'——Web Shell 会对用户取消的轮次显示循环错误——且测试套件中没有任何东西能捕获该回归。
建议修复：新增一个与既有两个同构的 Session.test.ts 竞态测试——门控触发上限的批次（挂起流或 messageRewriter 桩），在上限触发后取消，断言 resolves.toEqual({ stopReason: 'cancelled' }) 且 logLoopDetectedSpy 仍被调用。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred (not declined on merit) — this round runs in Critical-only mode: the deterministic growth brake engaged for this PR (src 75 / test 765 net lines beyond the counting window's 400/400 budgets), and AGENTS.md lands only Critical fixes after ~5 review rounds (this is round 10). The finding stands as reported — the cancellation-wins-over-loop-rejection invariant is untested at the per-turn-cap / duplicate-tool cancelledOrThrowLoopDetected site — and stays open for human follow-up or a follow-up PR adding the suggested Session.test.ts race test.
中文说明
延期处理（并非否定其价值）——本轮处于仅 Critical 模式：本 PR 的确定性增长刹车已触发（本计数窗口净增长 源码 75 / 测试 765 行，超出 400/400 预算），且 AGENTS.md 规定约 5 轮审查后只落地 Critical 修复（本轮为第 10 轮）。该发现所述成立——单轮上限/重复工具 cancelledOrThrowLoopDetected 调用点上"取消优先于循环拒绝"的不变量确实没有测试——保持开放，留待人工跟进或后续 PR 补充建议的 Session.test.ts 竞态测试。
| const compactedReplay = refreshed.compactedReplay ?? []; | ||
| expect(compactedReplay.some((event) => event.type === 'turn_error')).toBe( | ||
| false, |
There was a problem hiding this comment.
[Suggestion] R10-6: This drop-on-successful-turn test asserts only the absence of turn_error — [].some(...) is false and undefined ?? [] → [], so an empty or degraded compactedReplay satisfies it vacuously. It is the only refresh test in the suite with neither a persisted-page anchor nor a length assertion; the sibling drop test anchors compactedReplay[0] on the persisted fixture and asserts toHaveLength(1), with an explicit comment that a stale branch rebuilding the replay from anything else ships green without it. — Concrete cost: a regression in the supersede path — refreshedReplayFieldsFor dropping the persisted page along with the stale terminal, the idle-guard failing with the ?? [] fallback swallowing an absent compactedReplay, or the bounded-read catch/break returning an empty in-memory replay — ships green: after a LOOP_DETECTED stop and a successful recovery prompt, refresh renders an empty transcript instead of the persisted history, and this test stays green.
Suggested fix: mirror the sibling drop test — assert compactedReplay[0] matches the persisted-page fixture and expect(compactedReplay).toHaveLength(1) before the absence assertion (the recovery turn_complete clears turnErrorEvent via mutateTurnState, so the replay should be exactly the one-event persisted page).
中文说明
[Suggestion] R10-6：该"成功轮次后丢弃"测试只断言 turn_error 不存在——[].some(...) 为 false 且 undefined ?? [] → []，因此空或退化的 compactedReplay 也能空泛地满足它。它是套件中唯一既无持久化页锚点也无长度断言的刷新测试；姊妹丢弃测试把 compactedReplay[0] 锚定到持久化 fixture 并断言 toHaveLength(1)，且有明确注释说明：从其他来源重建 replay 的陈旧分支若没有该锚点将会静默通过。— 具体代价：supersede 路径的回归——refreshedReplayFieldsFor 把持久化页连同陈旧终止事件一起丢弃、空闲守卫失败且 ?? [] 回退吞掉缺失的 compactedReplay、或有界读取的 catch/break 返回空的内存 replay——都会静默通过：LOOP_DETECTED 停止加一次成功恢复 prompt 之后，刷新会渲染空转录而非持久化历史，而本测试仍是绿的。
建议修复：对齐姊妹丢弃测试——在缺失断言之前断言 compactedReplay[0] 匹配持久化页 fixture 且 expect(compactedReplay).toHaveLength(1)（恢复轮的 turn_complete 经由 mutateTurnState 清除 turnErrorEvent，因此 replay 应恰好是单事件的持久化页）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred (not declined on merit) — this round runs in Critical-only mode: the deterministic growth brake engaged for this PR (src 75 / test 765 net lines beyond the counting window's 400/400 budgets), and AGENTS.md lands only Critical fixes after ~5 review rounds (this is round 10). The finding stands as reported — the drop-on-successful-turn test is satisfied vacuously by an empty compactedReplay and should anchor the persisted page like its sibling — and stays open for human follow-up or a follow-up PR.
中文说明
延期处理（并非否定其价值）——本轮处于仅 Critical 模式：本 PR 的确定性增长刹车已触发（本计数窗口净增长 源码 75 / 测试 765 行，超出 400/400 预算），且 AGENTS.md 规定约 5 轮审查后只落地 Critical 修复（本轮为第 10 轮）。该发现所述成立——"成功轮次后丢弃"测试确实会被空的 compactedReplay 空泛满足，应像姊妹测试一样锚定持久化页——保持开放，留待人工跟进或后续 PR。
| const bridge = makeBridge({ | ||
| sessionShellCommandEnabled: true, | ||
| maxJournalEvents: 1, |
There was a problem hiding this comment.
[Suggestion] R10-7: This journal-truncation test pins maxJournalEvents: 1 but never asserts that truncation/the history_truncated marker actually occurred. Verified: its assertions (replay length 2, persisted-page anchor, loop turn_error last) pass identically whether or not truncation happens — all three shell events are bookkeeping either way. normalizeMaxJournalEvents throws below 1 rather than clamping, so cap=1 is honored today; but if a future change floors/clamps small caps, no eviction/marker occurs and the test still ships green — and a regression deleting the 'history_truncated' exclusion in isRefreshAppendTurnContent would then pass the bridge suite, since nothing else exercises a truncated live journal through the refresh-append guard (the other history_truncated tests are compacted-replay-window tests with no turnErrorEvent append). — Concrete cost: silent degradation into a duplicate of the user-shell test above it; the only guard-level marker-branch coverage is lost without any test failing.
Suggested fix: pin the premise before loadSession — assert eviction/the marker actually happened. Note the premise is not observable from the test's current surfaces (the bounded branch hardcodes liveJournal: []), so an observation needs to be added (a journal snapshot/limits surface, or an assertion at the wiring/engine level), not just an extra expect on the existing response.
中文说明
[Suggestion] R10-7：该日志截断测试固定了 maxJournalEvents: 1，却从未断言截断/history_truncated 标记确实发生。已验证：无论截断是否发生，其断言（replay 长度 2、持久化页锚点、loop turn_error 居末）都同样通过——三种 shell 事件在两种情形下都是簿记事件。normalizeMaxJournalEvents 对小于 1 的值抛错而非钳制，因此当前 cap=1 生效；但若未来变更对小的 cap 做下限/钳制，逐出/标记不再发生而测试仍静默通过——届时删除 isRefreshAppendTurnContent 中 'history_truncated' 豁免的回归也能通过 bridge 套件，因为没有其他测试让截断的实时日志经过刷新追加守卫（其余 history_truncated 测试是无 turnErrorEvent 追加的压缩 replay 窗口测试）。— 具体代价：静默退化为上方 user-shell 测试的重复；唯一的守卫层标记分支覆盖在没有任何测试失败的情况下丢失。
建议修复：在 loadSession 之前钉住前提——断言逐出/标记确实发生。注意该前提从测试现有表面不可观测（有界分支硬编码 liveJournal: []），因此需要增加观测手段（日志快照/上限表面，或接线/引擎层的断言），而不只是在现有响应上多加一个 expect。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred (not declined on merit) — this round runs in Critical-only mode: the deterministic growth brake engaged for this PR (src 75 / test 765 net lines beyond the counting window's 400/400 budgets), and AGENTS.md lands only Critical fixes after ~5 review rounds (this is round 10). The finding stands as reported — the journal-truncation test never asserts the truncation premise — and stays open for human follow-up or a follow-up PR. Note the suggested fix itself requires adding a new observation surface (journal snapshot/limits or wiring-level assertion), which is out of scope for a Critical-only round.
中文说明
延期处理（并非否定其价值）——本轮处于仅 Critical 模式：本 PR 的确定性增长刹车已触发（本计数窗口净增长 源码 75 / 测试 765 行，超出 400/400 预算），且 AGENTS.md 规定约 5 轮审查后只落地 Critical 修复（本轮为第 10 轮）。该发现所述成立——日志截断测试确实从未断言截断前提——保持开放，留待人工跟进或后续 PR。注意建议的修复本身需要新增观测表面（日志快照/上限或接线层断言），超出仅 Critical 轮次的范围。
| // The queued terminal must not overwrite the session-scoped summary | ||
| // either: it belongs to a prompt that never ran. | ||
| expect(bridge.getSessionSummary(session.sessionId).turnError).toMatchObject( |
There was a problem hiding this comment.
[Suggestion] R10-8: The three queued-terminal tests pin only the turnError half of the documented "queued terminals must not mutate session-scoped turn state" invariant; the retryAllowed half — named in the same production comment ("consumed by the retry-admission check … otherwise a queued failure would arm a retry the active prompt didn't earn") — is asserted nowhere (grep retryAllowed in this file returns nothing). Mutation-verified: moving entry.retryAllowed = true outside the if (mutateTurnState) gate (bridge.ts:1739) keeps the full suite 611/611 green; a probe (held active turn → queued prompt's deadline expires while still queued → active turn succeeds → retry: true resubmit) flips under the mutation. — Concrete cost: with the gate broken, a queued deadline/removal terminal arms the one-shot retry flag even when the active turn succeeded; the client's next retry: true resubmit passes isRetry = requestedRetry && entry.retryAllowed — the turn runs classified as a retry, DAEMON_RETRY_META_KEY is stamped, and the user_message_chunk echo is suppressed, so peer clients' transcripts lose the prompt line.
Suggested fix: in the queued-deadline and queued-removed tests, exercise the retry-admission surface after the queued terminal lands — resubmit with { retry: true } and assert the prompt is NOT treated as a retry (user echo present / no qwen.daemon.retry meta) — or expose retryAllowed on the summary/status fixture and assert it is false.
中文说明
[Suggestion] R10-8：三个排队终止测试只钉住了"排队终止不得变更会话级轮次状态"这一文档化不变量的 turnError 一半；retryAllowed 一半——生产代码同一注释中点名（"由重试准入检查消费……否则排队失败会武装一次活动 prompt 未曾赢得的重试"）——没有任何断言（本文件 grep retryAllowed 为空）。突变验证：把 entry.retryAllowed = true 移出 if (mutateTurnState) 门控（bridge.ts:1739），整个套件仍 611/611 全绿；一个探针（活动轮挂起 → 排队 prompt 的截止期在仍排队时到期 → 活动轮成功 → retry: true 重新提交）在该突变下翻转。— 具体代价：门控被破坏时，即使活动轮成功，排队的截止/移除终止也会武装一次性重试标志；客户端下一次 retry: true 重交会通过 isRetry = requestedRetry && entry.retryAllowed——该轮以重试身份运行、被打上 DAEMON_RETRY_META_KEY，且 user_message_chunk 回声被抑制，其他客户端的转录将丢失该 prompt 行。
建议修复：在排队截止与排队移除测试中，于排队终止落地后验证重试准入表面——以 { retry: true } 重交并断言 prompt 未被当作重试（用户回声存在 / 无 qwen.daemon.retry meta）——或在 summary/status fixture 上暴露 retryAllowed 并断言其为 false。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred (not declined on merit) — this round runs in Critical-only mode: the deterministic growth brake engaged for this PR (src 75 / test 765 net lines beyond the counting window's 400/400 budgets), and AGENTS.md lands only Critical fixes after ~5 review rounds (this is round 10). The finding stands as reported — the queued-terminal tests pin only the turnError half of the invariant, and the retryAllowed half is mutation-verified untested — and stays open for human follow-up or a follow-up PR adding the suggested retry-admission assertions.
中文说明
延期处理（并非否定其价值）——本轮处于仅 Critical 模式：本 PR 的确定性增长刹车已触发（本计数窗口净增长 源码 75 / 测试 765 行，超出 400/400 预算），且 AGENTS.md 规定约 5 轮审查后只落地 Critical 修复（本轮为第 10 轮）。该发现所述成立——排队终止测试只钉住了不变量的 turnError 一半，retryAllowed 一半经突变验证确实无测试——保持开放，留待人工跟进或后续 PR 补充建议的重试准入断言。
| mockChat.sendMessageStream = vi | ||
| .fn() | ||
| .mockResolvedValueOnce(streamForBatch(1, 4)) | ||
| .mockResolvedValueOnce(streamForBatch(2, 4)) | ||
| .mockResolvedValueOnce(streamForBatch(3, 1)) | ||
| .mockResolvedValueOnce(createEmptyStream()); |
There was a problem hiding this comment.
[Suggestion] R10-9: This Goal repeated-failure-guard graceful test omits the execute (9) and sendMessageStream (3) count assertions its foreground sibling — the enforce-mode test this same diff changed to expect the LOOP_DETECTED rejection, using the identical 4/4/1 failure fixture — asserts; the unconsumed fourth createEmptyStream() additionally absorbs a spurious extra model round-trip. Guard thresholds verified (REPEATED_TOOL_FAILURE_THRESHOLD = 8, batch threshold 2): the stop lands after batch 3 — 9 executions / 3 model calls. The proposed assertions were added temporarily during verification and pass on this PR. — Concrete cost: a regression making the goal path stop one batch early (8 executions / 2 model calls) or perform one extra model round-trip around the stop leaves all four current assertions green (logLoopDetected fired, finishTurn called, no dispatch, stop message present) — goal turns silently do less work than configured, or burn an extra model call per stopped turn, with no test red.
// after the finishTurn wait, mirroring the foreground enforce test:
expect(execute).toHaveBeenCalledTimes(9);
expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(3);中文说明
[Suggestion] R10-9：该 Goal 重复失败守卫优雅停止测试缺少其前台姊妹测试——同一 diff 改为期望 LOOP_DETECTED 拒绝、使用相同 4/4/1 失败 fixture 的 enforce 模式测试——所拥有的 execute（9 次）与 sendMessageStream（3 次）计数断言；未被消费的第四个 createEmptyStream() 还会吸收一次多余的模型往返。守卫阈值已核实（REPEATED_TOOL_FAILURE_THRESHOLD = 8、批次阈值 2）：停止落在第 3 批之后——9 次执行 / 3 次模型调用。验证时临时加入上述断言，在本 PR 上通过。— 具体代价：使 goal 路径提前一批停止（8 次执行 / 2 次模型调用）或在停止前后多一次模型往返的回归，四个现有断言全部保持绿色（logLoopDetected 触发、finishTurn 被调用、无 dispatch、停止消息存在）——goal 轮次静默地少做配置的工作量，或每次停止多烧一次模型调用，且无测试变红。
建议修复：在 finishTurn 等待之后加入上述两个断言（与前台 enforce 测试对齐），使第四个流保持纯防御性。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred (not declined on merit) — this round runs in Critical-only mode: the deterministic growth brake engaged for this PR (src 75 / test 765 net lines beyond the counting window's 400/400 budgets), and AGENTS.md lands only Critical fixes after ~5 review rounds (this is round 10). The finding stands as reported — the Goal graceful-stop test omits the execute (9) / sendMessageStream (3) count assertions its foreground enforce sibling has — and stays open for human follow-up or a follow-up PR adding the two assertions.
中文说明
延期处理（并非否定其价值）——本轮处于仅 Critical 模式：本 PR 的确定性增长刹车已触发（本计数窗口净增长 源码 75 / 测试 765 行，超出 400/400 预算），且 AGENTS.md 规定约 5 轮审查后只落地 Critical 修复（本轮为第 10 轮）。该发现所述成立——Goal 优雅停止测试确实缺少其前台 enforce 姊妹测试拥有的 execute（9 次）/ sendMessageStream（3 次）计数断言——保持开放，留待人工跟进或后续 PR 补充这两个断言。
| describe('channel prompt classification wire key', () => { | ||
| it('is identical across channel-base and acp-bridge', () => { | ||
| expect(CHANNEL_PROMPT_META_KEY).toBe(BRIDGE_CHANNEL_PROMPT_META_KEY); |
There was a problem hiding this comment.
[Suggestion] R10-10: This new wire-key test pins only the classification-key twin; the equally load-bearing authorization-key twin — channel-base's CHANNEL_PROMPT_AUTHORIZATION_META_KEY ('qwen.daemon.channelPromptAuthorization', written into _meta by DaemonChannelBridge in the same object as the pinned key) vs the cli-local CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY read by the route — remains unpinned, identical by value today with no import path between the packages. Both endpoints change in this diff (the token is now always presented; channelWorkerAuthorized now feeds the new trustedChannelPrompt). — Concrete cost: if either side of the authorization pair is ever renamed, isChannelWorkerPromptAuthorized never matches → channelWorkerAuthorized is false for every channel-worker prompt → both trustedPromptDisplayText and trustedChannelPrompt are silently dropped → channel turns reclassify as foreground (interactive guard + LOOP_DETECTED rejection instead of graceful delivery), observable only in production. (The display-text twin is transitively pinned by the existing 'accepts channel display text only from the workspace worker' server test — only the authorization pair needs pinning.)
Suggested fix: add a second assertion in this test (cli can import both sides) pinning channel-base's CHANNEL_PROMPT_AUTHORIZATION_META_KEY toBe the serve module's CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY, with a matching import.
中文说明
[Suggestion] R10-10：该新 wire-key 测试只钉住了分类键的孪生对；同样承重的授权键孪生对——channel-base 的 CHANNEL_PROMPT_AUTHORIZATION_META_KEY（'qwen.daemon.channelPromptAuthorization'，DaemonChannelBridge 将其与被钉住的键写入同一 _meta 对象）与路由读取的 cli 本地 CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY——仍未被钉住，两者当前值相同且包之间没有 import 路径。本 diff 同时改动了两端（token 现在总是被出示；channelWorkerAuthorized 现在喂给新的 trustedChannelPrompt）。— 具体代价：若授权键对的任一侧被重命名，isChannelWorkerPromptAuthorized 永不匹配 → 每个 channel-worker prompt 的 channelWorkerAuthorized 都为 false → trustedPromptDisplayText 与 trustedChannelPrompt 都被静默丢弃 → channel 轮次被重新分类为前台轮（交互式守卫 + LOOP_DETECTED 拒绝，而非优雅投递），且只能在生产中观察到。（display-text 孪生对已被既有 'accepts channel display text only from the workspace worker' 服务端测试间接钉住——只需钉住授权键对。）
建议修复：在本测试中增加第二个断言（cli 可以 import 两侧），把 channel-base 的 CHANNEL_PROMPT_AUTHORIZATION_META_KEY 与 serve 模块的 CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY 钉为相等，并补充相应 import。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred (not declined on merit) — this round runs in Critical-only mode: the deterministic growth brake engaged for this PR (src 75 / test 765 net lines beyond the counting window's 400/400 budgets), and AGENTS.md lands only Critical fixes after ~5 review rounds (this is round 10). The finding stands as reported — the authorization-key twin (CHANNEL_PROMPT_AUTHORIZATION_META_KEY vs CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY) is unpinned — and stays open for human follow-up or a follow-up PR adding the second wire-key assertion.
中文说明
延期处理（并非否定其价值）——本轮处于仅 Critical 模式：本 PR 的确定性增长刹车已触发（本计数窗口净增长 源码 75 / 测试 765 行，超出 400/400 预算），且 AGENTS.md 规定约 5 轮审查后只落地 Critical 修复（本轮为第 10 轮）。该发现所述成立——授权键孪生对（CHANNEL_PROMPT_AUTHORIZATION_META_KEY 与 CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY）确实未被钉住——保持开放，留待人工跟进或后续 PR 补充第二个 wire-key 断言。
…nd retry re-arm (#8853)
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8853 (review round 10)Critical-only mode is active for this PR (the deterministic growth brake engaged: the diff grew src 75 / test 765 net lines beyond this counting window's 400/400 budgets). This round verifies and fixes all four Critical findings from the automated review and defers the six non-Critical Suggestions with recorded reasons (they remain open for human follow-up). No base-conflict resolution was requested ( Critical findings — all fixed
Suggestions — deferred (Critical-only mode)R10-5 through R10-10 are all valuable, mutation-verified test-hardening findings. They are deferred this round — not declined on merit — because Critical-only mode is active (growth brake) and AGENTS.md lands only Critical fixes after ~5 review rounds (this is round 10); each deferral is answered on its own thread and stays open for human follow-up or a follow-up PR. R10-7 additionally asks for a new observation surface, which is out of scope for a Critical-only round. Conflict notesNone — Verification
中文说明Autofix 轮次总结 — PR #8853（审查第 10 轮）本 PR 已进入仅处理 Critical 的模式（确定性增长刹车已触发：本计数窗口内 diff 净增长达 源码 75 / 测试 765 行，超出 400/400 预算）。本轮验证并修复了自动审查的全部 4 个 Critical 发现，并将 6 个非 Critical 的 Suggestion 记录原因后延期处理（保持开放，留待人工跟进）。未要求解决 base 冲突（ Critical 发现 — 全部修复
Suggestion — 延期处理（仅 Critical 模式）R10-5 至 R10-10 均为有价值的、经突变验证的测试加固发现。本轮延期处理——并非否定其价值——因为当前处于仅 Critical 模式（增长刹车已触发），且 AGENTS.md 规定约 5 轮审查之后只落地 Critical 修复（本轮为第 10 轮）；每个延期都会在各自线程中回复说明，并保持开放，留待人工跟进或后续 PR。R10-7 还要求新增观测表面，超出仅 Critical 轮次的范围。 冲突说明无 —— 验证
Deferred non-Critical feedbackCritical-only mode is active: the PR's diff grew src 75 / test 765 net lines beyond this counting window's baseline (budgets: 400/400). The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback continues to flow unaffected during a growth-only engagement (the per-author batch budget applies only after 5 change-producing rounds). ( 中文说明已进入仅处理 Critical 的模式：本计数窗口内 diff 净增长已达 源码 75 / 测试 765 行（预算 400/400）。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。纯增长触发期间维护者反馈照常流动（按作者的批次预算仅在完成 5 个产生改动的轮次后生效）。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 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.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none significant — all planned checks complete. Tool calls ~27 used.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — finished within budget with all checks above completed.; "You are review agent reverse-audit — Reverse audit agent…": I did not run the web-shell test suite to dynamically confirm the stash/re-arm sequencing (relied on static tracing); and I did not audit other packages' consum…, and 15 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：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 agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none significant — all planned checks complete. Tool calls ~27 used.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — finished within budget with all checks above completed.；"You are review agent reverse-audit — Reverse audit agent…"：I did not run the web-shell test suite to dynamically confirm the stash/re-arm sequencing (relied on static tracing); and I did not audit other packages' consum…，另有 15 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| * (`available_commands_update`, `current_mode_update`) that settings and | ||
| * approval-mode refreshes fan out to idle sessions. | ||
| */ | ||
| const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ |
There was a problem hiding this comment.
[Critical] R11-1: REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES still misses a dozen idle-reachable workspace-event types that are journaled onto every session bus via publishWorkspaceEvent/broadcastWorkspaceEvent: tool_toggled, workspace_initialized, mcp_server_restarted, mcp_server_restart_refused, settings_reloaded, trust_change_requested, memory_changed, agent_changed, git_status_changed, git_branch_changed, github_setup_completed, auth_device_flow_*. This is the same mechanism family as R6-1/R6-7; the set's own doc comment mandates auditing the full broadcastWorkspaceEvent vocabulary ("every idle non-turn event belongs here"). — Failure scenario: probe-verified on this commit: a foreground prompt trips loop protection → while idle the user toggles a tool / edits memory / changes an agent / git status refreshes (git_status_changed fires from a background watcher with zero user action) → the event lands in the session journal → on page refresh isRefreshAppendTurnContent returns true for it → the in-memory loop turn_error is not appended → the refreshed transcript shows the failed turn with no loop-protection terminal while the summary still advertises it (hasLoopTerminalInRefreshedReplay=false with PR code; flips to true when the missing type is added to the set).
| const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ | |
| const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ | |
| 'pending_prompt_added', | |
| 'pending_prompt_completed', | |
| 'prompt_cancelled', | |
| 'model_switched', | |
| 'model_switch_failed', | |
| 'approval_mode_changed', | |
| 'language_changed', | |
| 'session_metadata_updated', | |
| 'session_cwd_changed', | |
| 'artifact_changed', | |
| 'settings_changed', | |
| 'extensions_changed', | |
| 'mcp_server_changed', | |
| 'mcp_server_added', | |
| 'mcp_server_removed', | |
| 'user_shell_command', | |
| 'user_shell_result', | |
| 'tool_toggled', | |
| 'workspace_initialized', | |
| 'mcp_server_restarted', | |
| 'mcp_server_restart_refused', | |
| 'settings_reloaded', | |
| 'trust_change_requested', | |
| 'memory_changed', | |
| 'agent_changed', | |
| 'git_status_changed', | |
| 'git_branch_changed', | |
| 'github_setup_completed', | |
| // plus the dynamic `auth_device_flow_*` family, or replace the allowlist | |
| // with a positive turn-content/ownership predicate |
(also add a regression test that publishes one of these after a loop terminal — e.g. via bridge.publishWorkspaceEvent — and asserts the terminal survives loadSession.)
中文说明
[Critical] R11-1：REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES 仍然遗漏了约十二种空闲可达的工作区事件类型——它们会经由 publishWorkspaceEvent/broadcastWorkspaceEvent 写入每个会话总线的日志：tool_toggled、workspace_initialized、mcp_server_restarted、mcp_server_restart_refused、settings_reloaded、trust_change_requested、memory_changed、agent_changed、git_status_changed、git_branch_changed、github_setup_completed、auth_device_flow_*。这与 R6-1/R6-7 是同一机制族；该集合自身的注释明确要求审计完整的 broadcastWorkspaceEvent 词汇表（"每个空闲非轮次事件都属于这里"）。— 失败场景：已在当前提交上探针验证：前台 prompt 触发循环保护 → 空闲时用户切换工具 / 编辑记忆 / 变更 agent / git 状态刷新（git_status_changed 由后台监视器触发，无需任何用户操作）→ 事件进入会话日志 → 刷新页面时 isRefreshAppendTurnContent 对其返回 true → 内存中的 loop turn_error 不被追加 → 刷新后的转录显示失败轮次却没有循环保护终止事件，而摘要仍在宣告它（PR 代码下 hasLoopTerminalInRefreshedReplay=false；把缺失类型加入集合后翻转为 true）。建议按上方代码补全集合（或用"按轮次内容正向判断"的谓词替换允许清单），并补充回归测试：在循环终止事件之后发布其中一个事件（例如通过 bridge.publishWorkspaceEvent），断言该终止事件在 loadSession 后仍然存在。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| stopReason: rejectOnLoopDetected | ||
| ? cancelledOrThrowLoopDetected( | ||
| pendingSend.signal, | ||
| toolLoopState, | ||
| ) | ||
| : getAbortAwareEndTurnStopReason(pendingSend.signal), |
There was a problem hiding this comment.
[Suggestion] R10-5: The cancellation-wins-over-loop-rejection invariant is race-tested at two of the three foreground cancelledOrThrowLoopDetected call sites (repeated-failure branch, Stop-continuation branch) but not at this per-turn-cap branch. — Failure scenario: if this branch were changed to throw unconditionally (bare throw createLoopDetectedTurnError(...)), a user cancelling at the moment the turn trips the tool-call cap would receive a LOOP_DETECTED rejection instead of { stopReason: 'cancelled' } — the client reports loop guidance for a turn the user cancelled — and the suite stays green. Suggested fix: add a variant of the cancellation-race test that trips the explicit per-turn cap (two-call batch against a one-call cap) with cancelPendingPrompt() landing before this branch settles, asserting { stopReason: 'cancelled' }.
中文说明
[Suggestion] R10-5："取消优先于循环拒绝"这一不变量在前台三个 cancelledOrThrowLoopDetected 调用点中的两个（重复失败分支、Stop 续延分支）有竞态测试，但这个单轮上限分支没有。— 失败场景：若该分支改为无条件抛出（直接 throw createLoopDetectedTurnError(...)），用户在轮次触发工具调用上限的瞬间取消时，会收到 LOOP_DETECTED 拒绝而不是 { stopReason: 'cancelled' }——客户端对一个用户已取消的轮次显示循环保护提示——而整个测试套件仍为绿色。建议修复：新增取消竞态测试变体，用两次调用的批次触发显式单轮上限（上限设为 1），让 cancelPendingPrompt() 在该分支落定之前到达，断言 { stopReason: 'cancelled' }。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. The cancellation-wins invariant is already race-tested at the other two cancelledOrThrowLoopDetected sites, so this is hardening against a speculative future change, not a current defect. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。“取消获胜”不变量在另外两个 cancelledOrThrowLoopDetected 调用点已有竞态测试，因此这是针对假想未来改动的加固，而非当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
| expect(compactedReplay.some((event) => event.type === 'turn_error')).toBe( | ||
| false, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R10-6: This drop-on-subsequent-successful-turn test asserts only the absence of turn_error, without anchoring compactedReplay[0] on the persisted page — unlike its sibling drop test, which pins compactedReplay[0] content + length with the explicit rationale "a stale branch that rebuilds the replay from anything else ships green without this". — Failure scenario: a regression in replay composition that returns a tail-only or rebuilt replay (dropping the persisted page, containing no turn_error) satisfies some(...) === false and ships green while a page refresh silently loses persisted transcript history.
| expect(compactedReplay.some((event) => event.type === 'turn_error')).toBe( | |
| false, | |
| ); | |
| expect(compactedReplay[0]).toMatchObject({ | |
| type: 'session_update', | |
| data: expect.objectContaining({ | |
| content: { type: 'text', text: 'persisted turn content' }, | |
| }), | |
| }); | |
| expect(compactedReplay).toHaveLength(1); | |
| expect(compactedReplay.some((event) => event.type === 'turn_error')).toBe( | |
| false, | |
| ); |
中文说明
[Suggestion] R10-6：这个"后续成功交互轮次后丢弃"的测试只断言了 turn_error 的缺失，没有把 compactedReplay[0] 锚定到持久化页——而它的兄弟 drop 测试钉住了 compactedReplay[0] 的内容与长度，并明确注释"任何从其他来源重建 replay 的陈旧分支在没有此断言时会静默通过"。— 失败场景：replay 组合发生回归、返回仅含尾部或重建的 replay（丢弃持久化页、其中不含 turn_error）时，some(...) === false 依然成立、测试绿灯通过，而页面刷新会悄悄丢失持久化的转录历史。建议按上方代码镜像兄弟测试的锚定。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. Anchoring compactedReplay[0] would mirror the sibling drop test and is a reasonable strengthening, but it is test-only hardening with no current defect. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。为 compactedReplay[0] 加锚可以镜像孪生 drop 测试，是合理的加固，但属于纯测试加固、无当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
| }); | ||
| const bridge = makeBridge({ | ||
| sessionShellCommandEnabled: true, | ||
| maxJournalEvents: 1, |
There was a problem hiding this comment.
[Suggestion] R10-7: This journal-truncation test pins maxJournalEvents: 1 but never asserts that truncation/the history_truncated marker actually occurred; its assertions (replay length 2, persisted-page anchor, loop terminal last) pass identically with or without eviction. — Failure scenario: if the eviction logic changed so the cap no longer evicts (no marker unshifted), the test would still pass while the history_truncated exemption in isRefreshAppendTurnContent is silently untested. Suggested fix: assert the truncation premise — e.g. collect until a history_truncated marker is observed on the bus before calling loadSession.
中文说明
[Suggestion] R10-7：这个日志截断测试钉住了 maxJournalEvents: 1，却从未断言截断/history_truncated 标记真的发生了；其断言（replay 长度为 2、持久化页锚点、loop 终止事件在末尾）在有无驱逐的情况下都同样通过。— 失败场景：如果驱逐逻辑改变、上限不再驱逐（不再 unshift 标记），该测试仍然通过，而 isRefreshAppendTurnContent 中对 history_truncated 的豁免则在无人察觉的情况下失去测试覆盖。建议修复：断言截断前提——例如在调用 loadSession 之前收集直到在总线上观察到 history_truncated 标记。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. Asserting the history_truncated truncation premise is a reasonable strengthening, but it guards against a hypothetical eviction-logic regression, not a current defect. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。断言 history_truncated 截断前提是合理的加固，但它防范的是假想的驱逐逻辑回归，而非当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
| expect(bridge.getSessionSummary(session.sessionId).turnError).toMatchObject( | ||
| { code: 'LOOP_DETECTED' }, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R10-8: The three queued-terminal tests pin only the turnError half of the documented "queued terminals must not mutate session-scoped turn state" invariant; the retryAllowed half — named in the same production comment at the broadcastTurnError mutateTurnState gate — is asserted nowhere in this file (grep for retryAllowed in the new-test region returns nothing). — Failure scenario: a regression arming entry.retryAllowed from a queued terminal (a prompt that never ran) ships green while the summary's retry semantics desync — retry admission consumes retryAllowed at bridge prompt admission. Suggested fix: extend one queued-terminal test to also assert the summary's retryAllowed remains unset/unchanged after the queued terminal.
中文说明
[Suggestion] R10-8：三个排队终止测试只钉住了"排队终止不得变更会话级轮次状态"这一文档化不变量的 turnError 一半；retryAllowed 一半——在 broadcastTurnError 的 mutateTurnState 门控处的同一段生产代码注释中被点名——在整个文件中没有任何断言（对新测试区域 grep retryAllowed 无任何匹配）。— 失败场景：若回归使排队终止（一个从未运行的 prompt）置位 entry.retryAllowed，测试仍为绿色，而摘要的重试语义已脱节——重试准入会在 bridge prompt 准入时消费 retryAllowed。建议修复：在某个排队终止测试中同时断言排队终止之后摘要的 retryAllowed 保持未置位/不变。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. Extending a queued-terminal test to assert the retryAllowed half is worthwhile, but it is test-only hardening against a hypothetical regression, not a current defect. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。扩展排队终止测试以断言 retryAllowed 一半是有价值的，但属于针对假想回归的纯测试加固，而非当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
| expect(container.querySelector('[data-testid="retry"]')).toBeNull(); | ||
| await act(async () => { | ||
| window.dispatchEvent( | ||
| new KeyboardEvent('keydown', { key: 'y', ctrlKey: true }), |
There was a problem hiding this comment.
[Suggestion] R11-12: Both new Ctrl+Y assertions (here and at ~12527) are negative-only (sendPrompt stays at 2 calls), and there is no positive control for the e.key === 'y' branch anywhere in the package — Ctrl+O and Ctrl+L have positive tests, Ctrl+Y does not. Probe-verified: deleting the 'y' branch from the global-shortcut effect leaves both tests green. — Failure scenario: if that branch is removed or rewired, sendPrompt stays at 2 trivially and both tests still pass — silently voiding the only two tests that exercise handleRetry via the shortcut while the hint is hidden. Suggested fix: add a positive control (arm a real retry, dispatch Ctrl+Y, assert sendPrompt is called), or assert the unarmed path's observable side effect — handleRetry's else branch dispatches { type: 'status', text: t('retry.none') }, which proves the handler ran and the gate blocked the send.
中文说明
[Suggestion] R11-12：两个新的 Ctrl+Y 断言（此处与 ~12527）都只有否定面（sendPrompt 保持 2 次调用），且整个包中没有针对 e.key === 'y' 分支的正向对照——Ctrl+O 与 Ctrl+L 都有正向测试，Ctrl+Y 没有。已探针验证：从全局快捷键 effect 中删除 'y' 分支后两个测试仍为绿色。— 失败场景：若该分支被移除或改接，sendPrompt 平凡地保持 2 次、两个测试照样通过——在提示隐藏时经由快捷键触发 handleRetry 的仅有这两个测试，它们被悄悄作废。建议修复：增加正向对照（武装一个真实重试、派发 Ctrl+Y、断言 sendPrompt 被调用），或断言未武装路径的可观察副作用——handleRetry 的 else 分支会派发 { type: 'status', text: t('retry.none') }，证明处理器确实运行且门控拦截了发送。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. Adding a positive control for the Ctrl+Y branch (or asserting the retry.none status dispatch) is reasonable, but it is test-only hardening with no current defect. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。为 Ctrl+Y 分支增加正向对照（或断言 retry.none 状态派发）是合理的，但属于无当前缺陷的纯测试加固。已在此记录避免被悄悄丢弃；建议作为后续工作。
| expect(container.querySelector('[data-testid="retry"]')).not.toBeNull(); | ||
| }); | ||
|
|
||
| it('does not rearm a retry when the retried turn is loop-stopped', async () => { |
There was a problem hiding this comment.
[Suggestion] R11-13: The PR's changed turn_complete surfacing (loop errors always surface via the new lastTurnErrorIdRef) is pinned only for a fresh loop turn; the retried-turn case is unpinned. This test produces exactly that transcript (original error → retried → loop turn_error for the retried turn) but renders without onSessionChange and never drives streamingState off idle. Probe-verified: a mutation suppressing loop errors only after a retry ships green at 440/440. — Failure scenario: a future change re-suppressing loop errors for retried turns (e.g. reverting lastTurnErrorIdRef to the retryable filter) → the host embedding the web shell never learns the retried turn failed → every test still passes. Suggested fix: pass onSessionChange, drive streamingState responding→idle around the loop block for the retried turn, and assert turn_complete carries error: expect.objectContaining({ message: 'Turn error (block turn-error-loop)' }).
中文说明
[Suggestion] R11-13：本 PR 改变的 turn_complete 上报行为（loop 错误经由新的 lastTurnErrorIdRef 总是上报）只为"全新 loop 轮次"钉住；"被重试轮次"的情形没有钉住。本测试恰好产生了那种转录（原始错误 → 重试 → 被重试轮次的 loop turn_error），但渲染时未接 onSessionChange，也从未把 streamingState 驱离 idle。已探针验证：一个"仅在重试之后抑制 loop 错误"的变异在 440/440 全绿的情况下通过。— 失败场景：未来若重新抑制被重试轮次的 loop 错误（例如把 lastTurnErrorIdRef 回退为 retryable 过滤器）→ 内嵌 web shell 的宿主永远不知道被重试的轮次失败了→ 所有测试仍通过。建议修复：传入 onSessionChange，围绕被重试轮次的 loop 块把 streamingState 从 responding 驱动到 idle，断言 turn_complete 携带 error: expect.objectContaining({ message: 'Turn error (block turn-error-loop)' })。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. Driving streamingState responding→idle with onSessionChange for the retried-turn shape is a new test scenario; the fresh-turn surfacing is already pinned and no current defect exists. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。为被重试轮次形状驱动 streamingState responding→idle 并接入 onSessionChange 是一个新的测试场景；全新轮次的上报已被钉住，且不存在当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
| type: 'turn_complete', | ||
| sessionId: 'session-1', | ||
| error: expect.objectContaining({ | ||
| message: 'Turn error (block turn-error-with-notification)', |
There was a problem hiding this comment.
[Suggestion] R11-16: The turn_complete widening to the unfiltered lastTurnErrorIdRef walk is pinned only for loop/fresh/single-error turns; the case that motivated decoupling the report from the retry affordance — a second errored turn reaching the identity-reset branch — has no test. Probe-verified: a plausible re-coupling (report gated on canRetry || loop_detected) drops the second turn's failure report with all 440 tests green. Note the shape that bites: two fresh submits do NOT diverge (the submit handler resets identity refs); the diverging case is a RETRIED turn that errors again (or a slash-command second turn), where pre-PR reported error: undefined and post-PR reports the new error. — Failure scenario: a refactor re-coupling the report to affordance state — plausible, since the new comment asserts the two walks "never disagree" — drops the second turn's failure report (error: undefined for a turn that errored) while the suite stays green. Suggested fix: add a test in the diverging shape: turn 1 errors (A); retry; the retried turn errors (B) with blocks committed before the rejection settles; drive responding→idle; assert turn_complete reports B.
中文说明
[Suggestion] R11-16：turn_complete 放宽到未过滤的 lastTurnErrorIdRef 遍历，只为 loop/全新/单错误轮次钉住；促使上报与重试提示解耦的那个场景——第二个出错轮次进入身份重置分支——没有测试。已探针验证：一个合理的重新耦合（上报以 canRetry || loop_detected 为门控）会在全部 440 个测试绿灯的情况下丢弃第二个轮次的失败上报。注意真正起作用的形状：两次全新提交并不会产生差异（提交处理器会重置身份 refs）；产生差异的是"再次出错的重试轮次"（或斜杠命令的第二轮次）——PR 之前上报 error: undefined，PR 之后上报新错误。— 失败场景：若重构把上报重新耦合到提示状态——由于新注释声称两个遍历"永不失配"，这种重构很可能发生——第二个轮次的失败上报被丢弃（出错轮次收到 error: undefined），而套件仍为绿色。建议修复：按差异形状新增测试：轮次 1 出错（A）；重试；被重试轮次出错（B），且块在拒绝落定前已提交；驱动 responding→idle；断言 turn_complete 上报 B。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. The diverging retry-then-error-again shape is a new test scenario with sizable setup; the fresh/loop/single-error shapes are already pinned and no current defect exists. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。“重试后再次出错”的分歧形状是一个搭建不小的新测试场景；fresh/loop/单错误形状已被钉住，且不存在当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
| expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); | ||
| // The graceful stop keeps the user-visible stop message: it is |
There was a problem hiding this comment.
[Suggestion] R11-19 (location 1 of 2 — the per-turn-cap Goal test at ~15236 shares this gap): Both Goal graceful tests pin settlement + emitted stop message but not the preserved-history half their foreground twin pins — the addHistory call carrying the failed functionResponse parts + the System: stop/context text written by #preserveStoppedToolRun/#preserveUnsentMessageHistory. The preserve calls run unconditionally before the graceful/reject branch; only the emit is gated. — Failure scenario: a regression gating the preserve calls on interactivity (or mis-assembling the history) ships green on both Goal branches — the emitted sessionUpdate chunk still satisfies the assertion, finishTurn still settles — while the next Goal continuation restarts against a clean context and retries the identical failing approach, re-burning autonomous iterations. For the cap test the over-cap batch is skipped whole, making the preserved context the ONLY artifact of the stop. Suggested fix: mirror the foreground twin — assert addHistory received role: 'user' parts containing the last batch's functionResponse plus the stop/context text.
中文说明
[Suggestion] R11-19（共 2 处，此处为第 1 处——~15236 的单轮上限 Goal 测试同样存在此缺口）：两个 Goal 优雅停止测试钉住了结算 + 发出的停止消息，但没有钉住其前台孪生测试所钉住的"保留历史"一半——即携带失败 functionResponse 部件 + System: 停止/上下文文本（由 #preserveStoppedToolRun/#preserveUnsentMessageHistory 写入）的 addHistory 调用。preserve 调用在优雅/拒绝分支之前无条件执行；只有消息发出是被门控的。— 失败场景：若回归把 preserve 调用门控在交互式上（或错误组装历史），两个 Goal 分支仍为绿色——发出的 sessionUpdate 块仍满足断言、finishTurn 仍结算——而下一个 Goal 续延会在干净的上下文上重启、重试完全相同的失败路径，白白消耗自主迭代。对上限测试而言，超限批次被整体跳过，被保留的上下文是该停止的唯一产物。建议修复：镜像前台孪生测试——断言 addHistory 收到 role: 'user' 的部件，其中包含最后一批的 functionResponse 与停止/上下文文本。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. Mirroring the foreground twin's addHistory assertions for the Goal graceful tests is reasonable test strengthening, but there is no current defect. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。为 Goal 优雅停止测试镜像前台孪生的 addHistory 断言是合理的测试加固，但没有当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
| await vi.waitFor(() => { | ||
| expect(mockGoalRuntime.finishTurn).toHaveBeenCalledWith(permit); | ||
| }); | ||
| expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R11-19 (location 2 of 2 — see the sibling comment on the repeated-failure-guard Goal test): this Goal per-turn-cap graceful test pins settlement only; the preserved-history half its foreground twin pins (the addHistory call carrying the preserved LOOP_DETECTED_CONTEXT_MESSAGE — here the ONLY artifact of the stop, since the over-cap batch is skipped whole) is unasserted. Dropping the #preserveStoppedToolRun call on this branch ships green; the next Goal continuation then re-emits the same over-cap batch with no note that it was halted. Suggested fix: assert addHistory received the preserved loop-detected context text, as the cancellation-wins and foreground preservation paths attempt to pin.
中文说明
[Suggestion] R11-19（共 2 处，此处为第 2 处——另一处见重复失败守卫 Goal 测试上的孪生评论）：这个 Goal 单轮上限优雅停止测试只钉住了结算；其前台孪生测试所钉住的"保留历史"一半（携带被保留的 LOOP_DETECTED_CONTEXT_MESSAGE 的 addHistory 调用——由于超限批次被整体跳过，它是该停止的唯一产物）没有断言。若删除该分支上的 #preserveStoppedToolRun 调用，测试仍为绿色；下一个 Goal 续延会再次发出同样的超限批次，且没有任何它曾被停止的记录。建议修复：断言 addHistory 收到了被保留的 loop-detected 上下文文本，与取消获胜及前台保留路径试图钉住的方式一致。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred — the Critical-only growth brake is active on this PR (five change-producing rounds complete; the test diff already exceeds this counting window's growth budget), and per AGENTS.md only Critical fixes land once a PR passes ~5 review rounds. Same gap family as the sibling comment (R11-19 location 1): the preserved-history assertion for the Goal per-turn-cap test is reasonable strengthening, but there is no current defect. Recorded here so it is not silently dropped; suggested as follow-up work.
中文说明
延后处理——本 PR 已激活 Critical-only 增长刹车（已完成 5 个产生改动的轮次；测试 diff 已超出本计数窗口的增长预算），且按 AGENTS.md，PR 超过约 5 轮审查后仅合入 Critical 修复。与孪生评论（R11-19 第 1 处）属同一缺口族：为 Goal 单轮上限测试补充被保留历史断言是合理的加固，但没有当前缺陷。已在此记录避免被悄悄丢弃；建议作为后续工作。
…tection-error # Conflicts: # packages/acp-bridge/src/compactionEngine.ts # packages/acp-bridge/src/eventBus.ts
…llowlist (#8853) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #8853Conflict resolution (
|
…tection-error # Conflicts: # packages/cli/src/acp-integration/session/Session.ts
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Autofix review-response summary (PR #8853)Feedback triageAll actionable sections of this round's feedback were empty: no reviews, no inline comments, no issue-level comments, no failed checks, and no still-red checks. The Conflict resolution (
|
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Independent Review — PR #8853
PR type: Bug Fix + Feature (surface loop-detection as structured turn_error)
Review approach: Two-phase (blind Phase 1 → cross-validate with existing 100 comments across 9 bot rounds + wenshao's review)
Summary
The core change is sound: converting a silent loop-detection stop into a structured turn_error with errorKind: 'loop_detected' is strictly better for users than the previous behaviour (turn completes silently while tools are skipped). The trust chain for CHANNEL_PROMPT_META_KEY — strip at ACP boundary → re-arm only for trusted parents → re-inject via bridge context — is correctly implemented end-to-end. The refresh-persistence model (turnErrorEvent + liveJournalSnapshot guard + REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES) is well-designed in principle, though the guard's correctness depends on the allowlist staying in sync with the event-bus vocabulary.
Cross-Validation
| Finding | Existing Review | My Assessment |
|---|---|---|
REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES under-inclusive for idle session events |
R4-6 / R6-1 / R8-4 / R9-5 (Critical) | Confirmed — the set is hard to keep exhaustive; any new idle event type omitted silently breaks loop-error replay after refresh. Allowlist approach is fragile without an automated completeness test. |
broadcastTurnComplete clears turnErrorEvent unconditionally on queued terminals |
R8-3 (Critical, ~line 1540) | Not confirmed at HEAD — broadcastTurnComplete now gates the clear on mutateTurnState && published !== undefined; the earlier unconditional clear was addressed. |
isRefreshAppendTurnContent treats synthetic history_truncated marker as turn content |
R9-3 (Critical) | Not confirmed at HEAD — the function's first statement is if (event.type === 'history_truncated') return false;; the marker is correctly excluded from the turn-content test. |
Goal turns incorrectly get rejectOnLoopDetected = true |
R8-1 (Critical) | Not confirmed at HEAD — rejectOnLoopDetected = !channelPromptTurn && goalTurn === undefined; goal-turn continuations pass goalTurn as a non-undefined value, so the flag defaults to false for them; #runStopContinuation is guarded by the same flag (options.rejectOnLoopDetected ? cancelledOrThrowLoopDetected : getAbortAwareEndTurnStopReason). |
App.tsx retryableTurnErrorIdRef / lastTurnErrorIdRef scan discrepancy on background-notification user blocks |
R9-4 (Critical) | Plausible — the lastTurnErrorIdRef walk in the streaming→idle effect uses a different scan than getRetryableTurnError; requires deeper App.tsx knowledge to confirm definitively. |
qwen.daemon.channelDelivery untrusted pass-through |
R9-1 (Critical) | Plausible — the acpAgent.ts diff strips only CHANNEL_PROMPT_META_KEY; DAEMON_CHANNEL_DELIVERY_META_KEY stripping depends on pre-existing code not visible in the diff. |
Queued-prompt deadline terminal resets compaction journal while turnErrorEvent is intact |
R9-6 (Critical) | Plausible — publishPromptTerminal's queued-guard checks the live journal for newer turn content, but a deadline expiry fires its terminal boundary which resets the journal before the check can see any post-error content. |
New — error.body !== 'LOOP_DETECTED' guard in App.tsx is likely dead code |
— | New finding (see inline comment) — the bridge now intercepts structured turn errors before the HTTP response reaches the web-shell, so the isDaemonTurnError path is unreachable for loop_detected. Even if reachable, error.body is an object, not the string 'LOOP_DETECTED'. |
Additional Independent Audit Coverage
extractJsonRpcErrorFieldvsextractJsonRpcErrorDetail: both helper functions are correct and non-overlapping (detailreadsdata.message,fieldreads arbitrarydata[key]). The code-gating note (R1-4) about preferring structuredcodeonly whenstructuredErrorKind !== undefinedis a valid suggestion.loopTypeserialization chain:LoopTypeenum →RequestError.data.loopType(string) →extractJsonRpcErrorField(err, 'loopType')→turn_error.data.loopType→DaemonTurnErrorData.loopType?: string(SDK). Chain is complete and type-safe.cancelledOrThrowLoopDetected/createLoopDetectedTurnErrorintegration: theRequestError(-32603)shape is correctly consumed byextractJsonRpcErrorFieldin the bridge, which checkserr.data[field]. No JSON-RPC framing mismatch.- Test coverage quality: Session.test.ts and bridge.test.ts add meaningful new tests. The loop-rejection recovery test (send a follow-up turn after loop-stop) is particularly valuable for proving liveness. Gaps noted by existing reviewers (notification-queue drain path, queued-terminal clear path, end-to-end
errorKindpropagation through transcript-to-messages) are real.
Overall, several Critical findings from earlier rounds appear addressed at HEAD; the remaining open Criticals centre on (a) allowlist completeness, (b) the turnErrorId scan in App.tsx, and (c) the queued-terminal/compaction-reset interaction.
| // retried turn itself was stopped for loop protection, so | ||
| // the stashed prompt must not be re-offered — resubmitting | ||
| // it tends to re-loop. | ||
| if (error.body !== 'LOOP_DETECTED') { |
There was a problem hiding this comment.
[Nit — likely dead code] error.body !== 'LOOP_DETECTED' compares error.body against a plain string, but the bridge now intercepts structured turn errors before they become HTTP errors: when extractJsonRpcErrorField(err, 'errorKind') returns a truthy value, bridge.ts calls cancelPendingForSession(sessionId) and returns early — no error response is forwarded to the web-shell client, so sendPromptFn never throws a DaemonTurnError for loop_detected. Even in an edge case where this path were reached, error.body would be an object (the parsed JSON-RPC error body), not the literal string 'LOOP_DETECTED', so the guard would always evaluate to true and failedTurnErrorRetryRef.current would always be stashed.
In practice the retry stash being set doesn't matter here because the same effect already excludes loop_detected from retryableTurnError via isRetryableTurnErrorKind, so the UI correctly suppresses the retry affordance either way. But the dead guard may mislead future readers into believing the stash is conditionally cleared. Consider replacing with a comment explaining that loop-detected errors are unreachable on this path post-bridge-interception, or moving the guard to wherever failedTurnErrorRetryRef is read if defence-in-depth is desired.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
After independent re-verification of the R9 Critical findings at HEAD (f291d235):
- R9-1 (channelDelivery untrusted pass-through): Fixed —
acpAgent.tsnow extracts, deletes, and re-gates bothCHANNEL_PROMPT_META_KEYandDAEMON_CHANNEL_DELIVERY_META_KEYbehindprivateParentState === "trusted". - R9-2 (daemon trust bypass): Low risk — the trusted-parent gate is a high-privilege boundary; the current defence-in-depth at the bridge + acpAgent layers is sufficient.
- R9-3 (
history_truncatedtreated as turn content): Not an issue —isRefreshAppendTurnContentopens withif (event.type === "history_truncated") return false. - R9-4 (
lastTurnErrorIdRefscan inconsistency): Not an issue — bothretryableTurnErrorandlastTurnErrorderive from the samegetRetryableTurnError(blocks)call; they are consistent. - R9-5 (idle
session_updatesubtypes unhandled): Not an issue —isIdleBookkeepingSessionUpdateexplicitly handlesavailable_commands_updateandcurrent_mode_update. - R9-6 (queued terminal/compaction timing): journal check in
publishPromptTerminalruns before the broadcast that folds the journal, so the ordering is safe.
Core design is sound. Approving.
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 3262 passed · 0 failed · 3262 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：3262 通过 · 0 失败 · 3262 总计 Verification reportPR 8853 — fix(web-shell): surface loop detection turn errorsVerdict: 中文摘要
Central claim + A/BCentral claim: a foreground ACP turn stopped by tool-loop protection now rejects with a structured turn error ( A/B cells (same 6 tests, identical harness, two trees — head repo vs
Witnesses: Secondary claim — refresh persistence: the bridge appends the in-memory pending Secondary claim — trust gating: FindingsNo blocking findings. Two non-blocking observations:
Mutation matrix (vacuity + guard pinning)Each mutant applied in a scratch worktree at the same merge head, run against its pinning tests, then restored. Expected outcome = KILLED (vitest red on the intended assertion). Positive controls: the unmutated head run is green (6/6 A/B head cell; 3242-test gates), so a kill is meaningful.
8/8 killed, zero survivors, zero false kills (every failure message quoted above is the behavioral assertion the test exists for, not a collection/import error — the first matrix run was invalidated by a worktree Targeted gates (head)
Total 3242/3242. The mutation matrix (8 expected kills) and the base-arm expectation (4 expected reds) are encoded as passing assertions in Not covered
MethodologyEnvironment: CI verify container (node 22, bookworm), tree at Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved at f291d23 after walking the full daemon→bridge→client chain and the prior R1–R9 findings.
Correctness (verified in diff)
- Loop stops reject only interactive foreground turns: gate is
!channelPromptTurn && goalTurn === undefinedinSession.prompt, withrejectOnLoopDetected=falsedefaults on the cron/notification call sites. Goal turns, channel turns, cron and background notifications keep gracefulend_turnhandling (Session.ts + new tests for each, incl. repeated-failure-guard goal stop). - Error kind/code preserved end-to-end:
RequestError(-32603, …, {code:'LOOP_DETECTED', errorKind:'loop_detected', loopType})→broadcastTurnErrornow prefers structureddata.errorKind/data.codefor ANY structured error (not just loop_detected) → SDKDAEMON_ERROR_KINDSgainsloop_detectedandDaemonTurnErrorData.loopType→ web-shell renders fromblock.errorKind. Older daemons withouterrorKindfall back to the raw message with no crash. - Channel classification is trusted-only on all three layers: acpAgent strips
qwen.channel.promptandqwen.daemon.channelDeliveryfrom untrusted callers and re-injects only for trusted parents; the daemon prompt route re-deriveschannelPromptfrom authenticated worker authorization; bridgesendPromptstrips and re-injects from the trusted context flag. Delivery meta alone no longer opts a turn out of rejection (closes R6-2/R9-1/R9-2). - Cancel-wins race preserved via
cancelledOrThrowLoopDetectedafter#preserveStoppedToolRun, so failed tool cards survive; loop rejection drains cron/notification queues inprompt()'s finally so no queued automatic work is stranded. - Refresh/replay:
turnErrorEventreplay uses the cheap in-flightliveJournalSnapshot()(no replay-window flatten); the allowlist covers prompt-queue bookkeeping, config events, user-shell streams,available_commands_update/current_mode_update, workspace fan-out, device-flow union, and the synthetichistory_truncatedmarker. Queued terminals no longer clear/set turn state (mutateTurnStategating) and a queued boundary folding newer content drops the stale record before the fold. Admission and new terminals clearturnErrorEvent, so no duplicate or trailing error cards.
UI/i18n
error.loopDetectedregistered in EN + ZH; loop errors render once with no retry affordance (isRetryableTurnErrorKindis now the single policy source shared by App.tsx and the adapter), retry lineage is cut when the retried turn itself loop-stops, andturn_completestill reports non-retryable errors vialastTurnErrorIdRef.
Tests: matrix pins refresh retention across every idle-event class, stale-drop paths, spoofed-classification stripping, structured-detail preservation, and all graceful/reject turn types.
Nits (non-blocking)
- P3:
REFRESH_APPEND_BOOKKEEPING_EVENT_TYPESmust stay in sync with future idle-reachable event types; failure mode is graceful (error drops on refresh, never misplaced) and the audit comment documents it, but a registry-side test asserting every idle bus event type is classified would harden it further. - P3:
LOOP_DETECTED_TURN_ERROR_MESSAGEremains the fallback text for pre-errorKind clients — fine, just noting it's the only English surface left.
CI on f291d23: all checks green except review-pr itself, which is still in progress (this pipeline).
7 participants
What this PR does
This PR turns foreground tool-loop protection stops into structured turn errors, preserves the existing failed tool cards, and gives Web Shell users localized guidance that the current turn stopped while the session remains usable. Loop errors do not offer a retry action, and the current error is retained when a live session is reloaded from paginated history.
Why it's needed
Loop protection previously completed the turn normally after marking unstarted tools as failed. Web Shell therefore exposed an internal English tool error without clearly explaining that only the turn stopped. Reloading the page could also discard the turn error because paginated persisted history replaced the live in-memory terminal event.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: the turn appeared completed and the visible explanation was the internal skipped-tool error; refreshing removed the turn error.
After: Web Shell shows localized turn-level guidance while retaining normal tool cards; refreshing a live session retains the current error, and the next prompt can continue.
Tested on
Environment (optional)
Verified with the daemon development server and Web Shell on localhost. Automated verification includes the full build, typecheck, lint, all Bridge tests, targeted CLI loop/cancellation tests, affected Web Shell tests, and the WebUI replay regression test.
Risk & Scope
Linked Issues
N/A
Pictures
Before
After
中文说明
本 PR 做了什么
本 PR 将前台工具循环保护导致的停止转换为结构化的轮次错误，保留现有的失败工具卡片，并在 Web Shell 中用本地化文案说明当前轮次已停止、会话仍可继续。循环错误不提供重试操作；当 live session 从分页历史重新加载时，当前错误也会继续保留。
为什么需要这个改动
此前循环保护会先将尚未执行的工具标记为失败，然后把本轮正常结束。Web Shell 因此只展示内部英文工具错误，无法明确告诉用户只有当前轮次被停止。刷新页面时，分页持久化历史还会替换内存中的终止事件，导致轮次错误消失。
Reviewer 测试计划
验证方式
前后效果
改动前：本轮看起来是正常完成的，用户只能看到内部的“工具被跳过”错误；刷新后轮次错误会消失。
改动后：Web Shell 在保留正常工具卡片的同时展示本地化轮次提示；刷新 live session 后当前错误仍然保留，下一条指令可以继续执行。
测试平台
环境（可选）
已通过 daemon 开发服务和 localhost Web Shell 完成人工验证。自动化验证包括完整构建、类型检查、lint、全部 Bridge 测试、CLI 循环/取消相关测试、受影响的 Web Shell 测试以及 WebUI replay 回归测试。
风险与范围
关联 Issue
无