forked from NousResearch/hermes-agent
-
Notifications
You must be signed in to change notification settings - Fork 0
fix(agent): persist recovered final responses at the finalize_turn chokepoint #282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
112 changes: 112 additions & 0 deletions
112
tests/agent/test_turn_finalizer_final_response_persistence.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| from types import SimpleNamespace | ||
|
|
||
| from agent.turn_finalizer import finalize_turn | ||
|
|
||
|
|
||
| class FakeAgent: | ||
| def __init__(self): | ||
| self.max_iterations = 90 | ||
| self.iteration_budget = SimpleNamespace(remaining=10, used=1, max_total=90) | ||
| self.quiet_mode = True | ||
| self.model = "test-model" | ||
| self.provider = "test-provider" | ||
| self.base_url = "" | ||
| self.session_id = "sess-test" | ||
| self.context_compressor = SimpleNamespace(last_prompt_tokens=0) | ||
| self.session_input_tokens = 0 | ||
| self.session_output_tokens = 0 | ||
| self.session_cache_read_tokens = 0 | ||
| self.session_cache_write_tokens = 0 | ||
| self.session_reasoning_tokens = 0 | ||
| self.session_prompt_tokens = 0 | ||
| self.session_completion_tokens = 0 | ||
| self.session_total_tokens = 0 | ||
| self.session_estimated_cost_usd = 0 | ||
| self.session_cost_status = "unknown" | ||
| self.session_cost_source = "test" | ||
| self._tool_guardrail_halt_decision = None | ||
| self._interrupt_message = None | ||
| self._response_was_previewed = True | ||
| self._skill_nudge_interval = 0 | ||
| self._iters_since_skill = 0 | ||
| self.valid_tool_names = [] | ||
| self.persisted_messages = None | ||
|
|
||
| def _handle_max_iterations(self, messages, api_call_count): | ||
| raise AssertionError("not expected") | ||
|
|
||
| def _emit_status(self, *_args, **_kwargs): | ||
| pass | ||
|
|
||
| def _safe_print(self, *_args, **_kwargs): | ||
| pass | ||
|
|
||
| def _save_trajectory(self, *_args, **_kwargs): | ||
| pass | ||
|
|
||
| def _cleanup_task_resources(self, *_args, **_kwargs): | ||
| pass | ||
|
|
||
| def _drop_trailing_empty_response_scaffolding(self, messages): | ||
| pass | ||
|
|
||
| def _persist_session(self, messages, conversation_history): | ||
| self.persisted_messages = list(messages) | ||
|
|
||
| def _file_mutation_verifier_enabled(self): | ||
| return False | ||
|
|
||
| def _turn_completion_explainer_enabled(self): | ||
| return False | ||
|
|
||
| def _drain_pending_steer(self): | ||
| return None | ||
|
|
||
| def clear_interrupt(self): | ||
| pass | ||
|
|
||
| def _sync_external_memory_for_turn(self, **_kwargs): | ||
| pass | ||
|
|
||
|
|
||
| def test_final_response_closes_tool_tail_before_persistence(monkeypatch): | ||
| """A recovered/previewed final response must be durable in session history. | ||
|
|
||
| Regression for turns where the caller receives a non-empty final_response, | ||
| but the message transcript still ends at a tool result. If persisted that | ||
| way, the next turn reloads a stale/malformed history and can appear to loop | ||
| because the assistant's visible final answer is missing from durable state. | ||
| """ | ||
| monkeypatch.setattr("hermes_cli.plugins.invoke_hook", lambda *_a, **_kw: []) | ||
| agent = FakeAgent() | ||
| messages = [ | ||
| {"role": "user", "content": "do it"}, | ||
| { | ||
| "role": "assistant", | ||
| "content": "I'll check.", | ||
| "tool_calls": [ | ||
| {"id": "call-1", "function": {"name": "terminal", "arguments": "{}"}} | ||
| ], | ||
| }, | ||
| {"role": "tool", "tool_call_id": "call-1", "name": "terminal", "content": "ok"}, | ||
| ] | ||
|
|
||
| result = finalize_turn( | ||
| agent, | ||
| final_response="Done.", | ||
| api_call_count=2, | ||
| interrupted=False, | ||
| failed=False, | ||
| messages=messages, | ||
| conversation_history=[], | ||
| effective_task_id="task", | ||
| turn_id="turn", | ||
| user_message="do it", | ||
| original_user_message="do it", | ||
| _should_review_memory=False, | ||
| _turn_exit_reason="fallback_prior_turn_content", | ||
| ) | ||
|
|
||
| assert result["messages"][-1] == {"role": "assistant", "content": "Done."} | ||
| assert agent.persisted_messages is not None | ||
| assert agent.persisted_messages[-1] == {"role": "assistant", "content": "Done."} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 New tail-closure code defeats '(empty)' sentinel stripping, causing '(empty)' to persist in session (bug)
During the
empty_response_exhaustedrecovery path inconversation_loop.py(lines 4827-4866), the model returns no content after exhausting retries. The loop appends an assistant message withcontent="(empty)"and the_empty_terminal_sentinel=Trueflag (line 4838), then setsfinal_response = "(empty)"(line 4865) and breaks. Infinalize_turn,_drop_trailing_empty_response_scaffoldingat line 168 correctly strips this flagged message (and pass 2 may rewind trailing tool result/assistant pairs). However, the new code at lines 199-205 then detects the tail is no longer "assistant" (the sentinel was just popped), and re-appends{"role": "assistant", "content": "(empty)"}without the_empty_terminal_sentinelflag. This unflagged message is then persisted by_persist_sessionat line 207 (_flush_messages_to_session_dbonly filters messages by_is_ephemeral_scaffoldingwhich checks the flag). The sentinel mechanism — explicitly designed to prevent(empty)from landing in durable storage (per docstring at lines 4832-4836, also per the comment at line 163-166) — is completely bypassed. A subsequent "continue" turn will replayassistant("(empty)")as if it were a real model response, which can keep long tool-heavy sessions stuck in empty-response loops.💡 Suggestion: Guard the new tail-closure block against the sentinel value so that recovery paths producing "(empty)" do not re-add the sentinel content. The cleanest fix is to exclude
final_response == "(empty)"from the guard condition, since this is the explicit sentinel value that_drop_trailing_empty_response_scaffoldingis designed to strip.📋 Prompt for AI Agents
In agent/turn_finalizer.py line 199, change the guard condition from
if final_response and not interrupted:toif final_response and not interrupted and final_response != "(empty)":. This prevents the new tail-closure logic from re-adding the "(empty)" sentinel content that_drop_trailing_empty_response_scaffoldingcorrectly stripped at line 168. The sentinel path is already handled by the conversation loop's own scaffolding mechanism and should not be re-closed here. Add a test in tests/agent/test_turn_finalizer_final_response_persistence.py that passesfinal_response="(empty)"with_turn_exit_reason="empty_response_exhausted"and asserts the persisted messages list does NOT contain an "(empty)" content message.