Skip to content

Classify a cell's 401 as a credential rejection, not a raw error - #2124

Open
Soph wants to merge 1 commit into
mainfrom
soph/search-401-classification
Open

Classify a cell's 401 as a credential rejection, not a raw error#2124
Soph wants to merge 1 commit into
mainfrom
soph/search-401-classification

Conversation

@Soph

@Soph Soph commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

An account whose home_jurisdiction is eu gets a 401 from every cell's semantic-search route, and we printed the service's body verbatim (#2121):

search failed: semantic search: search service error (401): Unauthorized

That names no region, offers no next step, and invites the one action that cannot possibly work — logging in again. The rejection is server-side and account-scoped (entirehq/entire-search#196); the same bearer is accepted by code search at the same host.

Now:

search failed: semantic search: your credentials were rejected in aws-eu-central-1 — the search service refused a login that works everywhere else, so retrying and logging in again are both dead ends; 'entire doctor bundle' packages the details for a bug report

Setting expectations on how much this helps. It doesn't make the search work — the index is server-side and the CLI has no local substitute — so all it can buy the user is the two things left: that retrying and re-authenticating are both dead ends (the #2121 reporter spent days on exactly that reflex), and the one action that changes anything. An earlier draft offered entire search --code as a consolation; that is now asserted against in the tests. Code search answers a different question, and pointing someone looking for session history at file matches reads as a fix while being none.

The thing that would actually help is a local fallback — #1923's second ask — and it is a feature, not a message. Worth knowing before anyone scopes it: local checkpoint trees reference blobs this clone does not have. git grep over entire/checkpoints/v1 in this repo does find matches, but takes 30s+ and triggers upload-pack: not our ref fetches, so a local search would silently answer from partial data. "No history" from a partial index is the #1923 failure mode all over again, which is why this PR does not fake one.

What changed

  • search.ErrCellUnauthorizedCellV4 classifies 401/403 the way it already classifies the two 404s, wrapping the status and the service's own message (%w (HTTP 401): Unauthorized) so debug logs keep the only clue to which auth stage refused. 403 joins 401: entire-search verifies a token and then declines it with 403 when the subject carries no account id.
  • A separate bucket in the fan-out. Unauthorized cells stay in failed (coverage flags and accounting unchanged) but are tracked apart from cells that simply lack query-serve.
  • Priority when nothing answered: credential rejection > transport/5xx > repo-filter miss > undeployed region. A rejection is the one outcome certain to repeat, and neither the repo nor the region message is true of it.
  • Partial-failure warning naming the rejecting regions, because "search failed in 1 of 2 regions" reads as transient and this is not.

This is the machine-readable distinction #1923 asks for, in the correct category. It is deliberately not reported as regional unavailability, which #2121 suggests as its fallback: that would mask a server bug as geography and promise an availability rollout that will never arrive. The existing errNoRegionAvailable still covers the real thing (a gateway 404 — query-serve genuinely not deployed).

Why the reporter's primary suggestion isn't here

#2121 concludes the token's aud=https://eu.entire.io mismatches the audience: https://entire.io in entire.io/.well-known/entire-api.json, and asks us to mint the advertised audience. Probing prod says otherwise:

Bearer semantic route code route
login JWT (what cell clients actually send) 200 both cells 200 both cells
identity token aud=https://eu.entire.io 200 both cells 200 eu / 401 InvalidAudience us
aud=https://entire.io (the advertised one) 401 401 InvalidAudience

Minting the advertised audience would break semantic search for everyone, and code search outright. aud is also never dialed, so eu.entire.io being NXDOMAIN is irrelevant — au/in are too, by design.

Drive-by: stale token comments

Chasing that audience theory cost real time because our own comments — and the CLAUDE.md "Token rule" — still describe cell clients as carrying minted per-jurisdiction identity tokens. 3df7ea461 ("Use login JWTs directly for cell auth") removed that exchange; CellClientFactory.ClientFor passes subject.loginJWT straight through, and CellTarget's jurisdiction now selects only the host. Four sites corrected (cell_fanout.go, search_v4.go, CellV4's doc, CLAUDE.md), noting that the minting path survives only behind entire auth token --jurisdiction.

Happy to split this into its own PR if you'd rather keep the diff to the classification.

Testing

  • mise run check — fmt, lint (0 issues), unit + integration + canary all green.
  • New: TestCellV4_Unauthorized (httptest, 401/403/raw-body, and that a 500 does not match); TestMergeSemanticV4Responses_AllCellsUnauthorized, _UnauthorizedOutranksOtherExplanations, _PartialUnauthorized.
  • TestCellV4_ErrorJSON repointed at a 500, so the generic wording stays pinned for every unclassified status.
  • Verified against prod: the probe table above, plus confirming entire search still works normally (my account is us-home).

Not in scope

Code search's 401 path is untouched. It already returns a typed *api.HTTPError, and peregrine only 401s on a genuine audience mismatch — which would be our routing bug, a different class needing a different message.

Three follow-ups I left alone: the older sentinels double the prefix (semantic search: semantic search is not yet available …) — my message omits it, but fixing theirs changes strings #1923 quotes; entire doctor still has no search-reachability check (#2121's third ask), which is where this error now points its users and so the natural next PR; and the local fallback discussed above.

Copilot AI lite review requested due to automatic review settings August 25, 2026 14:45
@Soph
Soph requested a review from a team as a code owner August 25, 2026 14:45

Copilot AI 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.

Pull request overview

Improves semantic-search UX and diagnosability when an entire-api cell responds to query-serve but rejects the caller’s bearer (401/403), ensuring the CLI reports this as a credential rejection (with actionable guidance) rather than a raw upstream error or mistaken regional unavailability.

Changes:

  • Add search.ErrCellUnauthorized and classify 401/403 responses from CellV4 as credential rejection while preserving status/body details for debug logs.
  • Track unauthorized cells separately during v4 semantic-search fan-out and prioritize “credentials rejected” when no cells return results; add partial-failure warnings naming rejecting regions.
  • Update docs/comments to reflect that cell clients carry the login JWT (not per-jurisdiction minted identity tokens).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cmd/entire/cli/search/search.go Adds ErrCellUnauthorized, classifies 401/403 in CellV4, and introduces serviceErrorMessage helper.
cmd/entire/cli/search/search_test.go Adds coverage for 401/403 classification and repoints generic error JSON test to a non-auth status.
cmd/entire/cli/search_v4.go Extends fan-out classification to track unauthorized cells and improves user-facing prioritization + warnings.
cmd/entire/cli/search_v4_test.go Adds tests for all-cells-unauthorized, prioritization, and partial-unauthorized warnings.
cmd/entire/cli/cell_fanout.go Updates comment/doc to match the current auth behavior (login JWT per cell client).
CLAUDE.md Updates documentation to reflect login-JWT cell auth and clarifies the remaining jurisdiction-token path.

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

Comment thread cmd/entire/cli/search/search.go
An EU-home-jurisdiction account gets 401 from every entire-api cell's
semantic-search route, and the CLI printed the service's body verbatim:

  search failed: semantic search: search service error (401): Unauthorized

That message names no region, offers no next step, and invites the one
action that cannot work — logging in again. The rejection is server-side
and account-scoped (entirehq/entire-search#196): the same bearer is
accepted by code search at the same host.

No CLI change can make this search succeed — the index is server-side and
there is no local substitute — so the error buys the user the only two
things left: that retrying and re-authenticating are both dead ends, and
the command that turns the dead end into a report someone can act on. It
deliberately does not offer 'entire search --code' as a consolation: code
search answers a different question, and pointing someone looking for
session history at file matches reads as a fix while being none.

CellV4 now classifies 401/403 as search.ErrCellUnauthorized, wrapping the
status and the service's message so debug logs keep them. The fan-out
tracks those cells separately from cells that lack query-serve, and when
nothing answered, the credential rejection outranks every other
explanation — it is the one outcome certain to repeat, and neither the
repo nor the region message is true of it. Partial failures gain a warning
naming the rejecting regions, since a rejection is not the transient
regional failure the generic count implies.

Deliberately NOT reported as regional unavailability, which cli#2121
suggests: that would mask a server bug as geography and promise an
availability rollout that will never fix it.

Also corrects comments (and the CLAUDE.md token rule) that still described
cell clients as carrying minted per-jurisdiction identity tokens. 3df7ea4
removed that exchange; they carry the login JWT. The stale docs are what
sent both the reporter and this investigation to audit token audiences.

Refs: #2121, #1923, entirehq/entire-search#196

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0WPHPWAW7GZG8J3NSQT4EAQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants