mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(agent): move dropped tool-call recovery to the finalization chokepoint
The initial fix guarded the no-tool-calls else branch, but that branch only SETS final_response — the turn actually finalizes later, in a separate block after final_msg is built. Runs that reached finalization via that path exited without the guard ever running (observed live: a scheduled PR reviewer stalled at tool_turns=1-2 with zero recovery nudges). Move the recovery to the finalization chokepoint (right after final_msg is built), so it catches every path that ends a turn. Single guard now: - increments a consecutive-stall counter and re-prompts (bounded to 3), - resets on any successful tool round, and - resets on a genuine (non-mismatch) turn end, so it guards each stall independently without capping the whole run and without looping forever. Verified live: the scheduled reviewer now recovers through the stalls and submits real reviews — PR 57800 APPROVED, PR 54826 COMMENTED — one PR per run.
This commit is contained in:
parent
63954d508c
commit
923704c7c2
2 changed files with 61 additions and 50 deletions
|
|
@ -5884,54 +5884,12 @@ def run_conversation(
|
|||
continue
|
||||
|
||||
else:
|
||||
# ── Dropped tool-call recovery (copilot/Claude) ────────
|
||||
# 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 INTENDED
|
||||
# to act but the payload shipped no call — the narration may land
|
||||
# in `content` OR only in the `reasoning` field (empty content).
|
||||
# Either way, treating it as a final answer silently ends the
|
||||
# turn with the task unstarted. The invariant we key on is the
|
||||
# provider contract violation itself: finish_reason=="tool_calls"
|
||||
# with zero tool_calls. Re-prompt (bounded) to make the model
|
||||
# emit the call. finish_reason=="stop" text finishes are
|
||||
# unaffected. The retry budget resets after any successful tool
|
||||
# round (see the post-tool reset), so it guards each stall, not
|
||||
# the whole run.
|
||||
_dropped_tc_mismatch = (
|
||||
finish_reason == "tool_calls"
|
||||
and not assistant_message.tool_calls
|
||||
)
|
||||
if _dropped_tc_mismatch and getattr(agent, "_dropped_toolcall_retries", 0) < 3:
|
||||
agent._dropped_toolcall_retries = getattr(agent, "_dropped_toolcall_retries", 0) + 1
|
||||
logger.warning(
|
||||
"finish_reason=tool_calls with empty tool_calls array "
|
||||
"(narration only) — re-prompting to emit the call "
|
||||
"(retry %d/3, model=%s provider=%s)",
|
||||
agent._dropped_toolcall_retries, agent.model, agent.provider,
|
||||
)
|
||||
agent._emit_status(
|
||||
"↻ Model signaled a tool call but sent none — "
|
||||
f"re-prompting ({agent._dropped_toolcall_retries}/3)"
|
||||
)
|
||||
interim_msg = agent._build_assistant_message(assistant_message, finish_reason)
|
||||
messages.append(interim_msg)
|
||||
messages.append({
|
||||
"role": "user",
|
||||
"content": (
|
||||
"Your previous turn indicated a tool call but none was "
|
||||
"included. Do not narrate a plan or restate intent — issue "
|
||||
"the actual tool call now to continue the task."
|
||||
),
|
||||
"_dropped_toolcall_nudge": True,
|
||||
})
|
||||
agent._session_messages = messages
|
||||
continue
|
||||
|
||||
# No tool calls - this is the final response
|
||||
# No tool calls - this is the final response.
|
||||
# (Dropped tool-call recovery — finish_reason=="tool_calls" with
|
||||
# an empty tool_calls array — is handled at the finalization
|
||||
# chokepoint below, after final_msg is built, so it catches
|
||||
# every path that reaches turn finalization, not just this one.)
|
||||
final_response = assistant_message.content or ""
|
||||
# A real final answer resets the dropped-tool-call retry budget.
|
||||
agent._dropped_toolcall_retries = 0
|
||||
|
||||
# Fix: unmute output when entering the no-tool-call branch
|
||||
# so the user can see empty-response warnings and recovery
|
||||
|
|
@ -6262,6 +6220,53 @@ def run_conversation(
|
|||
|
||||
final_msg = agent._build_assistant_message(assistant_message, finish_reason)
|
||||
|
||||
# ── Dropped tool-call recovery (copilot/Claude) ────────
|
||||
# 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.
|
||||
# Reaching finalization with that mismatch means the turn is
|
||||
# about to end with the task unstarted (the narration, which may
|
||||
# be in content or only in the reasoning field, gets treated as
|
||||
# the final answer). Re-prompt (bounded to 3 CONSECUTIVE stalls;
|
||||
# the budget resets after any successful tool round) to make the
|
||||
# model emit the call instead of exiting. finish_reason="stop"
|
||||
# text finishes never enter this guard.
|
||||
if (
|
||||
finish_reason == "tool_calls"
|
||||
and not assistant_message.tool_calls
|
||||
and getattr(agent, "_dropped_toolcall_retries", 0) < 3
|
||||
):
|
||||
agent._dropped_toolcall_retries = getattr(agent, "_dropped_toolcall_retries", 0) + 1
|
||||
logger.warning(
|
||||
"finish_reason=tool_calls with empty tool_calls array "
|
||||
"(narration only) — re-prompting to emit the call "
|
||||
"(retry %d/3, model=%s provider=%s)",
|
||||
agent._dropped_toolcall_retries, agent.model, agent.provider,
|
||||
)
|
||||
agent._emit_status(
|
||||
"↻ Model signaled a tool call but sent none — "
|
||||
f"re-prompting ({agent._dropped_toolcall_retries}/3)"
|
||||
)
|
||||
messages.append(final_msg)
|
||||
messages.append({
|
||||
"role": "user",
|
||||
"content": (
|
||||
"Your previous turn indicated a tool call but none was "
|
||||
"included. Do not narrate a plan or restate intent — issue "
|
||||
"the actual tool call now to continue the task."
|
||||
),
|
||||
"_dropped_toolcall_nudge": True,
|
||||
})
|
||||
agent._session_messages = messages
|
||||
final_response = None
|
||||
continue
|
||||
|
||||
# Reached finalization without the dropped-tool-call mismatch —
|
||||
# a genuine turn end. Clear the consecutive-stall budget so the
|
||||
# next turn starts fresh.
|
||||
agent._dropped_toolcall_retries = 0
|
||||
|
||||
# Pop thinking-only prefill and empty-response retry
|
||||
# scaffolding before appending either a final response or a
|
||||
# verification-stop follow-up. These internal turns are only
|
||||
|
|
|
|||
|
|
@ -150,9 +150,14 @@ class TestDroppedToolCallRecovery:
|
|||
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(10)
|
||||
]
|
||||
_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"),
|
||||
|
|
@ -161,7 +166,8 @@ class TestDroppedToolCallRecovery:
|
|||
):
|
||||
result = loop_agent.run_conversation("review the PR")
|
||||
|
||||
# 1 initial call + 3 bounded re-prompts = 4 total, then it stops.
|
||||
# 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)."
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue