fix(core): report whether the token refresh actually worked(YPE-4297)

[Dustin-Kelley]

The bug

refreshToken swallows failure by design — a transient failure must not sign anyone out — and returns nothing. A caller reading the token afterwards therefore cannot tell a refresh that worked from one that did not, and useHighlights.runWrite was doing exactly that: await ensureFreshToken(), then read authRef.current.accessToken and send it.

That is correct offline, where the write dies at the network layer with no HTTP status and classifies as transient. It is wrong when the token endpoint is down but the highlights API is reachable (an auth-service incident, a captive portal, a 5xx):

1. Access token expired; the refresh 5xxs or times out. Tokens are retained, per policy — and the token is still expired.
2. The write goes out with it anyway, and comes back 401.
3. 401 classifies as auth, and useHighlightPermissionFlow reads auth as a stale grant (ADR 0016).
4. A valid highlights grant is invalidated and consent is re-prompted.
5. The re-consent mints with the same expired token, 401s in turn, and dead-ends as not-permitted — which the docs describe as an app-key setting, so the user is told to check a console they cannot see.

A transient auth-server hiccup becomes a destroyed grant and a dead-end consent loop.

The fix

getAccessToken() on the auth context, exported through useYVAuth() and typed as AccessTokenResult:

getAccessToken(): Promise<
  | { status: 'ok'; token: string; userId: string | null }
  | { status: 'unavailable'; reason: 'signed-out' | 'refresh-failed' }
>

It runs the same leeway-gated, single-flight refresh as ensureFreshToken(), then re-reads the refs that refresh left behind and says which happened. Non-forced on purpose: the leeway gate already refreshes exactly when the token is expired or inside the window, and joining an in-flight refresh comes free — so concurrent callers make one HTTP call and all receive the new token. It never rejects, and makes no network call when there is no refresh token to spend.

runWrite sources its token from it. refresh-failed reverts the optimistic paint and settles transient without issuing the request — the line that stops step 2, and so the whole chain. signed-out keeps the existing not-signed-in path. The same-user identity guard now compares against the userId the accessor returns. That id is read in the same synchronous block as the token, and it has to be: the provider writes its token and identity refs together on sign-in, while the identityRef the guard used to read alone is synced from a passive effect a render later. A sign-in as somebody else landing while a write awaits the accessor moved the token first, so the old check passed against the departed user's captured id and sent the write under the new user's credentials. Regression test: abandons a write when the accessor returns a token owned by a different user.

What deliberately did not change

• isAuthenticated still means "has a session". An offline user with a retained session still renders as signed in. This is the regression the obvious fix (deriving isAuthenticated from token validity) would have introduced, and there is an explicit test for it.
• The retention policy. A transient refresh failure still keeps tokens in storage; the existing retention test at auth-provider.test.tsx:451 passes unmodified, as does the revoked-clears-state test.
• ensureFreshToken() stays on the context for callers that only want the side effect.
• useHighlightPermissionFlow is untouched. With a failed refresh now classified transient upstream, its auth branch is correct as written.

Tests

8 new tests. Auth provider: valid token needs no refresh; expired token refreshes and returns the new one; transient failure → refresh-failed with tokens and session retained; revoked → signed-out with state cleared; no refresh token → signed-out with no network call; concurrent callers join one refresh. useHighlights: the regression itself — expired token + refresh endpoint down + API reachable resolves transient (not auth, not not-signed-in), createHighlight is never called, and the paint reverts; plus signed-out mid-write mapping to not-signed-in.

Remaining test-file changes are mechanical fake updates for the new required context member.

Green: core 419/419 · ui 306 passed, 1 pre-existing skip · pnpm typecheck 5/5 · pnpm lint clean.

Reviewer notes

• Not device-verified yet. The original write-up's repro (sign in, force-quit, airplane mode, cold start, highlight) already produced a correct transient before this change, because an offline write has no HTTP status. The repro that actually exercises this fix is auth-endpoint-down-while-API-up, which needs a proxy or a stubbed token host.
• No ticket key — the write-up this came from carries none, and I did not want to invent one. Happy to add it to the commit and title if there is one.
• One test asserts two total refreshTokens calls rather than one: with an expired stored token, bootstrap always consumes the first refresh, so the test bootstraps with a still-stale response and asserts the accessor drives the second.
• Changeset is a patch: this ships as a bug fix, even though getAccessToken() / AccessTokenResult are additive. Note AccessTokenResult is a required member of AuthContextValue, so any consumer hand-rolling a mock of that type updates — which is what the three test-fake diffs here are.

🤖 Generated with Claude Code

Greptile Summary

The PR adds a refresh-aware access-token accessor and uses it to prevent highlight writes and permission mints from proceeding with an expired token after a transient refresh failure.

• Exposes AccessTokenResult and getAccessToken() through the public auth context.
• Maps failed refreshes to transient outcomes without issuing doomed API requests.
• Keeps token and owner identity in one snapshot and strengthens queued-write session guards.
• Adds regression coverage and updates typed auth-context test doubles.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported token-owner race is covered by the synchronized accessor result and current-identity guard, while null-owner writes are rejected before enqueueing.

Important Files Changed

Filename	Overview
packages/core/src/auth/auth-provider.tsx	Adds the refresh-aware token accessor, integrates it with permission minting, and keeps transient refresh failures distinct from sign-out.
packages/core/src/auth/auth-context.tsx	Defines and documents the new public AccessTokenResult and getAccessToken() context contract.
packages/core/src/highlights/use-highlights.ts	Sources write credentials from the accessor, rejects failed refreshes before network I/O, and verifies the returned token owner against the captured identity.
packages/core/src/auth/data-exchange.ts	Extends the documented transient outcome to include refresh failures before permission minting.
packages/core/src/auth/tests/auth-provider.test.tsx	Covers fresh, refreshed, failed, revoked, absent, and concurrent token-access outcomes.
packages/core/src/highlights/tests/use-highlights.test.tsx	Covers transient refresh handling, request suppression, optimistic rollback, sign-out, and cross-user ownership protection.

Sequence Diagram

sequenceDiagram
  participant Write as Highlight write
  participant Auth as getAccessToken()
  participant Token as Token endpoint
  participant API as Highlights API
  Write->>Auth: request usable token
  Auth->>Token: refresh when near expiry
  alt refresh succeeds
    Token-->>Auth: new token
    Auth-->>Write: ok(token, userId)
    Write->>Write: verify captured user still owns token
    Write->>API: send mutation
  else transient refresh failure
    Token-->>Auth: timeout or 5xx
    Auth-->>Write: unavailable(refresh-failed)
    Write->>Write: revert optimistic paint
    Note over Write,API: No highlights request is issued
  end

Loading

_{Reviews (4): Last reviewed commit: "docs: give Access Token Result a term in..." | Re-trigger Greptile}

Context used:

• Knowledge Base — Auth (PKCE OAuth, token storage, refresh)

[cameronapak]

Claude sent on behalf of Cam.

Two-axis review

Reviewed on two separate axes so neither masks the other. Standards asks whether the diff follows this repo's documented conventions. Spec asks whether it implements YPE-4297. Findings are formatted with Conventional Comments.

Nothing here is blocking. Inline comments carry the findings that anchor to changed lines. The rest are below.

---

Standards

praise: .changeset/core-get-access-token.md

Changeset present, and AGENTS.md:198 documents getAccessToken() alongside refreshNow / ensureFreshToken. The repo's documented release and docs convention is met.

suggestion (non-blocking): CONTEXT.md has no entry for the new concept.

CONTEXT.md already carries Highlight Write Outcome and Granted Permissions as ubiquitous-language terms. AccessTokenResult, with its signed-out / refresh-failed split, is the same kind of concept. This PR also changes what a transient Highlight Write Outcome means. AGENTS.md points at CONTEXT.md for domain language, so a term entry would keep it honest. Judgement call.

note: TOKEN_REFRESH_FAILED_MESSAGE is not an i18n violation.

.greptile/rules.md scopes localization to packages/ui/src/native/**, and CONTEXT.md:138 treats message as diagnostic. Flagging it so it does not get raised later.

---

Spec

praise: The spec's implementation note was already satisfied before this PR.

The ticket says: "The existing refresh lock skips rather than joins: refreshToken early-returns when isRefreshingRef.current is true... An accessor must await an in-flight refresh. It needs a shared promise ref, not a boolean."

origin/main still has isRefreshingRef. The base branch origin/highlights already carries refreshPromiseRef with join semantics, so the accessor inherits it. The new concurrency test does prove joining: two accessor calls plus bootstrap share one refreshTokens call, and all resolve on new-access rather than the expired token.

note (non-blocking): Nothing addresses the init() path, and that reads as intended.

The ticket says the state "can last indefinitely while refresh keeps failing", and auth-provider.test.tsx:393-408 still asserts isAuthenticated: true. That follows the ticket's own proposed direction: "keep isAuthenticated meaning 'has a session'". Recording it so the gap is a decision on the record, not an oversight.

isTokenValid is skipped. The ticket labels it a "Secondary optional idea", so that is fine.

question (non-blocking): The fetch path still sends the raw token.

runFetch at packages/core/src/highlights/use-highlights.ts:295 reads authRef.current.accessToken directly, so the ticket's "Make an authenticated request. Observe that it returns 401" still reproduces on GET. The spec only names runWrite, and a fetch 401 does not invalidate a grant (only write outcomes reach invalidatePermissions), so the damage the ticket describes is fixed. Was leaving runFetch alone a deliberate scope line?

---

Standards: 6 findings. Spec: 3 findings plus 2 notes. Nothing blocking on either axis. I verified the two strongest findings against the source rather than taking them on report.

[Dustin-Kelley]

Claude sent on behalf of Dustin.

Thanks Cam — the two-axis split was useful here, the Spec axis caught the one finding that changed behaviour. Replies are on the individual threads; this covers the three findings that live in the review body.

CONTEXT.md has no entry for the new concept

Agreed, and done in 6fcda4b. Access Token Result now sits before Granted Permissions, framed as the only thing in the SDK that can tell a refresh that worked from one that did not. The signed-out / refresh-failed split is written as two different situations rather than degrees of one, with an Avoid line against collapsing refresh-failed into signed-out — that would sign out a user who is merely offline.

Also added the relationship you pointed at: an unavailable result settles a write as transient and a Data Exchange as transient without a round trip, which is the change to what transient means in a Highlight Write Outcome.

TOKEN_REFRESH_FAILED_MESSAGE is not an i18n violation

Agreed, and thanks for pre-empting it. Diagnostic message, outside packages/ui/src/native/**.

The fetch path still sends the raw token

Deliberate scope line, yes.

The reasoning is the one you already reached: a fetch 401 sets error.reason = 'auth', but error is fetch-only and only write outcomes reach invalidatePermissions, so no grant is dropped and highlights keeps painting from the MMKV cache. The damage YPE-4297 describes is on the write path, and that is what this fixes.

The cost is not trivial either. runFetch's entry is synchronous, and the inFlightRef dedupe invariant depends on that — awaiting a token before the guard opens a window where two GETs for the same scope both pass it. Buying one avoided GET is not worth reopening that in a fix PR.

Where I would revisit it: if a consumer is ever misled by the spurious auth on a fetch error. That is a ticket of its own, not a rider here.

On the init() note

Recording agreement so it stays a decision rather than an oversight — isAuthenticated keeps meaning "has a session", and a session whose refresh keeps failing is still a session. refresh-failed deliberately leaves tokens in storage for the same reason.

[Dustin-Kelley]

@greptile review