From 3755dca7d6aa8b4da01cb3aec69fac8ede9c6c3c Mon Sep 17 00:00:00 2001 From: fangliquan Date: Thu, 3 Sep 2026 14:31:16 +0800 Subject: [PATCH] fix(agent): handle usage-less empty responses --- agent/conversation_loop.py | 32 ++++++++++---- agent/empty_response_guard.py | 42 +++++++++++-------- tests/agent/test_empty_response_guard.py | 30 ++++++++++--- .../test_empty_terminal_reasoning_surface.py | 14 +++---- tests/run_agent/test_run_agent.py | 26 ++++++++---- 5 files changed, 98 insertions(+), 46 deletions(-) diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 50d49be442..b6978abbc8 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -4609,6 +4609,11 @@ def run_conversation( "error": "First response truncated due to output length limit" } + # Count every completed provider attempt, including providers + # that omit usage. Token/cost accounting below remains gated on + # real usage, but the request itself must stay observable. + agent.session_api_calls += 1 + # Track actual token usage from response for context management if hasattr(response, 'usage') and response.usage: canonical_usage = normalize_usage( @@ -4783,7 +4788,6 @@ def run_conversation( agent.session_prompt_tokens += prompt_tokens agent.session_completion_tokens += completion_tokens agent.session_total_tokens += total_tokens - agent.session_api_calls += 1 agent.session_input_tokens += canonical_usage.input_tokens agent.session_output_tokens += canonical_usage.output_tokens agent.session_cache_read_tokens += canonical_usage.cache_read_tokens @@ -4930,6 +4934,15 @@ def run_conversation( f"{cached:,}/{prompt:,} tokens " f"({hit_pct:.0f}% hit, {written:,} written)" ) + else: + logger.info( + "API call #%d: model=%s provider=%s in=? out=? total=? " + "latency=%.1fs usage=unavailable", + agent.session_api_calls, + agent.model, + agent.provider or "unknown", + api_duration, + ) _retry.has_retried_429 = False # Reset on success # Note: don't clear the retry buffer here — an "API call @@ -8552,6 +8565,7 @@ def run_conversation( agent, finish_reason=finish_reason, response=response, + observed_generation=_has_structured, ) _empty_retry_budget = ( _empty_guard.empty_retry_budget(agent, response) @@ -8619,15 +8633,14 @@ def run_conversation( if _truly_empty and _deterministic_empty: logger.warning( - "Deterministic empty response detected " - "(consecutive zero-output completions, " - "model=%s provider=%s finish_reason=%s) — " + "Repeated empty response detected " + "(model=%s provider=%s finish_reason=%s) — " "skipping remaining retries", agent.model, agent.provider, finish_reason, ) agent._buffer_status( - "⚠️ Model is deterministically returning empty " - "(zero output tokens) — skipping further retries " + "⚠️ Model is repeatedly returning empty content — " + "skipping further retries " "to avoid repeat charges" ) @@ -8748,7 +8761,12 @@ def run_conversation( "answer:\n\n" + reasoning_preview ) else: - final_response = "(empty)" + final_response = agent._format_turn_completion_explanation( + _turn_exit_reason + ) or ( + "⚠️ No reply: the model returned empty content after " + "all retries and fallback attempts." + ) break # Reset retry counter/signature on successful content diff --git a/agent/empty_response_guard.py b/agent/empty_response_guard.py index fbde4b58b0..293d677bfd 100644 --- a/agent/empty_response_guard.py +++ b/agent/empty_response_guard.py @@ -15,15 +15,13 @@ like this). Two independent guards, both failing OPEN to today's behaviour: -1. **Deterministic-empty detection** — two consecutive empty attempts, - both with usage present and ``output_tokens == 0``, from the same - (model, provider, finish_reason), are treated as deterministic: the - same prompt will keep producing the same empty. Remaining retries are - skipped and the loop proceeds straight to the fallback chain (a - different model may behave differently). Attempts with missing usage - or ``output_tokens > 0`` (model generated *something* — think-block - stripping, whitespace, flaky decoding) never classify as deterministic - and keep the full retry budget. +1. **Deterministic-empty detection** — two consecutive empty attempts from + the same (model, provider, finish_reason) are treated as deterministic + when usage proves zero output, or when usage is absent and the assembled + responses contain neither content nor reasoning. Remaining retries are + skipped and the loop proceeds straight to the fallback chain (a different + model may behave differently). Mixed evidence or any generated tokens keep + the full retry budget. 2. **Cost-aware retry budget** — when the estimated input cost of a single empty attempt exceeds the configured threshold (default @@ -78,6 +76,7 @@ class EmptyAttempt: finish_reason: str usage_present: bool zero_output: bool + observed_generation: bool @property def signature(self) -> tuple: @@ -201,7 +200,13 @@ def _zero_output(agent: Any, response: Any) -> tuple: return (True, (output + reasoning) == 0) -def record_empty_attempt(agent: Any, *, finish_reason: str, response: Any) -> None: +def record_empty_attempt( + agent: Any, + *, + finish_reason: str, + response: Any, + observed_generation: bool = True, +) -> None: """Record one empty completion in the current streak. Must be called before ``_empty_content_retries`` is incremented for @@ -222,6 +227,7 @@ def record_empty_attempt(agent: Any, *, finish_reason: str, response: Any) -> No finish_reason=str(finish_reason or ""), usage_present=usage_present, zero_output=zero_output, + observed_generation=bool(observed_generation), ) ) @@ -234,10 +240,10 @@ def record_empty_attempt(agent: Any, *, finish_reason: str, response: Any) -> No def deterministic_empty(agent: Any) -> bool: """True when the current streak looks deterministic. - Requires >= 2 consecutive attempts, ALL with usage present, zero - output tokens, and an identical (model, provider, finish_reason) - signature. Any attempt with missing usage or non-zero output keeps - this False (fail open — transients deserve their retries). + Requires >= 2 consecutive attempts with an identical (model, provider, + finish_reason) signature. Usage-backed attempts must all prove zero output. + Usage-absent attempts must all have no observed content or reasoning. Mixed + evidence fails open so ambiguous transients keep their retries. """ if not guard_enabled(agent): return False @@ -245,10 +251,12 @@ def deterministic_empty(agent: Any) -> bool: if len(attempts) < 2: return False first = attempts[0] - return all( - a.usage_present and a.zero_output and a.signature == first.signature - for a in attempts + same_signature = all(a.signature == first.signature for a in attempts) + usage_proves_empty = all(a.usage_present and a.zero_output for a in attempts) + response_proves_empty = all( + not a.usage_present and not a.observed_generation for a in attempts ) + return same_signature and (usage_proves_empty or response_proves_empty) def empty_retry_budget(agent: Any, response: Any) -> int: diff --git a/tests/agent/test_empty_response_guard.py b/tests/agent/test_empty_response_guard.py index 3ec1a0c437..c8e6f0dac4 100644 --- a/tests/agent/test_empty_response_guard.py +++ b/tests/agent/test_empty_response_guard.py @@ -5,7 +5,8 @@ deterministic empties (unsignaled provider refusals with zero output tokens) while never tightening behaviour on ambiguous evidence. Fail-open contract under test: -- Missing usage -> never deterministic, default budget. +- Missing usage + no observed generation -> deterministic after two attempts. +- Missing usage + observed reasoning -> never deterministic. - Any generated tokens (output or reasoning) -> never deterministic. - Different model/provider/finish_reason across attempts -> not deterministic. - Guard disabled via config (agent.empty_response_guard.enabled: false) -> @@ -42,11 +43,21 @@ def _response(prompt_tokens=25_900, completion_tokens=0, usage_present=True): return SimpleNamespace(usage=usage) -def _record_streak(agent, responses, finish_reasons=None): +def _record_streak( + agent, responses, finish_reasons=None, observed_generations=None +): """Record attempts the way the loop does: record, then increment.""" finish_reasons = finish_reasons or ["stop"] * len(responses) - for resp, reason in zip(responses, finish_reasons): - guard.record_empty_attempt(agent, finish_reason=reason, response=resp) + observed_generations = observed_generations or [False] * len(responses) + for resp, reason, observed_generation in zip( + responses, finish_reasons, observed_generations + ): + guard.record_empty_attempt( + agent, + finish_reason=reason, + response=resp, + observed_generation=observed_generation, + ) agent._empty_content_retries += 1 @@ -62,12 +73,21 @@ class TestDeterministicEmpty: _record_streak(agent, [_response()]) assert guard.deterministic_empty(agent) is False - def test_missing_usage_fails_open(self): + def test_missing_usage_without_observed_generation_is_deterministic(self): agent = _agent() _record_streak( agent, [_response(usage_present=False), _response(usage_present=False)], ) + assert guard.deterministic_empty(agent) is True + + def test_missing_usage_with_observed_reasoning_fails_open(self): + agent = _agent() + _record_streak( + agent, + [_response(usage_present=False), _response(usage_present=False)], + observed_generations=[True, True], + ) assert guard.deterministic_empty(agent) is False def test_mixed_usage_presence_fails_open(self): diff --git a/tests/run_agent/test_empty_terminal_reasoning_surface.py b/tests/run_agent/test_empty_terminal_reasoning_surface.py index 7cfd1aea02..3b56144cfe 100644 --- a/tests/run_agent/test_empty_terminal_reasoning_surface.py +++ b/tests/run_agent/test_empty_terminal_reasoning_surface.py @@ -11,7 +11,7 @@ Invariants pinned here: ``_empty_terminal_sentinel`` marker (replay semantics unchanged). - Raw reasoning is NEVER promoted earlier in the ladder — a reasoning-only response still goes through prefill continuation first. -- A truly empty exhaustion (no reasoning either) still returns "(empty)". +- A truly empty exhaustion returns an actionable no-reply explanation. """ from __future__ import annotations @@ -109,11 +109,11 @@ def test_exhausted_reasoning_only_delivers_labeled_excerpt(tmp_path, monkeypatch ) -def test_exhausted_truly_empty_keeps_existing_behavior(tmp_path, monkeypatch): - """No reasoning anywhere → behavior unchanged from main: the '(empty)' - terminal (possibly rewritten by the downstream turn-completion explainer) - is delivered, and no reasoning excerpt appears.""" +def test_exhausted_truly_empty_delivers_no_reply_explanation(tmp_path, monkeypatch): + """No reasoning anywhere produces an honest terminal explanation even + when the downstream completion explainer is disabled.""" agent = _build_agent(tmp_path, monkeypatch) + monkeypatch.setattr(agent, "_turn_completion_explainer_enabled", lambda: False) monkeypatch.setattr( agent, "_interruptible_api_call", lambda api_kwargs: _truly_empty_response(), @@ -122,9 +122,7 @@ def test_exhausted_truly_empty_keeps_existing_behavior(tmp_path, monkeypatch): result = agent.run_conversation("hello?") final = result["final_response"] - # Either the raw sentinel (explainer off) or the explainer's rewrite — - # never the reasoning-excerpt frame, which requires reasoning to exist. - assert final == "(empty)" or final.startswith("⚠️ No reply:") + assert final.startswith("⚠️ No reply:") assert "only internal reasoning" not in final diff --git a/tests/run_agent/test_run_agent.py b/tests/run_agent/test_run_agent.py index 05af3b8d1d..3c03e5cde1 100644 --- a/tests/run_agent/test_run_agent.py +++ b/tests/run_agent/test_run_agent.py @@ -3513,12 +3513,12 @@ class TestRunConversation: assert result["api_calls"] == 6 # 1 original + 2 prefill + 3 retries - def test_truly_empty_response_retries_3_times_then_empty(self, agent): - """Truly empty response (no content, no reasoning) retries 3 times then falls through to (empty).""" + def test_truly_empty_response_stops_after_repeated_empty(self, agent): + """Repeated empty responses stop after one retry and return an explanation.""" self._setup_agent(agent) agent.base_url = "http://127.0.0.1:1234/v1" empty_resp = _mock_response(content=None, finish_reason="stop") - # 4 responses: 1 original + 3 nudge retries, all empty + # Extra responses prove the guard stops consuming after repetition. agent.client.chat.completions.create.side_effect = [ empty_resp, empty_resp, empty_resp, empty_resp, ] @@ -3532,7 +3532,7 @@ class TestRunConversation: # #34452: explanation replaces the bare "(empty)" sentinel. assert result["final_response"] != "(empty)" assert "No reply:" in result["final_response"] - assert result["api_calls"] == 4 # 1 original + 3 retries + assert result["api_calls"] == 2 # 1 original + 1 retry def test_deterministic_empty_stops_retries_early(self, agent): """NS-503: consecutive zero-output-token empties with identical @@ -3588,10 +3588,11 @@ class TestRunConversation: assert result["completed"] is True assert result["api_calls"] == 4 # legacy: 1 original + 3 retries - def test_empty_without_usage_keeps_full_retry_budget(self, agent): - """NS-503 fail-open: no usage data means no evidence of a - deterministic empty — legacy 3-retry behaviour must be preserved - (this is the flaky-provider case retries exist for).""" + def test_empty_without_usage_stops_after_one_retry_and_logs_calls( + self, agent, caplog + ): + """Two complete empty responses are enough evidence to stop even when + the provider omits usage; both attempts remain observable.""" self._setup_agent(agent) agent.base_url = "http://127.0.0.1:1234/v1" empty_resp = _mock_response(content=None, finish_reason="stop") @@ -3600,10 +3601,17 @@ class TestRunConversation: patch.object(agent, "_persist_session"), patch.object(agent, "_save_trajectory"), patch.object(agent, "_cleanup_task_resources"), + patch.object( + agent, "_turn_completion_explainer_enabled", return_value=False + ), + caplog.at_level(logging.INFO, logger="agent.conversation_loop"), ): result = agent.run_conversation("answer me") assert result["completed"] is True - assert result["api_calls"] == 4 # unchanged: 1 original + 3 retries + assert result["api_calls"] == 2 + assert agent.session_api_calls == 2 + assert result["final_response"].startswith("⚠️ No reply:") + assert caplog.text.count("usage=unavailable") == 2 def test_truly_empty_response_succeeds_on_nudge(self, agent): """Model produces content after being nudged for empty response."""