cli: stored-auth injection should honor refresh_token, not just access_token (#1665)

[cliffhall]

Closes #1665

Summary

--use-stored-auth previously injected the stored access token blindly, so an expired token failed even with a valid refresh_token on disk. Now, when a refresh_token is stored, the CLI runs the OAuth refresh grant to mint a fresh access token before connecting, and persists the rotation.

• New exported refreshStoredAuthToken(serverUrl, statePath, deps?):
  • Reuses the SDK's refreshAuthorization (not a hand-rolled token request) with the stored clientInformation + serverMetadata — discovered from --server-url via core getAuthorizationServerUrl + SDK discoverAuthorizationServerMetadata when the metadata wasn't persisted.
  • Writes rotated tokens back through serializeOAuthPersistBlob under the same {servers,idpSessions} key, so web and CLI stay consistent.
  • Missing refresh_token/clientInformation or a failed grant throw CliExitCodeError(AUTH_REQUIRED) with a clear message + envelope code (no_stored_token / refresh_failed).
• --use-stored-auth prefers refresh when a refresh_token is present (covers both absent and expired access tokens — the persisted blob carries no expiry, so the refresh token is the durable credential); falls back to injecting the stored access token otherwise (unchanged behavior).

Tests

• deps are injectable, so refresh/discovery branches are unit-tested without a live endpoint: happy path + persist, discovery-when-metadata-absent, no-refresh-token, no-client-information, and refresh-grant-failure.
• An e2e test drives the real SDK refresh path against a mock token endpoint (node:http) and asserts the refreshed token is injected on the MCP request + the rotation is persisted.

CLI coverage gate green (npm run test:coverage, EXIT 0; cli.ts 100/92/100/100).

Related: #665, #1160. Part of the #1579 decomposition (Wave 4, CLI lane). Per AGENTS.md the Closes line won't auto-fire on v2/main; the issue is closed and its board card moved on the Wave 4 rollup merge.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 19s —— View job

---

Review — stored-auth refresh_token support (#1673)

Reviewed against origin/v2/main. The design is solid: reusing the SDK's refreshAuthorization/discoverAuthorizationServerMetadata instead of hand-rolling a token request, injectable deps for testing, and writing back under the shared {servers,idpSessions} key are all the right calls. The deps-injected unit tests plus the real-endpoint e2e test give good coverage. A few things worth considering before merge:

1. Refresh failure hard-fails even when a valid access token is stored (behavior regression)

clients/cli/src/cli.ts:1078 — the new branch is:

if (stored?.state.tokens?.refresh_token) {
  token = await refreshStoredAuthToken(...);   // throws AUTH_REQUIRED on any failure
} else {
  /* inject stored access_token */
}

When a refresh_token is present, a failed grant (refresh_failed) now hard-exits 3, even if a still-valid access_token is also stored. Before this PR that same state would have injected the access token and very likely connected fine. So a transient auth-server hiccup (or a rotated-but-still-good access token) turns a previously-working invocation into a failure. Consider falling back to injecting the stored access_token when refresh fails and one exists, rather than throwing unconditionally. Fix this →

2. Raw writeFile bypasses writeStoreFile — loses 0o600 mode on the secrets file

clients/cli/src/cli.ts:628 writes the state file with a bare writeFile(statePath, ..., "utf8"). The canonical path (core/storage/store-io.ts writeStoreFile, used by createFileOAuthPersistBackend) writes with mode: 0o600 and mkdir -p. Since this file holds OAuth tokens, it should stay owner-only. On an existing file writeFile preserves prior perms, so it's usually fine — but if the CLI is ever the first writer (or the file was recreated), it'll land with the default umask. Prefer reusing writeStoreFile so CLI and web persist identically and the mode is guaranteed.

3. Always-refresh trades a network round-trip + refresh-token rotation on every call

Because there's no stored expiry, the code refreshes whenever a refresh_token is present — a defensible choice, and well-explained in the docstring. Two consequences worth noting:

• Every --use-stored-auth run now rotates the refresh token. A crash between a successful grant and the writeFile persist would invalidate the old refresh token server-side without saving the new one → the user is locked out until re-auth. The window is small, but see point 2 (atomic write helps here too).
• Two concurrent CLI invocations against the same state file would race a single-use refresh token; one loses. Probably out of scope, but worth a mention in the docs.

4. --wait-for-auth path does not refresh

waitForStoredToken (cli.ts:641) returns the raw access_token and never enters the refresh branch, yet the README says --wait-for-auth proceeds "as if --use-stored-auth were set." In practice a token that just landed from a completed browser flow is fresh, so impact is low — but the two paths are no longer equivalent. Either wire the refresh check in after the wait resolves, or tweak the doc wording.

5. Minor

• Envelope code: the missing-clientInformation case (cli.ts:594) throws with { code: "no_stored_token" }, but a token is stored — it's the client info that's missing. A distinct code (e.g. no_client_information) would be clearer for programmatic callers.
• metadata: undefined path: found.state.serverMetadata ?? (await discover(...)) ?? undefined (cli.ts:605) can pass undefined to refreshAuthorization, relying on the SDK to default the token endpoint. That's OK, but the "discover returned undefined" sub-branch isn't unit-tested — worth one case.

None of these are blockers; #1 is the one I'd most want addressed since it's a user-visible regression from prior behavior.
· v2/1665-refresh-token

[cliffhall]

Thanks — addressed the substantive findings in 59f3bd92:

• Progress notifications #1 (regression: refresh failure hard-fails even with a valid access token): now falls back to injecting a still-present stored access_token when the refresh grant fails; only a failure with nothing to fall back on exits 3. Added an e2e test (token endpoint returns 400 → CLI injects the stored token).
• Request logging #2 (raw writeFile loses 0600): rotated tokens now persist via the shared writeStoreFile (0o600 + mkdir -p), matching the web backend.
• #5a (envelope code): missing client info now uses a distinct no_client_information code.
• #5b: added a unit test for the discovery-returns-undefined metadata path.
• Completion requests #3/SSE transport support #4 (always-refresh rotation caveat; --wait-for-auth no longer claims parity): documented in the CLI README.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 6s —— View job

---

Re-review — follow-up commit 59f3bd92

Re-reviewed against origin/v2/main. All five findings from the prior pass are addressed, and the fixes are the right shape. Details below.

• Read the follow-up commit diff
• Verify Progress notifications #1 (refresh-failure fallback)
• Verify Request logging #2 (0o600 write)
• Verify #5a (distinct code)
• Verify #5b (discovery-undefined test)
• Verify Completion requests #3/SSE transport support #4 docs

✅ #1 — Refresh-failure fallback (the regression)

cli.ts:1082-1098. When a refresh_token is present, a failed grant now falls back to the still-present stored access_token (if (!storedAccess) throw err) rather than hard-exiting 3. This restores the pre-PR behavior for the transient-hiccup / rotated-but-still-good case, and only surfaces exit 3 when there's genuinely nothing to inject. The 400 → invalid_grant → inject "still-usable-access" e2e test locks it in. Resolved.

✅ #2 — Owner-only secrets write

cli.ts:632 now routes through writeStoreFile (core/storage/store-io.ts), which guarantees mode: 0o600 + mkdir -p — CLI and web now persist identically. Bonus: writeStoreFile uses atomically (temp-file + rename) and serializes concurrent writes to the same path via pendingWrites, which also tightens the crash-between-grant-and-persist window I raised in #3 — the write is now all-or-nothing. Resolved.

✅ #5a — Distinct envelope code

cli.ts:599 — missing-client-info now throws { code: "no_client_information" }, distinct from no_stored_token, with a matching unit test. Resolved.

✅ #5b — Discovery-returns-undefined path

New unit test asserts discover → undefined results in metadata: undefined reaching refreshAuthorization. Resolved.

✅ #3 / #4 — Documented rather than coded

The always-refresh rotation + concurrency caveat and the --wait-for-auth non-parity (it injects the freshly-landed token without refreshing, since a just-completed flow isn't expired) are now spelled out in the CLI README. Reasonable calls for both — no runtime change needed.

---

One small observation (non-blocking)

The catch at cli.ts:1089 is intentionally broad — it swallows any error from refreshStoredAuthToken (not just grant/auth failures) whenever a stored access token exists. That's the correct trade-off for the auth-related failures it's designed for, but it would also mask an unexpected error (e.g. a malformed state file that readOAuthSnapshot chokes on) behind a silent fallback to a possibly-stale token. Since the entry condition already guarantees the file parsed once (readOAuthServers succeeded just above at cli.ts:1080), this is close to a non-issue in practice — worth at most a one-line note that the fallback is deliberately catch-all. Not worth blocking on.

Nice work — this is ready to merge from my side. ✅
· v2/1665-refresh-token

[cliffhall]

Superseded by the Wave 4 rollup #1676, which contains this change (reviewed to LGTM here) merged base→tip. Closing in favor of the rollup.