fix: cover the attempt-line verdict through handle_api_error
The retryable kwarg only reached log_api_error_attempt from turn_api_error.handle_api_error, yet both tests called the helper directly, so dropping the forwarding line kept the suite green. Replace the retryable=True direct-call test with one that drives the production entry (classifier patched to a non-retryable 401) and asserts 'attempt 1/3, not retryable' on the log and status buffer: red with the kwarg removed, green with it restored. Also note why _touch_activity keeps the plain counter: it is a watchdog liveness label, not chat output.
This commit is contained in:
@@ -136,6 +136,8 @@ def handle_api_error(
|
||||
|
||||
retry_count += 1
|
||||
elapsed_time = time.time() - api_start_time
|
||||
# Liveness/watchdog label only (never shown in chat), so the classifier's
|
||||
# "not retryable" verdict is named on the logged attempt line below instead.
|
||||
agent._touch_activity(f"API error recovery (attempt {retry_count}/{max_retries})")
|
||||
|
||||
error_type, error_msg, _provider, _base, _model = log_api_error_attempt(
|
||||
|
||||
@@ -7,7 +7,8 @@ counter that failed to advance (#73237). The classifier's verdict now rides the
|
||||
same line on both surfaces (logger + buffered status trace)."""
|
||||
|
||||
import logging
|
||||
from unittest.mock import MagicMock
|
||||
import time
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from agent.turn_recovery import log_api_error_attempt
|
||||
|
||||
@@ -37,9 +38,30 @@ def test_non_retryable_failure_is_named_on_log_and_status_line(caplog):
|
||||
assert "not retryable" in agent._buffer_vprint.call_args_list[0].args[0]
|
||||
|
||||
|
||||
def test_retryable_failure_keeps_the_plain_attempt_counter(caplog):
|
||||
def test_production_entry_forwards_the_classifier_verdict_to_the_attempt_line(caplog):
|
||||
"""``handle_api_error`` must hand ``classified.retryable`` to the log line; a bare
|
||||
``attempt 1/3`` there is the exact regression #73237 reported."""
|
||||
from types import SimpleNamespace
|
||||
|
||||
from agent.error_classifier import ClassifiedError, FailoverReason
|
||||
from agent.turn_api_error import handle_api_error
|
||||
|
||||
agent = _agent()
|
||||
with caplog.at_level(logging.WARNING, logger="agent.conversation_loop"):
|
||||
_call(agent, retryable=True)
|
||||
assert "(attempt 1/3)" in caplog.text
|
||||
assert "not retryable" not in caplog.text
|
||||
agent._interrupt_requested = True # leave the loop right after the attempt line
|
||||
agent.clear_interrupt.return_value = True
|
||||
verdict = ClassifiedError(reason=FailoverReason.auth, status_code=401, retryable=False)
|
||||
with patch("agent.turn_api_error.classify_api_error", return_value=verdict), patch(
|
||||
"agent.turn_api_error.recover_before_classification", return_value=(False, "sys")
|
||||
), patch(
|
||||
"agent.turn_api_error.recover_after_classification", return_value=(False, False)
|
||||
), caplog.at_level(logging.WARNING, logger="agent.conversation_loop"):
|
||||
out = handle_api_error(
|
||||
agent, api_error=RuntimeError("401"), _retry=SimpleNamespace(), thinking_spinner=None,
|
||||
messages=[], api_messages=[], api_kwargs={}, system_message=None,
|
||||
active_system_prompt="sys", conversation_history=[], approx_tokens=10, retry_count=0,
|
||||
max_retries=3, compression_attempts=0, max_compression_attempts=1, api_call_count=1,
|
||||
api_request_id="r", api_start_time=time.time(), effective_task_id=None, turn_id="t",
|
||||
)
|
||||
assert out.action == "break"
|
||||
assert "attempt 1/3, not retryable" in caplog.text
|
||||
assert "not retryable" in agent._buffer_vprint.call_args_list[0].args[0]
|
||||
|
||||
Reference in New Issue
Block a user