feat(auth): add createKeyringTokenStore multi-account TokenStore (3/4)#27
Merged
Merged
Conversation
This was referenced May 16, 2026
doistbot
reviewed
May 16, 2026
Member
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR introduces the createKeyringTokenStore, thoughtfully integrating multi-account support with OS-level credential managers and local fallback storage. The write choreography and alignment with existing attachers establish a strong foundation for robust authentication handling. There are a few areas to refine, specifically around surfacing typed errors for missing defaults or corrupted keyring reads, ensuring atomic error handling when clearing records, removing duplicated target resolution logic, and adding dedicated tests for the shared write helpers to fully satisfy colocation guidelines.
scottlovegrove
force-pushed
the
feat/auth-store-read-failed
branch
from
644b8c5 to
2cd6aaf
Compare
scottlovegrove
force-pushed
the
feat/keyring-token-store-multi-account
branch
from
492efcf to
d196148
Compare
scottlovegrove
force-pushed
the
feat/auth-store-read-failed
branch
from
2cd6aaf to
5092110
Compare
scottlovegrove
force-pushed
the
feat/keyring-token-store-multi-account
branch
from
d196148 to
4c1b71a
Compare
scottlovegrove
force-pushed
the
feat/auth-store-read-failed
branch
3 times, most recently
from
e5b268a to
93bb598
Compare
scottlovegrove
force-pushed
the
feat/keyring-token-store-multi-account
branch
from
4c1b71a to
e808bd1
Compare
scottlovegrove
force-pushed
the
feat/auth-store-read-failed
branch
3 times, most recently
from
fe9ab44 to
3e86be6
Compare
scottlovegrove
force-pushed
the
feat/keyring-token-store-multi-account
branch
3 times, most recently
from
f700324 to
71ab36d
Compare
scottlovegrove
commented
May 16, 2026
scottlovegrove
force-pushed
the
feat/keyring-token-store-multi-account
branch
from
71ab36d to
943be1a
Compare
Collaborator
Author
|
@doistbot /review |
doistbot
reviewed
May 16, 2026
Member
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR introduces a robust multi-account TokenStore that smartly combines OS keyring storage with a fallback system and proper rollback handling. The careful orchestration between the secure store and user records creates a solid foundation for credential management while nicely setting up the upcoming migration work. There are a few areas to refine, primarily around preventing ambiguous account label resolution, decoupling CLI-specific UI copy from the persistence layer, and addressing minor state caching, test coverage, and mock configuration details.
scottlovegrove
force-pushed
the
feat/keyring-token-store-multi-account
branch
from
943be1a to
76f4a3e
Compare
Multi-account `TokenStore` that keeps secrets in the OS credential manager (via `createSecureStore`) and per-user metadata in a consumer `UserRecordStore` port. Satisfies the full multi-user `TokenStore` contract — `active(ref)` / `clear(ref)` / `list()` / `setDefault(ref)` — so it plugs straight into the existing `logout` / `status` / `token` attachers. Default ref matching is `id || label`; override via `matchAccount` for case-insensitive email, alias maps, etc. Behaviour: - Write: keyring first; on `SecureStoreUnavailableError`, the token lands on `fallbackToken` instead. `userRecords.upsert` failure after a successful keyring write triggers a rollback `deleteSecret` to avoid orphan credentials. - Default promotion is best-effort — a `setDefaultId` failure can't turn a durable credential write into `AUTH_STORE_WRITE_FAILED`. - Read: `fallbackToken` first, then keyring. A matching record with an unreadable keyring throws `AUTH_STORE_READ_FAILED` so `--user <ref>` doesn't silently collapse to `ACCOUNT_NOT_FOUND`. - Clear: record removal first (source of truth), then always-attempt keyring delete (even when the record carries `fallbackToken`, in case a prior online write parked an orphan there). - The keyring write/upsert/rollback choreography lives in a shared `writeRecordWithKeyringFallback` helper so the migration helper can reuse the same routing and rollback semantics. Reintroduces `DEFAULT_ACCOUNT_FOR_USER` in `secure-store.ts` (a new caller in this diff needs the shared default) and promotes `accountNotFoundError` in `auth/user-flag.ts` to a module-level export so the keyring `setDefault` can reuse the same error wording instead of redeclaring it. Test helpers live under `src/test-support/` (excluded from the build by `tsconfig.build.json`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scottlovegrove
force-pushed
the
feat/keyring-token-store-multi-account
branch
from
76f4a3e to
5fb4526
Compare
doist-release-bot Bot
added a commit
that referenced
this pull request
May 16, 2026
## [0.15.0](v0.14.0...v0.15.0) (2026-05-16) ### Features * **auth:** add createKeyringTokenStore (multi-account) ([#27](#27)) ([adc07d1](adc07d1))
Contributor
|
🎉 This PR is included in version 0.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
Stack position: 3 / 4 — base is #26 (
feat/auth-store-read-failed). Replaces the bulk of #24.Summary
Multi-account
TokenStorethat keeps secrets in the OS credential manager (viacreateSecureStorefrom #25) and per-user metadata in a consumerUserRecordStoreport. Satisfies the full multi-userTokenStorecontract (active(ref)/clear(ref)/list()/setDefault(ref)) so it plugs straight into the existinglogout/status/tokenattachers.Behaviour
Write order — keyring first; on
SecureStoreUnavailableError, the token lands onfallbackTokeninstead.userRecords.upsertfailure after a successful keyring write triggers rollbackdeleteSecretto avoid orphan credentials. Default promotion is best-effort.Read order —
fallbackTokenfirst, then keyring. A matching record with an unreadable keyring throwsAUTH_STORE_READ_FAILED(the typed code added in #26) so--user <ref>doesn't silently collapse toACCOUNT_NOT_FOUND.Clear order — record removal first (source of truth), then always-attempt keyring delete (orphan cleanup even when the record carried
fallbackToken).The shared keyring-write choreography lives in
writeRecordWithKeyringFallbackso PR 4 (migrateLegacyAuth) can reuse it.Files
src/auth/keyring/types.ts—UserRecord,UserRecordStoreport,TokenStorageResult.src/auth/keyring/record-write.ts— shared write/upsert/rollback helper.src/auth/keyring/token-store.ts(+ test) — the factory.src/test-support/keyring-mocks.ts— shared test mocks (excluded from build).Test plan
npm run type-check,npm run check,npm test(328 passing, +21 new for the keyring TokenStore)DBUS_SESSION_BUS_ADDRESSunset.🤖 Generated with Claude Code