Uh oh!
There was an error while loading. Please reload this page.
feat(auth): add OAuth refresh token session handling - #205
Conversation
🦋 Changeset detectedLatest commit: 9991c0c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dbb4068 to
b1ea0f7CompareNote Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OAuth refresh-token support across the CLI: credential storage now persists an OAuthSession (accessToken, refreshToken, expiresAt, tokenType) and exposes createOAuthSession, getStoredSession, hasStoredCredentials, and getValidToken. getValidToken validates expiry and attempts refresh via refreshAccessToken, deleting credentials and throwing AuthError(reason: "session_expired") on invalid_grant. Introduces AuthError and isAuthError. Commands and doctor/init/whoami/plapi call getValidToken instead of getToken. Tests and mocks updated to exercise session persistence and refresh flows; token-exchange gains refreshAccessToken. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli-core/src/commands/auth/login.test.ts (3)
521-521:⚠️ Potential issue | 🔴 CriticalBuild failure:
mockGetTokenis undefined.The pipeline reports
TS2304: Cannot find name 'mockGetToken'. This should bemockGetValidTokento match the mock defined at line 7.🐛 Fix
- mockGetToken.mockResolvedValue(null);+ mockGetValidToken.mockResolvedValue(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-core/src/commands/auth/login.test.ts` at line 521, The test references an undefined variable `mockGetToken`; replace that usage with the correctly defined mock `mockGetValidToken` in the failing test (the call at the line with mockGetToken.mockResolvedValue(null)) so the test uses the existing mock variable `mockGetValidToken` instead of the non-existent `mockGetToken`.
553-553:⚠️ Potential issue | 🔴 CriticalBuild failure: same
mockGetTokenreference.🐛 Fix
- mockGetToken.mockResolvedValue(null);+ mockGetValidToken.mockResolvedValue(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-core/src/commands/auth/login.test.ts` at line 553, The failing test reuses the same mockGetToken reference across test cases which causes value bleed and unpredictable behavior; update the tests to either reset/replace mockGetToken between tests (call mockGetToken.mockReset() / mockGetToken.mockClear() in beforeEach or reassign mockGetToken = jest.fn() per test) or use mockGetToken.mockResolvedValueOnce(...) when setting per-test return values so each test gets an isolated mock value; locate usages of mockGetToken in the login.test.ts file and apply one of these fixes to ensure each test configures its own mock behavior.
582-582:⚠️ Potential issue | 🔴 CriticalBuild failure: same
mockGetTokenreference.🐛 Fix
- mockGetToken.mockResolvedValue("existing-token");+ mockGetValidToken.mockResolvedValue("existing-token");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-core/src/commands/auth/login.test.ts` at line 582, The test is reusing the same mock reference mockGetToken which causes cross-test leakage; change the setup to create an isolated mock or set a one-time resolution so other tests aren't affected—for example replace mockGetToken.mockResolvedValue("existing-token") with a one-off behavior (mockGetToken.mockResolvedValueOnce("existing-token")) or recreate/reset mockGetToken before this test (via jest.fn() or jest.resetAllMocks()) so the mock is unique to this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/commands/init/heuristics.ts`:
- Around line 6-7: The isAuthenticated check currently relies on
getStoredSession()/getValidToken deserialization and can misclassify legacy
stored credentials; change isAuthenticated to perform a credential-presence
check instead (e.g., verify existence of any stored credential/token rather than
successful session deserialization). Update the logic in heuristics.ts where
isAuthenticated is implemented/used (referencing getStoredSession, getValidToken
and the isAuthenticated call) to detect presence of credentials in the store and
return true for legacy tokens, ensuring keyless init isn't incorrectly chosen;
keep existing validation flows for full session usage separate from this
presence check.
In `@packages/cli-core/src/lib/plapi.ts`:
- Around line 60-64: The API command tests are failing because getValidToken()
now throws an AuthError with reason "not_logged_in"; update the api command test
mocks in index.test.ts to either mock getValidToken() to throw an AuthError
instance with the same reason/message/docsUrl and assert that the command
surface handles/propagates that error, or change the mock to return a valid
token for tests that expect authenticated behavior; specifically adjust the mock
setup and assertions to reference AuthError and getValidToken() so the tests
reflect the new unauthenticated PLAPI path.
---
Outside diff comments:
In `@packages/cli-core/src/commands/auth/login.test.ts`:
- Line 521: The test references an undefined variable `mockGetToken`; replace
that usage with the correctly defined mock `mockGetValidToken` in the failing
test (the call at the line with mockGetToken.mockResolvedValue(null)) so the
test uses the existing mock variable `mockGetValidToken` instead of the
non-existent `mockGetToken`.
- Line 553: The failing test reuses the same mockGetToken reference across test
cases which causes value bleed and unpredictable behavior; update the tests to
either reset/replace mockGetToken between tests (call mockGetToken.mockReset() /
mockGetToken.mockClear() in beforeEach or reassign mockGetToken = jest.fn() per
test) or use mockGetToken.mockResolvedValueOnce(...) when setting per-test
return values so each test gets an isolated mock value; locate usages of
mockGetToken in the login.test.ts file and apply one of these fixes to ensure
each test configures its own mock behavior.
- Line 582: The test is reusing the same mock reference mockGetToken which
causes cross-test leakage; change the setup to create an isolated mock or set a
one-time resolution so other tests aren't affected—for example replace
mockGetToken.mockResolvedValue("existing-token") with a one-off behavior
(mockGetToken.mockResolvedValueOnce("existing-token")) or recreate/reset
mockGetToken before this test (via jest.fn() or jest.resetAllMocks()) so the
mock is unique to this test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b17f3a36-f5cd-4db0-a653-43dfc0f9b368
📒 Files selected for processing (18)
.changeset/oauth-refresh-token-support.mdpackages/cli-core/src/commands/auth/login.test.tspackages/cli-core/src/commands/auth/login.tspackages/cli-core/src/commands/doctor/checks.tspackages/cli-core/src/commands/doctor/context.tspackages/cli-core/src/commands/doctor/doctor.test.tspackages/cli-core/src/commands/doctor/types.tspackages/cli-core/src/commands/init/heuristics.tspackages/cli-core/src/commands/whoami/index.test.tspackages/cli-core/src/commands/whoami/index.tspackages/cli-core/src/lib/credential-store.tspackages/cli-core/src/lib/errors.tspackages/cli-core/src/lib/plapi.test.tspackages/cli-core/src/lib/plapi.tspackages/cli-core/src/lib/token-exchange.test.tspackages/cli-core/src/lib/token-exchange.tspackages/cli-core/src/test/integration/lib/harness.tspackages/cli-core/src/test/lib/stubs.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…henticated CLI commands
Align the implementation with the documented "pure credential-presence check" intent. The previous code deserialized via getStoredSession() and returned false for legacy raw-string credentials that don't match the OAuthSession shape, incorrectly sending those users to the keyless init flow.
6009648 to
026c76cCompare| const nextSession = createOAuthSession(tokenResponse, session.refreshToken); | ||
| await storeToken(nextSession); | ||
| return nextSession.accessToken; | ||
| } |
There was a problem hiding this comment.
If two parallel CLI calls hit getValidToken() while the token is expired, both will race into refreshStoredSession with the same refresh token. If the server rotates refresh tokens on use, the second call gets invalid_grant and nukes the credentials the first call just wrote.
Not likely in typical CLI usage, but could bite in CI scripts running clerk commands in parallel. A module-level promise guard would deduplicate:
| } | |
| letrefreshInFlight: Promise<string>|undefined; | |
| asyncfunctionrefreshStoredSession(session: OAuthSession): Promise<string>{ | |
| if(refreshInFlight)returnrefreshInFlight; | |
| refreshInFlight=doRefreshStoredSession(session).finally(()=>{ | |
| refreshInFlight=undefined; | |
| }); | |
| returnrefreshInFlight; | |
| } | |
| asyncfunctiondoRefreshStoredSession(session: OAuthSession): Promise<string>{ |
Non-blocking, happy to merge as-is and handle this as a follow-up.
There was a problem hiding this comment.
Yeah it can happen. Not sure about how to address it. I don't think I want to add system or user locks at this point, so let's wait for another PR?
There was a problem hiding this comment.
I got a version that's resilient to this change when more than one cli is operating at once. Don't think we needed the in-process protections as much as the cross-process ones.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 324-340: getValidToken treats any non-JSON legacy stored value as
an expired session because getStoredSession only handles the new JSON shape; to
fix this, update the flow to accept legacy raw tokens: in getStoredSession (or
in parseStoredSession) detect if readStoredValue() returns a plain string
(legacy raw token) and return an OAuthSession-like object (or at least an object
with accessToken populated) so callers like getValidToken can use it, or
alternatively modify getValidToken to call readStoredValue() directly when
getStoredSession() is null and hasStoredCredentials() is true and treat a plain
string value as a valid access token; reference functions: getStoredSession,
getValidToken, readStoredValue, parseStoredSession.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b91a154-39a2-4ecc-9516-cdbcd12329a3
📒 Files selected for processing (21)
.changeset/oauth-refresh-token-support.mdpackages/cli-core/src/commands/api/index.test.tspackages/cli-core/src/commands/auth/login.test.tspackages/cli-core/src/commands/auth/login.tspackages/cli-core/src/commands/doctor/checks.tspackages/cli-core/src/commands/doctor/context.tspackages/cli-core/src/commands/doctor/doctor.test.tspackages/cli-core/src/commands/doctor/types.tspackages/cli-core/src/commands/init/heuristics.tspackages/cli-core/src/commands/whoami/index.test.tspackages/cli-core/src/commands/whoami/index.tspackages/cli-core/src/lib/autolink.test.tspackages/cli-core/src/lib/credential-store.test.tspackages/cli-core/src/lib/credential-store.tspackages/cli-core/src/lib/errors.tspackages/cli-core/src/lib/plapi.test.tspackages/cli-core/src/lib/plapi.tspackages/cli-core/src/lib/token-exchange.test.tspackages/cli-core/src/lib/token-exchange.tspackages/cli-core/src/test/integration/lib/harness.tspackages/cli-core/src/test/lib/stubs.ts
✅ Files skipped from review due to trivial changes (3)
- packages/cli-core/src/lib/autolink.test.ts
- packages/cli-core/src/commands/whoami/index.test.ts
- packages/cli-core/src/commands/api/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- .changeset/oauth-refresh-token-support.md
- packages/cli-core/src/commands/doctor/types.ts
- packages/cli-core/src/commands/init/heuristics.ts
- packages/cli-core/src/lib/plapi.ts
- packages/cli-core/src/commands/whoami/index.ts
- packages/cli-core/src/lib/plapi.test.ts
- packages/cli-core/src/commands/auth/login.ts
- packages/cli-core/src/lib/token-exchange.test.ts
- packages/cli-core/src/test/lib/stubs.ts
| export async function getStoredSession(): Promise<OAuthSession | null> { | ||
| const value = await readStoredValue(); | ||
| if (!value) return null; | ||
| return parseStoredSession(value); | ||
| } | ||
| return fileGet(); | ||
| export async function hasStoredCredentials(): Promise<boolean> { | ||
| return (await readStoredValue()) !== null; | ||
| } | ||
| export async function getValidToken(): Promise<string | null> { | ||
| const session = await getStoredSession(); | ||
| if (!session) { | ||
| if (await hasStoredCredentials()) { | ||
| throw sessionExpiredError(); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Legacy raw credentials are treated as expired immediately.
getStoredSession() only recognizes the new JSON shape, so any pre-upgrade raw access token falls into this branch and becomes session_expired. Since authenticated commands now go through getValidToken(), upgrading will force every previously logged-in user to re-authenticate even when their stored token is still valid.
Suggested fix
export async function getValidToken(): Promise<string | null> {
- const session = await getStoredSession();- if (!session) {- if (await hasStoredCredentials()) {- throw sessionExpiredError();- }- return null;- }+ const value = await readStoredValue();+ if (!value) return null;++ const session = parseStoredSession(value);+ if (!session) {+ const expiresAt = getJwtExpiryMs(value);+ if (expiresAt !== null && expiresAt <= Date.now() + JWT_EXPIRY_LEEWAY_MS) {+ throw sessionExpiredError();+ }+ return value;+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-core/src/lib/credential-store.ts` around lines 324 - 340,
getValidToken treats any non-JSON legacy stored value as an expired session
because getStoredSession only handles the new JSON shape; to fix this, update
the flow to accept legacy raw tokens: in getStoredSession (or in
parseStoredSession) detect if readStoredValue() returns a plain string (legacy
raw token) and return an OAuthSession-like object (or at least an object with
accessToken populated) so callers like getValidToken can use it, or
alternatively modify getValidToken to call readStoredValue() directly when
getStoredSession() is null and hasStoredCredentials() is true and treat a plain
string value as a valid access token; reference functions: getStoredSession,
getValidToken, readStoredValue, parseStoredSession.
There was a problem hiding this comment.
@wyattjoh, understood — forcing a one-time re-authentication for users with a legacy raw token on upgrade is an intentional migration cut-over. I'll note this for future reviews.
✏️ Learnings added
Learnt from: wyattjoh
Repo: clerk/cli PR: 205
File: packages/cli-core/src/lib/credential-store.ts:324-340
Timestamp: 2026-04-22T17:39:54.554Z
Learning: In clerk/cli credential-store (`packages/cli-core/src/lib/credential-store.ts`), `getValidToken()` intentionally treats any stored credential that cannot be parsed as a valid `OAuthSession` JSON (i.e. a legacy raw access token string) as a `session_expired` error, forcing a one-time re-authentication on upgrade. Do not flag this as a bug — it is a deliberate migration design decision by the author.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: wyattjoh
Repo: clerk/cli PR: 118
File: packages/cli-core/src/commands/init/skills.ts:117-124
Timestamp: 2026-04-07T22:06:40.516Z
Learning: In clerk/cli, cancellation from inquirer/prompts (e.g., Ctrl+C resulting in UserAbortError / ExitPromptError) is handled globally in cli-program.ts (around the existing global catch). Therefore, command-level prompt call sites (e.g., confirm(), select()) in packages/cli-core/src should not wrap these prompt calls in local try/catch for UserAbortError/ExitPromptError cancellations—such local handling would be dead code and should remain consistent with the shared cancellation behavior.
Learnt from: wyattjoh
Repo: clerk/cli PR: 153
File: .claude/rules/scripts.md:36-36
Timestamp: 2026-04-13T23:09:41.178Z
Learning: In the clerk/cli repository, do not treat `Bun.$` (Bun Shell API) as experimental/unstable during code review. `Bun.$` is a stable, documented core Bun runtime feature (similar in status to `Bun.spawn` and `Bun.file`), so reviewers should not raise warnings that it relies on experimental APIs.
jfoshee
left a comment
There was a problem hiding this comment.
Some initial comments. I am still reviewing. :-)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| const nextSession = createOAuthSession(tokenResponse, session.refreshToken); | ||
| await storeToken(nextSession); | ||
| return nextSession.accessToken; | ||
| } |
There was a problem hiding this comment.
Yeah it can happen. Not sure about how to address it. I don't think I want to add system or user locks at this point, so let's wait for another PR?
jfoshee
left a comment
There was a problem hiding this comment.
Looks good. I did a manual test locally w/ a 5 min access token. Worked fine. It would just be nice to see something in the trace log that it was doing a refresh.
Another nice-to-have would be a manual refresh? like clerk auth refresh? wdyt? could be an internal only testing command.
| async function refreshStoredSession(session: OAuthSession): Promise<string> { | ||
| let tokenResponse: TokenResponse; | ||
| try { | ||
| tokenResponse = await refreshAccessToken(session.refreshToken); |
There was a problem hiding this comment.
I would like to see in the debug trace that we are doing a refresh. Is this the place to log it?
Summary
OAuth access tokens issued to the CLI are now short-lived JWTs. The credential store previously persisted only the raw access token, so once the token's
exppassed every authenticated command failed with a 401 and forced the user back throughclerk auth login. This change persists the full OAuth session, refreshes the access token transparently from the stored refresh token, and routes every auth failure through a single typed error so downstream code can react without string-matching.What changed
storeTokenaccepts either a legacy access-token string or anOAuthSession(accessToken,refreshToken,expiresAt,tokenType) and persists it as JSON to the same keyring/file slot.getToken()stays backwards-compatible for readers that only want the string, so a session written by an older build is still readable.getValidToken()decodes the access-token JWTexp, and if it is within a 30s leeway of expiry calls the OAuth token endpoint with the stored refresh token, writes the new session back, and returns the fresh access token.invalid_grantresponses wipe the stored session and raiseAUTH_REQUIREDso the user is told to re-login rather than hitting a silent 401 loop.getValidToken():login,whoami,plapirequests,initheuristics, and every doctor check that needs a live session. Presence-only probes (is the user logged in at all?) use the newhasStoredCredentials()so we don't refresh as a side effect of non-auth flows.getValidToken()soclerk doctorsurfaces refresh failures (expired refresh token, server-side revoke) the same way it surfaces other auth breakage, instead of reporting a healthy session while the next real call 401s.AuthErrorsubclass ofCliErrorcarries areasondiscriminator (not_logged_in/session_expired) with default per-reason messages, always codes toAUTH_REQUIRED, and replaces the hand-rolledCliErrorthrows inwhoami,plapi.getAuthToken, andcredential-store. Doctor's token check now uses a newisAuthError()type guard covering bothAuthErrorandApiError(401|403)instead of regex-matching(401|403)substrings in error messages, so message-copy changes can't silently break the check.Test plan
bun run format:check,bun run lint,bun run typecheck,bun run testgetValidToken(): valid JWT returns unchanged, near-expiry JWT triggers a refresh and rewrites the session,invalid_grantclears credentials and throwsAUTH_REQUIRED.AuthError({ reason: "session_expired" })intogetValidToken()and asserting the check fails with the re-login remedy.exp, runclerk whoamiand confirm a refresh request fires and the session is rewritten transparently.