Skip to content

auth attachers: accept global --user ref via a getRequestedUserRef callback #30

@scottlovegrove

Description

@scottlovegrove

Problem

cli-core's auth attachers (attachLoginCommand, attachLogoutCommand, attachStatusCommand, attachTokenViewCommand) all declare a per-command --user <ref> flag via attachUserFlag. Two consumers (todoist-cli and twist-cli) also want the global formtd --user 42 task list, tw --user 42 auth status — accepted before any subcommand. cli-core gives us the parsing primitives (parseGlobalArgs, stripUserFlag), but the integration with the auth attachers is reinvented in every consumer.

In todoist-cli (PR #341) and twist-cli (PR #228, expanded by PR #231) the same ~50-LOC dance is hand-rolled:

  1. parseGlobalArgs(process.argv) to extract --user
  2. stripUserFlag(process.argv.slice(2)) so commander doesn't reject --user at the root
  3. Cache the parsed value (getRequestedUserRef() accessor in a local global-args.ts)
  4. Wrap the TokenStore with a withUserRefAware(store, requestedRef) adapter that substitutes the cached ref when commander didn't see one
  5. Add an existence pre-check in the wrapper so tw --user <wrong> auth logout surfaces ACCOUNT_NOT_FOUND instead of cli-core's silent clear() no-op

The root cause: cli-core's attachLogoutCommand deliberately preflights store.active(ref) to convert a ref miss into ACCOUNT_NOT_FOUND — but only when ref !== undefined (where ref = extractUserRef(cmd)). When the global flag has been stripped from argv before commander runs, cmd.user === undefined, so the preflight is skipped and the consumer has to compensate. The preflight gating is correct given the information available to cli-core; cli-core just has no way of knowing about a ref that was never on the option list.

This is now the rule-of-three smell triggering on count two. A third consumer would rediscover the same pattern from scratch.

Proposed change

Add an optional getRequestedUserRef callback to each auth attacher's options:

type AttachLogoutCommandOptions<TAccount> = {
    store: TokenStore<TAccount>
    /** ... existing fields ... */
    /**
     * Optional source of the global `--user <ref>` value when the consumer
     * strips it from argv before commander runs. cli-core's preflight uses
     * this as the fallback for `ref` so a non-matching global ref surfaces
     * as `ACCOUNT_NOT_FOUND` instead of a silent `clear()` no-op.
     */
    getRequestedUserRef?: () => AccountRef | undefined
}

Same option on AttachStatusCommandOptions, AttachTokenViewCommandOptions, and (optionally) AttachLoginCommandOptions for consistency.

Internally, every place that currently reads extractUserRef(cmd) becomes:

const ref = extractUserRef(cmd) ?? options.getRequestedUserRef?.()

requireSnapshotForRef then sees a real ref on the global-flag path, the preflight catches the miss, and the consumer no longer needs to wrap the store at all.

Alternative (rejected): cli-core owns argv massaging

A more opinionated attachGlobalUserFlag(program) that strips argv + caches at startup. Cleaner for consumers (one call instead of three) but cli-core would mutate process.argv — surprising for a library and harder to test in isolation. The callback shape above keeps cli-core pure.

Downstream simplification

Once this lands, both consumer CLIs can drop ~30 LOC and one whole file each.

twist-cli (Doist/twist-cli)

  • Delete src/commands/auth/store-wrap.ts entirely (the withUserRefAware wrapper).
  • Update src/commands/auth/index.ts to stop wrapping the store; pass getRequestedUserRef straight into each attacher's options:
    const store = createTwistTokenStore()
    const userRef = getRequestedUserRef()
    attachTwistLoginCommand(auth, store)
    attachTwistLogoutCommand(auth, store, { getRequestedUserRef: () => userRef })
    attachTwistStatusCommand(auth, store, { getRequestedUserRef: () => userRef })
    attachTokenViewCommand(tokenCmd, { name: 'view', store, envVarName: TOKEN_ENV_VAR, getRequestedUserRef: () => userRef })
  • Delete the existence-check tests in src/commands/auth/auth.test.ts (blocks tw --user <wrong> auth logout with ACCOUNT_NOT_FOUND ...) — cli-core would own that assertion via its own attacher tests.
  • src/index.ts argv strip + getRequestedUserRef() accessor in src/lib/global-args.ts stay (still needed to extract the value from argv pre-commander).

todoist-cli (Doist/todoist-cli)

  • Delete src/commands/auth/store-wrap.ts entirely.
  • Update src/commands/auth/index.ts: stop building refAware, pass getRequestedUserRef callback into each attacher instead.
  • Delete the withUserRefAware tests under src/lib/auth-store.test.ts and the --user from global args cases in src/commands/auth/auth.test.ts.
  • todoist's argv strip in src/index.ts stays.

Acceptance criteria

  • New getRequestedUserRef?: () => AccountRef | undefined option on AttachLogoutCommandOptions, AttachStatusCommandOptions, AttachTokenViewCommandOptions, AttachLoginCommandOptions.
  • Each attacher resolves ref = extractUserRef(cmd) ?? options.getRequestedUserRef?.() before calling requireSnapshotForRef / the store methods.
  • requireSnapshotForRef already throws ACCOUNT_NOT_FOUND for explicit non-null refs that resolve to null — no change needed there.
  • Cli-core tests cover the new flag for at least logout (the preflight-driven ACCOUNT_NOT_FOUND path) and one read path (status or token view).
  • No breaking changes to existing consumers — the option is purely additive.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions