fix(agent): skip empty partial-stream assistant turns - #47280
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
What was changed
Fix for Gemini compatibility: when a partial-stream stub has no recoverable visible text (empty content, no tool calls) after a network reset, the loop now skips appending the empty assistant message. Previously this caused Gemini to reject the history on the next request.
The fix
Correctly gates the _build_assistant_message + messages.append call behind a check: (not _is_partial_stream_stub) or (not _trunc_content_empty). This is logically sound and prevents the empty message from being added while still requesting continuation.
Testing
New test test_empty_partial_stream_stub_skips_empty_assistant_turn covers the specific scenario. Also moves the _is_partial_stream_stub check earlier to avoid recalculating it.
Security
No concerns.
Reviewed by Hermes Agent
|
Rebased this PR onto current Local verification after rebase:
|
7d03cbd to
9591f18
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the empty partial-stream-stub case. The underlying main-path append is still present at agent/conversation_loop.py:1904-1947.
Problems
- The new skip at
agent/conversation_loop.py:1801can leave an initial multimodal user message (contentlist) directly followed by the string continuation user message.repair_message_sequence()only merges adjacent user messages when both contents are strings (agent/agent_runtime_helpers.py:529-550);ChatCompletionsTransportdoes not subsequently normalize role order (agent/transports/chat_completions.py:141-205). That can replace the empty-assistant failure with consecutive user roles on strict endpoints.
Suggested changes
- Coalesce the continuation with the preceding user message using list/string-safe handling, and add a list-content regression. The current test uses a string prompt at
tests/run_agent/test_partial_stream_finish_reason.py:397, so it does not cover this path.
Automated hermes-sweeper review.
33a9229 to
d65d1dc
Compare
d65d1dc to
9d3aafe
Compare
|
Rebased onto current Kept main's Local verification: python -m pytest -q -o 'addopts=' tests/run_agent/test_partial_stream_finish_reason.py
# 15 passed |
|
Resolved on main via PR #73028 (salvage of #68041). Your PR was submitted first for this layer — you identified the empty partial-stream-stub persistence bug a full month before anyone else, and the merged fix's loop guard is exactly your approach (skip appending the stub when there's no text and no tool calls, keep the continuation nudge). The merged version also adds two more layers on top (builder pad + send-time repair for already-poisoned sessions), which is why we merged that branch, but first-finder credit is yours. Thanks @Qwinty! Commit 309f06b. |
What does this PR do?
Skips appending an empty assistant history turn when a
PARTIAL_STREAM_STUB_IDhas no recoverable text and no tool calls. In that case the loop still asks the model to continue from the stream interruption, but it no longer sends a synthetic assistant message with neithercontentnortool_calls.This fixes a strict Chat Completions failure observed in production with Gemini through CLIProxyAPI:
The bad message is created by Hermes' partial-stream recovery path before the request reaches the proxy. A proxy-side sanitizer could mask it, but Hermes has the semantic context to avoid adding the synthetic empty assistant turn at all.
Related Issue
No separate issue filed. This is related to, but distinct from:
Those cover partial-stream continuation and empty assistant content with
tool_calls; this PR covers the empty partial-stream-stub case with no recoverable text and no tool calls.Type of Change
Changes Made
agent/conversation_loop.pyPARTIAL_STREAM_STUB_IDmessages before building an interim assistant history row.tests/run_agent/test_partial_stream_finish_reason.pyHow to Test
content=None,tool_calls=None, andfinish_reason="length".tool_calls.Commands run:
/usr/local/lib/hermes-agent/venv/bin/python -m pytest -q -o addopts='' tests/run_agent/test_partial_stream_finish_reason.py /usr/local/lib/hermes-agent/venv/bin/python -m py_compile agent/conversation_loop.py tests/run_agent/test_partial_stream_finish_reason.py /usr/local/lib/hermes-agent/venv/bin/python -m ruff check agent/conversation_loop.py tests/run_agent/test_partial_stream_finish_reason.py git diff --checkResults:
10 passedAll checks passed!git diff --checkcleanChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Observed production failure shape, redacted:
[ {"role": "tool", "content": "..."}, {"role": "assistant", "content": ""}, { "role": "user", "content": "[System: The previous response was cut off by a network error mid-stream. Continue exactly where you left off. Do not restart or repeat prior text. Finish the answer directly.]" } ]The upstream returned:
{"error":{"code":400,"message":"Request contains an invalid argument.","status":"INVALID_ARGUMENT"}}