feat(auth): add TokenBundle storage contract to TokenStore#37
Merged
Conversation
Widens the `TokenStore` contract to carry refresh-token state alongside
the access token, without changing behaviour for existing single-token
consumers. `flow.ts` still persists access only — the bundle-aware write
path lands in a follow-up that wires `runOAuthFlow` and the refresh
helper to it.
Contract additions (all backwards-compatible):
- `TokenBundle` and `RefreshInput` types.
- `TokenStore.active()` widened to return optional `bundle?: TokenBundle`.
- `TokenStore.setBundle?(account, bundle, options?)` optional method.
- `AuthProvider.refreshToken?(input)` optional method for future PKCE
refresh-grant impl.
- `UserRecord` gains `accessTokenExpiresAt`, `refreshTokenExpiresAt`,
`fallbackRefreshToken`, and a tri-state `hasRefreshToken` gate that
lets `active()` skip the second keyring IPC for records that have no
refresh token.
- `UserRecordStore.tryInsert?(record)` optional atomic-insert hook for
the follow-up migration PR.
Implementation:
- New `keyring/slot-naming.ts` single-sources the `<account>/refresh`
slot suffix so the runtime read path and any future rename can't
drift.
- `KeyringTokenStore.setBundle` is a required override (non-optional)
that writes both slots via the new `writeBundleWithKeyringFallback`
helper. Rollback on partial failure: refresh-slot non-keyring error
rolls back the access slot; upsert failure rolls back both slots via
`Promise.allSettled`.
- `active()` reads access and refresh slots in parallel, gated by
`hasRefreshToken`. `SecureStoreUnavailableError` on the refresh slot
downgrades to no-refresh; any other read error propagates as
`AUTH_STORE_READ_FAILED`. `undefined` gate (legacy records) probes
once and best-effort backfills `false` for the next read.
- `clear()` wipes both keyring slots — leaving an orphan refresh slot
would defeat the whole point.
- `set(account, token)` now writes `hasRefreshToken: false` so
subsequent `active()` calls skip the refresh probe for accounts
written via the single-token path.
- New `persist.ts` ships `persistBundle({ store, account, bundle,
promoteDefault? })` — prefers `setBundle` when available, falls back
to `set(account, bundle.accessToken)` otherwise, and uniformly wraps
non-`CliError` failures as `AUTH_STORE_WRITE_FAILED`. The bundle-
aware `flow.ts` and the refresh helper both call it in the
follow-up PR.
24 new tests, 384 total. `npm run check` / `type-check` / `test` /
`build` all clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
doistbot
reviewed
May 19, 2026
Member
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR expands the TokenStore contract to accommodate refresh tokens alongside access tokens, safely laying the groundwork for the upcoming PKCE refresh flow. The implementation thoughtfully handles backwards compatibility and parallel slot reads to keep single-token fast paths intact. A few adjustments will help solidify the behavior, particularly around handling empty-string tokens, preventing race conditions during legacy record backfills, correctly cleaning up keyring slots, and aligning error handling and test assertions across both token paths.
No behaviour change. Removes JSDoc that restated the code, speculated about future PRs, or over-explained obvious branches. Net -93 LOC. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses 13 findings from the automated review on #37. P1 — correctness - Drop the fire-and-forget legacy-record backfill. The previous implementation read a stale `UserRecord` snapshot and re-`upsert`ed it, which (per the replace-not-merge contract) would clobber a concurrent `setBundle` write. Legacy records now pay one keyring IPC per `active()` until a `setBundle` writes an explicit gate; that's an acceptable cost and removes the race. P2 — contract & correctness - Flatten `bundle?` off the snapshot returned by `active()`. Refresh state (`refreshToken?`, `accessTokenExpiresAt?`, `refreshTokenExpiresAt?`) now lives flat on the snapshot, removing the duplicate access-token field. New `ActiveTokenSnapshot<TAccount>` type captures the read shape. - `set(account, token)` now wipes the refresh keyring slot best-effort so a previous `setBundle` can't leave orphan refresh material behind. New optional `refreshStore` parameter on `writeRecordWithKeyringFallback`. - `set()` and `setBundle()` reject empty / whitespace-only access tokens with `AUTH_STORE_WRITE_FAILED` instead of writing an empty fallback. - `readAccessToken` wraps unexpected backend failures as `AUTH_STORE_READ_FAILED`, symmetric with `readRefreshToken`. - `setBundle`'s `lastStorageResult` is tied only to the access slot — a refresh-only fallback no longer downgrades the warning text (the existing copy says "token", which implies access; refresh-only fallback is silent today). - `hasRefreshToken: !!refreshToken` instead of `!== undefined` so empty strings (which trigger the no-refresh write path) don't set the gate. - `WriteBundleResult` demoted to a local type (no external consumer). - `persistBundle` JSDoc warns that the `set()` fallback can't honour `promoteDefault`; multi-account stores wanting silent-refresh-safe selection must implement `setBundle`. P3 — tests - New `getCalls` tracking on `buildKeyringMap`. The "skips refresh-slot IPC" test now asserts `getSecret` was not invoked instead of asserting on `deleteCalls` (which `active()` never populates). - The "picks the lone user" test switches to `buildKeyringMap`-style routing instead of the `hasRefreshToken: false` workaround. - New regression test: `set()` after `setBundle()` wipes the refresh slot. - Dropped the backfill timing-dependent test (now moot). 384 tests pass (was 383). `npm run check` / `type-check` / `test` / `build` all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scottlovegrove
commented
May 19, 2026
Per Scott on #37 — codebase convention is `Boolean(x)` for boolean coercion, not `!!x`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@doistbot /review |
doistbot
reviewed
May 19, 2026
Member
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR establishes a solid foundation for refresh-token support by widening the TokenStore contract to accommodate TokenBundle state alongside access tokens. The expanded storage interfaces and fallback mechanisms are well-structured and maintain seamless backwards compatibility for existing single-token consumers. To ensure a fully robust implementation, there are a few areas to refine regarding rollback handling during partial write failures, the performance impact of unconditional keyring IPC reads, and how mixed-storage results report plaintext fallbacks, alongside some opportunities for logic deduplication, test expansion, and documentation updates.
11 findings on #37, addressed in one push. P1 — correctness - Defer the refresh-slot wipe (when bundle has no refresh token) until AFTER `userRecords.upsert` succeeds. The previous order — wipe before upsert — would destroy refresh state on an upsert rollback, leaving the old record with `hasRefreshToken: undefined` and no slot to recover from. `writeRecordWithKeyringFallback` and the no-refresh branch of `writeBundleWithKeyringFallback` both now wipe post-upsert. P2 — read shape - Revert `active()` to its pre-PR1 shape `{ token, account }`. PR1 is storage-only on the contract; the read path stays narrow so common commands (status / logout / token-view) don't pay an extra keyring IPC for refresh state they never touch. Bundle-aware reads land alongside the silent-refresh helper in PR3. Drop `ActiveTokenSnapshot` and `readRefreshToken`; remove the tests that exercised reading refresh material from `active()`. The `hasRefreshToken` field stays on the record for that future read path. P2 — write helper consolidation - `writeRecordWithKeyringFallback` now delegates to `writeBundleWithKeyringFallback` with a refresh-less bundle. Trim / validate / setSecret / fallback / upsert / rollback live in one place. The optional `refreshStore` parameter is the only difference for legacy callers (migration). Empty-token guard is single-sourced. P2 — storage-result mapping - New `bundleStorageResult(accessStored, refreshStored)` composes the `lastStorageResult`. Either falsy slot downgrades to `config-file` with subject-specific text ("access token" / "refresh token" / "access + refresh tokens saved as plaintext"). Refresh-only fallback is no longer invisible. - New shared `promoteDefaultIfNeeded` helper closure; `set` and `setBundle` both call it. P2 — test hygiene - Hoist `mapFixture` to module level; "round-trips" and "picks the lone user" now use it instead of inlining the wiring. - New regression test: `writeBundleWithKeyringFallback` defers the refresh-slot wipe until after upsert (callOrder assertion). - New regression test: headless / WSL bundle path (both slots offline) persists both tokens as fallbacks on the record. - Extend the `active()` failure matrix to cover non-keyring backend errors (in addition to `SecureStoreUnavailableError`) so the new wrap-as-`AUTH_STORE_READ_FAILED` logic is pinned. P3 — comment drift - `hasRefreshToken` JSDoc no longer references the dropped backfill. 382 tests pass. `npm run check` / `type-check` / `test` / `build` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
doist-release-bot Bot
added a commit
that referenced
this pull request
May 19, 2026
## [0.17.0](v0.16.1...v0.17.0) (2026-05-19) ### Features * **auth:** add TokenBundle storage contract to TokenStore ([#37](#37)) ([6513aaa](6513aaa))
Contributor
|
🎉 This PR is included in version 0.17.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Widens the
TokenStorecontract to carry refresh-token state alongside the access token. No behaviour change for existing single-token consumers —runOAuthFlowstill persists access only. The bundle-aware write path lands in PR3 (refresh helper +flow.tsswap).First of three PRs that together deliver refresh-token support to cli-core. Supersedes #36, which Scott will close.
UserRecordshape (atomic-insert via the newtryInsert?hook).refreshAccessTokenhelper,pkce.refreshTokenviaoauth4webapi(optional peer dep),flow.tsbundle persistence.Contract additions (all backwards-compatible)
TokenBundleandRefreshInputtypes.TokenStore.active()widened to return optionalbundle?: TokenBundle.TokenStore.setBundle?(account, bundle, options?)optional method (required override onKeyringTokenStore).AuthProvider.refreshToken?(input)optional method for PR3's PKCE refresh-grant impl.UserRecordgainsaccessTokenExpiresAt,refreshTokenExpiresAt,fallbackRefreshToken, and a tri-statehasRefreshTokengate that letsactive()skip the second keyring IPC for records with no refresh token.UserRecordStore.tryInsert?(record)optional atomic-insert hook for PR2 migration.persistBundle({ store, account, bundle, promoteDefault? })helper — preferssetBundlewhen available, falls back toset(account, bundle.accessToken), uniformly wraps non-CliErrorfailures asAUTH_STORE_WRITE_FAILED.Implementation
keyring/slot-naming.ts(new, internal) single-sources<account>/refreshslot suffix.writeBundleWithKeyringFallbackwrites both slots with rollback: refresh-slot non-keyring error rolls back the access slot; upsert failure rolls back both viaPromise.allSettled.active()reads access + refresh slots in parallel, gated byhasRefreshToken.SecureStoreUnavailableErroron the refresh slot downgrades to no-refresh; other read errors propagate asAUTH_STORE_READ_FAILED.undefinedgate (legacy records) probes once and best-effort backfillsfalse.clear()wipes both keyring slots — no orphan refresh material left behind.set(account, token)now writeshasRefreshToken: falseso subsequentactive()calls on single-token records skip the refresh probe.Test plan
npm run check— cleannpm run type-check— cleannpm test— 384 pass (was 364; net +20 covering bundle round-trip, slot gating, two-slot write rollback, persistBundle fallback paths)npm run build— cleanoutline-clilinked against this branch, verifyol auth loginpersists bundle into both keyring slots🤖 Generated with Claude Code