fix(agents): key generic tool calls on all non-narration args in loop detection (#5872)

* fix(agents): key generic tool calls on all non-narration args in loop detection

Layer 1 keyed every tool without a dedicated rule on its first salient
field (path/url/query/command/...) and dropped all other arguments, so
paging a URL or a search (start_index, page, cursor) and rewriting one
skill file collapsed onto one key and hard-stopped on the fifth distinct
call. Key on every argument except the narration field (description),
which keeps the #1905 bypass closed.

Fixes #5871

* fix(agents): drop description only for sandbox UI narration in loop keys

update_agent/setup_agent persist description and MCP tools such as
create_issue send it, so description-only updates must stay distinct.
Drop it only for bash/ls/glob/grep, where it is the UI explanation of
the call and was already outside the key before this PR.

---------

Co-authored-by: destire-mio <248462155+destire-mio@users.noreply.github.com>
This commit is contained in:
destire-mio 2026-09-28 15:23:51 +08:00 committed by GitHub
parent c0c28a3f13
commit 711035d2d1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 104 additions and 6 deletions

View File

@ -101,6 +101,11 @@ _MAX_PENDING_WARNINGS_PER_RUN = 4
# Stands in for ``read_file``'s omitted ``end_line`` in a call key: the read
# runs to the last line, which is not the same window as any numbered bound.
_OPEN_ENDED_READ = "end"
# Sandbox tools on the generic key path whose ``description`` is only the UI
# explanation of the call. Elsewhere ``description`` can be the operation's
# payload (``update_agent``/``setup_agent`` persist it; MCP tools such as
# ``create_issue`` send it), so it stays in the key.
_UI_NARRATION_TOOLS = frozenset({"bash", "ls", "glob", "grep"})
type _RunScopeKey = tuple[str, str | None]
@ -170,7 +175,7 @@ def _normalized_read_range(args: dict) -> tuple[int, int | None]:
def _stable_tool_key(name: str, args: dict, fallback_key: str | None) -> str:
"""Derive a stable key from salient args without overfitting to noise."""
"""Derive a stable key from the args that change what a call does."""
if name == "read_file" and fallback_key is None:
path = args.get("path") or ""
start_line, end_line = _normalized_read_range(args)
@ -184,14 +189,17 @@ def _stable_tool_key(name: str, args: dict, fallback_key: str | None) -> str:
return fallback_key
return json.dumps(args, sort_keys=True, default=str)
salient_fields = ("path", "url", "query", "command", "pattern", "glob", "cmd")
stable_args = {field: args[field] for field in salient_fields if args.get(field) is not None}
if stable_args:
return json.dumps(stable_args, sort_keys=True, default=str)
if fallback_key is not None:
return fallback_key
# Every other argument can change what the call does: keying only a salient
# field (``path``/``url``/``query``...) collapsed paging a URL or a search
# (``start_index``, ``page``, ``cursor``) and rewriting one skill file onto a
# single key, so the fifth *distinct* call hard-stopped the run. A sandbox
# tool's UI narration is dropped, so rewording it cannot make a repeated
# call look new (#1905).
if name in _UI_NARRATION_TOOLS:
args = {field: value for field, value in args.items() if field != "description"}
return json.dumps(args, sort_keys=True, default=str)

View File

@ -277,6 +277,96 @@ class TestReadFileRangeKey:
assert _hash_tool_calls([self._read_call(path="/w/a.py", start_line=1, end_line=40)]) != _hash_tool_calls([self._read_call(path="/w/b.py", start_line=1, end_line=40)])
class TestGenericToolKey:
"""Tools without a dedicated key rule must keep every argument that changes the call.
Keying only the first salient field (``path``/``url``/``query``/...) collapsed
distinct calls onto one key: paging a URL or a search, or rewriting one
skill file with new content, hard-stopped on its fifth *distinct* call.
Only a sandbox tool's UI narration (``description``) may vary without
making a call new.
"""
@staticmethod
def _call(tool_name, args):
return {"name": tool_name, "id": f"call_{tool_name}", "args": args}
@pytest.mark.parametrize(
("name", "first", "second"),
[
("fetch", {"url": "https://example.com/doc", "start_index": 0}, {"url": "https://example.com/doc", "start_index": 5000}),
("search_issues", {"query": "is:open label:bug", "page": 1}, {"query": "is:open label:bug", "page": 2}),
("list_uploaded_files", {"query": "report"}, {"query": "report", "cursor": "next-page"}),
("skill_manage", {"action": "write_file", "name": "etl", "path": "scripts/run.py", "content": "v1"}, {"action": "write_file", "name": "etl", "path": "scripts/run.py", "content": "v2"}),
("grep", {"path": "/w", "pattern": "foo", "case_sensitive": False}, {"path": "/w", "pattern": "foo", "case_sensitive": True}),
],
)
def test_non_salient_args_affect_hash(self, name, first, second):
assert _hash_tool_calls([self._call(name, first)]) != _hash_tool_calls([self._call(name, second)])
@pytest.mark.parametrize(
("name", "args"),
[
("bash", {"command": "make test"}),
("ls", {"path": "/w"}),
("glob", {"path": "/w", "pattern": "*.py"}),
("grep", {"path": "/w", "pattern": "foo"}),
],
)
def test_sandbox_ui_narration_does_not_affect_hash(self, name, args):
"""Regression guard for #1905: rewording the narration must not dodge detection."""
first = self._call(name, {**args, "description": "run it"})
second = self._call(name, {**args, "description": "run it once more"})
assert _hash_tool_calls([first]) == _hash_tool_calls([second])
@pytest.mark.parametrize(
("name", "args"),
[
("update_agent", {}),
("setup_agent", {"soul": "You are helpful."}),
("create_issue", {"title": "Crash on start"}),
],
)
def test_description_payload_affects_hash(self, name, args):
"""``description`` is the operation's payload outside the sandbox narration tools."""
first = self._call(name, {**args, "description": "Reviews pull requests."})
second = self._call(name, {**args, "description": "Reviews pull requests and triages issues."})
assert _hash_tool_calls([first]) != _hash_tool_calls([second])
def test_description_only_agent_updates_do_not_hard_stop(self):
mw = LoopDetectionMiddleware(warn_threshold=3, hard_limit=5)
runtime = _make_runtime()
for version in range(6):
call = self._call("update_agent", {"description": f"Assistant v{version}"})
assert mw._apply(_make_state(tool_calls=[call]), runtime) is None, f"update to v{version} was treated as a loop"
assert mw.consume_stop_reason("test-run") is None
def test_paging_a_url_does_not_hard_stop(self):
mw = LoopDetectionMiddleware(warn_threshold=3, hard_limit=5)
runtime = _make_runtime()
for start in range(0, 50_000, 5_000):
call = self._call("fetch", {"url": "https://example.com/doc", "max_length": 5_000, "start_index": start})
assert mw._apply(_make_state(tool_calls=[call]), runtime) is None, f"page at start_index={start} was treated as a loop"
assert mw.consume_stop_reason("test-run") is None
def test_repeating_one_page_still_hard_stops(self):
mw = LoopDetectionMiddleware(warn_threshold=3, hard_limit=5)
runtime = _make_runtime()
call = [self._call("fetch", {"url": "https://example.com/doc", "start_index": 5_000})]
for _ in range(4):
assert mw._apply(_make_state(tool_calls=call), runtime) is None
hard_stop = mw._apply(_make_state(tool_calls=call), runtime)
assert hard_stop is not None
assert hard_stop["messages"][0].tool_calls == []
assert mw.consume_stop_reason("test-run") == "loop_capped"
class TestLoopDetection:
def test_no_tool_calls_returns_none(self):
mw = LoopDetectionMiddleware()