feat(auth): report a persistent 401 as an expired key, not a server outage

[asaiacai]

A customer's API key expired mid-morning and they read the resulting logs as a Pluto outage. Nothing in the output pointed at their key: the client retried the 401 (transient token-validation races do clear on retry), logged response code 401 ... from https://pluto-api..., and finally raised Failed to create run: HTTP 401: Unauthorized.

A 401 that survives every retry isn't that race — the key is expired or revoked. This says so, names the key source, and gives the command that fixes it.

Before

Failed to create run: HTTP 401: Unauthorized

After

authentication failed (HTTP 401): the Pluto server rejected the API key in the
PLUTO_API_KEY environment variable. The server says: API key has expired. This
is an authentication failure, not a server outage. Create a new key at
https://pluto.trainy.ai/api-keys, then update PLUTO_API_KEY with the new key.

The server was already telling us why

Worth stating plainly, because it shaped the fix: the reason was on the wire the whole time and the client discarded it.

service	401 body today
web API (/api/slug, create-run, status, tags…)	{"error": "Unauthorized", "message": "API key has expired"} — also "...has been revoked", "Key not found"
ingest (sync uploads)	{"code": 1002, "message": "API key has expired"}

_server_error_message read only error, so it saw the status phrase Unauthorized and nothing else. It now prefers error when that carries a real reason (validation failures put it there — "A run can have at most one group:* tag.") and falls through to message when error is only the status phrase. So the message reports the fact; the most likely expired or been revoked hedge is kept only for when the body genuinely says nothing.

No server change is needed for this. The one thing the server lacks is a stable code — the match is on prose — which is noted in CLAUDE.md for whoever wants to harden it later.

Changes

• http_401_message() (pluto/iface.py) builds the one user-facing message: names the key source (PLUTO_API_KEY / MLOP_API_TOKEN / the pluto login keyring, since the fix differs), states the server's reason, and gives the command that fixes it.
• PlutoAuthError — a PlutoRequestError subclass, so existing except PlutoRequestError handlers keep working — is raised on retry exhaustion by both ServerInterface._try and _SyncUploader._post_with_retry. The sync path matters because a key can expire mid-run, where the only thing the user sees is Failed to upload metrics: <error>.
• init() re-raises it verbatim instead of wrapping it in Failed to create run:, which implied a server problem.
• Fire-and-forget callers (heartbeat, status update, logName registration) don't raise, so _try logs the message at CRITICAL — once per run, reset in ServerInterface.__init__. Not once per process: a sweep creating many runs in one process would otherwise report the first bad key and then silence every later run's upload failures.
• login() catches this earliest, before create-run: a 401 on a pre-provided key is definitive, so it no longer reports "token may still be valid". 5xx and network failures keep that softer wording, since those genuinely say nothing about the key.

Retry behavior is unchanged — 401 stays in RETRYABLE_STATUS_CODES; only what's reported after retries are exhausted changed.

Two constraints worth knowing before editing this code

• Settings holds _auth, so a string read off it — even a public URL — is credential-adjacent and shouldn't be interpolated into a log line. Log paths resolve the key page from PLUTO_URL_APP via _api_page_url_from_env(); the raised PlutoAuthError still carries the exact settings.url_token, so a self-hosted deployment configured through init(host=...) keeps the precise URL where it matters most.
• The helpers are named for what they are (http_401_message, _source_and_fix, _api_page_url_from_env, _log_401_once) and stay clear of credential words on purpose. CodeQL's py/clear-text-logging-sensitive-data classifies a call's result as sensitive by the callee's name — it flagged a call taking no arguments at all — and inline # codeql[...] suppressions are not honored by this repo's setup. Renaming is what cleared it; the message text was unaffected.

Tested (run the relevant ones):

• Code formatting: bash format.sh — ruff + mypy clean
• Any manual or new tests for this PR (please specify below)

New/updated unit tests, all offline:

• tests/test_iface_errors.py — _server_error_message reads the reason out of both real body shapes (web and ingest); the message states a server-supplied reason as fact and only hedges without one; persistent 401 raises PlutoAuthError; the message names the right key source; an omitted page URL resolves from the environment; the non-raising path logs exactly once per run, and a second run gets its own notice. Also covers login(): it reports a 401 as an expired key with the server's own words, never as "may still be valid", while 5xx and unreachable-server paths keep their existing wording and don't blame the key.
• tests/test_sync_process.py — persistent 401 in the uploader still retries, then raises the actionable message, for both plain-text and JSON (ingest-shaped) bodies.

Also verified end-to-end by stubbing a 401 from the server and calling pluto.init() — login() logs the CRITICAL auth message and init() raises RuntimeError with the same text.

---

> [!NOTE]
> Low Risk
> Changes are limited to error messaging and exception typing on existing retry paths; no auth protocol or retry policy changes.
>
> Overview
> Persistent 401s are treated as bad API keys, not Pluto outages. Retry behavior for 401 is unchanged; only what users see after retries fail is different.
>
> Adds auth_error_message() in pluto/iface.py to build one user-facing string: which key was used (PLUTO_API_KEY, deprecated MLOP_API_TOKEN, or pluto login keyring), that this is auth not an outage, a link to the API-keys page (url_token for self-hosted), and how to fix it. Optional server text is appended only when it adds detail beyond generic "Unauthorized".
>
> Adds PlutoAuthError (PlutoRequestError subclass). ServerInterface._try raises it when a 401 survives all retries with raise_on_error=True; otherwise it logs the same message at CRITICAL once per process (heartbeats ~every 4s). _SyncUploader._post_with_retry does the same after sync retries, so keys that expire mid-run show up in upload failures. init() catches PlutoAuthError and re-raises the message without a "Failed to create run" wrapper. login() logs CRITICAL with auth_error_message on 401 for pre-provided keys instead of "token may still be valid"; 5xx/network keep softer wording.
>
> Sync settings dict now includes url_token for auth error links in the child process. CLAUDE.md documents the behavior. New/updated unit tests cover login, iface, and sync uploader 401 paths.
>
> ^{Reviewed by Cursor Bugbot for commit aa9b576. Configure here.}

Summary by CodeRabbit

• Bug Fixes

  • Improved authentication error messages for expired or invalid API keys.
  • Distinguished persistent authentication failures from temporary server outages and connectivity issues.
  • Included server-provided error details and relevant API-key guidance, including self-hosted environments.
  • Ensured repeated authentication failures are reported clearly after retry attempts.

• Tests

  • Added coverage for login, request retry, server error parsing, and environment-specific authentication guidance.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds persistent HTTP 401 handling with key-source guidance, server error details, PlutoAuthError propagation, one-time logging for non-raising paths, updated login messages, and regression tests.

Changes

Authentication failure flow

Layer / File(s)	Summary
Authentication error contracts pluto/iface.py	Adds PlutoAuthError, API-key URL resolution, key-source detection, persistent-401 message construction, generic error filtering, and improved response parsing.
Retry and propagation paths pluto/iface.py, pluto/sync/process.py, pluto/op.py, tests/test_iface_errors.py, tests/test_sync_process.py, CLAUDE.md	Exhausted HTTP 401 retries raise PlutoAuthError for raising paths and log one critical message for non-raising paths. Operation and sync-process paths preserve actionable authentication details.
Login authentication reporting pluto/auth.py, tests/test_auth_errors.py	Pre-provided keys receiving HTTP 401 produce definitive expired-key guidance. 5xx and connection failures retain softer or connectivity-specific messages.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant _post_with_retry
  participant HTTPServer
  participant PlutoAuthError
  participant Logger
  Client->>_post_with_retry: request with retries
  _post_with_retry->>HTTPServer: POST request
  HTTPServer-->>_post_with_retry: HTTP 401 response
  _post_with_retry->>_post_with_retry: exhaust retries
  _post_with_retry->>PlutoAuthError: raise formatted authentication error
  _post_with_retry->>Logger: log formatted authentication error once

Loading

Possibly related PRs

• Trainy-ai/pluto#131: Both changes modify pluto/auth.py login handling for authentication-validation failures and API-key state.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: persistent HTTP 401 responses are reported as expired API keys instead of server outages.
Description check	✅ Passed	The description explains the problem, solution, implementation details, risks, and completed formatting and test checks required by the template.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/dazzling-franklin-9oo2lr

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

tests/test_auth_errors.py (1)

46-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the non-generic 401 reason.

_response(401, text='Unauthorized') exercises only the generic fallback. Use a specific server reason and assert that the exact reason appears in caplog. This test should fail if http_401_message() is called without server_msg.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_auth_errors.py` around lines 46 - 62, Update
test_login_401_reports_expired_key_not_maybe_valid to use a specific non-generic
server reason in the mocked 401 response, then assert that exact reason appears
in caplog alongside the existing assertions. Ensure the test exercises
http_401_message with server_msg rather than the generic fallback.

tests/test_sync_process.py (1)

1218-1253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a JSON-body case for the sync-process 401 path.

This test only exercises a plain-text 401 body, so it never exercises _server_error_message's JSON-parsing branch ({"error": "Unauthorized", "message": "API key has expired"}) through _post_with_retry, unlike the equivalent coverage in tests/test_iface_errors.py. Add a variant using a JSON body to confirm the server-provided reason surfaces through the sync-process path too.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_sync_process.py` around lines 1218 - 1253, The 401 sync-process
test only covers plain-text responses and misses the JSON parsing path in
_server_error_message. Add a JSON-body variant of
test_persistent_401_raises_actionable_auth_error through _post_with_retry, using
an error/message payload, and assert the server-provided expiration reason is
surfaced in the resulting PlutoAuthError.

pluto/iface.py (1)

475-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Server message is recovered by parsing a formatted display string.

server_msg = error_info.partition(': ')[2] (Line 483) reconstructs the server reason by splitting error_info, which was built purely for the failure log (f'HTTP {r.status_code}: {server_msg[:200]}', Line 524) and is also written to _log_failed_request. Coupling the auth-message extraction to that display format means a future change to the error_info format (e.g. adding a prefix, changing the separator) silently corrupts the extracted reason without any test catching the mismatch beyond string-content assertions.

Carry server_msg through the recursive _try calls as its own parameter, the same way last_status already is, instead of re-deriving it from error_info.

♻️ Proposed refactor

     def _try(
         self,
         method,
         url,
         headers,
         content,
         name: Union[str, None] = None,
         drained: Optional[List[Any]] = None,
         retry: int = 0,
         error_info: str = '',
         last_status: Optional[int] = None,
+        last_server_msg: str = '',
         max_retries: Optional[int] = None,
         timeout: Optional[float] = None,
         suppress_httpx_logs: bool = False,
         raise_on_error: bool = False,
     ):
         ...
             if last_status == 401 and error_info.startswith('HTTP 401'):
-                server_msg = error_info.partition(': ')[2]
+                server_msg = last_server_msg
                 if raise_on_error:
                     raise PlutoAuthError(...)
                 self._log_401_once(server_msg)
                 return None
         ...
             server_msg = _server_error_message(r)
             error_info = f'HTTP {r.status_code}: {server_msg[:200]}'
             last_status = r.status_code
+            last_server_msg = server_msg
         ...
         return self._try(
             ...
             last_status=last_status,
+            last_server_msg=last_server_msg,
             ...
         )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pluto/iface.py` around lines 475 - 497, Update the recursive _try flow to
carry the latest server message as a dedicated parameter alongside last_status,
propagating it through every retry call and setting it from the response before
constructing error_info. In the final 401 handling block, pass that carried
value directly to http_401_message and _log_401_once, and remove the
error_info.partition parsing while preserving the existing network-error
behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pluto/auth.py`:
- Around line 93-106: Update the 401 branch in the exception-handling flow to
extract the server-provided reason from e.response using the same
response-format handling as pluto/iface.py, then pass it as server_msg to
http_401_message. Preserve the existing generic fallback when no usable reason
is available and keep the non-401 warning path unchanged.

In `@pluto/iface.py`:
- Around line 23-25: Move the one-time 401 tracking from module-wide state into
each newly created Op or ServerInterface instance, or reset _logged_401 during
that interface’s initialization. Ensure heartbeat, status-update, and
logName-registration failures can emit one notice per run while preserving
suppression of repeated notices within the same run.

---

Nitpick comments:
In `@pluto/iface.py`:
- Around line 475-497: Update the recursive _try flow to carry the latest server
message as a dedicated parameter alongside last_status, propagating it through
every retry call and setting it from the response before constructing
error_info. In the final 401 handling block, pass that carried value directly to
http_401_message and _log_401_once, and remove the error_info.partition parsing
while preserving the existing network-error behavior.

In `@tests/test_auth_errors.py`:
- Around line 46-62: Update test_login_401_reports_expired_key_not_maybe_valid
to use a specific non-generic server reason in the mocked 401 response, then
assert that exact reason appears in caplog alongside the existing assertions.
Ensure the test exercises http_401_message with server_msg rather than the
generic fallback.

In `@tests/test_sync_process.py`:
- Around line 1218-1253: The 401 sync-process test only covers plain-text
responses and misses the JSON parsing path in _server_error_message. Add a
JSON-body variant of test_persistent_401_raises_actionable_auth_error through
_post_with_retry, using an error/message payload, and assert the server-provided
expiration reason is surfaced in the resulting PlutoAuthError.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 22ff2a1b-baca-4099-bfd8-2dbb813abb70

📥 Commits

Reviewing files that changed from the base of the PR and between d337446 and 2bff115.

📒 Files selected for processing (8)

• CLAUDE.md
• pluto/auth.py
• pluto/iface.py
• pluto/op.py
• pluto/sync/process.py
• tests/test_auth_errors.py
• tests/test_iface_errors.py
• tests/test_sync_process.py

[ryanhayame]

nice