refactor(auth): swap custom TwistTokenStore for cli-core's keyring store by scottlovegrove · Pull Request #231 · Doist/twist-cli

scottlovegrove · 2026-05-17T07:58:06Z

Summary

Replaces twist's hand-rolled TwistTokenStore + lib/auth.ts probe/save/clear with cli-core's createKeyringTokenStore, backed by the UserRecordStore adapter from #229. No on-disk schema change, no behavioural change for end users, no keyring slot migration.

Net diff: +324 / −1069. Most of the LOC is deletion — the new surface is a 5-line createKeyringTokenStore({…}) call plus thin shims that preserve the existing getApiToken / probeApiToken / getAuthMetadata signatures.

Changes

lib/auth-provider.ts — createTwistTokenStore() is now createKeyringTokenStore({ serviceName: 'twist-cli', accountForUser: () => 'api-token', userRecords: createTwistUserRecordStore(), recordsLocation: getConfigPath() }). accountForUser is overridden to the existing 'api-token' keyring slot so tokens already in users' keychains stay readable. The custom ~120 LOC of resolveByRef / matchesRef / loadStoredSnapshot is deleted; cli-core owns all of it now. createTwistAuthProvider (OAuth flow) is untouched.
lib/auth.ts — getApiToken / probeApiToken / getAuthMetadata are thin shims that check TWIST_API_TOKEN first then delegate to the store / record list. saveApiToken / clearApiToken / cleanupAuthFallbackState and friends are deleted. probeApiToken's source field is derived by peeking at the UserRecordStore.list(): presence of fallbackToken indicates the keyring was unavailable at write time.
commands/auth/token.ts — writes via store.set({ id: '', label: '', authMode: 'unknown', authScope: '' }, token) and reads store.getLastStorageResult() for the keyring-fallback warning. Token validation drops the arbitrary 10-char minimum in favour of non-empty-after-trim.
commands/auth/helpers.ts — imports TokenStorageResult from @doist/cli-core/auth (cli-core's keyring store owns the shape).
commands/auth/store-wrap.ts — adds an existence check via store.list() when substituting the global --user ref, so PR feat(auth): support pre-subcommand tw --user <ref> <subcommand> #228's tw --user <wrong> auth logout → ACCOUNT_NOT_FOUND behaviour survives the swap (cli-core's KeyringTokenStore.clear is a silent no-op on a non-matching ref).
Tests — deletes the obsolete createTwistTokenStore custom-impl tests; rewrites lib/auth.test.ts and the token / logout / global-flag wiring tests against the new store-backed path.

What this does NOT do

No on-disk config schema change (still flat authUserId / authUserName / authMode / authScope / token fields)
No keyring slot migration (accountForUser: () => 'api-token' keeps existing tokens reachable)
No multi-user support (the accountForUser override + adapter's single-record shape both constrain it)
No migrateLegacyAuth (only needed if we ever move to v2 users[])

Multi-user remains a separate follow-up (PR γ).

Test plan

npm run type-check
npm run lint:check
npm test — 589 pass (down from 624 after deleting ~35 obsolete custom-impl tests, plus 6 new shim tests + adjusted wiring cases)
npm run build
tw auth --help / tw auth token --help render the same surface
Manual end-to-end after tw auth login:
- tw auth status → success (existing keyring entry reads through cli-core)
- tw auth token view → prints the bearer token
- tw auth token <new_token> → overwrites (warning surfaces if keyring unavailable)
- tw auth logout → clears (success line + warning surfaces if keyring unavailable)
Manual fallback (keyring absent — temporarily move node_modules/@napi-rs/keyring aside): tw auth token <token> falls through to config.json with the "system credential manager unavailable" warning, then tw auth status reads back via the same fallback.

🤖 Generated with Claude Code

PR β of α/β: replaces twist's hand-rolled `TwistTokenStore` + `lib/auth.ts` probe/save/clear with cli-core's `createKeyringTokenStore`, backed by the `UserRecordStore` adapter introduced in #229. - `lib/auth-provider.ts`: `createTwistTokenStore()` is now a 5-line `createKeyringTokenStore({ serviceName: 'twist-cli', accountForUser: () => 'api-token', userRecords: createTwistUserRecordStore(), recordsLocation: getConfigPath() })`. `accountForUser` is overridden to the existing `'api-token'` slot so tokens already in users' keychains stay readable — no keyring slot migration needed. The custom `TwistTokenStore` impl (~120 LOC of resolveByRef / matchesRef / loadStoredSnapshot / probeApiToken-fed `active()`) is deleted; the cli-core store handles all of that. `createTwistAuthProvider` (the OAuth flow) is untouched. - `lib/auth.ts`: `getApiToken` / `probeApiToken` / `getAuthMetadata` are now thin shims that check `TWIST_API_TOKEN` first then delegate to the store / record list. `saveApiToken` / `clearApiToken` / `cleanupAuthFallbackState` and friends are deleted — call sites use `store.set()` / `store.clear()` directly. `probeApiToken`'s `source` field is derived by peeking at the `UserRecordStore.list()` (presence of `fallbackToken` indicates the keyring was unavailable at write time). - `commands/auth/token.ts`: writes via `store.set({ id: '', label: '', authMode: 'unknown', authScope: '' }, token)` and reads `store.getLastStorageResult()` for the keyring-fallback warning. Token validation drops the arbitrary 10-char minimum in favour of `non-empty after trim`; the API will reject malformed tokens at next request with a typed 401. - `commands/auth/helpers.ts`: imports `TokenStorageResult` from `@doist/cli-core/auth` (cli-core's keyring store owns the shape). - `commands/auth/store-wrap.ts`: adds an existence check via `store.list()` when substituting the global `--user` ref, so PR #228's `tw --user <wrong> auth logout` → `ACCOUNT_NOT_FOUND` behaviour survives the swap (cli-core's `KeyringTokenStore.clear` is a silent no-op on a non-matching ref). - Tests: deletes the obsolete `createTwistTokenStore` custom-impl tests; rewrites `lib/auth.test.ts` and the token / logout / global-flag wiring tests in `commands/auth/auth.test.ts` against the new store-backed code path. Net +323 / −1058. No on-disk schema change, no behavioural change for end users, no keyring slot migration. Single-user model is preserved (the `accountForUser` override and the adapter's single-record shape constrain it). Multi-user remains a separate follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

doistbot

This PR successfully simplifies the authentication layer by swapping out the custom token store for cli-core's keyring implementation, resulting in a great reduction in overall codebase size. Relying on the shared utility is an excellent step for maintainability, though a few edge cases around backward compatibility and abstraction boundaries need adjusting. Specifically, a few tweaks are necessary to ensure environment variables remain fully supported for status checks, to restore flexible user reference parsing like id: prefixes, and to ensure the legacy keyring contract and ref-normalization logic are properly covered by tests.

_{Share Feedback • Review Logs}

P1 fixes: - `TWIST_API_TOKEN=… tw auth status` was broken: cli-core's `KeyringTokenStore.active()` doesn't know about env vars, so env-only users would report "Not authenticated". `createTwistTokenStore` now wraps `active()` with env-var precedence (ref-less calls short-circuit to the env snapshot; explicit `--user <ref>` still routes to the stored account). - `--user id:<n>` and case-insensitive label refs were dropped because the cli-core default matcher uses strict equality. Pass a `matchAccount: matchTwistAccount` to `createKeyringTokenStore` so the user-facing ref formats (`42`, `id:42`, `Ada`/`ada`) keep working uniformly — both for per-command `--user` (cli-core's `resolveTarget`) and for the global wrapper's existence check. P2 fixes: - Move `probeApiToken`'s source detection out of `lib/auth.ts` and behind the auth-provider seam (`getActiveTokenSource`); the shim no longer pokes the `UserRecordStore`'s `fallbackToken` field directly. - Drop `store-wrap.ts`'s existence pre-check on `active()`. cli-core's `KeyringTokenStore.active` already returns `null` on a non-matching ref, which the attachers surface via `onNotAuthenticated`. Keep the pre-check on `clear()` — that path is a silent no-op without it. - Add a `createTwistTokenStore` wiring test asserting `serviceName`, `accountForUser`, `recordsLocation`, and `matchAccount` match the legacy keyring contract. - Add ref-format coverage (`42`, `id:42`, case-insensitive label) via a `matchTwistAccount` unit test. P3: - Extract `toAccountFields` helper for the duplicated `TwistAccount → AuthMetadata` field mapping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

doist-release-bot · 2026-05-17T10:59:41Z

🎉 This PR is included in version 2.40.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

doistbot requested a review from nvignola May 17, 2026 07:58

scottlovegrove self-assigned this May 17, 2026

scottlovegrove changed the title ~~feat(auth): swap custom TwistTokenStore for cli-core's keyring store~~ refactor(auth): swap custom TwistTokenStore for cli-core's keyring store May 17, 2026

doistbot reviewed May 17, 2026

View reviewed changes

scottlovegrove commented May 17, 2026

View reviewed changes

Comment thread src/commands/auth/store-wrap.ts Outdated

scottlovegrove mentioned this pull request May 17, 2026

auth attachers: accept global --user ref via a getRequestedUserRef callback Doist/cli-core#30

Open

scottlovegrove added the 👀 Show PR PR must be reviewed before or after merging label May 17, 2026

scottlovegrove merged commit 920f2a2 into main May 17, 2026
5 checks passed

scottlovegrove deleted the feat/auth-keyring-token-store branch May 17, 2026 08:29

doist-release-bot Bot added the released label May 17, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(auth): swap custom TwistTokenStore for cli-core's keyring store#231

refactor(auth): swap custom TwistTokenStore for cli-core's keyring store#231
scottlovegrove merged 2 commits into
mainfrom
feat/auth-keyring-token-store

scottlovegrove commented May 17, 2026 •

edited

Loading

Uh oh!

doistbot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doist-release-bot Bot commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

scottlovegrove commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

What this does NOT do

Test plan

Uh oh!

doistbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doist-release-bot Bot commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scottlovegrove commented May 17, 2026 •

edited

Loading