fix(agent): emit recovery-path final response through stream_delta_callback - #31477
fix(agent): emit recovery-path final response through stream_delta_callback#31477briandevans wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR addresses a regression where, on certain “empty final response” exit paths, the agent returns a recovered final_response but fails to emit that text through stream_delta_callback, causing SSE/TUI clients to see a finish chunk with no content.
Changes:
- Emit recovered/prior-turn final text via
agent.stream_delta_callback(...)forpartial_stream_recoveryandfallback_prior_turn_contentexit reasons. - Add regression tests ensuring the recovered text is streamed and the callback is closed (
None) for both exit paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/run_agent/test_run_agent.py | Adds regression tests to validate recovered responses are streamed and callback is closed. |
| agent/conversation_loop.py | Pushes recovered/prior-turn final text through stream_delta_callback before breaking out of the loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Callback should also have been closed so the SSE writer can drain. | ||
| assert deltas[-1] is None, ( | ||
| f"stream_delta_callback was not closed with None; deltas={deltas!r}" | ||
| ) |
| assert deltas[-1] is None, ( | ||
| f"stream_delta_callback was not closed with None; deltas={deltas!r}" | ||
| ) |
| if final_response and agent.stream_delta_callback: | ||
| try: | ||
| agent.stream_delta_callback(final_response) | ||
| agent.stream_delta_callback(None) | ||
| except Exception: | ||
| pass |
| if final_response and agent.stream_delta_callback: | ||
| try: | ||
| agent.stream_delta_callback(final_response) | ||
| agent.stream_delta_callback(None) | ||
| except Exception: | ||
| pass |
| # Push the recovered text through the streaming | ||
| # callback so SSE/TUI clients see it before the | ||
| # finish chunk lands. The recovered text wasn't | ||
| # necessarily streamed in full this turn — the | ||
| # current SSE writer drains an empty queue and | ||
| # emits a finish chunk with zero content delta | ||
| # otherwise, indistinguishable from a crash | ||
| # (#31449, mirrors the guardrail-halt site in | ||
| # #31448). |
| # Push the prior-turn content through the streaming | ||
| # callback so SSE/TUI clients see it before the | ||
| # finish chunk lands. The text was streamed on the | ||
| # *previous* SSE response, so the current SSE writer | ||
| # drains an empty queue otherwise (#31449, mirrors | ||
| # the guardrail-halt site in #31448). |
alt-glitch
added
type/bug
|
Competes with #31478 for fixing #31449. Both address recovery-path text not streamed to SSE/TUI clients. This PR covers partial_stream_recovery + fallback_prior_turn_content with inline callbacks and |
|
@copilot All review findings addressed in f61a584e7:
One deliberate hold: the empty- The 3 failing CI tests ( |
f61a584 to
3332d5e
Compare
3332d5e to
ea765ae
Compare
ea765ae to
7bc8fbb
Compare
7bc8fbb to
00467c9
Compare
00467c9 to
3412031
Compare
3412031 to
755334b
Compare
755334b to
5203da0
Compare
5203da0 to
fa311dc
Compare
fa311dc to
a9b76e2
Compare
e5c78ed to
6a95677
Compare
6a95677 to
619e69c
Compare
619e69c to
49f9de7
Compare
49f9de7 to
0d05ec3
Compare
0d05ec3 to
d5a00ed
Compare
d5a00ed to
93958cb
Compare
93958cb to
5440566
Compare
5440566 to
8cdf7cd
Compare
8cdf7cd to
1e01604
Compare
1e01604 to
6e8a67b
Compare
6e8a67b to
5824803
Compare
5824803 to
35d0037
Compare
|
@copilot All findings addressed.
CI reds on this PR are baseline-only (test_compression_concurrent_sessions lock-serialization + s6-overlay build 504), not in touched code. |
|
@alt-glitch heads-up — #31478 was closed on 2026-06-04 without merging, so this is now the only open fix remaining for #31449. It's green across CI and covers both recovery-path sites that weren't streaming to SSE/TUI clients (partial_stream_recovery + the fallback branch), routed through a single shared flush helper. Ready for review whenever you have a moment. |
…llback When the conversation loop exits via `partial_stream_recovery` (~L3566) or `fallback_prior_turn_content` (~L3593), `final_response` is assigned to text that was not (fully) streamed this turn: - `partial_stream_recovery` recovers from `_current_streamed_assistant_text` after a partial stream died — only what landed before the disconnect was streamed. - `fallback_prior_turn_content` reuses the previous turn's user-visible content (delivered alongside housekeeping tool calls) — that text was streamed on the *previous* SSE response, so the current SSE writer drains an empty queue. Both sites then `break` out of the loop without pushing the assembled text through `agent.stream_delta_callback`. SSE/TUI clients see a finish chunk with zero content delta — indistinguishable from a crash for Open WebUI and similar clients. `_response_was_previewed = True` is already set, so the gateway also suppresses its own final send, making the user-visible failure complete. Mirrors the pattern teknium1 used at the guardrail-halt site in NousResearch#31448: push `final_response` and then `None` through the callback inside a try/except. Unlike NousResearch#31448, no `_safe_print` here — the CLI has already seen the partial / prior-turn content on screen, and re-printing would look like a stray echo. Tests cover both sites and assert (a) the recovered text reaches the callback as a text delta, (b) the callback is closed with `None` so the SSE writer can drain, and (c) `response_previewed` is still set so the gateway suppresses duplicate delivery. Refs NousResearch#31449
Addresses Copilot review on NousResearch#31477: - Extract `_flush_synthesized_final_to_stream(agent, text)` helper to remove the duplicated emit-then-close logic across the partial_stream_recovery and fallback_prior_turn_content branches. - Use `try/finally` so the `None` close sentinel always fires when `callback(text)` raises — SSE writers still drain instead of hanging on a half-emitted recovery turn. - Split emit-text and close into independent `try/except` blocks so a single bad callback can't break the surrounding turn-exit flow; both log at debug level via `logger.debug(..., exc_info=True)` instead of silently swallowing exceptions. - Robustify regression tests so `deltas[-1] is None` only runs when `deltas` is non-empty — failing the intended assertion with a clear message instead of an `IndexError` traceback if the callback is never invoked. Behavior is unchanged for non-empty `final_response`. Empty `final_response` is still a no-op (no synthesized text to deliver and no streamed content this turn that would otherwise be left dangling).
|
Re-verified against current |
What does this PR do?
Closes the same blank-stream gap teknium1 fixed at the guardrail-halt site in #31448, but at the two structurally-identical sibling sites in
agent/conversation_loop.pythat #31449 calls out:partial_stream_recovery(~L3566) —final_response = _recovered(text reconstructed from a partial stream). The recovered text wasn't necessarily streamed in full this turn.fallback_prior_turn_content(~L3593) —final_response = agent._strip_think_blocks(fallback).strip()wherefallbackis the previous turn's content. That text was streamed on the previous SSE response, so the current SSE writer drains an empty queue.Both sites set
_response_was_previewed = True(so the gateway suppresses its own final send) and thenbreak— but never actually push the assembled text throughagent.stream_delta_callback. SSE/TUI clients see a finish chunk with zero content delta, indistinguishable from a crash for Open WebUI and similar clients.The fix mirrors teknium1's pattern from #31448: push
final_responseand thenNonethrough the callback inside a try/except, immediately beforebreak. No_safe_printhere — unlike the guardrail-halt site, the CLI has already seen the partial / prior-turn content on screen, and re-printing would look like a stray echo.Related Issue
Fixes #31449
Type of Change
Changes Made
agent/conversation_loop.py— at bothpartial_stream_recoveryandfallback_prior_turn_contentsites, pushfinal_responseand thenNonethroughstream_delta_callbackbeforebreak. Mirrors the try/except pattern from fix(agent): emit guardrail-halt message to client before closing stream #31448.tests/run_agent/test_run_agent.py— two new tests underTestRunConversation:test_partial_stream_recovery_emits_final_response_through_stream_callbacktest_fallback_prior_turn_content_emits_final_response_through_stream_callbackEach asserts (a) the recovered text reaches the callback as a text delta, (b) the callback is closed with
Noneso the SSE writer can drain, and (c)response_previewedremainsTrueso the gateway suppresses its own final send.How to Test
response_previewedconsumer side):response_previewedlogic still triggers, so this fix does not introduce duplicate delivery for streamed clients.Checklist
Code
fix(agent): ...)Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/ARelated / Positioning
This is the explicit follow-up issue teknium1 filed in #31449 to keep #31448 narrowly scoped to the reported guardrail-halt bug. The recovery-path sites have the same failure shape (blank Open WebUI bubble) but trigger less frequently, so they're worth a separate PR rather than bundling onto #31448.