fix(errors): never mask a 4xx server message with the re-auth hint - #273
Conversation
format_fail_message replaced the body of ANY 4xx response with "API key is
invalid. Run 'hotdata auth login'" whenever the stored-profile credential
probe reported Invalid. The probe checks a *different* credential than the
request may have used (--api-key), so a stale stored session masked every
real 4xx: in the 2026-08-22 prod session, 400 planning errors ("Invalid
function 'st_quadkey'." + did-you-mean suggestions) printed as an auth
failure for ~48 requests, sending debugging down an auth/rate-limit rabbit
hole. Envoy access logs confirmed the wire response was a correct 400
via_upstream; the masking was purely client-side.
Now:
- The server's message is never discarded; hints are appended after it.
- The re-auth hint fires only on 401/403, where a broken credential is a
plausible cause. A 400/404 body is the server explaining itself.
- ApiError::print only runs the stored-profile probe on 401/403, dropping
a wasted GET /v1/workspaces round-trip from every other 4xx.
Tests: regression test for the masked-planning-error incident; 404 test
inverted (it encoded the old behavior); ACCESS_DENIED+Invalid test now also
asserts the server message survives.
Verified live against prod: unknown-function queries print the engine's
planning error (old binary: "API key is invalid"), valid queries unaffected,
bogus key prints the server 401 body plus the appended hint.
Claude-Session: https://claude.ai/code/session_01AEq1HcG9b29mnRAFWesB7j
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| match self { | ||
| ApiError::Status { status, body } => { | ||
| let auth_status = if status.is_client_error() { | ||
| // Probe the stored credential only when auth plausibly caused the |
There was a problem hiding this comment.
nit: the print doc comment just above (lines 248-250) still describes the old behavior and now directly contradicts the code:
On a 4xx, re-probe the auth status so a masked 404/403 is upgraded into the "run hotdata auth login" hint; otherwise surface the server body.
A 404 is now explicitly never upgraded, and the body is never replaced. Since this PR is largely about making the behavior legible, worth updating that paragraph too. (not blocking)
| Some(&AuthStatus::Invalid(403)), | ||
| ); | ||
| assert!(msg.contains("API key is invalid"), "got: {msg}"); | ||
| assert!(msg.contains('x'), "server message must be kept: {msg}"); |
There was a problem hiding this comment.
nit: contains('x') is a very weak guard for "the server message survived" — it passes on any output containing the letter x anywhere, and the two hint strings are one wording tweak away from containing one. A distinctive body message makes the assertion mean what the comment says:
| assert!(msg.contains('x'), "server message must be kept: {msg}"); | |
| assert!( | |
| msg.contains("server-explanation-here"), | |
| "server message must be kept: {msg}" | |
| ); |
(with the body at line 1061 changed to {"error":{"code":"ACCESS_DENIED","message":"server-explanation-here"}}). (not blocking)
There was a problem hiding this comment.
Verified the premise holds: credentials::check_status prefers the cached JWT session over the api_key fallback (src/client/credentials.rs:39-43), so a stale stored session yields Invalid(401) even when the request's own --api-key is fine — exactly the masking described. Narrowing the hint to 401/403 and appending rather than substituting is the right fix, and the probe elision in print follows from it.
Also checked that no other code, test, or doc depends on the old error: API key is invalid. Run ... string, so the wording change is self-contained.
Two non-blocking nits inline. Note CI (fmt/test/clippy) was still queued when this review ran, so I can't confirm the suite result.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Problem
format_fail_messagereplaced the body of any 4xx response witherror: API key is invalid. Run 'hotdata auth login' to re-authenticate.whenever the stored-profile credential probe reportedInvalid. The probe validates a different credential than the request may have used (--api-key), so a stale stored session masked every real 4xx.Observed in prod (2026-08-22): ~48 queries using a valid
--api-keyauthenticated fine, reached RuntimeDB, and got correct 400 planning errors — Envoy access logs showresponse_code: 400, response_code_details: "via_upstream"— yet the CLI printed the auth message for every one. The engine was even returning did-you-mean suggestions (Invalid function 'st_quadkey'. Did you mean 'st_equals'?) that the CLI discarded. The misleading output sent debugging down an auth/rate-limit rabbit hole before logs proved the responses were fine on the wire.Fix
ApiError::printonly probes the stored credential on 401/403, dropping a wastedGET /v1/workspacesround-trip (and misleading signal) from every other 4xx.Behavior
API key is invalid...Invalid function 'st_quadkey'. Did you mean 'st_equals'?API key is invalid...Tests
Invalidprobe must surface the server message, not the hint).format_fail_messagetests pass; full suite green; clippy clean.Live validation (release build vs prod)
API key is invalid; fixed binary prints the engine's planning error with suggestion.ST_Distance_Sphere->111195.08..., 3ms).--api-key: server'sInvalid api keybody plus the appended re-auth hint.https://claude.ai/code/session_01AEq1HcG9b29mnRAFWesB7j