diff --git a/anton/chat.py b/anton/chat.py index c4322375..c8a6a4ae 100644 --- a/anton/chat.py +++ b/anton/chat.py @@ -20,7 +20,7 @@ replace_at_image_paths, save_clipboard_image, ) -from anton.core.session import ChatSession, ChatSessionConfig +from anton.core.session import ChatSession, ChatSessionConfig, _is_provider_auth_error from anton.core.llm.prompt_builder import SystemPromptContext from anton.core.llm.provider import ( EndpointConfigurationError, @@ -1279,6 +1279,24 @@ def _desktop_greeting(console: Console, settings) -> None: +def _default_turn_error_action(exc: BaseException) -> str: + """Which action the turn-failure prompt should default to. + + ModelUnavailableError (plan/kill-switch gate) and EndpointConfigurationError + (wrong base URL / route) are deterministic for the identical request — + retrying re-sends a doomed call — so the default steers to "setup" (switch + model / provider / fix endpoint). A provider-auth 401 (ENG-1310) is + equally deterministic — see `_is_provider_auth_error` — and is included + here for the same reason: three past PRs (#236, #247, #288) flagged that a + bare ConnectionError defaults to "retry" even when the failure can't + possibly be fixed by retrying (review feedback on ENG-1310). Any other + ConnectionError (transient) keeps "retry" as the default. + """ + if isinstance(exc, (ModelUnavailableError, EndpointConfigurationError)) or _is_provider_auth_error(exc): + return "setup" + return "retry" if isinstance(exc, ConnectionError) else "setup" + + def run_chat( console: Console, settings: AntonSettings, *, resume: bool = False, first_run: bool = False, desktop_first_run: bool = False ) -> None: @@ -2017,17 +2035,7 @@ def _bottom_toolbar(): " (anton) Switch LLM provider, update API key, or retry?", choices=["setup", "retry", "s", "r"], choices_display="setup/retry", - # ModelUnavailableError (plan/kill-switch gate) and - # EndpointConfigurationError (wrong base URL / route) are both - # deterministic for the identical request — retrying re-sends a - # doomed call — so steer the default to "setup" (switch model / - # provider / fix endpoint). Other ConnectionErrors (transient) - # keep retry as the default. - default=( - "setup" - if isinstance(exc, (ModelUnavailableError, EndpointConfigurationError)) - else ("retry" if isinstance(exc, ConnectionError) else "setup") - ), + default=_default_turn_error_action(exc), ) if choice in ("setup", "s"): session = await handle_setup_models( diff --git a/anton/core/session.py b/anton/core/session.py index 65c40b80..b398bbac 100644 --- a/anton/core/session.py +++ b/anton/core/session.py @@ -366,6 +366,21 @@ def _safe_error_detail(exc: BaseException) -> str: return name +def _is_provider_auth_error(exc: BaseException) -> bool: + """A provider-auth 401 — anton's "Invalid API key — …" copy from + `openai.py`/`anthropic.py` (ENG-1310): the credential is wrong, not the + request, so retrying can't succeed either. The substring match mirrors + cowork-server's `turn_errors.is_auth_error()`; the `isinstance` check is + an anton-only narrowing on top of it (both 401 raise sites always type + it this way, so it's a no-op in practice) — anything else (a bare + "temporarily unavailable" ConnectionError) is a different failure. + + Shared by both `turn_stream` re-raise sites so the check can't drift + between them (review feedback on ENG-1310). + """ + return isinstance(exc, ConnectionError) and "invalid api key" in str(exc).lower() + + # Shared closing instruction for every path that hands control back to the # user (STUCK, budget-exhausted, verifier-call failure): a plain self- # assessment of solvability, not just a status dump. Without this, a @@ -2912,6 +2927,11 @@ async def _turn_stream_inner( ): raise + # Same reasoning applies to a provider-auth 401 (ENG-1310) + # — see _is_provider_auth_error. + if _is_provider_auth_error(_agent_exc): + raise + # ENG-673: a mid-stream transient failure that had NO prior # retry (overload smuggled into a 200, or a truncated stream). # Back off and retry the SAME step within a per-turn budget — @@ -3025,16 +3045,35 @@ async def _turn_stream_inner( if isinstance(event, StreamTextDelta): assistant_text_parts.append(event.text) yield event - except (TokenLimitExceeded, ModelUnavailableError): - # Curated provider failures must FAIL the turn, not - # get wrapped into assistant prose: the server maps - # them to actionable error cards (token_limit / - # model-unavailable), which can only fire when the - # exception propagates. Wrapping them as text is - # how "Server returned 403" ended up mid-chat with - # "please rephrase your request" advice. - raise except Exception as e: + if isinstance(e, (TokenLimitExceeded, ModelUnavailableError, EndpointConfigurationError)): + # Curated provider failures must FAIL the turn, not + # get wrapped into assistant prose: the server maps + # token_limit/model_unavailable to actionable cards, + # which can only fire when the exception propagates. + # Wrapping them as text is how "Server returned 403" + # ended up mid-chat with "please rephrase your + # request" advice. EndpointConfigurationError added + # here to match the immediate re-raise site above — + # this wrap-up call had been the one place it still + # fell through (review feedback on ENG-1310). NOTE: + # cowork-server has no dedicated card for + # EndpointConfigurationError yet (grepped — zero + # hits, friendly_turn_error falls through to the + # generic message for it); re-raising it here still + # stops the misleading "adjust your approach" prose, + # it just doesn't get a *better* card until that + # mapping exists server-side. + raise + if _is_provider_auth_error(e): + # Same reasoning for a provider-auth 401 — see + # _is_provider_auth_error. cowork-server's + # turn_errors.is_auth_error() matches this exact + # text and renders the "Reconnect MindsHub" / + # BYOK-key action card, but only if the exception + # propagates instead of being flattened into chat + # text here (ENG-1310). + raise fallback = f"An unexpected error occurred: {e}. Please try again or rephrase your request." assistant_text_parts.append(fallback) yield StreamTextDelta(text=fallback) diff --git a/tests/test_chat_error_action_default.py b/tests/test_chat_error_action_default.py new file mode 100644 index 00000000..efccacf5 --- /dev/null +++ b/tests/test_chat_error_action_default.py @@ -0,0 +1,37 @@ +"""`_default_turn_error_action` — which action the interactive CLI's +turn-failure prompt defaults to (review feedback on ENG-1310). + +ModelUnavailableError and EndpointConfigurationError are deterministic for +the identical request, so the CLI already steered them to "setup". A +provider-auth 401 is equally deterministic (ENG-1310 made it propagate +instead of flattening into chat text), but nothing steered its default away +from "retry" — a gap three past PRs (#236, #247, #288) flagged for this +exact ConnectionError-defaults-to-retry pattern. This pins the fix. +""" + +from __future__ import annotations + +from anton.chat import _default_turn_error_action +from anton.core.llm.provider import EndpointConfigurationError, ModelUnavailableError, TokenLimitExceeded + +_AUTH_ERROR_MESSAGE = "Invalid API key — check your OpenAI API key configuration." + + +def test_provider_auth_error_defaults_to_setup(): + assert _default_turn_error_action(ConnectionError(_AUTH_ERROR_MESSAGE)) == "setup" + + +def test_generic_connection_error_still_defaults_to_retry(): + assert _default_turn_error_action(ConnectionError("temporarily unavailable")) == "retry" + + +def test_model_unavailable_error_defaults_to_setup(): + assert _default_turn_error_action(ModelUnavailableError("blocked", code="model_access_denied", model="sonnet")) == "setup" + + +def test_endpoint_configuration_error_defaults_to_setup(): + assert _default_turn_error_action(EndpointConfigurationError("bad endpoint")) == "setup" + + +def test_non_connection_error_defaults_to_setup(): + assert _default_turn_error_action(TokenLimitExceeded("out of tokens")) == "setup" diff --git a/tests/test_session_auth_error_reraise.py b/tests/test_session_auth_error_reraise.py new file mode 100644 index 00000000..8e420695 --- /dev/null +++ b/tests/test_session_auth_error_reraise.py @@ -0,0 +1,168 @@ +"""ENG-1310 — a persistent provider-auth failure must propagate, not flatten. + +A `ConnectionError` (anton's "Invalid API key — …" copy for a 401 from the +LLM gateway, see `openai.py`/`anthropic.py`) used to fall into a generic +`except Exception` branch and get dumped into the chat as "An unexpected +error occurred: Invalid API key … Please try again or rephrase your +request." instead of reaching cowork-server's `turn_errors.is_auth_error()`, +which already renders the correct "Reconnect MindsHub" / BYOK-key card. + +Two sites in `turn_stream` needed the same auth-shaped check, mirroring how +ENG-1139 treats `EndpointConfigurationError` (also deterministic — retrying +can't fix it): + +1. The immediate re-raise at the top of the retry loop — an invalid key + fails on the FIRST attempt instead of burning the count-based retry + budget on doomed retries. +2. The retry-exhaustion fallback's own wrap-up call — belt-and-suspenders + for the case where retries were legitimately spent on a DIFFERENT + failure and the key only turns out to be bad on the final summary call. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from tests.conftest import make_mock_llm + +from anton.core.llm.provider import EndpointConfigurationError +from anton.core.session import ChatSession, ChatSessionConfig, _is_provider_auth_error + +_AUTH_ERROR_MESSAGE = "Invalid API key — check your OpenAI API key configuration." + + +@pytest.fixture() +def workspace(): + # Keep scratchpad venvs inside the repo workspace (pytest runs sandboxed + # and can't write to the real home directory). + base = Path(__file__).resolve().parents[1] / ".pytest-workspace" + base.mkdir(parents=True, exist_ok=True) + return MagicMock(base=base) + + +class _AlwaysRaisingPlanStream: + """`plan_stream` fake that raises the same exception on every call — + every retry attempt AND the final wrap-up call see the same failure, + the way a genuinely invalid key does.""" + + def __init__(self, exc: Exception): + self._exc = exc + self.calls = 0 + + def __call__(self, **kwargs): + self.calls += 1 + raise self._exc + + +class _ScriptedExceptionPlanStream: + """`plan_stream` fake that raises a scripted sequence of exceptions, one + per call, holding on the last entry once the script runs out — so a + fixed prefix (e.g. retries that legitimately exhaust the count-based + budget) can be followed by a different failure on the final call.""" + + def __init__(self, excs: list[Exception]): + self._excs = list(excs) + self.calls = 0 + + def __call__(self, **kwargs): + self.calls += 1 + idx = min(self.calls - 1, len(self._excs) - 1) + raise self._excs[idx] + + +async def _run_turn(session: ChatSession, prompt: str = "what's in my inbox?"): + events = [] + try: + async for event in session.turn_stream(prompt): + events.append(event) + finally: + await session.close() + return events + + +async def test_persistent_auth_failure_fails_immediately_without_wasting_retries(workspace): + """An invalid key can't be fixed by retrying — it must fail on the first + attempt, the same way EndpointConfigurationError (ENG-1139) does, not + after burning the count-based retry budget on doomed re-attempts.""" + mock_llm = make_mock_llm() + script = _AlwaysRaisingPlanStream(ConnectionError(_AUTH_ERROR_MESSAGE)) + mock_llm.plan_stream = script + session = ChatSession(ChatSessionConfig(llm_client=mock_llm, workspace=workspace)) + + with pytest.raises(ConnectionError, match="Invalid API key"): + await _run_turn(session) + + assert script.calls == 1, "an auth failure must not be retried" + + +async def test_auth_failure_on_the_final_wrapup_call_still_reraises(workspace): + """Retries legitimately exhaust on a DIFFERENT, retryable failure — the + key only turns out to be bad on the retry-exhaustion fallback's own + wrap-up call. That must still propagate instead of flattening into chat + text, even though the auth error never triggered the fast-fail path + above.""" + mock_llm = make_mock_llm() + script = _ScriptedExceptionPlanStream( + [RuntimeError("boom"), RuntimeError("boom"), RuntimeError("boom"), + ConnectionError(_AUTH_ERROR_MESSAGE)] + ) + mock_llm.plan_stream = script + session = ChatSession(ChatSessionConfig(llm_client=mock_llm, workspace=workspace)) + + with pytest.raises(ConnectionError, match="Invalid API key"): + await _run_turn(session) + + # 3 retry attempts (max_auto_retries=2) on the unrelated RuntimeError, + # then the final direct wrap-up call hits the auth error. + assert script.calls == 4 + + +def test_is_provider_auth_error_matches_only_the_invalid_key_copy(): + """The predicate both re-raise sites share — pinned directly so the two + call sites can't drift from each other (review feedback on ENG-1310).""" + assert _is_provider_auth_error(ConnectionError(_AUTH_ERROR_MESSAGE)) + assert _is_provider_auth_error(ConnectionError("INVALID API KEY — case insensitive")) + assert not _is_provider_auth_error(ConnectionError("temporarily unavailable")) + assert not _is_provider_auth_error(RuntimeError(_AUTH_ERROR_MESSAGE)) + + +async def test_endpoint_configuration_error_on_the_final_wrapup_call_still_reraises(workspace): + """The wrap-up call's except block must treat EndpointConfigurationError + (ENG-1139 — also deterministic, also must default to 'setup' not + 'retry') the same way the immediate re-raise site already does, instead + of flattening it into chat text (review feedback on ENG-1310).""" + mock_llm = make_mock_llm() + script = _ScriptedExceptionPlanStream( + [RuntimeError("boom"), RuntimeError("boom"), RuntimeError("boom"), + EndpointConfigurationError("The model endpoint returned 404.")] + ) + mock_llm.plan_stream = script + session = ChatSession(ChatSessionConfig(llm_client=mock_llm, workspace=workspace)) + + with pytest.raises(EndpointConfigurationError): + await _run_turn(session) + + assert script.calls == 4 + + +async def test_generic_connection_error_still_falls_back_to_chat_text(workspace): + """Only the auth-shaped message re-raises — an unrelated ConnectionError + (e.g. the generic 'temporarily unavailable' case) keeps the existing + fallback-text behavior instead of failing the turn.""" + mock_llm = make_mock_llm() + script = _AlwaysRaisingPlanStream(ConnectionError("temporarily unavailable")) + mock_llm.plan_stream = script + session = ChatSession(ChatSessionConfig(llm_client=mock_llm, workspace=workspace)) + + events = await _run_turn(session) + + from anton.core.llm.provider import StreamTextDelta + + fallback_text = "".join( + e.text for e in events if isinstance(e, StreamTextDelta) + ) + assert "temporarily unavailable" in fallback_text + assert "unexpected error occurred" in fallback_text diff --git a/tests/test_status_error_mapper.py b/tests/test_status_error_mapper.py index b875bf50..37f2b9b5 100644 --- a/tests/test_status_error_mapper.py +++ b/tests/test_status_error_mapper.py @@ -36,6 +36,7 @@ classify_transient, wallet_denial_code, ) +from anton.core.session import _is_provider_auth_error def _sdk_error(status_code, json_body=None, text_body=None, headers=None): @@ -98,6 +99,11 @@ def test_401_maps_to_invalid_key_connection_error(): # cowork-server's provider_auth detection keys on this exact phrase. assert "Invalid API key" in str(err.value) assert not isinstance(err.value, ModelUnavailableError) + # session.py's own re-raise checks (ENG-1310) key on this predicate, not + # the raw text — pin against the REAL mapper output so an edit to this + # copy that drops "invalid api key" fails here too, not just silently in + # production (review feedback on ENG-1310). + assert _is_provider_auth_error(err.value) def test_401_html_body_maps_to_invalid_key(): @@ -106,6 +112,7 @@ def test_401_html_body_maps_to_invalid_key(): with pytest.raises(ConnectionError) as err: _raise_for_status_error(exc, "sonnet") assert "Invalid API key" in str(err.value) + assert _is_provider_auth_error(err.value) # ── 429 (quota) ─────────────────────────────────────────────────────── @@ -498,6 +505,20 @@ def test_wallet_denial_code_reads_both_dialects(): # ── the anthropic twin (ENG-1169) ───────────────────────────────────── +def test_anthropic_401_maps_to_invalid_key_connection_error(): + # No real-SDK 401 coverage existed for the anthropic mapper before this + # (review feedback on ENG-1310) — only openai's 401 was pinned against + # actual SDK output; anthropic's own "Invalid API key — …" copy was + # untested except via a hand-built ConnectionError. + exc = _anthropic_sdk_error(401, json_body={"type": "error", "error": { + "type": "authentication_error", + "message": "invalid x-api-key", + }}) + with pytest.raises(ConnectionError) as err: + _raise_anthropic(exc, model="claude-sonnet") + assert "Invalid API key" in str(err.value) + assert _is_provider_auth_error(err.value) + def _anthropic_sdk_error(status_code, json_body=None, headers=None): """Real `anthropic.APIStatusError` from the pinned SDK — the anthropic twin of `_sdk_error`. The anthropic SDK does NOT unwrap the error