refactor(auth): adopt cli-core 0.16 createSecureStore - #226
Merged
Conversation
Replace the local @napi-rs/keyring wrapper in src/lib/secure-store.ts with @doist/cli-core/auth's createSecureStore. cli-core's version is a strict superset (memoised AsyncEntry, cause propagation) and owning the keyring contract in one place is the prerequisite for the larger multi-user TokenStore / migrateLegacyAuth migration to follow. - Delete src/lib/secure-store.ts + its test (cli-core owns the surface). - Bump @doist/cli-core 0.12.0 → 0.16.1. - Move SECURE_STORE_DESCRIPTION + service/account constants into auth.ts (cli-core doesn't publicly re-export the description string). - Rewire auth-provider.ts, commands/config/view.ts, and the three test files (auth.test.ts, auth-provider.test.ts, config.test.ts) onto @doist/cli-core/auth. No user-facing change. Single-user model preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
doistbot
reviewed
May 16, 2026
doistbot
left a comment
doistbot
left a comment
Member
There was a problem hiding this comment.
This refactoring successfully adopts @doist/cli-core/auth's createSecureStore, cleanly removing the custom local implementation and reducing the overall code footprint. The changes lay a solid foundation for future auth migrations while preserving the existing single-user model. A few minor adjustments were noted to improve testing and architectural boundaries, specifically around centralizing error imports, re-exporting external auth types from the local library instead of pulling them directly into command handlers, and strengthening a mock to verify the correct keyring arguments are requested.
Address doistbot review on #226: - auth.test.ts: convert the createSecureStore mock to vi.fn and add a test asserting it is called with `{serviceName:'twist-cli', account:'api-token'}` — coverage previously lived in the deleted secure-store.test.ts. - auth-provider.test.ts: hoist `SecureStoreUnavailableError` to a top-level static import; collapse three duplicated dynamic imports. Declined the third nit (re-export `SecureStoreUnavailableError` from lib/auth.ts for config/view): the proposed boundary doesn't exist — commands/auth/{login,logout,status}.ts already depend on @doist/cli-core/auth directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The earlier dedupe pass only collapsed the 8-space-indented dynamic
import; the two copies at 12-space indent inside `describe('ref-aware
lookups')` slipped through. Remove them so all three call sites use the
top-level static import.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
Contributor
|
🎉 This PR is included in version 2.38.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
7 tasks
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
3 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
src/lib/secure-store.ts(+ its test) and switch every consumer tocreateSecureStorefrom@doist/cli-core/auth— cli-core's version is a strict superset (memoisedAsyncEntrypromise,causepropagation onSecureStoreUnavailableError).@doist/cli-core0.12.0 → 0.16.1.SECURE_STORE_DESCRIPTIONand theserviceName/accountliterals local insrc/lib/auth.ts(cli-core doesn't publicly re-export the description string).createKeyringTokenStore+migrateLegacyAuth.Test plan
npm run type-checknpm run lint:checknpm test— 36 files / 605 tests passnpm run buildtw auth login→tw auth status→tw auth logout→tw auth statustw auth token <token>falls through to config-file with the "system credential manager unavailable" warning🤖 Generated with Claude Code