fix(token-persistence): bundle user + real tokens at the caller#207
Open
bird-m wants to merge 4 commits intofeat/direct-signup-v2from
Open
fix(token-persistence): bundle user + real tokens at the caller#207bird-m wants to merge 4 commits intofeat/direct-signup-v2from
bird-m wants to merge 4 commits intofeat/direct-signup-v2from
Conversation
Replaces the split "internal write tokens, caller later upgrades the
User record" pattern with a single bundled write at each caller site.
The auth functions now return tokens including the real `expiresAt`
from the OAuth token response, and persistence is the caller's sole
responsibility — once the caller has a real user, it calls
`storeToken(user, tokens)` once.
Why: the previous shape had the real bug (three caller sites hardcoded
a 1-hour expiresAt because they didn't know the real value), and the
follow-up that introduced `updateStoredUser` traded that for a hidden
precondition: callers had to know that some earlier step had already
written the OAuth fields to disk, or the upgrade silently no-op'd.
Bundling eliminates both smells: the caller has everything needed to
write a complete record at one site, and partial `{id:'pending'}`
entries never land on disk in the first place.
Changes:
- `AmplitudeAuthResult` gets `expiresAt: string`; `performAmplitudeAuth`
populates it on both the cached-session and fresh-OAuth paths.
- `performAmplitudeAuth` no longer persists internally.
- `performSignupOrAuth` no longer persists internally; returns tokens
(with expiresAt) and `userInfo | null`. Caller decides how to handle
userInfo fetch failure.
- Three call sites (`bin.ts:1264`, `bin.ts:1592`, `setup-utils.ts:528`)
swap the fabricated 1h `expiresAt` for `auth.expiresAt` and the TUI
site hoists the `storeToken` call out of the else-branch so the
signup-success path also persists.
- Tests updated to reflect the new contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
Contributor
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Results will be posted here when complete. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Tokens not persisted in agent/CI/classic signup paths
- Added a
storeTokencall inrunDirectSignupIfRequestedafter a successfulperformSignupOrAuth(using the returneduserInfoandzone), with a soft-fail branch whenuserInfois null so agent/CI/classic modes now correctly persist direct-signup tokens to~/.ampli.json.
- Added a
Or push these changes by commenting:
@cursor push 1758046b05
Preview (1758046b05)
diff --git a/bin.ts b/bin.ts
--- a/bin.ts
+++ b/bin.ts
@@ -499,7 +499,32 @@
getUI().log.info(
`Direct signup did not produce credentials; continuing to ${fallbackLabel}.`,
);
+ } else if (tokens.userInfo === null) {
+ // Without userInfo we can't persist a complete record (id/email/zone),
+ // so treat this as a soft failure and let the caller's fallback path
+ // handle credential resolution.
+ getUI().log.info(
+ `Direct signup succeeded but user profile fetch failed; continuing to ${fallbackLabel}.`,
+ );
} else {
+ // Persist tokens to ~/.ampli.json so downstream credential resolution
+ // (resolveCredentials / resolveNonInteractiveCredentials) finds them.
+ const { storeToken } = await import('./src/utils/ampli-settings.js');
+ storeToken(
+ {
+ id: tokens.userInfo.id,
+ firstName: tokens.userInfo.firstName,
+ lastName: tokens.userInfo.lastName,
+ email: tokens.userInfo.email,
+ zone: tokens.zone,
+ },
+ {
+ accessToken: tokens.accessToken,
+ idToken: tokens.idToken,
+ refreshToken: tokens.refreshToken,
+ expiresAt: tokens.expiresAt,
+ },
+ );
getUI().log.info('Direct signup succeeded; using newly created account.');
if (onSuccess) {
await onSuccess();You can send follow-ups to the cloud agent here.
…etch failure Keeps the bundled-at-caller shape in the happy path (no internal write, caller persists once it has real userInfo) but re-introduces an internal storeToken in the user-fetch-failure branch only. Why: the whole point of --signup is to avoid the browser redirect. If fetchAmplitudeUser fails (provisioning race, network hiccup, etc.) and tokens are only in memory, we lose recovery: - TUI: user is pushed back to browser OAuth on next run, defeating the feature's UX goal. - CI / --agent: no browser fallback available, and re-running --signup fails because the account already exists. The run dead-ends. Writing under a pending sentinel on failure costs a small asymmetry (one side writes internally, one doesn't) but the asymmetry is clearly scoped to the error-recovery path, not a hidden precondition at the happy-path caller site. Next run's `performAmplitudeAuth` cached-session lookup finds the pending entry and the caller completes the upgrade to a real user. Also includes a whitespace-normalization test for the pending-sentinel fullName parse, preserved from the earlier behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Tokens not persisted in non-TUI signup paths
- Added a storeToken call in runDirectSignupIfRequested's success branch so agent/CI/classic signup paths persist tokens for downstream credential resolution.
Or push these changes by commenting:
@cursor push acfbfc6320
Preview (acfbfc6320)
diff --git a/bin.ts b/bin.ts
--- a/bin.ts
+++ b/bin.ts
@@ -500,6 +500,30 @@
`Direct signup did not produce credentials; continuing to ${fallbackLabel}.`,
);
} else {
+ // Persist tokens so downstream credential resolution finds them.
+ // performSignupOrAuth only writes on the user-fetch failure path
+ // (pending sentinel); success-path persistence is the caller's
+ // responsibility. Without this, agent/CI/classic modes read
+ // ~/.ampli.json, find nothing, and either fail with AUTH_REQUIRED
+ // or fall back to browser OAuth — silently discarding the signup.
+ if (tokens.userInfo) {
+ const { storeToken } = await import('./src/utils/ampli-settings.js');
+ storeToken(
+ {
+ id: tokens.userInfo.id,
+ firstName: tokens.userInfo.firstName,
+ lastName: tokens.userInfo.lastName,
+ email: tokens.userInfo.email,
+ zone: tokens.zone,
+ },
+ {
+ accessToken: tokens.accessToken,
+ idToken: tokens.idToken,
+ refreshToken: tokens.refreshToken,
+ expiresAt: tokens.expiresAt,
+ },
+ );
+ }
getUI().log.info('Direct signup succeeded; using newly created account.');
if (onSuccess) {
await onSuccess();You can send follow-ups to the cloud agent here.
…ding
Three small tightening changes:
1. `OAuthEntrySchema` now validates `OAuthExpiresAt` as ISO-8601 via
`z.string().datetime().catch(...)`, falling back to a 1-hour-from-now
default on malformed input. This matches the pre-PR fabricated default
callers used to supply, so downstream behavior is no worse than baseline
when on-disk data is corrupt, and token-refresh math never sees NaN.
2. `storeToken` now deletes any sibling `{id:'pending'}` entry for the
same zone when writing a non-pending user. The pending sentinel is
only ever written as a transient crash-recovery marker (see
`performSignupOrAuth`'s failure branch); once the caller has a real
user, leaving the sentinel on disk is orphan state that risks
returning stale tokens from a no-userId `getStoredToken()` lookup.
3. Log message at `[oauth] token exchange complete` no longer includes
code-contract commentary; the contract lives in the function's JSDoc.
Tests added for the schema fallback and the sibling-pending cleanup,
including zone isolation (a US-zone write doesn't disturb an EU pending).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
bugbot run |
Contributor
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 655c9d8. Configure here.
…sic) The earlier bundled-at-caller refactor updated the three TUI-adjacent call sites (bin.ts:1264, bin.ts:1592, setup-utils.ts:528) to call storeToken explicitly, but missed a fourth site: runDirectSignupIfRequested, which is the entry point used by agent, CI, and classic modes (invoked from three places in bin.ts around lines 851/876/902). Before the refactor, performSignupOrAuth wrote tokens internally and runDirectSignupIfRequested relied on that side effect. After, the function returned tokens but the caller discarded them — so downstream resolveNonInteractiveCredentials / resolveCredentials read an empty ~/.ampli.json and signup silently had no effect in non-TUI modes. Agent/CI would fail with AUTH_REQUIRED; classic would fall back to browser OAuth, defeating the --signup UX goal. Adds the missing storeToken call in the success branch. Uses tokens.userInfo (populated on success path); when userInfo is null the failure branch of performSignupOrAuth has already written a pending-sentinel crash-recovery entry, so downstream reads still find something. Credit: found by Cursor BugBot on PR #207. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
4 tasks
kaiapeacock-eng
approved these changes
Apr 27, 2026
Collaborator
kaiapeacock-eng
left a comment
kaiapeacock-eng
left a comment
There was a problem hiding this comment.
LGTM — autofix applied for prior bugbot finding; bugbot reports no new issues.
5 tasks
Collaborator
|
Conflicts deepen with the recent auth/signup merges. Could you rebase + force-push? I'll admin-merge once green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #165. Supersedes #185 — different architectural approach.
Summary
Three credential-persistence call sites hardcoded a 1-hour
expiresAtwhen callingstoreToken, overwriting the real OAuth expiry. This PR addsexpiresAttoAmplitudeAuthResultand bundles user + real tokens into a singlestoreTokencall at each caller site.Why this shape (vs. #185)
PR #185 fixed the same bug by introducing
updateStoredUser(user)— a function that upgraded a pending-sentinel entry to a real-user entry while preserving the OAuth fields written earlier. That works, but it trades one smell for another: callers see a(user) => voidsignature that silently no-ops if some earlier step didn't run. The coupling is invisible at the call site.This PR takes the alternative: give callers the
expiresAtso they can construct a complete record themselves. Eliminates the hidden precondition, removes the new API surface, and keeps the happy-path write bundled at the caller.Scope
Auth return shape:
AmplitudeAuthResultgetsexpiresAt: string. Populated on both the cached-session path (from the stored token) and the fresh-OAuth path (from the token response).performSignupOrAuthsurfacesexpiresAton its return shape.Persistence responsibility:
performAmplitudeAuthno longer writes to disk. Callers are browser-enabled contexts; if tokens are lost between OAuth and userInfo fetch, next-run browser OAuth recovers cheaply.performSignupOrAuthno longer writes on the success path — caller persists once it has userInfo. On user-fetch failure, it writes tokens under a{id:'pending'}sentinel so next-runperformAmplitudeAuthcached-session lookup can recover. Without this,--signupusers who hit a userInfo race would be pushed back to a browser redirect (defeating the whole point of the flag) or dead-end in CI mode (re-signup fails because the account already exists).Call sites bundled:
bin.ts:1264— TUI flow. HoistsstoreTokenout of the else-branch so signup-success and plain-OAuth paths both persist once.bin.ts:1592—/loginslash command.src/utils/setup-utils.ts:528— classic-mode auth.Not in scope:
token-refresh.tslogic,storeTokensignature — unchanged.Asymmetry, called out honestly
One auth function (
performSignupOrAuth) retains an internal write on its failure branch; the other (performAmplitudeAuth) has no internal writes. This is intentional —--signupneeds crash-recovery-on-disk because browser fallback defeats the feature; OAuth-path recovery is cheap and doesn't need the same safeguard. The asymmetry is scoped to the error path, not a hidden precondition at the happy-path caller site.Severity
Low in practice for the
expiresAthardcode itself — token refresh absorbs the inaccuracy. Worth fixing for correctness hygiene and to prevent futureexpiresAtconsumers (proactive refresh math, telemetry on token age) from inheriting the wrong value.Verification
grep -rn "3600 \* 1000" bin.ts src/returns no non-test matches~/.ampli.jsonOAuthExpiresAtreflects real value/loginslash command, same verification--yes/ non-TUI), same verificationGating
Not a merge blocker for #165. Worth landing before the
wizard-direct-signupflag ramps off 0%.Note
Medium Risk
Touches authentication/session persistence across TUI, classic, and
loginflows; mistakes could break cached-session reuse or leave stalependingentries, though changes are localized and covered by tests.Overview
Ensures stored OAuth sessions keep the real token expiry by threading
expiresAtthroughAmplitudeAuthResultand updating all persistence call sites to writeexpiresAt: auth.expiresAtinstead of fabricating a 1-hour value.Shifts persistence responsibility:
performAmplitudeAuthnow returns tokens without writing~/.ampli.json, while callers (bin.tsTUI + direct-signup wrapper,/login, and classic setup) persist a complete{user + tokens}record in a singlestoreTokencall oncefetchAmplitudeUsersucceeds.Hardens storage behavior in
ampli-settings:getStoredTokennow tolerates malformedOAuthExpiresAtby falling back to a 1h-from-now ISO timestamp, andstoreTokendeletes same-zonependingsentinel entries when writing a real user; tests updated/added to cover these cases and the revised signup persistence semantics.Reviewed by Cursor Bugbot for commit 0343ccc. Bugbot is set up for automated code reviews on this repo. Configure here.