fix(think_scrubber): recover final answer from unclosed <think> block - #53391
fix(think_scrubber): recover final answer from unclosed <think> block#53391samaidev wants to merge 1 commit into
Conversation
## Problem
Some OpenAI-compatible LLM gateways (e.g. relays fronting MiniMax-M1 /
Step-3.7-Flash / Intern-S2-Preview) inline the model's reasoning AND
the final answer inside `delta.content` wrapped in a single `<think>`
open tag with NO matching `</think>` close. The model intends:
reasoning = "The user is asking for 2+2. The answer is 4."
final_answer = "4"
…and streams it as:
delta1: "<think>The user is asking for 2+2. The answer is 4.\n4"
(stream ends — no </think>)
StreamingThinkScrubber sees the unclosed `<think>` as a truncated
reasoning block and, at flush() time, discards EVERYTHING held back
in the buffer — including the final answer "4" that came after the
newline. The agent then sees an empty content string and reports
"Empty response from model — retrying (1/3)".
This was observed in production with the aicq-hermes plugin (v1.2.4)
connecting to http://aicq.online:3000/v1 with model "mymodel". The
workaround shipped in aicq-hermes v1.2.4 was a monkey-patch on
StreamingThinkScrubber.flush; this PR upstreams the fix properly.
## Root cause
Two issues in `agent/think_scrubber.py`:
1. `feed()` (line ~125, the `_in_block=True` branch with no close tag
found): when inside an open block and the current delta has no
close tag, the code only held back a partial close-tag prefix
(`_max_partial_suffix`) and DISCARDED everything else. This meant
reasoning + final answer prose was dropped in real time — by the
time flush() ran, `_buf` only contained a few chars of partial
close-tag prefix (usually empty).
2. `flush()` (line ~204, the `_in_block=True` branch): unconditionally
returned `""` when still inside an unterminated block, with no
attempt to recover visible content. The docstring explicitly
justified this as "leaking partial reasoning is worse than a
truncated answer" — a reasonable default for truly truncated
streams, but wrong for the "inline reasoning + answer, no close
tag" pattern.
## Fix
**feed() change**: when `_in_block=True` and no close tag is found in
the current delta, accumulate the ENTIRE delta into `_buf` (instead of
discarding everything except a partial close-tag prefix). The next
feed() call prepends `_buf` to the new text and re-scans for close
tags across the boundary, so partial close-tags split across deltas
are still detected. This change is safe because:
- When a close tag IS found later, `buf = buf[close_idx + close_len:]`
discards everything before it (the reasoning), so accumulated
content is correctly dropped on close.
- When no close tag ever arrives (the bug scenario), the accumulated
content is available for flush() to recover the final answer.
**flush() change**: when `_in_block=True` at end-of-stream, look for
the last newline in `_buf` and emit whatever came after it as the
visible response. Reasoning models that follow the
reasoning-then-newline-then-answer pattern are recovered. If there is
NO newline (e.g. pure truncated reasoning with no answer), the
original discard-everything behaviour is preserved — this protects
the existing test contract:
_drive(s, ["<think>reasoning text with no close"]) == ""
_drive(s, ["<think>", "The user wants", " to know something"]) == ""
An opt-out knob `recover_unclosed_final_answer=False` is added on the
scrubber instance for plugins/users who want the strict discard
behaviour (e.g. when running against a gateway that emits properly
closed tags and the recovery is undesirable).
## Test coverage
4 new tests in `TestFlushBehaviour`:
- `test_flush_recovers_final_answer_after_newline_in_unclosed_block`:
the core bug repro — single delta with reasoning + newline + answer.
- `test_flush_recovers_final_answer_split_across_deltas`: same, but
reasoning and answer arrive in separate deltas.
- `test_flush_recovers_final_answer_multiline_reasoning`: multi-line
reasoning; recovery picks the LAST newline so only the final answer
is emitted.
- `test_flush_opt_out_recovers_unclosed_final_answer`: setting
`recover_unclosed_final_answer=False` restores the strict discard
behaviour.
All 31 existing tests in `test_think_scrubber.py` still pass
unchanged — the fix is backward-compatible because every existing
unterminated-block test case has no newline in the held-back buffer,
so the new "find last newline" logic falls through to the original
`return ""` path.
All 8 tests in `tests/cli/test_stream_delta_think_tag.py` also still
pass.
## Verification
End-to-end tested on a mos3 container with hermes-agent 0.17.0 and
the aicq-hermes plugin v1.2.4 (with the monkey-patch removed to
isolate the upstream fix):
# Before fix (vanilla 0.17.0):
hermes -z "What is 2+2? Reply briefly."
# -> "Empty response from model — retrying (1/3)" x3, then "❌ Model
# returned no content after all retries."
# After fix (this PR applied):
hermes -z "What is 2+2? Reply briefly."
# -> "4"
# 11-prompt regression suite via the aicq.me chat network:
# 11/11 PASS (math, capitals, translations, instruction-following)
The aicq-hermes plugin's monkey-patch shim (added in v1.2.4) is no
longer needed once this PR lands — the plugin will remove its shim in
a follow-up release.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Good fix for the think_scrubber to recover final answers from unclosed blocks. The change from holding back only a partial suffix to accumulating the full buffer is correct — it allows flush() to recover the final answer when the model inlines reasoning + answer inside an unclosed tag. The new flush() logic properly handles the recovery case.
Looks Good
- Correct behavioral change to handle edge case
- Well-documented with clear comments
- New flush() recovery logic is thorough
- No security concerns
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real current-main failure: agent/think_scrubber.py:125-130 drops unclosed-block content and flush() returns empty at :212-215, so the reported answer cannot be recovered today.
Problems
agent/think_scrubber.py:260-266emits the last line after any newline in an unclosed block. A truncated multiline reasoning trace is indistinguishable from the proposed gateway format at this layer, so this can expose reasoning. That conflicts with the established safety contract at current-mainagent/think_scrubber.py:204-215and the scrubber's original purpose in commit2a285d5ec.tests/agent/test_think_scrubber.py:195-248lacks the inverse case: an unclosed multiline reasoning-only trace must remain suppressed.
Suggested changes
- Keep strict discard as the generic behavior; only recover where a provider/gateway-specific signal can reliably identify the malformed inline-answer format.
- Add the multiline reasoning-only non-leak regression alongside the gateway reproduction.
Automated hermes-sweeper review.
| # Find the last newline; emit whatever came after it as | ||
| # the visible response. Reasoning models that follow the | ||
| # reasoning-then-newline-then-answer pattern are recovered. | ||
| last_nl = held.rfind("\n") |
There was a problem hiding this comment.
A newline does not establish that the following text is a final answer: an interrupted ordinary multiline `` trace has the identical shape. This would expose its last reasoning line, contrary to the existing strict non-leak contract. Please gate recovery on a signal that can distinguish this gateway format, or retain generic discard.
| ] | ||
| assert _drive(s, deltas) == "4" | ||
|
|
||
| def test_flush_recovers_final_answer_multiline_reasoning(self) -> None: |
There was a problem hiding this comment.
Please add the inverse regression here: an unclosed multiline reasoning-only trace must still return empty. The current tests establish recovery examples but do not protect the scrubber's existing no-reasoning-leak guarantee.
Problem
Some OpenAI-compatible LLM gateways (e.g. relays fronting MiniMax-M1 / Step-3.7-Flash / Intern-S2-Preview) inline the model''s reasoning AND the final answer inside
delta.contentwrapped in a single<think>open tag with NO matching</think>close. The model intends:鈥nd streams it as:
StreamingThinkScrubbersees the unclosed<think>as a truncated reasoning block and, atflush()time, discards EVERYTHING held back in the buffer 鈥?including the final answer "4" that came after the newline. The agent then sees an empty content string and reports"Empty response from model 鈥?retrying (1/3)".This was observed in production with the aicq-hermes plugin (v1.2.4) connecting to
http://aicq.online:3000/v1with modelmymodel. The workaround shipped in aicq-hermes v1.2.4 was a monkey-patch onStreamingThinkScrubber.flush; this PR upstreams the fix properly.Root cause
Two issues in
agent/think_scrubber.py:1.
feed()discarded block content in real timeWhen
_in_block=Trueand no close tag was found in the current delta, the code only held back a partial close-tag prefix (_max_partial_suffix) and discarded everything else. This meant reasoning + final answer prose was dropped in real time 鈥?by the timeflush()ran,_bufonly contained a few chars of partial close-tag prefix (usually empty), so there was nothing to recover from.2.
flush()unconditionally returned""for unterminated blocksThe docstring explicitly justified this as "leaking partial reasoning is worse than a truncated answer" 鈥?a reasonable default for truly truncated streams, but wrong for the "inline reasoning + answer, no close tag" pattern where the answer IS recoverable (it sits on the line after the reasoning prose).
Fix
feed()changeWhen
_in_block=Trueand no close tag is found in the current delta, accumulate the entire delta into_buf(instead of discarding everything except a partial close-tag prefix). The nextfeed()call prepends_bufto the new text and re-scans for close tags across the boundary, so partial close-tags split across deltas are still detected. This change is safe because:buf = buf[close_idx + close_len:]discards everything before it (the reasoning), so accumulated content is correctly dropped on close.flush()to recover the final answer.flush()changeWhen
_in_block=Trueat end-of-stream, look for the last newline in_bufand emit whatever came after it as the visible response. Reasoning models that follow thereasoning 鈫?newline 鈫?answerpattern are recovered. If there is NO newline (e.g. pure truncated reasoning with no answer), the original discard-everything behaviour is preserved 鈥?this protects the existing test contract:An opt-out knob
recover_unclosed_final_answer=Falseis added on the scrubber instance for plugins/users who want the strict discard behaviour (e.g. when running against a gateway that emits properly closed tags and the recovery is undesirable).Test coverage
4 new tests in
TestFlushBehaviour:test_flush_recovers_final_answer_after_newline_in_unclosed_block鈥?the core bug repro: single delta with reasoning + newline + answer.test_flush_recovers_final_answer_split_across_deltas鈥?same, but reasoning and answer arrive in separate deltas.test_flush_recovers_final_answer_multiline_reasoning鈥?multi-line reasoning; recovery picks the LAST newline so only the final answer is emitted.test_flush_opt_out_recovers_unclosed_final_answer鈥?settingrecover_unclosed_final_answer=Falserestores the strict discard behaviour.All 31 existing tests in
test_think_scrubber.pystill pass unchanged 鈥?the fix is backward-compatible because every existing unterminated-block test case has no newline in the held-back buffer, so the new "find last newline" logic falls through to the originalreturn ""path.All 8 tests in
tests/cli/test_stream_delta_think_tag.pyalso still pass.Verification
End-to-end tested on a mos3 container with hermes-agent 0.17.0 and the aicq-hermes plugin v1.2.4 (with the monkey-patch shim removed to isolate the upstream fix):
11-prompt regression suite via the aicq.me chat network (master user 1000008 鈫?hermes agent
ai_a0d038b2):Test prompts covered: arithmetic (2+2, 7+8, 12+30, 25脳4, 100-50), world capitals (France, Japan), translations (English鈫扴panish "Hello", English鈫扜erman "Goodbye"), and instruction-following ("Reply with: PONG", "What color is the sky? One word.").
The aicq-hermes plugin''s monkey-patch shim (added in v1.2.4) is no longer needed once this PR lands 鈥?the plugin will remove its shim in a follow-up release.
Related
reasoning_contentpopulated butcontentempty case (different root cause, same symptom).Checklist
flush()explains the new behaviour and the opt-out knob)python -m pytest tests/agent/test_think_scrubber.py tests/cli/test_stream_delta_think_tag.py -vand all 43 tests pass