Skip to content

feat(auth): support pre-subcommand tw --user <ref> <subcommand>#228

Merged
scottlovegrove merged 5 commits into
mainfrom
feat/auth-global-user-flag
May 16, 2026
Merged

feat(auth): support pre-subcommand tw --user <ref> <subcommand>#228
scottlovegrove merged 5 commits into
mainfrom
feat/auth-global-user-flag

Conversation

@scottlovegrove
Copy link
Copy Markdown
Collaborator

@scottlovegrove scottlovegrove commented May 16, 2026

Summary

cli-core's auth attachers (attachLogoutCommand, attachStatusCommand, attachTokenViewCommand) already declare a per-command --user <ref> flag, but commander has no root-level --user option, so the global form tw --user 42 auth status was rejected at parse time. This PR completes the plumbing started by the cli-core 0.16 adoption (#226).

  • src/index.ts — warm the global-args cache via getRequestedUserRef() before mutating argv (so the original --user value is captured), then strip the pre-subcommand --user from process.argv via cli-core's stripUserFlag so commander sees a clean argv. Per-command --user (after the subcommand name) survives the strip and reaches commander unchanged.
  • src/lib/global-args.ts — expose getRequestedUserRef() reading store.get().user from the existing lazy parser cache. The user field was already part of TwGlobalArgs; this just adds an accessor.
  • src/commands/auth/store-wrap.ts (new)withUserRefAware(store, requestedRef) adapter built with Object.create(store) and overriding only active / clear so future TwistTokenStore methods inherit automatically. The captured ref is substituted whenever the attachers call active(ref) / clear(ref) without an explicit ref; per-command --user wins because the explicit ref short-circuits the substitution. Pure adapter — does not reach into global state.
  • src/commands/auth/index.ts — resolve getRequestedUserRef() once at registration and hand it to withUserRefAware; wire the wrapper to logout / status / token view (login does not accept --user).
  • Skill docs (src/lib/skills/content.ts + regenerated skills/twist-cli/SKILL.md) — document the global form narrowed to the three auth subcommands that actually honour it.

Single-user note: until the keyring TokenStore + migrateLegacyAuth land (#2+#5 in the migration plan), the only valid --user <ref> is the lone stored account — any other ref surfaces ACCOUNT_NOT_FOUND. The plumbing being in place now means those follow-up PRs ship without extra wiring.

Test plan

  • npm run type-check
  • npm run lint:check
  • npm test — 3 new wiring tests at the user-visible boundary (global ref flows into auth token view, global ref flows into auth status, wrong global ref on auth logout surfaces ACCOUNT_NOT_FOUND despite cli-core's swallow-active-error contract); 614 total pass
  • npm run build
  • tw --user 1 doctor --offline — global flag accepted at root, doctor runs
  • tw --user 1 auth --help — strip works; commander does not reject --user
  • Manual end-to-end after tw auth login:
    • tw --user 1 auth status → success
    • tw auth status --user 1 → success (regression)
    • tw --user 999 auth statusACCOUNT_NOT_FOUND
    • tw --user 999 auth status --user 1 → success (per-command wins)
    • tw --user 1 auth token view → prints stored token

🤖 Generated with Claude Code

scottlovegrove and others added 2 commits May 16, 2026 21:50
cli-core's auth attachers (`attachLogoutCommand`, `attachStatusCommand`,
`attachTokenViewCommand`) already declare a per-command `--user <ref>`
flag, but commander has no root-level `--user` option, so the global
form `tw --user 42 auth status` was rejected at parse time. This PR
completes the plumbing started by the cli-core 0.16 adoption.

- `src/index.ts`: snapshot the cached global-args parser before mutating
  argv (so the original `--user` value is captured), then strip the
  pre-subcommand `--user` from `process.argv` via cli-core's
  `stripUserFlag` so commander sees a clean argv. Per-command `--user`
  (after the subcommand name) survives the strip and reaches commander
  unchanged.
- `src/lib/global-args.ts`: expose `getRequestedUserRef()` reading
  `store.get().user` from the existing lazy parser cache. The `user`
  field was already part of `TwGlobalArgs`; this just adds an accessor.
- `src/commands/auth/store-wrap.ts`: new `withUserRefAware(store)`
  adapter that substitutes `getRequestedUserRef()` whenever `active(ref)`
  or `clear(ref)` is called without an explicit ref. Per-command `--user`
  wins because the explicit ref short-circuits the substitution.
- `src/commands/auth/index.ts`: wire the wrapper to logout / status /
  token view registrations (login does not accept `--user`).
- Skill content + regenerated `skills/twist-cli/SKILL.md`: document the
  global form alongside the existing per-command examples.

Single-user note: until the keyring `TokenStore` + `migrateLegacyAuth`
land (#2+#5 in the migration plan), the only valid `--user <ref>` is
the lone stored account — any other ref surfaces `ACCOUNT_NOT_FOUND`.
The plumbing being in place now means those follow-up PRs ship without
extra wiring.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the multi-paragraph WHY blocks from the previous commit; keep only
the short hints needed to read the code (cli-core / strip / global vs
per-command precedence). No behaviour change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@doistbot doistbot requested a review from henningmu May 16, 2026 20:53
scottlovegrove and others added 2 commits May 16, 2026 21:55
The wrapper is 15 lines of `ref ?? getRequestedUserRef()` delegation;
the dedicated unit file unit-tested the JS `??` operator and was deleted
wholesale. The wiring tests in auth.test.ts kept the three that prove
behavior at the user-visible boundary: global ref flows through (token
view), per-command wins over global (precedence), wrong ref surfaces
ACCOUNT_NOT_FOUND despite cli-core's swallowed-active-error contract
(logout — the only non-trivial path).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
It tests JS's `??` operator inside a 3-line wrapper — the type system
plus reading `effectiveRef` makes precedence obvious by inspection.
Keeping only the two twist-specific bridge tests:

- happy path proves the argv-strip → cache → wrapper → store chain
- logout ACCOUNT_NOT_FOUND pins twist's reliance on cli-core's
  swallow-active-error behaviour so an upstream change can't silently
  regress it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@doistbot doistbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR successfully introduces global --user flag support for auth subcommands by stripping it before Commander evaluation and mapping the resolved reference through a new token store adapter. This is a great step toward fuller cli-core integration that cleanly sets up the infrastructure for upcoming multi-account features. There are a few areas that could use refinement, including tightening flag validation so unsupported commands do not silently accept the flag, decoupling the store adapter from global state, optimizing double-lookups during logout, expanding test coverage for the root parser and status command, and ensuring the skill documentation precisely matches the implemented scope.

Share FeedbackReview Logs

Comment thread src/index.ts Outdated
Comment thread src/commands/auth/auth.test.ts Outdated
Comment thread src/commands/auth/store-wrap.ts Outdated
Comment thread src/index.ts Outdated
Comment thread src/commands/auth/store-wrap.ts Outdated
Comment thread src/commands/auth/store-wrap.ts Outdated
Comment thread src/commands/auth/auth.test.ts
Comment thread src/commands/auth/auth.test.ts
Comment thread src/lib/skills/content.ts Outdated
- store-wrap.ts: rewrite as Object.create(store) + override active/clear
  only, so future TwistTokenStore methods inherit automatically; accept
  the requested ref as a constructor argument instead of reaching into
  the global-args cache (pure adapter, no hidden coupling).
- auth/index.ts: resolve getRequestedUserRef() once and hand it to
  withUserRefAware at registration time.
- src/index.ts: drop the redundant manual `--user` scan; call
  getRequestedUserRef() unconditionally to warm the cache.
- skills/content.ts: narrow the global --user entry to the three auth
  commands that actually honour the flag.
- auth.test.ts: lift STORED_METADATA to the outer describe; add a
  `tw --user <ref> auth status` wiring case to cover the third refAware
  consumer.

Declined three nits: scoping the strip per-subcommand (chicken-and-egg
with commander parse order; consistent with todoist), extracting the
src/index.ts bootstrap into a helper just to test it, and caching the
preflight to skip the second probeApiToken on logout (premature
optimisation).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove self-assigned this May 16, 2026
@scottlovegrove scottlovegrove added the 👀 Show PR PR must be reviewed before or after merging label May 16, 2026
@scottlovegrove scottlovegrove merged commit 423f429 into main May 16, 2026
6 checks passed
@scottlovegrove scottlovegrove deleted the feat/auth-global-user-flag branch May 16, 2026 21:07
doist-release-bot Bot added a commit that referenced this pull request May 16, 2026
## [2.39.0](v2.38.0...v2.39.0) (2026-05-16)

### Features

* **auth:** support pre-subcommand `tw --user <ref> <subcommand>` ([#228](#228)) ([423f429](423f429))
@doist-release-bot
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 2.39.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Copy Markdown
Contributor

@henningmu henningmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice ✅

(this might make my prior comment about cli-core moot 😅)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released 👀 Show PR PR must be reviewed before or after merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants