Skip to content

feat(auth): refresh an expiring OAuth token before token view / status - #63

Merged
scottlovegrove merged 2 commits into
mainfrom
feat/refresh-before-token-view
Sep 16, 2026
Merged

scottlovegrove merged 2 commits into
mainfrom
feat/refresh-before-token-view

Conversation

@scottlovegrove

Copy link
Copy Markdown
Collaborator

Problem

tdc auth token view read the access token straight from the store and never triggered a refresh. Once an OAuth access token expired it kept printing the same dead token until some other API command happened to rotate it, which defeats the point of export COMMS_API_TOKEN="$(tdc auth token view)" for scripts and MCP servers. tdc auth status worked around the same gap by re-reading the store through getApiTokenSnapshot inside fetchLive.

Change

Bumps @doist/cli-core to 1.6.0 (Doist/cli-core#135) and wires its new refresh option on both attachers via a shared getTokenRefreshOptions() in src/lib/auth.ts. The DCR handshake is resolved per account through the function form, since cli-core doesn't persist the client metadata the refresh grant needs; getCommsOAuthRefreshHandshake still raises NO_TOKEN for partial metadata, so a stored OAuth token without a client id fails the same way it does on the API path rather than letting cli-core POST a grant with no client id.

  • token view: rotates an expiring OAuth token before printing. Manual-token accounts and COMMS_API_TOKEN have no refresh token and are served from the single bundle read, so their behaviour is unchanged. The env-var guard still runs before any refresh.
  • status: fetchLive now takes the token cli-core hands it instead of re-reading through getApiTokenSnapshot. One fewer store read, and the same fallback rules as token view.
  • getActiveSnapshot (the API path) now threads ref into refreshAccessToken, so tdc --user <ref> … rotates the selected account rather than the default one.

Tests

  • auth.test.ts: the global --user status test now drives cli-core's real refresh with a fake provider and asserts the ref reaches every bundle read, the rotated bundle is persisted, and fetchLive probes with the new token.
  • lib/auth.test.ts: getTokenRefreshOptions returns the expected lock path / skew and its handshake resolver delegates to getCommsOAuthRefreshHandshake, including the NO_TOKEN rejection for partial metadata.

Skill content and README updated for the token view behaviour; SKILL.md regenerated.

…atus`

`tdc auth token view` printed whatever access token was stored, so once an
OAuth token expired it kept handing scripts a dead token until some other
command happened to refresh it. Wire cli-core 1.6.0's `refresh` option on
both `token view` and `status` so an expiring token is rotated first.

`status` no longer re-reads the store through `getApiTokenSnapshot` inside
`fetchLive`; cli-core hands it the post-refresh token directly. The API
path's own refresh now also threads `ref` through, so a `--user` snapshot
rotates the selected account rather than the default one.
@scottlovegrove scottlovegrove self-assigned this Sep 16, 2026
@doistbot
doistbot requested a review from engfragui September 16, 2026 16:35

@doistbot doistbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR wires cli-core 1.6.0's new refresh option into token view and status via a shared getTokenRefreshOptions() helper, so expiring OAuth tokens get rotated before being printed or probed, and threads the global --user ref through the API-path refresh. The env-var guard, handshake reuse, and lock-path/skew constants all line up with the existing auth flow, and no security or reuse issues surfaced.

Few things worth tightening:

  • Preserve the legacy-token fallback: cli-core now resolves the handshake as soon as a bundle enters the 60-second refresh skew, so an OAuth record without oauthClientId throws even while its access token is still valid — auth status and auth token view can fail up to a minute earlier than API commands, which deliberately serve that token until it's actually expired. Defer the missing-metadata failure until refresh is truly needed, or have these attachers retain the valid token.
  • The headline token view refresh path is untested: every token view test runs with getTokenRefreshOptions mocked to return undefined, so the refresh option on attachTokenViewCommand is never exercised — removing it (or a cli-core regression) would keep the suite green while a dead token prints again. Add a test mirroring the new status test: expiring OAuth bundle + fake provider, assert stdout prints the rotated token.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (3)
  • P3 src/lib/auth.ts:74: getTokenRefreshOptions() now owns the refresh wiring, but getActiveSnapshot still builds the same provider / skewMs / lockPath / handshake values inline when it calls refreshAccessToken (lines 145-148). Reuse the builder there — e.g. refreshAccessToken({ store, ref, ...getTokenRefreshOptions() }) — so the API path and the two attachers can't drift.
  • P3 src/commands/auth/auth.test.ts:626: not.toHaveBeenCalledWith(undefined) is vacuous here. withUserRefAware.activeBundle substitutes the global ref (ref ?? requestedRef) before the underlying storeMocks.activeBundle is reached, so with --user 1 the mock only ever sees '1'. This assertion cannot detect a missing ref on cli-core's under-lock re-read despite the comment above it. Drop it, or assert call count/arguments at the wrapper level.
  • P3 src/commands/auth/auth.test.ts:633: The comment says the rotated bundle is persisted, but the assertion only checks the new access token. The fake provider also returns rt_refreshed_1234567890; if cli-core persisted the access token while dropping the rotated refresh token, this test would still pass and later refreshes would reuse the stale token. Assert the new refresh token (and expiry) as well, or narrow the comment.

Share FeedbackReview Logs

Comment thread src/lib/auth.ts
Comment thread src/commands/auth/auth.test.ts
…iring

Drive cli-core's real refresh through `tdc auth token view` with a fake
provider, assert the whole rotated pair is persisted on the status path,
and drop an assertion the `withUserRefAware` wrapper made vacuous. The
API path now spreads `getTokenRefreshOptions()` too so the three refresh
call sites share one provider / lock path / skew.
@scottlovegrove scottlovegrove added the 👀 Show PR PR must be reviewed before or after merging label Sep 16, 2026
@scottlovegrove
scottlovegrove merged commit c1926bd into main Sep 16, 2026
6 checks passed
@scottlovegrove
scottlovegrove deleted the feat/refresh-before-token-view branch September 16, 2026 17:11
doist-release-bot Bot added a commit that referenced this pull request Sep 16, 2026
## [3.3.0](v3.2.0...v3.3.0) (2026-09-16)

### Features

* **auth:** refresh an expiring OAuth token before `token view` / `status` ([#63](#63)) ([c1926bd](c1926bd))
@doist-release-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants