feat(auth): add account current/remove Commander attachers#46
Conversation
Complete the account-management attacher family alongside list/use so consumers stop hand-rolling these subcommands. `attachAccountCurrentCommand` is selector-less: reads `store.active()`, derives the default marker from `store.list()`, and routes out-of-store credential sources (env/legacy) through an `onNotAuthenticated` hook. `attachAccountRemoveCommand` resolves `<ref>` via `store.active(ref)` and clears by the resolved canonical `account.id` — never the raw ref, which `clear()` would silently no-op on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR completes the auth subpath's account-management family by adding the current and remove Commander attachers, bringing welcome consistency to how consumer CLIs handle these operations. The thorough test coverage and thoughtful integration alongside existing commands establish a solid foundation for the upcoming tool rewrites. There are a few areas to refine before merging, primarily decoupling the removal and current-account logic from secret readability to prevent keychain-related cleanup issues and performance overhead, alongside opportunities to extract shared helpers for formatting, JSON validation, and view emission.
- remove: resolve the target token-free by diffing `store.list()` around `store.clear(ref)` instead of `store.active(ref)`, so a broken/unreadable keyring entry can still be removed (active would throw AUTH_STORE_READ_FAILED). - Extract shared `formatAccountLine` and `buildAccountPayload` helpers so list and current can't drift on rendering or INVALID_TYPE handling; the payload serializability probe now also catches throwing values (circular, BigInt). - remove: reuse `emitView` for the json/ndjson/human split, matching `use`. - current: build the render context once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@doistbot /review |
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR successfully expands the auth subpath by introducing the account current and account remove attachers, marking a great step forward in unifying our CLI account management. There are a few architectural adjustments needed to ensure robust store interactions, specifically around preventing race conditions during account removal, avoiding duplicated I/O reads during lookups, and properly handling implicit default account states. Finally, expanding test coverage for hook execution and serialization errors, along with a minor style tweak to remove a status glyph from the standard output, will ensure everything aligns perfectly with our core standards.
Second-review follow-up. - TokenStore.clear(ref) now returns the removed account + its prior effective-default bit (ClearedAccount), or null on a miss. Resolution + deletion happen atomically in the store, so `account remove` drops the racy before/after list() diff and the token-free guarantee is now contractual (a store can't satisfy clear() by reading the token). - Add optional token-free TokenStore.activeAccount(ref) resolver; KeyringTokenStore implements it (required override). `account current` uses it so it no longer pays a token read plus a separate list() on every call. - remove: drop the (default) marker copy that could falsely claim the default was cleared; the marker now states only what was removed. - Tests: clear() return + activeAccount coverage on KeyringTokenStore; a serializer-throwing (BigInt) INVALID_TYPE case; onRemoved gated-promise ordering; current prefers activeAccount. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## [0.23.0](v0.22.0...v0.23.0) (2026-05-23) ### Features * **auth:** add account current/remove Commander attachers ([#46](#46)) ([294f1fe](294f1fe))
|
🎉 This PR is included in version 0.23.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Summary
authsubpath alongsideattachAccountListCommand/attachAccountUseCommand, so consumer CLIs (todoist-cli, etc.) can drop their hand-rolledcurrent/removesubcommands.attachAccountCurrentCommand: selector-less. Readsstore.active(), derives the(default)marker fromstore.list()(keyed onaccount.id, identical toaccount list), and renders viarenderText/renderJson. When nothing resolves it invokesonNotAuthenticated(or throwsNOT_AUTHENTICATED) — the hook where consumers render out-of-store credential sources (env-var token, legacy single-user creds).attachAccountRemoveCommand: required positional<ref>, resolved viastore.active(ref)(miss →ACCOUNT_NOT_FOUND). Clears by the resolved canonicalaccount.id, never the raw ref —clear()is keyed by id and would silently no-op on an email/label ref.--jsonemits{ ok: true, removed: <id> };--ndjsonsilent; human prints✓ Removed <label ?? id>plus a cleared-default line.onRemovedhook for store-specific follow-ups (e.g. keyring-fallback warning).src/auth/index.ts; README "What's in it" row and account-selection section updated per the AGENTS.md same-commit rule.Scope is cli-core only — the todoist-cli rewrite follows a cli-core release, matching how list/use rolled out.
Test plan
npm run type-check— re-export surface validates via tscnpm test— 486 pass (20 new account.test.ts cases covering both attachers: default + custom renders, json/ndjson payloads,INVALID_TYPEguard, clear-by-id,ACCOUNT_NOT_FOUND, wasDefault hint, hook invocation)npm run check— oxlint + oxfmt clean🤖 Generated with Claude Code