Doist / Doist/cli-core

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

Open
#30 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
0
Forks
0
Avg merge
9h 16m
Merged PRs (30d)
15

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the implementations of attachLoginCommand, attachLogoutCommand, attachStatusCommand, and attachTokenViewCommand, tracing extractUserRef and requireSnapshotForRef. Review the existing cli-core attacher tests, then cover the global-ref logout preflight and one read path; done means all four option types accept the callback without breaking existing consumers.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
cli
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.