Skip to content

[ENG-1310] Unable to authenticate against the llm model invalid api key - #327

Merged
tino097 merged 7 commits into
stagingfrom
tino097/eng-1310-unable-to-authenticate-against-the-llm-model-invalid-api-key
Aug 11, 2026
Merged

[ENG-1310] Unable to authenticate against the llm model invalid api key#327
tino097 merged 7 commits into
stagingfrom
tino097/eng-1310-unable-to-authenticate-against-the-llm-model-invalid-api-key

Conversation

@tino097

@tino097 tino097 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

https://linear.app/mindsdb/issue/ENG-1310/unable-to-authenticate-against-the-llm-model-invalid-api-key-error

  • Added a narrow check , mirroring cowork-server own predicate
  • Fails fast instead of burning the retry budget on doomed retries
  • The retry-exhaustion fallback's own wrap-up call

@tino097
tino097 requested review from alecantu7 and pnewsam and a lite review from Copilot August 10, 2026 16:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses ENG-1310 by ensuring provider authentication failures (“Invalid API key …” surfaced as a ConnectionError from the LLM adapters) propagate out of ChatSession.turn_stream() so upstream (cowork-server) can render the correct reconnect/BYOK remediation UI, instead of flattening the error into generic assistant text.

Changes:

  • Add a fast-fail path in the main retry loop to avoid wasting retry budget on deterministic “invalid API key” failures.
  • Ensure the retry-exhaustion wrap-up plan_stream call also re-raises “invalid API key” errors rather than converting them to fallback chat text.
  • Add focused regression tests covering (1) immediate auth failure, (2) auth failure occurring only on the final wrap-up call, and (3) non-auth ConnectionError retaining fallback-text behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/test_session_auth_error_reraise.py Adds regression coverage for auth-shaped ConnectionError propagation and non-auth fallback behavior.
anton/core/session.py Adds auth-shaped ConnectionError fast-fail + wrap-up re-raise to preserve actionable upstream error handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread anton/core/session.py Outdated
Comment thread anton/core/session.py Outdated

@pnewsam pnewsam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Verdict: APPROVE

Reviewed the full diff against the turn_stream retry loop, confirmed the predicate against both provider 401 sites and cowork-server's is_auth_error, and ran the tests. The change is narrow, correct, and matches the EndpointConfigurationError (ENG-1139) precedent it cites. One minor test-coverage gap, non-blocking.

Validation

  • Ran the 5 new tests: all pass.
  • Ran the session/retry/stream suite: 583 passed, 7 skipped, no regressions from the restructured except block.
  • Confirmed both providers raise a plain ConnectionError containing "Invalid API key" (anton/core/llm/openai.py:89, anthropic.py:61) — the isinstance(ConnectionError) + substring predicate matches them and nothing else.
  • Confirmed cowork-server's is_auth_error (turn_errors.py:248) keys on the identical "invalid api key" substring, so the propagated error reaches the "Reconnect MindsHub" / BYOK card as intended.
  • Confirmed session.py has exactly one chat-text flatten site, so the "two sites" scope is complete; the EndpointConfigurationError wrap-up gap was real and is now closed and tested.

Nicely scoped fix with genuinely useful tests and comments. Two inline notes below, neither blocking.

Comment thread tests/test_session_auth_error_reraise.py
Comment thread anton/core/session.py

@alecantu7 alecantu7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review — head 2da0427, base origin/staging 5f3f2f7

Validated rather than read: checked out the PR, ran the tests, mutation-checked them against unfixed code, and executed cowork-server's real turn_errors mapper against the real anton exception types.

The implementation is sound and the tests are honest. Full suite: 1840 passed, 28 skipped, no regressions. Mutation check — reverted session.py to origin/staging and re-ran the new file: 3 failed, 2 passed, with the 2 passing being the predicate unit test and the negative control, which should pass on both. These tests genuinely fail on unfixed code, which is more than most PRs can claim.

Five findings below. None of them is that the mechanism doesn't work — I confirmed it does. Findings 1 and 2 are about what this PR is attached to, and both get more expensive after merge.

Copilot's two earlier notes are both addressed in this head. @pnewsam's test-coupling note is a real gap and I'd take it — it's the same class as finding 3 (a claim nobody resolved against the consumer repo). Their isinstance nit and mine were the same point, so I've dropped mine.


1. Merging this closes an Urgent ticket whose Urgent half is untouched

high · confirmed (process, not a code defect)

ENG-1310 is Urgent for the 401 itself — the user cannot authenticate after a full fresh login, which rules out a stale local key. This PR fixes only how that 401 is displayed. The ticket says so directly:

The message-wrapping bug explains the double period and the wrong "OpenAI" label, but it does not explain the 401 itself. That is the Urgent part and it is still undiagnosed.

The lead named there — verify provisionAntonApiKey (cowork repo, src/main/index.ts / minds-auth.ts) actually issues a working key on the preview channel — has no PR. ENG-1310's attachment list contains exactly one PR: this one.

Merging → auto-transition to Ready for QA → QA verifies the error card renders → ticket closes → Ubuntu users still can't authenticate.

Suggested: retitle this PR to the surfacing fix and move it under a new ticket, or keep ENG-1310 open for the provisioning failure. The point is not to let the auto-transition decide.


2. This PR edits the exact lines ENG-1361 owns, and implements two of its four items

medium · confirmed

ENG-1361 (To Do) specifies the fix to this same re-raise guard and requires TransientProviderError + ProviderOverloadedError in the tuple, plus converting an exhausted count-based transient into ProviderOverloadedError. This PR rewrites those lines and adds ConnectionError(auth) + EndpointConfigurationError — neither of ENG-1361's two.

After merge: the guard has been touched, ENG-1361 needs a rebase onto a rewritten block, and the failure it was filed for is still live — "Could not reach the model provider … Please try again or rephrase your request." (trace 477d66021d3b3ad9770efc8a9f5069ed).

Suggested: cheapest path is adding TransientProviderError and ProviderOverloadedError to the isinstance tuple here — two identifiers, and the existing tests already cover the shape. That reduces ENG-1361 to its item 1. Otherwise state the merge order and expect the conflict.


3. The EndpointConfigurationError re-raise produces no card, and its comment claims one that doesn't exist

medium · confirmed — executed

anton/core/session.py:3011 and the comment above it:

if isinstance(e, (TokenLimitExceeded, ModelUnavailableError, EndpointConfigurationError)):
    # ... the server maps them to actionable error cards (token_limit /
    # model-unavailable / endpoint-config) ...

There is no endpoint-config card. git grep EndpointConfigurationError origin/staging across all of cowork-server returns zero hits. EndpointConfigurationError(ConnectionError) carries no code attribute, so every duck-typed check in turn_errors.py misses it. Running the real staging mapper against the real anton types:

'auth ConnectionError (this PR's target)'       -> ('provider_auth', 'Your MindsHub session is no longer valid — reconnect…')
'EndpointConfigurationError (added here)'       -> None
remote path: 'EndpointConfigurationError: …'    -> ('anton_error', 'An unexpected error occurred.')

Net effect at this site — before: chat prose "An unexpected error occurred: The model endpoint returned 404 — check the base URL includes /v1. Please try again or rephrase your request." Bad copy, but it carries the fix. After: a card reading "An unexpected error occurred." The actionable part is gone.

Partial refutation, which is why this is medium and not high: the immediate re-raise site at :2888 already contained EndpointConfigurationError before this PR, so the common path already produced the generic card. This change makes the rare path consistent rather than newly broken — but it buys that consistency by discarding information, and the comment asserts a card that was never built.

Worth flagging that both prior reviews state this card exists (Copilot requested the change on that premise; the approving review repeats it). The auth half of the consumer contract was verified and holds — this half was inherited rather than checked.

Fix: either drop "endpoint-config" from the comment, or add the mapping to turn_errors.py in a follow-up so the re-raise pays off.
Test: in cowork-server/tests/test_turn_errors.py, assert friendly_turn_error(EndpointConfigurationError(...)) returns a non-None endpoint-config code. Fails today.


4. The fast-fail newly routes auth errors to a CLI prompt that defaults to "retry"

medium · confirmed by trace

anton/chat.py:2011:

except (TokenLimitExceeded, ConnectionError) as exc:
    ...
    # ModelUnavailableError ... and EndpointConfigurationError ... are both
    # deterministic for the identical request — retrying re-sends a
    # doomed call — so steer the default to "setup" ...
    default=("setup" if isinstance(exc, (ModelUnavailableError, EndpointConfigurationError))
             else ("retry" if isinstance(exc, ConnectionError) else "setup")),

Before this PR a persistent auth failure essentially never reached here — it fell through the retry loop and got flattened into prose at the wrap-up. The new fast-fail makes it reach here on the first attempt, every time.

An invalid API key is deterministic by this PR's own stated reasoning, but it falls into the else and the prompt defaults to retry — which re-sends the doomed call, fast-fails again, and re-prompts. The doomed retry this PR removes from the session loop reappears as a prompt loop. The comment two lines above already enumerates exactly the class the key belongs to.

Checked and ruled out: anthropic.AuthenticationError at chat.py:1971 does not catch it first — anton's mapper raises a plain ConnectionError, never the SDK type.

Fix: one clause — "setup" if isinstance(exc, (ModelUnavailableError, EndpointConfigurationError)) or _is_provider_auth_error(exc) else …
Test: assert the computed default is "setup" for ConnectionError("Invalid API key — …"). Fails today.


5. Re-raising skips _persist_history() and the turn-count increment

low · confirmed by trace · pre-existing, widened here

self._turn_count += 1 and self._persist_history() sit after the try/finally in _turn_stream_inner, so any propagating exception skips them. Already true for TokenLimitExceeded / ModelUnavailableError; this PR adds two more types to that set at the wrap-up site.

Impact is small in the product — the desktop app persists conversations server-side — but the anton CLI loses the turn from its on-disk history. Worth a comment rather than a code change.


What I checked that produced nothing

  • False positives on the new predicate. TransientProviderError and ProviderOverloadedError both subclass ConnectionError, so I checked whether either could carry "invalid api key" text. classify_transient (provider.py:544-570) builds fully curated messages and never embeds the provider body — no false positive.
  • Crash regressions across turn_stream's other callers. goal.py:167 (except Exception), local_runtime.py:370 (_safe_error_message), cloud_turn (scrubbed to a string; remote_turn_error already maps ConnectionError + "api key" → provider_auth). All handle it.
  • Security. The re-raise improves posture: cowork-server returns a fixed AUTH_ERROR_USER_MESSAGE constant rather than str(exc), so raw provider text stops reaching the client — the opposite of the old prose path. No credential, injection, or authz surface touched.
  • Non-streaming turn(). plan_with_recovery catches only ContextOverflowError, so auth already fast-fails there. No parallel gap.

- extract _is_provider_auth_error helper, shared by both re-raise sites
  instead of duplicating the isinstance+substring check
- re-raise EndpointConfigurationError in the retry-exhaustion wrap-up
  call too, matching the immediate re-raise site
- pin _is_provider_auth_error against real provider-mapper output
  (existing openai 401 tests + a new anthropic 401 test that didn't
  exist before)
- clarify the helper's docstring: the isinstance narrowing is
  anton-only, not shared with cowork-server's predicate
@tino097
tino097 requested a review from alecantu7 August 10, 2026 21:37
@alecantu7

Copy link
Copy Markdown
Contributor

849e79a verified — 44 tests pass locally, CI green. The coupling gap is properly closed: _is_provider_auth_error is now pinned against the real mapper output on both the JSON and HTML 401 paths, so editing the copy in openai.py to drop "invalid api key" fails a test instead of silently breaking the feature. The anthropic 401 case against real SDK output is a genuine addition — there was no real-SDK 401 coverage for that mapper before. Good catch @pnewsam.

Worth flagging something that changed underneath this PR: verdict-eval actually executed on this head — 90s, passed. Every earlier run on this branch was a 15–21s skip (MINDSHUB_API_KEY landed partway through today). So the session.py change has real verifier coverage now that it did not have when this was approved.

Two things still open from my review, both cheap:

  • session.py:3017 — the comment names an endpoint-config card among those the server maps. git grep EndpointConfigurationError across all of cowork-server at origin/staging returns zero hits, and friendly_turn_error returns None for it, so it lands on the generic "An unexpected error occurred." Either drop the word from the comment or add the mapping in a follow-up.
  • chat.py:2011 — still defaults to retry for an auth failure this PR made deterministic and fast-failing. One clause: ... or _is_provider_auth_error(exc) alongside the two types already steered to setup.

Before merge — a decision on ENG-1310's scope, not on this code. Merging auto-moves the ticket to Ready for QA, and QA will confirm the error card renders and close it. But the Urgent half — the 401 after a full fresh login, which the ticket's own notes say is undiagnosed — is untouched and has no PR. The provisionAntonApiKey check named in the ticket has not happened. So either ENG-1310 stays open for the provisioning failure, or this work moves under a new ticket for the surfacing fix. Happy either way; the thing to avoid is the ticket closing on a green QA pass while Ubuntu users still cannot authenticate.

…nable-to-authenticate-against-the-llm-model-invalid-api-key
- session.py: correct the wrap-up re-raise comment — cowork-server has
  no dedicated card for EndpointConfigurationError yet (verified via
  grep: zero hits on origin/staging, friendly_turn_error falls through
  to the generic message for it). Re-raising it still stops the
  misleading fallback text; it just doesn't get a better card until
  that mapping exists server-side.
- chat.py: extract _default_turn_error_action and steer a
  provider-auth 401 to the "setup" default instead of "retry" — the
  ConnectionError-defaults-to-retry gap three past PRs (#236, #247,
  #288) flagged for this exact case, now that ENG-1310 makes the
  failure deterministic and propagating.
@tino097

tino097 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

5290783 closes both remaining items:

On ENG-1310's scope: found ENG-1308 (filed by the same reporter, 14 minutes before this ticket, same day) — root-caused and already fixed on staging (auth#252 + mindshub_frontend#1406): an org-role permission gate blocked invited/non-owner org members from creating or using API keys, surfacing as a 403 at the gateway. Its own root-cause writeup cross-references ENG-327 as the same bug family.

Not a byte-for-byte match to ENG-1310's literal 401 text (ENG-1308's gateway symptom is a 403; ENG-1310's report is anton's 401 copy, a different mapper branch), so I'm not certifying it as the cause — but a live retest on Linux post-ENG-1308 came back clean.

Recommend: don't split into a new ticket. Keep this PR under ENG-1310, close it once QA confirms both (a) the error card renders correctly for a provider-auth failure [this PR], and (b) a fresh login no longer 401s [ENG-1308, retest the original repro]. If it resurfaces after that, re-open with the new evidence rather than treating it as closed.

@tino097
tino097 merged commit 85c496f into staging Aug 11, 2026
12 checks passed
@tino097
tino097 deleted the tino097/eng-1310-unable-to-authenticate-against-the-llm-model-invalid-api-key branch August 11, 2026 20:30
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 11, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants