fix(agent): warn instead of silently spiraling on repeated identical tool calls - #5344
fix(agent): warn instead of silently spiraling on repeated identical tool calls#5344albatrossflyon-coder wants to merge 4 commits into
Conversation
…tool calls The tool-call loop had no repeat/loop detection at all: a stuck agent could burn its entire max_iterations budget calling the exact same tool with the exact same arguments over and over, with no signal to the user or the model that it was happening. Adds a lightweight per-turn signature tracker (order-independent, name+args) to AgentRunner.run(). After 3 identical tool-call rounds in a row, it injects one system-role warning into the conversation, then resets -- informs without blocking, so legitimate repeated reads still execute normally. Known scope: this catches repeats *across* separate iterations, not several identical calls bundled into one turn (e.g. 3 parallel read_file calls in a single response) -- that's a different pattern and intentionally out of scope for this change. Two new tests cover the warn-once-after-three behavior and confirm no false positive on two repeats or varied arguments.
CI's "Python (latest, 3.14 + coverage)" job runs basedpyright --strict, which the other three test jobs don't. The bare `parts = []` in _tool_call_signature() inferred as list[Unknown], tripping reportUnknownMemberType on .append() and reportUnknownArgumentType on sorted(). Explicit `list[str]` annotation resolves both. Verified: basedpyright clean (0 errors), full suite 5776 passed/0 failed, ruff clean.
A model finalizing with no tools offered could still emit literal <tool_call><function=...> text instead of a real answer, and that raw markup could reach a real user-facing channel. A prior pass added a check on the max-iterations retry path only; this closes the gaps a follow-up code-review found in that fix: - The dominant per-turn finalize path (runner.py _run_core, where most turns actually end) had no guard at all -- only the much rarer max-iterations retry path did. Root cause: the leak check was only ever wired into one of the two places a turn can finalize. - Only the email channel had the egress filter; the other 16 channels didn't. Root cause: the filter was added per-channel instead of at the single funnel (ChannelManager._send_once) all non-streaming channel sends actually pass through. Centralized there instead of duplicating into every channel implementation. - The regex missed the plural <tool_calls>/</tool_calls> wrapper tag entirely (verified live), not just the opening-tag case caught previously. - The blocked-leak warning logged the raw leaked content unredacted. Fixed at the source: the centralized check never logs raw content. - MessageTool-suppression now treats a leaked-markup fallback notice the same as an empty-response notice: always suppressed once MessageTool already sent real content this turn. Verified: full suite 5915 passed / 44 skipped / 0 failed, vuln-hunter scan_diff clean (one unrelated pre-existing SHA1-as-dedup-key false positive, not touched), basedpyright --strict clean on all touched files (caught and fixed one new strict-mode error introduced by this fix itself), /code-review high pass triaged. Known gaps, not fixed here, documented in BUILDLOG.md: streaming responses bypass the filter entirely since tokens display live before any finalize-time check runs (needs mid-stream detection, a bigger design change); the underlying regex's <function\s*=/TOOL_CALL: patterns can theoretically false-positive on legitimate prose that discusses the agent's own tool-call syntax (pre-existing, checked production logs, never observed firing).
_detect_tool_call_loop tracks one signature per whole round, so it only catches a loop that repeats identically across separate rounds. It can't see N identical calls issued together within a single round (e.g. three parallel read_file calls with the same path) -- that round produces its own distinct joined signature exactly once, so it never looks like a repeat. Adds _detect_intra_round_duplicate_calls(), checked alongside the existing cross-round guard, firing the first time a round contains 3+ identical calls rather than requiring the round to repeat. Disclosed as a known gap in PR HKUDS#5344's description; closing it now per that PR's own "happy to extend coverage there too" note. Two new tests mirroring the existing loop-guard tests' conventions: warn-on-three-batched-identical-calls, no-false-positive-on-two-or-varied.
|
Pushed an extension closing the gap disclosed in the PR description —
Added Two new tests: warn-on-three-batched-identical-calls, no-false-positive-on-two-or-varied. Full suite: 5915 passed, 2 pre-existing failures unrelated to this change (filed separately as #5348), 44 skipped. |
Problem
The tool-call loop has no repeat/loop detection at all. A stuck agent can burn its entire
max_iterationsbudget calling the exact same tool with the exact same arguments over and over, with no signal to the user or the model that it's happening -- it just looks frozen from the outside until the iteration cap finally kicks in.Ran into this directly: an agent spiraled for several minutes checking the same env var through half a dozen different mechanisms (
exec,grep, reading its own session log, etc.) before eventually landing on the right answer -- no single one of those calls repeated identically enough times in a row to be an obvious "stuck" signal by iteration count alone, but a simpler variant of the same problem (identical tool + identical args, repeated) is easy to construct and easy to detect.Fix
Adds a lightweight, order-independent per-turn signature tracker (tool name + arguments) to
AgentRunner.run(). After 3 identical tool-call rounds in a row, it injects onesystem-role warning into the conversation telling the model to stop and try something else, then resets the tracker -- it informs, it does not block, so legitimate repeated reads/checks still execute normally.Scope
This catches repeats across separate iterations (the failure pattern above). It intentionally does not catch several identical calls bundled into a single turn (e.g. 3 parallel
read_filecalls in one response) -- confirmed live during testing that this is genuinely a different pattern. Happy to extend coverage there too if maintainers think it's worth it, but wanted to keep this PR scoped to the pattern actually observed.Testing
tests/agent/test_runner_safety.py: warn-once-after-three-identical-calls, and no-false-positive-on-two-repeats-or-varied-arguments.ruff checkclean on both changed files.Non-goals
Not attempting a more general loop-detection framework here (state hashing across N steps, progress detection, etc.) -- keeping this to the smallest change that addresses the concrete failure pattern, per the contributing guide's "prefer the smallest change that solves the real problem."