fix(core): honor skipLoopDetection for the deterministic tool-call loop#5128
Conversation
QwenLM#5036 carved the deterministic identical-tool-call check out of the `model.skipLoopDetection` gate, turning it into a hard-stop that fires even when loop detection is disabled. Because `skipLoopDetection` defaults to true (settingsSchema: "to avoid false-positive interruptions"), this silently re-enabled loop halts for the default configuration and broke the documented escape hatch — the non-interactive guidance in nonInteractiveCli.ts told users to set `model.skipLoopDetection: true`, which no longer disabled the halt and is unreachable in non-interactive mode (no disable dialog). Gate both the deterministic and heuristic detector paths behind the single flag again. The deterministic split, retry-reset, and pending tool-call splice introduced by QwenLM#5036 still apply once detection is explicitly enabled (skipLoopDetection: false), so the runaway guard remains available as opt-in without overriding the default-off contract.
|
@qwen-code /triage |
|
Thanks for the PR, @wenshao! Template: The body uses custom headings ("Tests", "Verification", "Risk & scope") instead of the template's "Reviewer Test Plan" / "### How to verify" / "### Evidence (Before & After)" / "### Tested on" structure. The substance is all there — not blocking on this, but would be good to align next time. Direction: Clear regression fix. #5036 moved the deterministic identical-tool-call detector outside the Approach: Minimal and focused — 2 files, +55/-6. The core change is adding Moving on to code review and testing. 🔍 中文说明感谢贡献 @wenshao！ 模板： PR 正文使用了自定义标题（"Tests"、"Verification"、"Risk & scope"），未使用模板的 "Reviewer Test Plan" / "### How to verify" / "### Evidence (Before & After)" / "### Tested on" 结构。内容都齐全，这次不作为阻塞项，下次建议对齐模板。 方向： 明确的回归修复。#5036 把确定性相同工具调用检测器移到了 方案： 最小化、聚焦——2 个文件，+55/-6。核心改动是在一行前加 进入代码审查和测试阶段。🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: The fix is straightforward — add PR approach: Matches exactly. The change in Tests: Three key scenarios covered:
TestingUnit testsAll passing:
Before/After (tmux)Before (main branch — deterministic check runs even with Before: 2 tests assert that the deterministic check does run and does halt even when After (with PR applied — loop checks properly gated behind After: 5 tests — 2 flipped to assert detection does not run under default config, 1 new test confirms the guard still fires when explicitly enabled ( 中文说明代码审查独立方案： 在确定性检查处加上 PR 方案： 完全匹配。 测试： 三个关键场景：
测试单元测试全部通过：
Before/After（tmux）修改前（main 分支—— 修改后（应用 PR——循环检测正确受 — Qwen Code · qwen3.7-max |
|
Thanks for the triage. I've realigned the PR description to the template — 中文感谢 triage。已按模板重排 PR 描述—— |
|
This is a clean regression fix and I'm confident it should ship. #5036 introduced a well-intentioned improvement (splitting the deterministic identical-tool-call detector) but accidentally placed it outside the The code is 3 lines of logic change + a good explanatory comment + proper test updates. Before/after tests confirm the behavioral shift: detection is now correctly skipped under the default config, while still available as an explicit opt-in. No concerns. Approving. ✅ 中文说明这是一个干净的回归修复，可以放心合入。 #5036 引入了一个合理的改进（拆分确定性相同工具调用检测器），但意外地把它放在了 代码是 3 行逻辑改动 + 解释性注释 + 正确的测试更新。Before/After 测试确认了行为变化：默认配置下正确跳过检测，显式 opt-in 时兜底仍可用。 无顾虑。批准。✅ — 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. ✅
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Overall the gating is correct and the tests are solid — tsc/eslint clean, 5 loop-related client tests + 45 loop-detection service tests all pass. One inline suggestion below on the new block comment.
— qwen3.7-max via Qwen Code /review
| // Loop detection is opt-in: `model.skipLoopDetection` defaults to true | ||
| // (see settingsSchema) to avoid false-positive interruptions. Keep BOTH | ||
| // the deterministic identical-tool-call check and the heuristic checks | ||
| // behind this single flag so the documented `model.skipLoopDetection` |
There was a problem hiding this comment.
[Suggestion] This sentence claims "The deterministic split, retry-reset, and pending-call splice below still apply once detection is enabled." Of the three items, only the deterministic split and the pending-call splice are actually below this comment in client.ts. The retry-reset (resetToolCallCount()) lives inside addAndCheckDeterministicToolCallLoop in loopDetectionService.ts:184 — reachable through the call graph but not "below" in this file. Referencing unchanged code elsewhere with a positional pointer like "below" makes the comment more likely to become stale when someone refactors the service or reorders this block.
Consider dropping "retry-reset" from the list (it is an implementation detail of the deterministic path and already covered by "the deterministic split … still apply"), or rephrasing to avoid the positional claim, e.g.:
| // behind this single flag so the documented `model.skipLoopDetection` | |
| // Loop detection is opt-in: `model.skipLoopDetection` defaults to true | |
| // (see settingsSchema) to avoid false-positive interruptions. Keep BOTH | |
| // the deterministic identical-tool-call check and the heuristic checks | |
| // behind this single flag so the documented `model.skipLoopDetection` | |
| // escape hatch stays honest (including the non-interactive hint in | |
| // nonInteractiveCli.ts). The deterministic split and the pending-call | |
| // splice below still apply once detection is enabled. |
— qwen3.7-max via Qwen Code /review
LaZzyMan
left a comment
LaZzyMan
left a comment
There was a problem hiding this comment.
Approving — this is a correct, well-scoped regression fix.
#5036 moved the deterministic identical-tool-call check outside the skipLoopDetection gate, so the hard-stop fired even under the default config (skipLoopDetection defaults to true in both the CLI wiring and the settings schema). That also broke the documented escape hatch — the nonInteractiveCli.ts hint ("set model.skipLoopDetection to true to disable") no longer worked, and non-interactive runs have no disableForSession() dialog to fall back on. This PR re-gates both detector paths behind the single flag while preserving #5036's improvements (deterministic split, retry-reset, pending-call splice) on the opt-in path.
Verified locally:
- Gating logic is correct for both flag values;
client.tsis the only production consumer of the detector methods, so the single gate covers interactive + non-interactive + subagent streaming. - No stale-state issue — the detector's counters are only read inside the loop-detected branch, so skipping the
addwhen disabled is safe. - Tests are genuine integration checks: the renamed cases run against a fresh real
LoopDetectionService(theldMockis local to one test), and the newskipLoopDetection: falsecase still hard-stops at the threshold of 5.
One thing worth confirming consciously before merge: this reverts #5036's intent of catching obvious 5×-identical-call loops by default. The argument here is sound — the runtime behavior had diverged from the documented contract, and the non-interactive escape hatch genuinely broke — but since it reverses a deliberate decision, a sign-off from @he-yufeng would be good to avoid churn.
3 participants
What this PR does
Restores
model.skipLoopDetectionas the single gate for all streaming loop detection, including the deterministic identical-tool-call check, so that loop detection stays fully off under the default configuration and the documented escape hatch works again.Why it's needed
#5036 split the identical-tool-call detector into a "deterministic" path and deliberately ran it outside the
skipLoopDetectiongate (only the heuristic detectors stayed behind the flag). That breaks two established contracts. First,skipLoopDetectiondefaults totrue— both in the CLI wiring (settings.model?.skipLoopDetection ?? true) and in the settings schema, which documents the intent verbatim: "Defaults to true to avoid false-positive interruptions; set to false to re-enable as an unattended-run guardrail." After #5036 the identical-tool-call hard-stop fired even for that default config, silently re-introducing the very false-positive interruptions the default was chosen to avoid. Second, it broke the documented escape hatch:nonInteractiveCli.tstells users "Set themodel.skipLoopDetectionsetting to true to disable", but that no longer disabled this halt — and in non-interactive mode there is no disable dialog (disableForSession()is unreachable), so a halted run had no documented way out.This PR gates both detector paths behind the one flag again. The runaway-identical-call guard from #5036 stays available as an explicit opt-in (
skipLoopDetection: false) without overriding the default-off product decision, and all of #5036's improvements — the deterministic split, the retry-reset of the consecutive counter, and the pending-tool-call splice — are preserved for the opt-in path.Reviewer Test Plan
How to verify
Confirm the gate is applied to both paths in
packages/core/src/core/client.ts(theskipLoopDetectionvariable now short-circuitsaddAndCheckDeterministicToolCallLoopas well asaddAndCheckHeuristicLoops), then run the targeted checks below.Expected: the service-level loop tests pass unchanged (the #5036 retry-reset /
getConsecutiveToolCallCounttests are untouched), and the three client-level tests below pass —does not run loop checks when skipLoopDetection is true(neither detector path is invoked under the default),does not hard-stop identical tool calls when skipLoopDetection is true(5 identical calls produce noLoopDetectedunder the default), andhard-stops identical tool calls when loop detection is enabled(withskipLoopDetection: false, 5 identical calls still hard-stop withCONSECUTIVE_IDENTICAL_TOOL_CALLS).Evidence (Before & After)
This is request-halting behavior gated by a config flag, not a visible TUI state, so screenshots are N/A; the deterministic evidence is the unit tests. Before (with #5036, default
skipLoopDetection: true): five identical tool calls emitLoopDetected/CONSECUTIVE_IDENTICAL_TOOL_CALLSand the run is halted with "A potential loop was detected … The request has been halted." After (this PR, same default): the same five calls emit noLoopDetectedand the stream proceeds; withskipLoopDetection: falsethe hard-stop still fires. Local run:loopDetectionService.test.ts45 passed;client.test.tsloop tests 6 passed (including the 3 above); prettier / eslint /tsc --noEmitclean.Tested on
Environment (optional)
Local unit tests only (vitest under
packages/core); no runtime/sandbox needed.Risk & Scope
skipLoopDetection: false. No threshold or detector-logic change.addAndCheckDeterministicToolCallLoop/addAndCheckHeuristicLoopshave exactly one consumer (client.ts); the non-interactive path consumes theLoopDetectedevent, so this single gate covers both interactive and non-interactive runs.Linked Issues
Relates to #5036 (corrects a regression introduced there). No closing keyword — there is no separate open issue.
中文说明
这个 PR 做了什么
把
model.skipLoopDetection重新恢复为所有流式循环检测(包括确定性的"连续相同工具调用"检测)的唯一开关,使默认配置下循环检测完全关闭,并让有文档的逃生通道重新生效。为什么需要
#5036 把"相同工具调用"检测拆成了一条确定性路径,并刻意让它跑在
skipLoopDetection开关之外(只有启发式检测器还在开关后面)。这破坏了两个既定契约。其一,skipLoopDetection默认就是true——CLI 层(settings.model?.skipLoopDetection ?? true)和 settings schema 都是,schema 描述还原文写明了意图:"Defaults to true to avoid false-positive interruptions; set to false to re-enable as an unattended-run guardrail." #5036 之后,相同工具调用的 hard-stop 对该默认配置也会触发,等于悄悄把"默认关闭以避免误报打断"这个决策推翻了。其二,它破坏了有文档的逃生通道:nonInteractiveCli.ts提示用户*"把model.skipLoopDetection设为 true 即可关闭"*,但这已经关不掉该 halt;而非交互模式没有 disable 对话框(disableForSession()够不着),撞上后没有任何有文档的关法。本 PR 把两条检测路径重新关回同一个开关后面。#5036 的"相同调用失控"兜底作为显式 opt-in(
skipLoopDetection: false)依然可用,不推翻默认关闭的产品决策;同时 #5036 的全部改进——确定性拆分、连续计数的 retry 重置、重复 pending 调用的 splice——在 opt-in 路径上全部保留。Reviewer Test Plan
如何验证
确认
packages/core/src/core/client.ts里的开关同时作用于两条路径(skipLoopDetection变量现在对addAndCheckDeterministicToolCallLoop和addAndCheckHeuristicLoops都做了短路),然后运行下面的针对性检查:预期:service 层循环测试照常通过(#5036 的 retry 重置 /
getConsecutiveToolCallCount测试未改动),下面三个 client 层测试通过——does not run loop checks when skipLoopDetection is true(默认下两条路径都不被调用)、does not hard-stop identical tool calls when skipLoopDetection is true(默认下 5 次相同调用不产生LoopDetected)、hard-stops identical tool calls when loop detection is enabled(skipLoopDetection: false时 5 次相同调用仍以CONSECUTIVE_IDENTICAL_TOOL_CALLShard-stop)。证据(前后对比)
这是由配置开关控制的"请求中止"行为,不是可见的 TUI 状态,因此截图不适用;确定性证据是单元测试。修改前(#5036,默认
skipLoopDetection: true):5 次相同工具调用发出LoopDetected/CONSECUTIVE_IDENTICAL_TOOL_CALLS,运行被中止并提示*"A potential loop was detected … The request has been halted."* 修改后(本 PR,同样默认):同样 5 次调用不再发出LoopDetected,流继续;skipLoopDetection: false时 hard-stop 仍触发。本地运行:loopDetectionService.test.ts45 通过;client.test.ts循环测试 6 通过(含上述 3 个);prettier / eslint /tsc --noEmit干净。测试平台
环境(可选)
仅本地单元测试(
packages/core下的 vitest);无需运行时/沙箱。风险与范围
skipLoopDetection: false保留兜底。未改任何阈值或检测逻辑。addAndCheckDeterministicToolCallLoop/addAndCheckHeuristicLoops只有client.ts一个消费点;非交互路径消费的是LoopDetected事件,所以这一个开关同时覆盖交互/非交互。关联 Issue
关联 #5036(修正其引入的回归)。不使用关闭关键字——没有单独的 open issue。