fix: drop messages-None writeback guard, reframe for invariant lineage
Per Greptile's review on #5717: the previous `if result.get('messages') is not None` guard created an asymmetric condition where the silent-failure classifier could miss the injected fallback when the agent result had `messages: None` (which never actually happens in practice since `finalize_turn` always returns a list, but the asymmetry read as code smell). Drop the guard; the writeback is now unconditional on `isinstance(result, dict)`. Per the maintainer's review on #5717: there may be a `finalize_turn` invariant on the agent branch this WebUI ships against that already appends the fallback as an assistant row, in which case the live bug is gone and this PR becomes a regression guard. Reframe the test docstrings as dual-purpose pins of the user- visible closure-text contract so they read correctly under both interpretations. Co-authored-by: b3nw <b3nw@users.noreply.github.com>
This commit is contained in:
1 parent
02692cd64a
commit
34fcc3a387
2 files changed
+21
-8
No files matched your search
+7
-3
@@ -8278,14 +8278,18 @@ def _run_agent_streaming(
|
||||
# message but result['final_response'] carries a graceful fallback
|
||||
# string, inject it as a final assistant turn so the user sees
|
||||
# closure text instead of a bare tool_limit_reached error. Apply
|
||||
# the synthesis to result['messages'] AND _result_messages so
|
||||
# the synthesis to result['messages'] AND _result_messages so the
|
||||
# downstream _all_result_messages checks (silent-failure detection
|
||||
# at #all_result_messages on the same turn) see the fallback too.
|
||||
# at api/streaming.py:_assistant_reply_added_after_current_turn)
|
||||
# see the fallback too. `finalize_turn` in the agent always returns
|
||||
# messages as a list, but we write back unconditionally so the
|
||||
# contract is "if we built a result-messages list, the silent-failure
|
||||
# classifier reads the augmented version."
|
||||
if _tool_limit_reached:
|
||||
_result_messages = _maybe_inject_max_iteration_summary_fallback(
|
||||
_result_messages, result
|
||||
)
|
||||
if isinstance(result, dict) and result.get('messages') is not None:
|
||||
if isinstance(result, dict):
|
||||
result = {**result, 'messages': _result_messages}
|
||||
if cancel_event.is_set():
|
||||
_finalize_cancelled_turn(s, ephemeral=False)
|
||||
|
||||
@@ -281,9 +281,14 @@ def test_streaming_tool_limit_without_final_answer_emits_no_final_apperror(tmp_p
|
||||
|
||||
|
||||
def test_streaming_tool_limit_with_fallback_final_response_surfaces_closure_text(tmp_path, monkeypatch):
|
||||
"""#5494 — when the agent produced no usable summary but returned a graceful
|
||||
fallback string (handle_max_iterations() always returns one), inject that as a
|
||||
final assistant turn so the user sees closure text instead of a bare error.
|
||||
"""#5494 — handle_max_iterations() guarantees a non-empty ``final_response``
|
||||
on iteration-limit exhaustion. This test pins the WebUI contract that,
|
||||
when ``messages`` ends without a final assistant turn and ``final_response``
|
||||
is set, the user sees that closure text instead of a bare
|
||||
``tool_limit_reached`` error. Serves both as a live-bug fix pin and as a
|
||||
regression guard for the agent's "delivered final_response ⇒ assistant
|
||||
row" invariant: if a future agent regression drops that invariant, this
|
||||
test still passes because the WebUI honors the contract locally.
|
||||
"""
|
||||
graceful = "I reached the iteration limit and couldn't generate a summary."
|
||||
result = {
|
||||
@@ -300,6 +305,9 @@ def test_streaming_tool_limit_with_fallback_final_response_surfaces_closure_text
|
||||
events, payload = _run_streaming_with_fake_agent(tmp_path, monkeypatch, result)
|
||||
|
||||
# The user sees the graceful fallback, not a bare tool_limit_reached error.
|
||||
# Either (a) we synthesized the fallback here, or (b) the agent guarantee
|
||||
# already added an assistant row and `_mark_latest_assistant_tool_limit_status`
|
||||
# attached the status card. Both routes satisfy the contract.
|
||||
assert not [
|
||||
ap for ev, ap in events if ev == "apperror"
|
||||
and ap.get("type") == "tool_limit_reached"
|
||||
@@ -323,8 +331,9 @@ def test_streaming_tool_limit_with_fallback_final_response_surfaces_closure_text
|
||||
|
||||
|
||||
def test_streaming_tool_limit_with_fallback_does_not_double_inject_when_assistant_exists(tmp_path, monkeypatch):
|
||||
"""#5494 — when the agent already appended a model-generated summary, the
|
||||
fallback flow is a no-op; the existing summary is preserved as before.
|
||||
"""#5494 — when the agent already appended a model-generated summary
|
||||
AND ``final_response`` carries the same text, the WebUI must not duplicate
|
||||
the assistant turn. Pins the no-op contract on the synthesis path.
|
||||
"""
|
||||
summary = "I reached the limit; here is the summary."
|
||||
result = {
|
||||
|
||||
Reference in new issue
Block a user