auth: refresh user tokens earlier and clarify refresh failures

[ArthurNie]

Summary

• refresh user access tokens when they are within 60 minutes of expiry instead of 5 minutes
• keep using a still-valid access token when refresh fails with a transport/retryable/non-refresh-token-invalidating error
• continue clearing stored tokens for refresh-token invalid/expired/revoked/reused cases that require reauthorization
• include the classified auth error subtype in refresh failure diagnostics
• add coverage for the refresh-ahead token status window, still-valid fallback behavior, retry fallback behavior, and revoked-token clearing

Why

User access tokens are short-lived, and the previous 5-minute refresh-ahead window is easy for scheduled or intermittent CLI usage to miss. If refresh later fails with a non-retryable OAuth business code such as 20064, the CLI clears the stored user token and subsequent commands only see missing authorization. A wider refresh window makes normal CLI usage more likely to rotate tokens before expiry.

The fallback avoids widening the blast radius of refresh-side failures: when the current access token is still valid and the failure does not prove the refresh token is unusable, the CLI keeps using the existing token until expiry. Revoked, reused, invalid, or expired refresh-token classifications still clear the stored token and require reauthorization.

Test

• go test ./internal/auth ./internal/errclass

No raw tokens, user IDs, log IDs, or Feishu response bodies are included in this PR.

Summary by CodeRabbit

• Bug Fixes
  • Extended the token “refresh-ahead” window from 5 to 60 minutes.
  • Improved token refresh behavior to preserve a still-valid access token across more network and retry failure scenarios, and to clear unusable refresh tokens when appropriate.
  • Updated user diagnostics to consistently report identity as ready after successful verification.
• Tests
  • Added/expanded unit tests covering refresh timing, retryable refresh failures, keep-vs-clear outcomes, and revoked refresh token handling.

[albertnusouo]

Overall: Solid, well-tested change addressing a real pain point — the 5-min window is easy for scheduled/intermittent CLI
usage to miss. Code compiles, meta/metaOK are correctly in scope, and the new test meaningfully guards the constant
(59/61 straddle the boundary and would fail under the old value). A few things worth considering before merge.

Main concern — widening the window enlarges the blast radius of clearing a still-valid token

In GetValidAccessToken, the needs_refresh path clears the token and returns NeedAuthorizationError on any non-retryable
refresh failure. With the old 5-min window, the in-hand access token had ≤5 min left, so discarding it was cheap. At 60 min,
a transient refresh-side hiccup (or an errclass misclassification) can now discard an access token with up to ~59 min of
remaining validity and force a re-login. This trades availability for earlier refresh, and is strictly more fragile when the
refresh endpoint is flaky.

Suggestion: on a non-retryable refresh failure, check now < stored.ExpiresAt first — if the access token is still
actually valid, keep using it (warn only) instead of clearing. If immediate invalidation on revoked/reused codes is an
intentional security policy, please state that explicitly, as it's a deliberate tradeoff.

Implicit assumption — access token TTL must exceed the refresh-ahead window

This relies on TTL > 60 min (currently 7200s default). If the server ever issues tokens with TTL ≤ 60 min, ExpiresAt - 60min is always in the past and every CLI invocation triggers a refresh. Worth a one-line comment at the constant
documenting this dependency.

Minor

• Refresh call frequency roughly doubles (one extra rotation per token lifetime) — negligible at current scale, but a noted
  cost.
• Nit: the PR body mentions running go test ./internal/errclass, but no errclass code/tests are touched here
  (regression-only, no issue).

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f7c7c8e2-87c8-487c-b44d-2de22be5f509

📥 Commits

Reviewing files that changed from the base of the PR and between 8c21ebd and 2b97269.

📒 Files selected for processing (1)

• internal/identitydiag/diagnostics.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/identitydiag/diagnostics.go

---

📝 Walkthrough

Walkthrough

The PR increases the refresh-ahead window from 5 to 60 minutes, updates refresh failure handling to preserve still-valid access tokens unless the refresh token is classified as unusable, and simplifies post-verification identity status reporting to unconditionally mark verified users as ready.

Changes

Token Refresh and Identity Verification

Layer / File(s)	Summary
Refresh-ahead window adjustment internal/auth/token_store.go, internal/auth/token_store_test.go	refreshAheadMs increased from 5 to 60 minutes. Test verifies TokenStatus returns "valid" outside the window and "needs_refresh" inside it.
Smart token preservation on refresh failure internal/auth/uat_client.go, internal/auth/token_store_test.go	doRefreshToken now preserves still-unexpired access tokens on network and authentication error paths, clearing only when shouldClearTokenAfterRefreshFailure classifies the refresh token as invalid/expired/revoked/reused. Tests cover transient, retryable, and revoked-refresh scenarios.
Identity verification status reporting internal/identitydiag/diagnostics.go	After successful --verify, user identity is unconditionally set to ready, available, and marked with "User identity: ready" message (previously conditional on prior state).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• larksuite/cli#957: Introduced identity-diagnostics separation of bot vs user verification in internal/identitydiag/diagnostics.go, which this PR refines with unconditional post-verification status handling.

Suggested reviewers

• liangshuo-1

Poem

🐰 I hop through tokens, old and new,
Sixty minutes feels so true!
When refreshes stumble, slip, and slip,
Good tokens ride the waiting ship.
Till verified identities bloom right through! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the two main changes: extending the token refresh window (earlier refresh) and improving error handling (clarifying failures).
Description check	✅ Passed	The description includes all required sections with substantive content: Summary explains the motivations, Changes lists key modifications, Test Plan specifies verification method, and Related Issues is addressed.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@internal/auth/token_store_test.go`:
- Around line 74-82: The test currently only exercises the helper
shouldClearTokenAfterRefreshFailure; add a unit test that directly invokes
doRefreshToken (or the exported function that performs the refresh flow) to
simulate an unrecoverable refresh failure and assert it both clears the stored
token in the TokenStore and emits the expected "auth login" guidance message;
mock or stub the refresh RPC to return an errclass with
Category=errs.CategoryAuthentication and
Subtype=errs.SubtypeRefreshTokenRevoked, call doRefreshToken with a TokenStore
pre-populated with a refresh token, then verify the TokenStore no longer
contains the token and that the logger/output contains the auth login guidance.

In `@internal/auth/uat_client.go`:
- Around line 271-281: The code drops the session or returns on callEndpoint()
transport/retry failures without applying the same "keep valid token until
expiry" fallback used for parsed OAuth errors; update the error handling around
callEndpoint() and the retry branch to check stored.ExpiresAt and
shouldClearTokenAfterRefreshFailure(meta, metaOK) (using meta/metaOK when
available) and, when the access token is still valid and
shouldClearTokenAfterRefreshFailure returns false, log a warning (including
opts.UserOpenId and the error/transport info) and return the existing stored
token instead of clearing it; ensure the same subtype/log message pattern used
in the parsed-error path is applied for transport/retry failures so transient
outages don’t force re-login.

🪄 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

Run ID: 7c24f21f-2e3d-41df-8b65-0628b19301b5

📥 Commits

Reviewing files that changed from the base of the PR and between c11cf3b and 5b10122.

📒 Files selected for processing (3)

• internal/auth/token_store.go
• internal/auth/token_store_test.go
• internal/auth/uat_client.go

[github-actions]

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d6c0c0351c92933e4b7d8d45da7743bb8580f216

🧩 Skill update

npx skills add ArthurNie/cli#codex/auth-refresh-ahead-diagnostics -y -g

[CLAassistant]

All committers have signed the CLA.