diff --git a/nisse-dev/src/nisse_dev/delegation.py b/nisse-dev/src/nisse_dev/delegation.py index 9940904..c79212c 100644 --- a/nisse-dev/src/nisse_dev/delegation.py +++ b/nisse-dev/src/nisse_dev/delegation.py @@ -36,7 +36,7 @@ from nisse_dev.canonical import ( ToolResult, ) from nisse_dev.git import GitError, GitRepo -from nisse_dev.llm import BudgetExhausted, LLMClient, LLMError +from nisse_dev.llm import LLMClient, LLMError, UpstreamUnavailable from nisse_dev.reconcile import Reconciler from nisse_dev.state import Intent, StateStore from nisse_dev.tools import EXECUTOR_TOOLS, MUTATING_TOOLS, Workspace @@ -330,10 +330,10 @@ class Delegation: model=self._model, system=system, ) - except BudgetExhausted as exc: + except UpstreamUnavailable as exc: return DelegationResult( "interrupted", - f"external models are unavailable: {exc}", + f"stopped by something upstream, not by the work: {exc}", branch=branch, artifacts=sorted(touched), tokens=spent, diff --git a/nisse-dev/src/nisse_dev/llm.py b/nisse-dev/src/nisse_dev/llm.py index 7f3a39a..cfaf0eb 100644 --- a/nisse-dev/src/nisse_dev/llm.py +++ b/nisse-dev/src/nisse_dev/llm.py @@ -54,12 +54,24 @@ def max_output_tokens(env: Mapping[str, str] = os.environ) -> int: return int(raw) if raw else DEFAULT_MAX_OUTPUT_TOKENS -class BudgetExhausted(LLMError): +class UpstreamUnavailable(LLMError): + """The proxy or provider could not be reached, or refused for reasons + unrelated to what was asked: a network blip, a timeout, a 5xx, a rate + limit that did not clear, an exhausted budget. + + Worth distinguishing because the *work* is not at fault. A delegation + stopped by one of these has a branch that may be perfectly good, so it is + preserved and resumed rather than discarded — unlike a delegate that made + a mess, which is cleaned up. + """ + + +class BudgetExhausted(UpstreamUnavailable): """The proxy refused because a spend limit is gone. - Distinct from a rate limit, which clears in a minute: an exhausted daily - budget takes external models away until it resets, so retrying wastes - time and the operator needs telling rather than the loop quietly failing. + Distinct from the rest of its family because it will not clear on its + own within the hour: an exhausted daily budget takes external models away + until it resets, so retrying wastes time and the operator needs telling. A5 keeps the budget itself at the proxy; recognising its refusal is ours. """ @@ -174,10 +186,7 @@ class LiteLLMClient: self._http, "/chat/completions", body, self._sleep ) if response.status_code != 200: - if _looks_like_budget_refusal(response.text): - raise BudgetExhausted( - f"the proxy refused on budget grounds: {response.text[:300]}" - ) + _raise_for_upstream(response) raise LLMError( f"LLM request returned {response.status_code}: {response.text[:500]}" ) @@ -294,10 +303,7 @@ class AnthropicPassthroughClient: self._http, "/v1/messages", body, self._sleep ) if response.status_code != 200: - if _looks_like_budget_refusal(response.text): - raise BudgetExhausted( - f"the proxy refused on budget grounds: {response.text[:300]}" - ) + _raise_for_upstream(response) raise LLMError( f"passthrough returned {response.status_code}: {response.text[:500]}" ) @@ -353,7 +359,9 @@ def _post_with_rate_limit_retry( try: response = http.post(path, json=body) except httpx.HTTPError as exc: - raise LLMError(f"LLM request failed: {exc}") from exc + # Could not reach the proxy at all: nothing about the request was + # wrong, so callers holding work in progress should keep it. + raise UpstreamUnavailable(f"LLM request failed: {exc}") from exc if response.status_code != RATE_LIMIT_STATUS or attempt == 2: return response sleep(RATE_LIMIT_RETRY_S) @@ -383,3 +391,20 @@ def select_client( which is the opposite of the vendor fluidity A5 requires. """ return passthrough if has_native_adapter(model) else normalized + + +def _raise_for_upstream(response: httpx.Response) -> None: + """Classify a non-200 as upstream trouble where it plainly is. + + A 4xx that is not a rate limit means the request itself was wrong, and + preserving work on the strength of one would hide a real bug — so the + benefit of the doubt stops at the server's own door. + """ + if _looks_like_budget_refusal(response.text): + raise BudgetExhausted( + f"the proxy refused on budget grounds: {response.text[:300]}" + ) + if response.status_code >= 500 or response.status_code == RATE_LIMIT_STATUS: + raise UpstreamUnavailable( + f"upstream returned {response.status_code}: {response.text[:300]}" + ) diff --git a/nisse-dev/tests/test_delegation.py b/nisse-dev/tests/test_delegation.py index e42b4ce..8a7fa55 100644 --- a/nisse-dev/tests/test_delegation.py +++ b/nisse-dev/tests/test_delegation.py @@ -21,7 +21,7 @@ from nisse_dev.delegation import ( delegation_reconciler, ) from nisse_dev.git import GitRepo -from nisse_dev.llm import BudgetExhausted, LLMError +from nisse_dev.llm import BudgetExhausted, LLMError, UpstreamUnavailable from nisse_dev.oplog import OpLog from nisse_dev.state import StateStore from nisse_dev.tools import Workspace @@ -614,3 +614,36 @@ def test_an_ordinary_failure_still_discards( assert not result.interrupted assert repo.current_branch() == "main" assert store.pending_intents() == [] + + +def test_a_network_outage_also_preserves_the_work( + store: StateStore, ws: Workspace, repo: GitRepo +) -> None: + # Same reasoning as an exhausted budget: the branch may be fine, and the + # outage says nothing about the quality of the work on it. + class FlakyClient(ScriptedClient): + def complete(self, *args: Any, **kwargs: Any): + if self.calls: + raise UpstreamUnavailable("LLM request failed: connection refused") + return super().complete(*args, **kwargs) + + client = FlakyClient([[call("write_file", path="progress.py", content="real")]]) + result = build(client, store, ws, repo).run(brief="b", branch="d") + + assert result.status == "interrupted" + assert (repo.root / "progress.py").exists() + assert [i.kind for i in store.pending_intents()] == [DELEGATION_INTENT] + + +def test_a_bad_request_is_not_given_the_benefit_of_the_doubt( + store: StateStore, ws: Workspace, repo: GitRepo +) -> None: + # A 4xx that is not a rate limit means we sent something wrong; treating + # that as an outage would preserve branches and hide a real bug. + class BadRequestClient(ScriptedClient): + def complete(self, *args: Any, **kwargs: Any): + raise LLMError("LLM request returned 400: malformed tool schema") + + result = build(BadRequestClient([]), store, ws, repo).run(brief="b", branch="d") + assert result.status == "failed" + assert not result.interrupted diff --git a/nisse-dev/tests/test_llm.py b/nisse-dev/tests/test_llm.py index c68c701..9f6b7fa 100644 --- a/nisse-dev/tests/test_llm.py +++ b/nisse-dev/tests/test_llm.py @@ -15,10 +15,12 @@ from nisse_dev.llm import ( DEFAULT_MAX_OUTPUT_TOKENS, RATE_LIMIT_RETRY_S, AnthropicPassthroughClient, + BudgetExhausted, LiteLLMClient, LLMError, RoleMap, TruncatedResponse, + UpstreamUnavailable, has_native_adapter, max_output_tokens, select_client, @@ -343,3 +345,38 @@ def test_truncation_is_an_llm_error_so_callers_already_handle_it() -> None: # The planner retries, the executor and delegation gate. None of them # need to learn a new exception type. assert issubclass(TruncatedResponse, LLMError) + + +def test_a_5xx_is_upstream_trouble() -> None: + client = make_client(lambda request: httpx.Response(503, text="bad gateway")) + with pytest.raises(UpstreamUnavailable, match="503"): + client.complete(EVENTS, tools=None, model="local-fast") + + +def test_a_connection_failure_is_upstream_trouble() -> None: + def handler(request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("refused") + + with pytest.raises(UpstreamUnavailable, match="request failed"): + make_client(handler).complete(EVENTS, tools=None, model="local-fast") + + +def test_a_persistent_rate_limit_is_upstream_trouble() -> None: + # It cleared for neither attempt, so the caller should keep its work + # rather than discard it. + client = make_client(lambda request: httpx.Response(429, text="slow down")) + with pytest.raises(UpstreamUnavailable): + client.complete(EVENTS, tools=None, model="local-fast") + + +def test_a_bad_request_is_not_upstream_trouble() -> None: + client = make_client(lambda request: httpx.Response(400, text="bad schema")) + with pytest.raises(LLMError) as excinfo: + client.complete(EVENTS, tools=None, model="local-fast") + assert not isinstance(excinfo.value, UpstreamUnavailable) + + +def test_budget_refusal_is_a_kind_of_upstream_trouble() -> None: + # So one catch covers the family, but it can still be told apart where + # the difference matters — it will not clear within the hour. + assert issubclass(BudgetExhausted, UpstreamUnavailable)