fix(core): per-turn tool-call circuit breaker — always-on cap + opt-in loop heuristics (#5234) - #5279
Conversation
…ing pattern, turn cap
…in CLI The always-on turn tool-call cap logged LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS, which mislabels telemetry — the cap fires on total per-turn volume, not on consecutive identical calls. Add a dedicated LoopType.TURN_TOOL_CALL_CAP and use it in checkTurnToolCallCap (both lastLoopType and the LoopDetectedEvent), with a test asserting the reported loop_type. Also add nonInteractiveCli LOOP_TYPE_LABELS entries for the three new loop types (global duplicate, alternating pattern, turn cap). LOOP_TYPE_LABELS is typed Record<LoopType, string>, so without these the CLI typecheck breaks once core is built, and headless TEXT-mode runs would halt with no reason printed.
|
Re-run after author fix-ups ( Template looks good ✓ — all required headings present, bilingual, reviewer test plan included. On direction: unchanged from the initial triage — runaway tool-call loops are a real reliability problem (#5234), and an always-on circuit breaker is the right shape of fix. Clearly aligned. On approach: the scope remains tight. The two fix-up commits addressed the review findings (wrong headless hint, double-emit, retry handling) without scope creep. Title now correctly distinguishes the always-on cap from the opt-in heuristics. Moving on to code review. 🔍 中文说明作者修复后重跑（ 模板完整 ✓ — 所有必需标题齐全，双语，包含审查测试计划。 方向：与初次 triage 一致——工具调用死循环是真实的可靠性问题（#5234），始终生效的断路器是正确的修复形态。明确对齐。 方案：范围仍然紧凑。两个修复 commit 处理了审查发现（错误的 headless 提示、重复打印、retry 处理），没有范围蔓延。标题现在正确区分了始终生效的上限和 opt-in 启发式。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
Re-run after author fix-ups ( Code ReviewThe two fix-up commits properly addressed the prior findings:
No new critical blockers or AGENTS.md violations found. The code is clean, well-structured, and the fix-ups are minimal and focused. Test ResultsUnit Tests (worktree, PR branch
|
|
Re-run after author fix-ups ( Final ReflectionThe two fix-up commits closed every gap the prior review identified. The code is now in better shape than when it was first approved:
My independent proposal would still look essentially the same. This PR does one thing well, the fix-ups are minimal and focused, and CI is green on all three platforms. Verdict: Approve ✅ 中文说明最终评估两个修复 commit 关闭了此前审查指出的所有缺口。代码现在比初次批准时更好：
我的独立方案本质上仍然相同。这个 PR 只做一件事、做得很好，修复最小且聚焦，CI 三平台全绿。 结论：批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Maintainer verification — real local + tmux E2EVerified Methodology
1. Live E2E results (tmux + mock model)
Example live halt (the always-on cap, with This confirms the core design claims end-to-end:
2. Tests, typecheck, dist harness
3. Code review
Findings (both in the headless halt message; detection logic is correct)🟠 Finding 1 — the halt hint is wrong for
|
| 场景 | 配置 | mock 模式 | 结果（stderr） |
|---|---|---|---|
| 回合上限（始终生效） | skipLoopDetection: true |
101 次工具调用 | ✅ turn_tool_call_cap —— 即使循环检测"已关闭"仍触发 |
| 回合上限（真实多轮） | skipLoopDetection: true |
每次 7 调用 × 15 轮 | ✅ 在第 101 次调用触发 —— 跨轮次累计（mock 看到 toolResults 从 0 涨到 98） |
| 交替模式 | skipLoopDetection: false |
ls /A、ls /B… |
✅ 6 次（A B A B A B）后 alternating_tool_call_pattern |
| 全局重复 | skipLoopDetection: false |
ls /stuck 夹杂唯一 glob |
✅ 第 6 次非连续 /stuck 时 global_tool_call_duplicate |
真机中断示例（始终生效的上限，且 skipLoopDetection 已为 true）：
Loop detection halted the run (turn_tool_call_cap: the model exceeded the maximum number
of tool calls allowed in a single turn). Set the `model.skipLoopDetection` setting to true to disable.
这端到端印证了核心设计：
- 硬上限在
skipLoopDetection门控之前运行、且无法被关闭（flag 设为true时仍触发）。 - 单回合计数器在整个 agentic loop 中累计——续轮用
SendMessageType.ToolResult，非顶层交互，故轮次之间不会调用loopDetector.reset()。真机验证（上限在跨 15 个模型响应、第 101 次调用时触发）。 - 顺带观察：
skipLoopDetection: false时所有检测器都生效，谁先到阈值谁先触发——比如read_file填充会让既有的read_file_loop先于全局重复（阈值 6）触发。符合预期，只是推断真实循环会报哪种类型时要注意。
2. 测试、类型检查、dist 直测
loopDetectionService.test.ts+client.test.ts→ 258 passed (258)，与 PR 一致。tsc --noEmit→ core 与 cli 均干净（即Record<LoopType,string>的LOOP_TYPE_LABELS是穷尽的——少一个标签会在此处报错）。- 针对编译产物的 dist 直测 → 11/11：上限在第 101 次触发（
turn_tool_call_cap）、100 次内不触发、disableForSession()后仍触发、reset()刷新预算；全局重复在第 6 次非连续触发（global_tool_call_duplicate）、对不同 (name,args) 不触发；交替在 ABABAB 触发（alternating_tool_call_pattern）、伙伴键变化时不触发。
3. 代码审查
client.ts顺序正确：checkAlwaysOnSafeties（上限）无条件先于!skipLoopDetection门控的addAndCheckDeterministicToolCallLoop/addAndCheckHeuristicLoops。getLastLoopType()!（client.ts:2134）是安全的——checkTurnToolCallCap在返回true前已设lastLoopType = TURN_TOOL_CALL_CAP（loopDetectionService.ts:618）。CI bot 的提示属风格问题、非 bug。- 始终生效块与门控块的重复（CI bot 提示）可接受；门控块额外做了
CONSECUTIVE_IDENTICAL的 splice。
发现（都在 headless 中断文案上；检测逻辑正确）
🟠 发现 1 —— turn_tool_call_cap 的中断提示是错的
emitLoopDetectedMessage（nonInteractiveCli.ts:120-133）对所有 loop type 追加同一句补救提示：
... Set the `model.skipLoopDetection` setting to true to disable.
但上限的全部意义就是始终生效、skipLoopDetection 关不掉它。真机已证：我用 skipLoopDetection: true 跑、上限照样触发——却让用户去设一个他已经设好的开关。命中上限并照此操作的用户会困惑（甚至以为这个设置坏了）。
建议： 对 TURN_TOOL_CALL_CAP 单独出文案，例如*"这是单回合硬性安全上限（100 次工具调用），无法关闭"*；skipLoopDetection 的提示只对它真正管辖的门控检测器显示。
🟡 发现 2 —— 上限中断会重复打印提示 + 多花一次模型请求
两次回合上限实跑里，中断提示都打印了两次，且上限首次触发后 mock 又多收到一次请求（101-合一：2 次请求；分批：在第 15 次请求触发，mock 又看到第 16 次、toolResults 仍为 98）。两个启发式实跑（每响应 1 个工具调用）则各打印一次并干净停止。也就是说，当上限被一个携带多个工具调用（并行调用——真实场景）的响应触发时，始终生效的提前返回（client.ts:2143 return turn）没有完全短路 non-interactive 的回合循环：又起了一轮、立刻再次触发上限（计数已过 100、续轮不会重置），并经第二处（nonInteractiveCli.ts:1265）再次打印。
不是功能性失败——run 仍会终止（exit 0），不会死循环——但确实多了一条重复的用户可见提示和一次浪费的模型调用。值得看看始终生效中断与主/drain 两处打印点（1042 / 1265）的交互。
结论
检测逻辑正确且覆盖充分——258 单测 + dist 直测 11/11 + 三个检测器真机触发且标签正确、始终生效的上限确实关不掉、跨轮次累计正常。两个发现都在 headless 文案路径、非检测逻辑：发现 1（上限的补救提示错误）面向用户且改动很小，建议随合并一起修；发现 2 是次要的健壮性后续项。
Verified by maintainer wenshao: local build + dist harness + live tmux against a mock OpenAI server. HEAD 5e762af52.
|
问题解决就行，你们自己维护的代码质量好一点，我们使用的时候就不会浪费太多token，我很抠门的没钱充值token，如果不是严重浪费token的行为，另可重新启动实例也不去修复它，这是真实使用者被逼无奈不得不修复的原因 |
There was a problem hiding this comment.
Detection logic and the live E2E verification look right to me — no blockers on the detection side. Left two non-blocking nits inline (both P3).
On the open [Critical] retry thread: the gap is real, but the suggested turnToolCallTotal = 0 over-corrects — the counter is whole-turn cumulative across ToolResult continuations (reset() is gated on isTopLevelInteraction, client.ts:1722), so zeroing on every Retry drops the count from prior completed round-trips, not just the discarded attempt. I left a fuller reply there with the concrete trace and a snapshot-and-roll-back fix that removes only the discarded delta. [P2]
Review on QwenLM#5279 surfaced edge cases in the always-on turn cap: - Retry handling: turnToolCallTotal counted tool calls from failed/retried attempts. Commit the running total on each Finished (round-trip boundary) and roll back to that floor on Retry, so a retry discards only the failed attempt while prior completed round-trips still accumulate (per-turn totals persist across ToolResult continuations). Avoids the naive reset-to-zero that would drop earlier round-trips' counts. - Headless hint: emitLoopDetectedMessage told users to set model.skipLoopDetection even for the cap, which runs before that gate and cannot be disabled by it. Show a cap-specific message instead. - Always-on halt now clears turn.pendingToolCalls so a multi-call response that trips the cap does not execute the remaining calls, spawn a continuation, and re-print the halt message. - Compute the tool-call key once per heuristic event; drop the dead param on checkTurnToolCallCap. - Add a client-level test for the always-on halt path plus tests for the retry rollback and cross-round-trip accumulation.
wenshao
changed the title
|
Addressed the review — pushed
Tests: 262 passing (loopDetection 59 + client 203, +4 new), core Thanks @yiliang114 and the triage bot for the careful review. 🙏 中文已处理 review，推送
测试：262 通过（loopDetection 59 + client 203，新增 4 个），core 感谢 @yiliang114 和 triage bot 的细致 review。🙏 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running. Detection logic looks sound — the turn cap + commit/rollback for Retry is well-designed, and tests pass locally. Three non-blocking suggestions below, plus a small test-coverage nit.
— qwen3.7-max via Qwen Code /review
- addAndCheckHeuristicLoops now clears the new global-duplicate and alternating
counters on Retry, so a retried replay can't inflate them into a false
positive (mirrors the deterministic path's resetToolCallCount; the always-on
cap keeps its own counter accurate via commit/rollback).
- The always-on halt uses the defensive `...(loopType && { value })` spread like
the gated block instead of a non-null assertion on getLastLoopType().
- The always-on integration test now populates turn.pendingToolCalls before the
halt so the clear runs against a non-empty array (confirmed it fails without
the production clear).
- Assert getLastLoopType() for the global-duplicate and alternating detectors
(the getter the client reads), plus a retry-replay regression test.
|
Second review pass addressed — pushed
CI was green on the prior commit and is re-running on 中文第二轮 review 已处理——推送
上一个 commit 的 CI 已全绿， |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running. — qwen3.7-max via Qwen Code /review
|
I did another local verification pass on the latest head Green local checks:
I also ran a tmux headless smoke test against the built The test still reproduced a remaining issue in headless mode. The CLI emits the new
So the core-side Could we make the headless path treat Update: after weighing the scope, I don't think this remaining headless/nonInteractive edge case needs to block this PR. The main fix looks good to move forward with, and this can be tracked as a follow-up. |
| 'the model repeated the same tool call across the turn, even when not back-to-back', | ||
| [LoopType.ALTERNATING_TOOL_CALL_PATTERN]: | ||
| 'the model alternated between the same two tool calls in a repeating pattern', | ||
| [LoopType.TURN_TOOL_CALL_CAP]: |
There was a problem hiding this comment.
The message here is correct, but the headless caller still needs to stop the current run, not just print the message. In the tmux headless smoke test, LoopDetected was emitted, but the already-collected toolCallRequests array still flowed into processToolCallBatch(...), so the first 100 streamed read_file calls were still dispatched before the run halted.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Overall this looks good to me. I ran several local tmux/headless rounds around the tool-call cap behavior; the main path looks fine to move forward with. I left one small headless/nonInteractive edge-case note, but I think it can be followed up separately.
4 participants
What this PR does
Adds an always-on safety layer that halts a turn when its tool calls run away, so a stuck model can no longer loop forever. The core of it is a hard per-turn tool-call cap that runs before the existing
model.skipLoopDetectiongate, which means it cannot be switched off by configuration. Alongside the cap, two new heuristic detectors are added to the existing (opt-in) loop-detection path: a non-consecutive "global duplicate" detector that catches the same(tool, args)call repeating across a turn even when interleaved with other calls, and an "alternating pattern" detector that catches a model flip-flopping between the same two calls (A B A B …). Each detector reports its own telemetry loop type, and the non-interactive (headless) CLI prints a human-readable reason for each when it halts a run.Why it's needed
Tool calls can get stuck in an infinite loop (#5234). The triage of that issue identified the root cause:
model.skipLoopDetectiondefaults totrue, so loop detection is off for most users and the only backstop isMAX_TURNS. This change adds a safety net that does not depend on that opt-in flag — the hard per-turn cap always applies — while the two new heuristics make the opt-in detection smarter about loop shapes the existing consecutive-identical check misses.Reviewer Test Plan
How to verify
This is internal loop-detection logic plus a headless stderr message — not a TUI change — so verification is via unit tests and typecheck rather than a screen recording. From the repo root:
What a reviewer should confirm: (1) the new circuit-breaker tests pass — turn cap fires on the call that exceeds the cap and reports
loop_type: 'turn_tool_call_cap', global-duplicate fires non-consecutively at threshold, alternating-pattern fires on a clean ABAB and resets after a break; (2) the cap fires regardless ofdisableForSession(); (3) the newLoopTypevalues each map to a label in the non-interactive CLI (LOOP_TYPE_LABELSisRecord<LoopType, string>, so a missing one fails typecheck).Evidence (Before & After)
N/A (non-UI change). Test output:
Tested on
Environment (optional)
Local unit tests +
tsc --noEmiton macOS (Node via repo toolchain). Windows/Linux left to CI.Risk & Scope
MAX_TURNSbackstop philosophy and cannot be disabled by design (that is the point of a circuit breaker).skipLoopDetection(defaulttrue), matching the existing opt-in design — so for default users only the hard cap is active.LoopTypevalues are additive. One pre-existing telemetry mislabel is corrected: the turn cap previously loggedconsecutive_identical_tool_callsand now logsturn_tool_call_cap, so any dashboard filtering on the old value for cap events should be updated.Linked Issues
Closes #5234
中文说明
这个 PR 做了什么
新增一个始终生效的安全层：当一个回合内的工具调用失控时直接中断，让卡住的模型不再无限循环。核心是一个单回合工具调用硬上限，它运行在现有
model.skipLoopDetection门控之前，因此无法被配置关闭。除硬上限外，还在现有（opt-in）的循环检测路径上增加了两个启发式检测器：一个非连续的「全局重复」检测器，能抓到同一个(tool, args)调用在一个回合内反复出现（即使中间夹杂了其他调用）；以及一个「交替模式」检测器，能抓到模型在同样的两个调用之间来回横跳（A B A B …）。每个检测器都上报各自的遥测 loop type，非交互（headless）CLI 在因此中断时会为每种类型打印一句可读的原因。为什么需要
工具调用可能陷入死循环（#5234）。该 issue 的 triage 已确认根因：
model.skipLoopDetection默认为true，所以对多数用户而言循环检测是关闭的，唯一兜底是MAX_TURNS。本改动加了一个不依赖该 opt-in 开关的安全网——单回合硬上限始终生效；同时两个新启发式让 opt-in 检测能识别现有「连续相同」检测漏掉的循环形态。审查测试计划
如何验证
这是内部循环检测逻辑加上一条 headless stderr 提示，不是 TUI 改动，所以通过单元测试和类型检查验证，而不是录屏。在仓库根目录：
审查者应确认：(1) 新断路器测试通过——超过上限的那一次调用触发，且上报
loop_type: 'turn_tool_call_cap'；全局重复在阈值处非连续触发；交替模式在干净的 ABAB 上触发并在被打断后重置；(2) 即使disableForSession()后硬上限仍会触发；(3) 每个新LoopType都在非交互 CLI 里有对应标签（LOOP_TYPE_LABELS是Record<LoopType, string>，缺一个就会类型检查失败）。证据（Before & After）
N/A（非 UI 改动）。测试输出：
测试平台
运行环境（可选）
macOS 上本地单元测试 +
tsc --noEmit（Node 走仓库工具链）。Windows/Linux 交给 CI。风险与范围
MAX_TURNS兜底的思路，且按设计无法关闭（这正是断路器的意义）。skipLoopDetection（默认true）门控之后，沿用现有 opt-in 设计——所以对默认用户只有硬上限生效。LoopType值是增量的。修正了一处既有遥测误标：硬上限此前记为consecutive_identical_tool_calls，现在记为turn_tool_call_cap，所以若有看板按旧值过滤上限事件，应更新。关联 Issue
Closes #5234