fix(core): give Stop-hook continuations a fresh per-turn tool-call budget; make the cap configurable - #6238
Merged
Merged
Conversation
Collaborator
Author
E2E Test ReportAll scenarios run against the built bundle (
Notes:
|
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present (What, Why, Reviewer Test Plan with How to verify and Evidence). Problem: real, observed bug. The PR describes a concrete scenario — Direction: aligned. The per-turn cap was double-counting across Stop-hook continuations — fixing the accounting is the right call, not raising the cap. Making it configurable ( Approach: the scope feels right. Four changes — reset at Stop-hook continuation, configurable cap, Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必需章节齐全。 问题：真实的、已观测的 bug。PR 描述了具体场景——在多步任务上执行 方向：对齐。上限在 Stop-hook 续传之间重复计算——修复计数方式是正确的做法，而不是简单地提高上限。让它可配置（ 方案：范围合理。四个改动——Stop-hook 续传时重置、可配置上限、 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
…dget; make the cap configurable A blocking Stop-hook continuation (e.g. a /goal iteration) feeds a fresh user-role prompt to the model — a new logical turn — but the loop detector never reset, so an entire goal chain billed one per-turn tool-call budget and healthy long-running goals halted with turn_tool_call_cap. The ACP daemon path already used per-continuation budgets; core now matches. - Reset loop detection at each blocking Stop-hook continuation - Add model.maxToolCallsPerTurn setting (default 100; <= 0 disables), resolved once in Config (<= 0 maps to Infinity) - Honor the in-session 'Disable loop detection for this session' choice in the per-turn cap, as the dialog always claimed - Point the headless halt message at the setting; update dialog/docs
tanzhenxin
force-pushed
the
fix/turn-tool-call-cap-goal-chains
branch
from
July 3, 2026 07:15
32a2162 to
b60d63c
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Contributor
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. |
Collaborator
Code ReviewClean diff — the four changes (reset at Stop-hook continuation, configurable cap, The key fix is The configurable cap follows the existing pattern ( No correctness bugs, no security issues, no scope creep. The Test ResultsAll relevant unit tests pass against the PR branch: Turn Tool Call Cap tests (8/8)Config getter tests (4/4)Stop-hook continuation budget test (1/1)Non-interactive halt message test (1/1)LoopDetectionConfirmation UI snapshot (2/2)Updated snapshot now shows the dialog correctly references all three always-on guards and the Build ( 中文说明代码审查Diff 整洁——四个改动（Stop-hook 续传时重置、可配置上限、 核心修复是 可配置上限遵循现有模式（ 无正确性 bug，无安全问题，无范围蔓延。测试 mock 在所有相关文件中一致更新。 测试结果所有相关单元测试通过：Turn Tool Call Cap 8/8、Config getter 4/4、Stop-hook 续传预算 1/1、非交互提示信息 3/3、UI 快照 2/2。构建和打包均成功。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a solid, well-scoped bugfix. The core problem — goal chains tripping the per-turn cap because every iteration's tool calls accumulated into one "turn" — is a real usability issue, and the fix is exactly right: reset the loop detector at Stop-hook continuations so each iteration gets its own budget. The ACP daemon path already had this semantic; this PR brings the core path in line. The four changes hang together naturally — you can't fix the reset without also making the cap configurable (otherwise users who hit the cap legitimately have no escape), and you can't make it configurable without also honoring Code is clean, tests are thorough (19 relevant tests all pass), build and bundle succeed, and the updated snapshot confirms the UI text is correct. No correctness concerns, no security issues, no scope creep. The Approving. ✅ 中文说明这是一个扎实的、范围合理的 bugfix。核心问题——目标链因为每次迭代的工具调用累积到一个"轮次"而触发每轮上限——是一个真实的可用性问题，修复方式完全正确：在 Stop-hook 续传时重置循环检测器，让每次迭代有独立的预算。ACP daemon 路径已有此语义，本 PR 让 core 路径对齐。 四个改动自然关联——修复重置的同时必须让上限可配置（否则合法触发上限的用户无法解除），让它可配置的同时必须让 代码整洁，测试充分（19 个相关测试全部通过），构建和打包成功，更新后的快照确认 UI 文字正确。无正确性问题，无安全问题，无范围蔓延。移除 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 3, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
tanzhenxin
enabled auto-merge
July 3, 2026 07:46
wenshao
approved these changes
Jul 3, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /review |
Contributor
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28689912120)._ |
| * cap and is returned as Infinity so callers can compare unconditionally | ||
| * (mirrors getTruncateToolOutputThreshold). | ||
| */ | ||
| getMaxToolCallsPerTurn(): number { |
Collaborator
There was a problem hiding this comment.
[Suggestion] getMaxToolCallsPerTurn() doesn't guard against NaN input. Since NaN <= 0 evaluates to false in JS, a NaN value passes through and the getter returns NaN. Downstream, this.turnToolCallTotal > NaN in checkTurnToolCallCap is always false — the cap silently disables with no diagnostic.
Consider adding a Number.isFinite() guard:
Suggested change
| getMaxToolCallsPerTurn(): number { | |
| getMaxToolCallsPerTurn(): number { | |
| if (!Number.isFinite(this.maxToolCallsPerTurn) || this.maxToolCallsPerTurn <= 0) { | |
| return Number.POSITIVE_INFINITY; | |
| } | |
| return this.maxToolCallsPerTurn; | |
| } |
— qwen3.7-max via Qwen Code /review
Collaborator
There was a problem hiding this comment.
Verified this finding against the head checkout — it's real, and two deltas are worth adding:
-
It's not just programmatic NaN. The settings load path is raw
JSON.parsewith no schema-type enforcement (packages/cli/src/config/settings.ts), andloadCliConfigpassessettings.model?.maxToolCallsPerTurnstraight through — so"maxToolCallsPerTurn": "abc"in settings.json reaches this getter as a string. The two consumers then fail in opposite directions: core'sturnToolCallTotal > NaNis always false (circuit breaker silently gone), while the daemon path'stotalToolCalls <= NaN(Session.ts:279) is also false — it flags a loop on the first tool-call batch. A fractional value like0.5halts every turn on its first call on both paths. -
Direction of the suggested fix. Returning
Infinityfor non-finite input makes a typo silently disable a safety cap — fail-open. The local convention for these knobs is fail-safe fallback-to-default:normalizeMaxSubagentDepth(core config.ts) andnormalizeStopHookBlockingCap(hooks/stopHookCap.ts) both dotypeof/isFinite check → defaultplusMath.floor. Mirroring that here (non-finite/non-number →DEFAULT_MAX_TOOL_CALLS_PER_TURN,Math.floorotherwise) handles NaN, strings, and fractionals in one move and keeps the breaker armed on misconfig.
中文
已在 head 检出上核实——问题属实，另补两点：
-
不只是编程传入 NaN 的问题。 设置加载路径就是裸
JSON.parse，没有任何类型校验（packages/cli/src/config/settings.ts），loadCliConfig又原样透传——settings.json 里的"maxToolCallsPerTurn": "abc"会以字符串形式到达此 getter。两个消费方随后朝相反方向失效：core 的turnToolCallTotal > NaN恒为 false（熔断器静默失效），而 daemon 路径的totalToolCalls <= NaN（Session.ts:279）同样为 false——第一批工具调用就会被误判为循环。小数（如0.5）则让两条路径的每一轮都在第一次调用即中断。 -
建议修复的方向问题。 对非有限值返回
Infinity意味着一个笔误会静默关闭安全上限——fail-open。本仓库同类旋钮的既有约定是 fail-safe 回退默认值：normalizeMaxSubagentDepth（core config.ts）与normalizeStopHookBlockingCap（hooks/stopHookCap.ts）都是"类型/isFinite 检查 → 默认值"外加Math.floor。照此处理（非有限/非数字 →DEFAULT_MAX_TOOL_CALLS_PER_TURN，其余取整）可一次覆盖 NaN、字符串与小数，且配置错误时熔断器仍然在岗。
| // Runaway protection is preserved: the cap still bounds each | ||
| // iteration, and the chain itself is bounded by | ||
| // stopHookBlockingCap / MAX_GOAL_ITERATIONS. | ||
| this.loopDetector.reset(prompt_id); |
Collaborator
There was a problem hiding this comment.
[Suggestion] this.loopDetector.reset(prompt_id) wipes all loop detection state — not just turnToolCallTotal, but also toolCallRepetitionCount, lastToolCallKey, shellInspectionStreak, globalToolCallCounts, and recentToolCallKeys. This means the consecutive-identical-tool-call streak and shell-stagnation streak are cleared at each Stop-hook continuation.
While stopHookBlockingCap bounds the chain at 8 iterations (default), pattern detectors lose cross-iteration accumulation. Consider introducing a narrower method in loopDetectionService.ts that only zeros turnToolCallTotal and turnToolCallTotalCommitted, preserving the other detectors:
Suggested change
| this.loopDetector.reset(prompt_id); | |
| this.loopDetector.resetTurnBudget(prompt_id); |
Where resetTurnBudget in loopDetectionService.ts would be:
resetTurnBudget(promptId: string): void {
this.promptId = promptId;
this.turnToolCallTotal = 0;
this.turnToolCallTotalCommitted = 0;
this.loopDetected = false;
// Deliberately NOT resetting: lastToolCallKey, toolCallRepetitionCount,
// shellInspectionStreak, globalToolCallCounts, recentToolCallKeys
}— qwen3.7-max via Qwen Code /review
Collaborator
There was a problem hiding this comment.
Verified the mechanics: reset() does clear the identical-call streak, shell-stagnation streak, and global-duplicate state, and pre-PR the only loopDetector.reset in client.ts was the top-level one — so streaks genuinely carried across Stop-hook continuations before. Two counterpoints before adopting resetTurnBudget, though:
-
The narrow reset re-creates a false-positive class the blanket reset avoids. A poll-shaped goal chain legitimately repeats the same call (
npm test,gh run view) once per iteration; with cross-boundary streak accumulation it falsely halts at the 5th iteration, even though the guard's premise — "an identical call returns an identical result" — doesn't hold across iterations where edits land and time passes. Manual user "continue" already resets identically, and the ACP daemon path already uses fresh per-continuation state, so the blanket reset matches both precedents. -
Exposure is bounded, and the bound is checked before the reset runs:
stopHookBlockingCap(default 8) gates the continuation at client.ts:2430-2456, so the worst case for a repeating-but-under-threshold pattern is ~(threshold−1)×8 unproductive calls before the chain hard-stops. The PR's Risk section names exactly this tradeoff.
So this looks like a deliberate, bounded design choice rather than an oversight — the narrow reset would just trade this failure mode for the poll false-positive one. Non-blocking either way.
中文
已核实机制：reset() 确实会清空连续相同调用计数、shell 停滞计数与全局重复状态，且本 PR 之前 client.ts 里唯一的 loopDetector.reset 在顶层入口——所以此前计数确实会跨 Stop 钩子续跑累积。但在采纳 resetTurnBudget 之前有两点反方观点：
-
窄化重置会重新引入全量重置所避免的误报类型。 轮询型 goal 链每次迭代合法地重复同一调用（
npm test、gh run view），若跨边界累积计数，第 5 次迭代就会被误停——而"相同调用必然返回相同结果"这一前提在代码已变、时间已过的跨迭代场景并不成立。用户手动 "continue" 本就做同样的全量重置，ACP daemon 路径也已是每次续跑全新状态，全量重置与这两个先例一致。 -
暴露面有界，且边界检查发生在重置之前：
stopHookBlockingCap（默认 8）在 client.ts:2430-2456 处先行把关，低于阈值的重复模式最坏也就浪费约 (阈值−1)×8 次调用后整条链硬停。PR 的 Risk 一节明确写了这一权衡。
因此这更像是有意为之且有界的设计选择，而非疏漏——窄化重置只是用轮询误报换掉这一失效模式。两个方向都不阻塞合并。
| } | ||
|
|
||
| if (this.checkTurnToolCallCap()) { | ||
| if (!this.disabledForSession && this.checkTurnToolCallCap()) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Each of the three guards in checkAlwaysOnSafeties independently checks !this.disabledForSession. The method name says "AlwaysOn" but the guards are suppressed by disabledForSession. A future maintainer adding a new guard could forget the !this.disabledForSession prefix, and that guard would fire even after the user chose "Disable for this session" — contradicting the dialog's promise.
Consider centralizing with an early return at the top of the ToolCallRequest branch:
if (this.disabledForSession) {
return false;
}This makes it impossible for a new guard to accidentally bypass the session-disable contract.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 4, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
reviewed
Jul 4, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Ran a multi-angle review on this PR (8 finder angles, every candidate independently verified against the head checkout). Net result:
- 2 new findings posted inline below (the ACP surface never got the new setting hint; a misleading test comment).
- 3 findings converged with the existing bot review (NaN guard, blanket
reset(), repeated!disabledForSessiongate) — instead of duplicating those threads I added verification deltas as replies where my verification went further (the NaN thread: string/fractional inputs and the fail-open direction of the suggested fix; the reset thread: the counterpoint that a narrow reset re-creates a poll-style false positive, plus the boundedness that makes the current form defensible). - 1 candidate refuted and not posted:
requiresRestart: falseon a constructor-captured setting looked wrong but exactly matches the siblingmodel.*convention (skipLoopDetection,skipNextSpeakerCheck), and no live-apply mechanism exists formodel.*keys anyway — the headless hint is accurate since the process exits and the next run rebuilds Config.
中文
对本 PR 做了多角度审查（8 个查找角度，每个候选项均在 head 检出上独立核实）。结论：
- 2 个新发现以行内评论形式发布（ACP 路径没有获得新设置的提示；一处误导性的测试注释）。
- 3 个发现与已有的机器人审查重合（NaN 防护、全量
reset()、重复的!disabledForSession门控）——为避免重复开线程，我在核实结论超出原评论的两条线程下补充了回复（NaN 线程：字符串/小数输入及所建议修复的 fail-open 方向问题；reset 线程：窄化重置会重新引入轮询式误报的反方观点，以及使当前实现站得住脚的有界性）。 - 1 个候选项被证伪、未发布：构造时捕获的设置标注
requiresRestart: false看似有误，但与同级model.*设置（skipLoopDetection、skipNextSpeakerCheck）的既有约定完全一致，且model.*键本就没有热生效机制——无头模式的提示也是准确的（进程退出后下次运行会重建 Config）。
| // disabled). Unlike core there is no in-session disable check — that flag is | ||
| // only set by the interactive loop-detection dialog, which has no ACP | ||
| // equivalent. | ||
| if (loopState.totalToolCalls <= config.getMaxToolCallsPerTurn()) return false; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The cap on this path is now driven by the same model.maxToolCallsPerTurn knob, but the knob hint landed on only two of the three halt surfaces in this PR: nonInteractiveCli.ts gained "Raise the model.maxToolCallsPerTurn setting to allow longer turns, or set it to 0 to disable the cap.", and the TUI dialog now names the setting — while the ACP path still emits only Stopping ACP turn after N tool calls in one turn. (debug log + telemetry), and the client-visible failed tool_call_update carries the generic LOOP_DETECTED_SKIP_MESSAGE ("Skipped because loop detection stopped the current turn…"). No ACP-visible string mentions the setting, so an IDE user who hits the cap gets no pointer to the escape hatch that now exists.
Consider appending the same hint to the daemon halt/skip message so all three surfaces stay in sync (or, longer-term, sharing one formatter keyed by LoopType across the three surfaces).
中文
[建议] 此路径的上限现在也由 model.maxToolCallsPerTurn 驱动，但本 PR 只在三个中断提示面中的两个加上了设置提示：nonInteractiveCli.ts 新增了 "Raise the model.maxToolCallsPerTurn setting…"，TUI 对话框也点名了该设置——而 ACP 路径仍只输出 Stopping ACP turn after N tool calls in one turn.（debug 日志 + 遥测），IDE 客户端实际看到的失败 tool_call_update 携带的是通用的 LOOP_DETECTED_SKIP_MESSAGE。ACP 可见的字符串没有一处提到该设置，IDE 用户撞到上限时不知道现在已有出口。
建议在 daemon 的中断/跳过消息中追加同样的提示，保持三个提示面一致（长期看可以按 LoopType 共享一个格式化器）。
| getSessionTokenLimit: vi.fn().mockReturnValue(0), | ||
| getStopHookBlockingCap: vi.fn().mockReturnValue(8), | ||
| // Mimics the resolved Config getter: always a number. The daemon-cap | ||
| // test overrides this with a small value. |
Collaborator
There was a problem hiding this comment.
[Nit] This comment promises "The daemon-cap test overrides this with a small value", but that test pins mockReturnValue(100) — exactly DEFAULT_MAX_TOOL_CALLS_PER_TURN, i.e. a value-level no-op, not a small value. The pin itself is defensible (it decouples the pre-existing 102-call fixture from future changes to the default, replacing the removed DAEMON_TURN_TOOL_CALL_CAP), so the cheap fix is rewording this to "the daemon-cap test pins the cap explicitly"; the alternative is to genuinely pin a small cap (e.g. 3) and shrink the 102-call fixture, which would also remove the coupled 100/102 magic-number pair.
中文
[细节] 这条注释说 daemon-cap 测试会"以一个较小的值覆盖"，但该测试实际固定的是 mockReturnValue(100)——恰好等于 DEFAULT_MAX_TOOL_CALLS_PER_TURN，是值层面的空操作，并非小值。固定本身没问题（它把既有的 102 次调用夹具与默认值的未来变化解耦，接替了被删除的 DAEMON_TURN_TOOL_CALL_CAP），所以最省事的修法是把这句改成"daemon-cap 测试会显式固定上限"；或者真正固定一个小上限（如 3）并相应缩小 102 次调用的夹具，顺带消除 100/102 这对耦合的魔法数字。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
/goaliteration) as a new turn for loop detection, so every iteration gets its own tool-call budget instead of the whole chain being billed against a single per-turn cap. This matches what the IDE (ACP) path already did.model.maxToolCallsPerTurnsetting (default100;0or negative disables it) so the always-on volume cap is no longer hardcoded.Why it's needed
/goal all tests pass) and let it iterate: after a handful of perfectly healthy iterations the session halts with "A potential loop was detected", even though every iteration was making progress. The per-turn cap (100 tool calls) was counting every tool call across the entire goal chain as one turn.Reviewer Test Plan
How to verify
"model": { "maxToolCallsPerTurn": 2 }in~/.qwen/settings.json, run a prompt that triggers 3+ tool calls in one turn (e.g. "read these three files"). Expected: the turn halts withturn_tool_call_capand the halt message points at the setting./goal <condition>on a small multi-step task, or a configured Stop hook that blocks once then allows. Expected: the run completes with no loop halt. Before this PR it halted mid-chain once the cumulative count crossed the cap."maxToolCallsPerTurn": 0the same over-cap prompt completes; in interactive mode, choosing "Disable loop detection for this session" in the dialog stops the cap from firing again that session.Evidence (Before & After)
E2E summary (mock OpenAI server driving deterministic tool-call turns against the built bundle; full report in a follow-up comment):
turn_tool_call_cap)model.maxToolCallsPerTurnTested on
Environment (optional)
npm run build && npm run bundle, scenarios run againstnode dist/cli.jswith a local mock OpenAI-compatible server; interactive scenario via tmux.Risk & Scope
/goalE2E run (the judge's model call is awkward to mock headlessly); verified via a configured blocking Stop hook, which shares the identical continuation path. The ACP daemon's batch-level cap accounting is pre-existing and untouched.Linked Issues
Related: #5279 (introduced the per-turn cap), #5234 (the loop the cap guards against).
中文说明
本 PR 做了什么
/goal的一次迭代）视为循环检测中的新一轮对话，每次迭代拥有独立的工具调用预算，而不是整条链共享一个单轮上限。此行为与 IDE（ACP）路径已有的语义保持一致。model.maxToolCallsPerTurn设置（默认100；0或负数表示禁用），常开的调用量熔断上限不再硬编码。为什么需要
/goal 所有测试通过）并让其自动迭代：若干次完全健康的迭代之后，会话会以"检测到潜在循环"中断，尽管每次迭代都在推进。原因是单轮上限（100 次工具调用）把整条 goal 链的所有调用都计入了同一轮。审阅者测试计划
如何验证
~/.qwen/settings.json中设置"model": { "maxToolCallsPerTurn": 2 }，运行一个单轮内触发 3 次以上工具调用的提示词（如"读取这三个文件"）。预期：该轮以turn_tool_call_cap中断，且提示信息指向该设置项。/goal <条件>执行小型多步任务，或配置一个先阻塞一次再放行的 Stop 钩子。预期：运行完整结束且无循环中断。本 PR 之前，累计计数越过上限时会在链中途中断。"maxToolCallsPerTurn": 0后同样的超限提示词可完整运行；交互模式下在对话框选择"为本会话禁用循环检测"后，该会话内上限不再触发。证据（前后对比）
E2E 摘要（本地 mock OpenAI 服务驱动确定性工具调用轮次，针对构建后的 bundle；完整报告见后续评论）：
turn_tool_call_cap）model.maxToolCallsPerTurn测试环境
macOS 已验证；Windows / Linux 未本地验证（依赖 CI）。
风险与范围
/goal做 E2E（judge 的模型调用难以在无头环境 mock）；改用配置的阻塞式 Stop 钩子验证，二者共享完全相同的续跑代码路径。ACP 守护进程按批次计数的既有行为未改动。关联 Issue
相关：#5279（引入单轮上限）、#5234（该上限所防护的循环问题）。