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
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions