mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
5a0f51325c
Review follow-up for the dropped tool-call recovery (#69630): the re-prompt pair was tagged _dropped_toolcall_nudge, but that marker was not part of the ephemeral-scaffolding contract. _persist_session / _flush_messages_to_session_db would therefore write the synthetic 'issue the actual tool call now' user message (and the narration-only interim assistant turn) as real transcript rows — a resumed session could replay the internal retry instruction as user-authored context and prompt unsolicited tool use. - Add _dropped_toolcall_nudge to _EPHEMERAL_SCAFFOLDING_FLAGS (run_agent.py) so both SQLite and JSON persistence skip the pair. - Add it to _SYNTHETIC_USER_FLAGS (conversation_compression.py) so the compressor never treats the nudge as human intent. - Flag the interim assistant half of the pair too, and include the marker in the finalization scaffolding pop so a genuine turn end strips the pair from the live transcript (mirrors the _empty_recovery_synthetic pattern). - Regression tests: flagged messages classify as ephemeral, the returned transcript contains no scaffolding, and the turn tail stays on the real assistant answer.
239 lines
10 KiB
Python
239 lines
10 KiB
Python
"""Regression tests for dropped tool-call recovery.
|
|
|
|
Some providers (observed: claude-opus-4.8 / claude-sonnet-4.5 on GitHub
|
|
Copilot, ~2026-07) return ``finish_reason="tool_calls"`` while the parsed
|
|
``tool_calls`` array is empty — the model signalled it wanted to act but the
|
|
payload shipped no call. Before the fix, the conversation loop took the
|
|
no-tool-calls ``else`` branch, treated the turn's narration as the final
|
|
answer, and exited with the task unstarted. On an unattended multi-step job
|
|
(e.g. a scheduled PR reviewer) this silently did nothing.
|
|
|
|
The fix keys on the provider contract violation itself
|
|
(``finish_reason == "tool_calls"`` with zero ``tool_calls``) and re-prompts,
|
|
bounded to 3 consecutive stalls, with the budget resetting after any
|
|
successful tool round so it guards each stall rather than the whole run. A
|
|
genuine ``finish_reason="stop"`` text turn is unaffected.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from types import SimpleNamespace
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
|
|
@pytest.fixture()
|
|
def loop_agent():
|
|
"""AIAgent with a mocked OpenAI client (mirrors test_run_agent's fixture)
|
|
so we can stage a dropped-tool-call response + continuation pair on
|
|
``.chat.completions.create``."""
|
|
from run_agent import AIAgent
|
|
with (
|
|
patch("run_agent.get_tool_definitions", return_value=[]),
|
|
patch("run_agent.check_toolset_requirements", return_value={}),
|
|
patch("run_agent.OpenAI"),
|
|
):
|
|
agent = AIAgent(
|
|
api_key="test-key-1234567890",
|
|
base_url="https://openrouter.ai/api/v1",
|
|
quiet_mode=True,
|
|
skip_context_files=True,
|
|
skip_memory=True,
|
|
)
|
|
agent.client = MagicMock()
|
|
agent._cached_system_prompt = "You are helpful."
|
|
agent._use_prompt_caching = False
|
|
agent.tool_delay = 0
|
|
agent.compression_enabled = False
|
|
agent.save_trajectories = False
|
|
return agent
|
|
|
|
|
|
def _dropped_tool_call_response(content: str):
|
|
"""A response whose finish_reason claims a tool call, but tool_calls is
|
|
empty — the provider contract violation this fix recovers from."""
|
|
from tests.run_agent.test_run_agent import _mock_assistant_msg
|
|
return SimpleNamespace(
|
|
id="chatcmpl-dropped",
|
|
model="test/model",
|
|
choices=[SimpleNamespace(
|
|
index=0,
|
|
message=_mock_assistant_msg(content=content, tool_calls=None),
|
|
finish_reason="tool_calls",
|
|
)],
|
|
usage=None,
|
|
)
|
|
|
|
|
|
class TestDroppedToolCallRecovery:
|
|
def test_dropped_tool_call_reprompts_instead_of_exiting(self, loop_agent):
|
|
"""finish_reason=tool_calls with an empty tool_calls array must
|
|
re-prompt the model to emit the call rather than exiting the loop
|
|
with the narration as the final answer."""
|
|
from tests.run_agent.test_run_agent import _mock_response
|
|
|
|
loop_agent.client.chat.completions.create.side_effect = [
|
|
_dropped_tool_call_response("Let me verify the PR and gather evidence."),
|
|
_mock_response(content="All checks pass. Approved.", finish_reason="stop"),
|
|
]
|
|
|
|
with (
|
|
patch.object(loop_agent, "_persist_session"),
|
|
patch.object(loop_agent, "_save_trajectory"),
|
|
patch.object(loop_agent, "_cleanup_task_resources"),
|
|
):
|
|
result = loop_agent.run_conversation("review the PR")
|
|
|
|
assert loop_agent.client.chat.completions.create.call_count == 2, (
|
|
"A dropped tool call must trigger a re-prompt (second API call), "
|
|
"not exit the loop after one call."
|
|
)
|
|
|
|
# The loop must have injected a nudge user-message telling the model to
|
|
# issue the actual tool call.
|
|
second_call = loop_agent.client.chat.completions.create.call_args_list[1]
|
|
msgs = second_call.kwargs.get("messages") or second_call.args[0].get("messages")
|
|
last_user = next(
|
|
(m for m in reversed(msgs) if m.get("role") == "user"), None,
|
|
)
|
|
assert last_user is not None
|
|
assert "tool call" in (last_user.get("content") or "").lower(), (
|
|
"The nudge must explicitly ask the model to issue the tool call."
|
|
)
|
|
assert "All checks pass" in result["final_response"]
|
|
|
|
def test_empty_content_dropped_tool_call_still_reprompts(self, loop_agent):
|
|
"""The narration may live only in the reasoning field, leaving content
|
|
empty. The recovery must still fire — it keys on the finish_reason /
|
|
tool_calls mismatch, not on content being present."""
|
|
from tests.run_agent.test_run_agent import _mock_response
|
|
|
|
loop_agent.client.chat.completions.create.side_effect = [
|
|
_dropped_tool_call_response(""), # no visible content at all
|
|
_mock_response(content="Done.", finish_reason="stop"),
|
|
]
|
|
|
|
with (
|
|
patch.object(loop_agent, "_persist_session"),
|
|
patch.object(loop_agent, "_save_trajectory"),
|
|
patch.object(loop_agent, "_cleanup_task_resources"),
|
|
):
|
|
result = loop_agent.run_conversation("review the PR")
|
|
|
|
assert loop_agent.client.chat.completions.create.call_count == 2, (
|
|
"An empty-content dropped tool call must still re-prompt."
|
|
)
|
|
assert result["completed"] is True
|
|
|
|
def test_clean_stop_text_turn_is_unaffected(self, loop_agent):
|
|
"""A genuine finish_reason=stop text response must exit normally — the
|
|
recovery path must not fire on ordinary final answers."""
|
|
from tests.run_agent.test_run_agent import _mock_response
|
|
|
|
loop_agent.client.chat.completions.create.side_effect = [
|
|
_mock_response(content="Here is your answer.", finish_reason="stop"),
|
|
]
|
|
|
|
with (
|
|
patch.object(loop_agent, "_persist_session"),
|
|
patch.object(loop_agent, "_save_trajectory"),
|
|
patch.object(loop_agent, "_cleanup_task_resources"),
|
|
):
|
|
result = loop_agent.run_conversation("hello")
|
|
|
|
assert loop_agent.client.chat.completions.create.call_count == 1, (
|
|
"A clean finish_reason=stop turn must not trigger a re-prompt."
|
|
)
|
|
assert "Here is your answer." in result["final_response"]
|
|
|
|
def test_persistent_dropped_tool_calls_are_bounded(self, loop_agent):
|
|
"""If the model never emits a call, the recovery must give up after a
|
|
bounded number of consecutive stalls instead of looping forever."""
|
|
from tests.run_agent.test_run_agent import _mock_response
|
|
|
|
# Stage plenty of dropped-tool-call responses followed by a clean stop,
|
|
# so that if the bound is respected the loop exits on its own well
|
|
# before exhausting the staged responses (no StopIteration).
|
|
loop_agent.client.chat.completions.create.side_effect = [
|
|
_dropped_tool_call_response("Let me check.") for _ in range(9)
|
|
] + [_mock_response(content="done", finish_reason="stop")]
|
|
|
|
with (
|
|
patch.object(loop_agent, "_persist_session"),
|
|
patch.object(loop_agent, "_save_trajectory"),
|
|
patch.object(loop_agent, "_cleanup_task_resources"),
|
|
):
|
|
result = loop_agent.run_conversation("review the PR")
|
|
|
|
# 1 initial call + at most 3 bounded re-prompts = 4 total before the
|
|
# guard stops firing. It must NOT consume all 9 staged stalls.
|
|
assert loop_agent.client.chat.completions.create.call_count <= 4, (
|
|
"Consecutive dropped tool calls must be bounded (no infinite loop)."
|
|
)
|
|
assert result is not None
|
|
|
|
def test_nudge_pair_is_ephemeral_scaffolding(self, loop_agent):
|
|
"""The re-prompt pair (interim assistant turn + synthetic user nudge)
|
|
must be flagged as ephemeral scaffolding so persistence never writes
|
|
it to the durable transcript — a resumed session must not replay the
|
|
internal "issue the actual tool call now" instruction as user-authored
|
|
context (#69630 review follow-up)."""
|
|
from run_agent import _is_ephemeral_scaffolding
|
|
from tests.run_agent.test_run_agent import _mock_response
|
|
|
|
loop_agent.client.chat.completions.create.side_effect = [
|
|
_dropped_tool_call_response("Let me verify the PR."),
|
|
_mock_response(content="All checks pass. Approved.", finish_reason="stop"),
|
|
]
|
|
|
|
with (
|
|
patch.object(loop_agent, "_persist_session"),
|
|
patch.object(loop_agent, "_save_trajectory"),
|
|
patch.object(loop_agent, "_cleanup_task_resources"),
|
|
):
|
|
result = loop_agent.run_conversation("review the PR")
|
|
|
|
assert result["completed"] is True
|
|
# The finalization pop strips the answered pair from the live list —
|
|
# no flagged scaffolding may survive into the returned transcript.
|
|
leftover = [
|
|
m for m in result["messages"]
|
|
if isinstance(m, dict) and m.get("_dropped_toolcall_nudge")
|
|
]
|
|
assert not leftover, (
|
|
"The re-prompt pair must be stripped at finalization, not kept "
|
|
"in the returned transcript."
|
|
)
|
|
# And the persistence filter must classify the flag as ephemeral so a
|
|
# mid-turn flush can never write the pair to the durable store either.
|
|
assert _is_ephemeral_scaffolding(
|
|
{"role": "user", "content": "nudge", "_dropped_toolcall_nudge": True}
|
|
), (
|
|
"_dropped_toolcall_nudge messages must be classified as "
|
|
"ephemeral scaffolding so they are never persisted."
|
|
)
|
|
|
|
def test_unanswered_nudge_tail_is_stripped_at_finalization(self, loop_agent):
|
|
"""If the model answers the nudge with a genuine final text turn, the
|
|
trailing scaffolding must not leave the transcript tail on a synthetic
|
|
user message (strict role alternation on the next turn)."""
|
|
from tests.run_agent.test_run_agent import _mock_response
|
|
|
|
loop_agent.client.chat.completions.create.side_effect = [
|
|
_dropped_tool_call_response("Let me check."),
|
|
_mock_response(content="Final answer.", finish_reason="stop"),
|
|
]
|
|
|
|
with (
|
|
patch.object(loop_agent, "_persist_session"),
|
|
patch.object(loop_agent, "_save_trajectory"),
|
|
patch.object(loop_agent, "_cleanup_task_resources"),
|
|
):
|
|
result = loop_agent.run_conversation("review the PR")
|
|
|
|
tail = result["messages"][-1]
|
|
assert tail.get("role") == "assistant", (
|
|
"The turn must end on the real assistant answer, not scaffolding."
|
|
)
|
|
assert not tail.get("_dropped_toolcall_nudge")
|