feat(llm): shared status-error mapper + typed model-403s (ENG-598)

[alecantu7]

Part 1 of 3 for ENG-598 (with cowork-server and cowork companion PRs).

Problem

A gateway 403 for a tier-locked/disabled model surfaced in chat as:

"An unexpected error occurred: Server returned 403 — the LLM endpoint may be temporarily unavailable. Try again in a moment.. Please try again or rephrase your request."

Wrong cause, useless advice, double-wrapped. Two layers: (1) openai.py mapped every non-401/429 APIStatusError to the generic copy, in four copy-pasted blocks that had already drifted in wording; (2) session.py's plan_stream fallback swallowed the exception into assistant text, so the turn "succeeded" and cowork-server's error-card machinery never ran.

Fix

• One shared _raise_for_status_error(exc, model) used by all four call paths (chat/stream × completions/responses) — the mapping can't drift again. 401 and 429 behavior byte-compatible (same copy the downstream provider_auth / token_limit detections key on).
• New ModelUnavailableError(ConnectionError) carrying {code, model} for the gateway's structured 403s (error.code of model_access_denied / model_disabled):
  • model_access_denied → "isn't included in your current MindsHub plan — upgrade or switch models in Settings"
  • model_disabled → hedged copy (can be tier lock or admin kill switch until mindshub-inference#335 / ENG-596 lands)
  • Detection is code-exact: BYOK OpenAI 403s (region blocks), Anthropic permission errors, and Cloudflare HTML 403s carry no such code and fall through to the generic message — never the plan copy. Subclassing ConnectionError keeps every legacy call site working unchanged.
• session.py: TokenLimitExceeded / ModelUnavailableError now propagate from the plan_stream fallback instead of being wrapped into prose — the turn fails, so the server can render an actionable card. All other exceptions keep the existing text fallback.

Tests

11 new in tests/test_status_error_mapper.py: 401 copy pin, 429 quota (+ precedence over model-403), both gateway codes (attrs + copy), ConnectionError compatibility, BYOK/HTML/code-less 403 fall-through, 500 generic.

Suite: 1147 passed. The 2 test_chat_scratchpad failures pre-exist on clean staging (verified via stash — environment-dependent, fail identically without this diff).

Ticket: ENG-535's sibling — ENG-598. Related: ENG-516 (this introduces the typed-exception pattern it asked for).

🤖 Generated with Claude Code

[lucas-koontz]

Reviewed as Part 1 of the ENG-598 chain (with cowork-server#168 and cowork#353). The core is solid: one shared _raise_for_status_error across all four call paths kills the copy-paste drift, code-exact 403 detection (never bare-status or string matching) is the right call, 429-before-403 precedence is correct, and the session.py change to propagate ModelUnavailableError/TokenLimitExceeded instead of wrapping them as assistant text is exactly what makes the downstream card possible. Test coverage is genuinely thorough (BYOK region 403, Cloudflare HTML body, no-code 403, quota-wins-over-403). Approving.

One thing worth fixing, and two nits — none blocking:

Should fix — the CLI now defaults tier-lock errors to "retry" (chat.py:1889, not in this diff). ModelUnavailableError subclasses ConnectionError, and the CLI turn handler does default="retry" if isinstance(exc, ConnectionError) else "setup". So a model_access_denied (a deterministic plan gate) now highlights retry as the default action — the user hits Enter, re-sends the identical doomed request, and fails the same way. This directly contradicts the comment you added in session.py ("deterministic — the auto-retry below would just re-send the same doomed request"). Since you're already importing the type, consider default="setup" if isinstance(exc, ModelUnavailableError) else ("retry" if isinstance(exc, ConnectionError) else "setup") so the interactive default steers toward switching models. (model_disabled is hedged, so retry-as-default there is more defensible.)

Nits: the 429 docstring bullet is grammatically broken (inline comment), and _raise_for_status_error is typed -> None but always raises — typing.NoReturn would document/enforce that the four except sites can rely on it. The NoReturn one is truly optional (this repo runs no type checker in CI, and the except sites do fall through to a raise today).

[lucas-koontz]

Grammar: "checked before the 403 branch can't shadow it" is malformed. The 429 and 403 branches gate on mutually-exclusive status_code values, so ordering can't cause shadowing between statuses anyway — the precedence that actually matters is only for a body carrying BOTH detail and error.code on the same status (your test_429_with_detail_wins_even_if_error_code_present pins exactly that). Suggest: "429 with a quota detail → TokenLimitExceeded, checked first so a body carrying both detail and error.code stays token_limit."

[alecantu7]

Fixed in 9dd769b — reworded to your suggestion: "429 with a quota detail → TokenLimitExceeded, checked first so a body carrying both detail and error.code stays token_limit."

[lucas-koontz]

Optional: this always raises on every branch, so -> None slightly understates the contract that the four except APIStatusError: _raise_for_status_error(...) sites depend on (they dereference response right after). -> typing.NoReturn documents "never returns normally" and would let a strict checker catch a future non-raising branch that leaves response unbound. No functional change; skip if you'd rather not introduce NoReturn (it's used nowhere else here).

[alecantu7]

Done — typed -> NoReturn in 9dd769b (imported from typing). Documents the contract the four except-sites rely on.

[alecantu7]

Thanks @lucas-koontz — pushed 9dd769b:

• Should-fix (CLI default): fixed. The handler now defaults to setup for ModelUnavailableError (deterministic gate → switching is the useful action), keeping retry as the default for other (transient) ConnectionErrors. Resolves the contradiction with the session.py comment.
• 429 docstring reworded to your text; _raise_for_status_error now typed -> NoReturn.

Mapper/session tests green (11).

[lucas-koontz]

Re-checked the polish — CLI default now branches setup for ModelUnavailableError (deterministic plan/kill-switch, retry can't help) and keeps retry only for genuine transient ConnectionErrors. Matches session.py's don't-retry intent. Approval stands.