refactor(auth): AuthOutcome contract — auth functions own persistence#209
Draft
bird-m wants to merge 1 commit intofollowup/token-persistence-bundledfrom
Draft
refactor(auth): AuthOutcome contract — auth functions own persistence#209bird-m wants to merge 1 commit intofollowup/token-persistence-bundledfrom
bird-m wants to merge 1 commit intofollowup/token-persistence-bundledfrom
Conversation
…ontract
Collapses the "caller remembers to call storeToken after auth" sequencing
brittleness into a typed contract where persistence is guaranteed by the
function's return type. Forgetting to persist is now structurally impossible.
## Design
New discriminated-union `AuthOutcome` in `oauth.ts`:
type AuthOutcome =
| { status: 'complete'; user: StoredUser; userInfo: AmplitudeUserInfo;
tokens: StoredOAuthToken; zone: AmplitudeZone }
| { status: 'pending-recovery'; tokens: StoredOAuthToken; zone: AmplitudeZone };
type SignupOutcome = AuthOutcome | { status: 'skipped' };
Both `performAmplitudeAuth` and `performSignupOrAuth`:
- Fetch userInfo internally (performAmplitudeAuth grows a `fetchAmplitudeUser`
call; performSignupOrAuth already had one via `fetchUserWithProvisioningRetry`).
- Persist the record to `~/.ampli.json` before returning — complete record
on success, pending sentinel on userInfo-fetch failure for crash recovery.
- Return an AuthOutcome that callers pattern-match on.
`performAmplitudeAuth` also absorbs the EU-on-US region detection that
setup-utils used to do inline, via `detectRegionFromToken`.
## Why
BugBot caught a bug on the previous refactor (#207) where
`runDirectSignupIfRequested` silently discarded tokens because I updated
three of four caller sites to call storeToken but missed the fourth.
That class of bug is only preventable by a contract that makes forgetting
impossible — which this is.
The previous refactor also traded a persistent low-grade wrongness
(hardcoded 1h expiresAt) for a dormant high-grade failure mode (forgetting
to persist → total loss). This contract closes that trade: persistence is
inside the function, so there's no way for a caller to forget.
## Caller simplifications
All four caller sites collapse substantially:
- `bin.ts` TUI authTask: ~60 lines → ~35 lines; no separate fetchAmplitudeUser,
no storeToken, no signupUserInfo threading.
- `bin.ts` /login: ~55 lines → ~30 lines.
- `setup-utils.ts` askForWizardLogin: the `if (userInfo)` wrapper goes away
entirely; graceful fallback becomes a simple pattern match on status.
- `bin.ts` runDirectSignupIfRequested: the just-fixed storeToken call goes
away — the function now handles its own persistence.
## Deleted
- `AmplitudeAuthResult` interface (replaced by AuthOutcome)
- `PerformSignupOrAuthResult` type
- `performOAuthFlow` legacy shim + `OAuthConfig` type (unused)
## Tests
- login-flow.test.ts: rewritten to mock `performAmplitudeAuth` returning
AuthOutcome shapes, dropping the per-piece mocks for fetchAmplitudeUser,
detectRegionFromToken, and storeToken (those are now internal to
performAmplitudeAuth and not observable from setup-utils tests).
- signup-or-auth.test.ts: status transitions updated (null → skipped,
tokens-on-top → complete outcome shape).
- cli.test.ts: default auth mocks return AuthOutcome; caller-side
storeToken assertions removed.
1128 tests pass, lint clean, TypeScript build clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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. |
This was referenced Apr 25, 2026
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
1 participant
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.
Stacked on #207.
Problem this solves
On #207, BugBot caught a high-severity bug where I updated three of four caller sites to call `storeToken` after auth but missed the fourth (`runDirectSignupIfRequested`), silently discarding tokens in agent/CI/classic modes. That class of bug — "forgot to do the right thing at one of N call sites" — is only structurally preventable if persistence is inside the function's contract.
This PR makes forgetting impossible.
Design
New discriminated-union `AuthOutcome` in `oauth.ts`:
```ts
type AuthOutcome =
| { status: 'complete'; user: StoredUser; userInfo: AmplitudeUserInfo;
tokens: StoredOAuthToken; zone: AmplitudeZone }
| { status: 'pending-recovery'; tokens: StoredOAuthToken; zone: AmplitudeZone };
type SignupOutcome = AuthOutcome | { status: 'skipped' };
```
Both `performAmplitudeAuth` and `performSignupOrAuth`:
`performAmplitudeAuth` also absorbs EU-on-US region detection (`detectRegionFromToken`), which setup-utils used to do inline.
Caller simplifications
All four sites collapse substantially (net -77 lines):
Deleted
What this closes out
What's preserved
Verification