fix(signup): wipe prior account entries on signup success#217
Merged
Conversation
Direct signup now uses a new `replaceStoredUser()` helper instead of
the additive `storeToken()`. Before writing the new user entry, it
removes every existing `User-*` / `User[*]-*` key in ~/.ampli.json,
preserving other config keys.
Why: `~/.ampli.json` can hold many user entries, but every reader
(`getStoredUser()`, called from every auth path) returns the first
real user it finds in insertion order. The direct-signup path writes
non-destructively via `storeToken`, which means signup-over-existing-
user leaves the new account stranded behind the old one — the next
wizard run silently logs in as the old user and ignores the freshly
issued signup tokens.
Narrow fix at the signup success path (not inside `storeToken`
itself): OAuth and token-refresh callers legitimately write under an
existing user's key and should not wipe a coexisting `ampli` CLI
session that was installed for a different account. Only signup
expresses the "this account replaces any prior one" intent.
Also clears lingering `{ id: 'pending' }` sentinels from prior failed
signup user-fetch fallbacks — those already accumulated in the file
and `getStoredUser` skips them, but they're cruft worth sweeping.
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. |
bird-m
commented
Apr 23, 2026
| ): void { | ||
| const config = readConfig(configPath); | ||
| for (const k of Object.keys(config)) { | ||
| if (k.startsWith('User-') || k.startsWith('User[')) { |
Collaborator
Author
There was a problem hiding this comment.
is this the best way to do this? This should be linked to how/where the values are set
kelsonpw
reviewed
Apr 23, 2026
kelsonpw
left a comment
kelsonpw
left a comment
Member
There was a problem hiding this comment.
Reviewed for prod readiness. Overall this looks solid to ship — the scope is appropriately narrow, the doc comments at both the definition and the call site make the intent clear, and the unit coverage is thorough (empty file, multi-user sweep, cross-zone wipe, pending sweep, EU key, configPath passthrough). Using the existing userKey() + atomicWriteJSON infra means there's no new crash-corruption surface to worry about.
A couple of observations that aren't blockers but are worth being aware of:
1. The sweep drops ampli-CLI non-credential state for the prior user (intentional, but worth noting).
~/.ampli.json entries under User-{id} can also contain ampli-CLI state like SelectedOrganization alongside the OAuth tokens. Because replaceStoredUser deletes the entire entry rather than just credential fields, a signup will also wipe that state for whatever account was previously logged in. This is consistent with the stated "this account replaces any prior one" intent, and the PR body addresses it explicitly, so I think it's fine. Just flagging it in case someone reads this later and wonders why their ampli CLI suddenly forgot which workspace was selected.
2. Consider a debug log when the sweep actually deletes something.
Right now the sweep is silent. If a user reports "I signed up but then it logged me in as someone else" — or the inverse, "my ampli CLI session disappeared after running the wizard" — a log.debug('replaceStoredUser: wiped N prior user entries', { keys: [...] }) line would make that diagnosable from the session log without needing to repro. Not required, but cheap insurance for a code path that by design destroys data. Keys are non-sensitive (User-123 / User[eu]-456); tokens don't need to be logged.
Everything else — cross-process race with a concurrent ampli CLI write, the User-/User[ prefix match, the lack of spread into the new entry — is either pre-existing from storeToken or safe by construction (all User-* keys got deleted before the write). No regressions introduced here.
LGTM for merge into feat/direct-signup-v2.
The User-key predicate (`k.startsWith('User-') || k.startsWith('User[')`)
appeared in three call sites after the recent signup-sweep work —
`getStoredUser`, `getStoredToken`, and the new `replaceStoredUser`. Rule
of three met: extract a module-private `isUserKey()` helper so the sites
can't drift in what they consider a "User entry."
Also trims four recently-added comment blocks that drifted past the repo
convention ("don't explain WHAT the code does; don't narrate the
change"):
- `EMAIL_REGEX` JSDoc (constants.ts): 11 lines → 3
- `replaceStoredUser` JSDoc (ampli-settings.ts): 7 lines → 1
- `region:` inline comment (wizard-session.ts): 6 lines → 2 (full
reasoning already exists above on `signupEmail`)
- call-site comment in signup-or-auth.ts: 10 lines → 3 (the
replaceStoredUser-vs-storeToken rationale is now carried by the
function name plus the one-line JSDoc)
No behavior change. Surfaced by `/simplify` after landing commit 91845eb.
Follow-up to the review on PR #217: - `replaceStoredUser` now emits a `debug` log listing the `User-*` keys it deletes (count + keys — keys are non-sensitive; tokens stay out). This makes "I signed up but the wizard logged me in as someone else" and the inverse "my ampli CLI session disappeared" diagnosable from the session log alone, without needing a live repro. The log is skipped when nothing was wiped to keep noise down on the common case. - A one-line comment at the new-entry assignment documents why the write doesn't spread an existing entry (`storeToken` does; this function deliberately doesn't, because the preceding loop has already deleted every `User-*` key). Pins the invariant so a future edit that reorders or removes the sweep doesn't silently drop fields. No behavior change for callers. Existing tests (ampli-settings 33, signup-or-auth 14) pass unchanged; the debug log isn't asserted on because it isn't part of the observable contract.
bird-m
added a commit
that referenced
this pull request
Apr 24, 2026
* feat: add direct signup via headless provisioning endpoint Adds support for signing up new Amplitude accounts directly from the wizard without a browser OAuth handoff. Uses a headless provisioning endpoint that accepts email + org name and returns credentials synchronously. Replaces the need for a separate /signup flow for new-user onboarding. Recreated from bird-m's closed PR #113 onto the flattened open-source main. Original was auto-closed when git history was reset on 2026-04-20. All commits authored by @bird-m are preserved via --author. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: rename extractProjectId to extractAppId in credential-resolution test mock Applied via @cursor push command * fix: resolve zone from stored state in runDirectSignupIfRequested for non-TUI modes In non-TUI modes (agent, CI, classic), runDirectSignupIfRequested was called before credential resolution, so session.region was always null and zone silently defaulted to 'us'. This would cause EU users to have their account created in the US data center with US-zone tokens. Now mirrors the zone resolution priority from resolveCredentials: project config Zone > stored user zone > DEFAULT_AMPLITUDE_ZONE. Also persists the resolved zone back to session.region for consistency with subsequent code. Applied via @cursor push command * feat(telemetry): agentic signup attempted event [follow-up to #165] (#197) * chore: gitignore .claude/worktrees Prevent accidental tracking of local worktree scratch space. Matches the existing .claude/skills convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: add implementation plan for agentic signup attempted telemetry Breaks the spec into 9 bite-sized TDD tasks: signup prop on session started, fetch helper refactor, per-status emissions, non-emission guards, wrapper_exception catch, and final verification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(telemetry): add signup prop to session started event Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(signup-or-auth): return discriminated union from fetch retry helper * feat(telemetry): emit agentic signup attempted status=success Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(telemetry): emit agentic signup attempted status=requires_redirect Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(telemetry): emit agentic signup attempted status=signup_error Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(telemetry): emit agentic signup attempted status=user_fetch_failed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(telemetry): lock in non-emission on flag_off and missing_inputs paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(telemetry): emit agentic signup attempted status=wrapper_exception Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(telemetry): share SignupAttemptStatus type and tighten wrapper_exception test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(telemetry): decouple wrapper_exception test from test ordering Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(telemetry): emit wrapper_exception from TUI path too Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: move signup telemetry spec and plan out of repo Both documents moved to ~/repos/docs/projects/wizard-direct-signup/ to follow the convention used for the parent PR #165's planning artifacts. Keeps the wizard repo focused on code; planning docs live alongside the rest of the project's history in the internal docs repo. - docs/superpowers/specs/2026-04-21-signup-outcome-telemetry-design.md → ~/repos/docs/projects/wizard-direct-signup/2026-04-21-direct-signup-outcome-telemetry-design.md - docs/superpowers/plans/2026-04-21-signup-outcome-telemetry.md → ~/repos/docs/projects/wizard-direct-signup/2026-04-21-direct-signup-outcome-telemetry-plan.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(telemetry): extract trackSignupAttempt helper and event const Addresses PR review feedback on #197: - Rename emitAttempted → trackSignupAttempt; convert to const arrow function. "Track" matches Amplitude SDK vocabulary (client.track), "trackSignupAttempt" names the specific event domain. - Simplify helper body with spread + inline conditional — replaces imperative if-push-to-record with a declarative object literal. - Export AGENTIC_SIGNUP_ATTEMPTED_EVENT const so the event-name string isn't duplicated across emission sites. - bin.ts both call sites now go through the shared helper instead of open-coding the wizardCapture call + SignupAttemptStatus type cast. - cli.test.ts mock preserves the real trackSignupAttempt via vi.importActual so bin.ts's dynamic import resolves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(telemetry): pass Amplitude-keyed properties object to trackSignupAttempt Helper is now a one-line passthrough. Call sites compose the exact event property bag that goes over the wire — no camelCase-to-spaced-keys translation layer inside the helper. Matches Amplitude property naming conventions at the call site. Also adds AgenticSignupAttemptedProperties as an exported type so callers get TypeScript enforcement on the allowed property keys. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(telemetry): hoist trackSignupAttempt import out of catch blocks Addresses bugbot finding: with the dynamic import and trackSignupAttempt call inside the catch, any throw from either would suppress the recovery log.warn and bubble past the catch. Hoisting the import alongside the existing performSignupOrAuth import means both symbols resolve before the try block, so the catch body is a single non-throwing function call. Probability of the original scenario was ~zero (import is a cache hit, trackSignupAttempt wraps fire-and-forget SDK) but the hoist is smaller and cleaner than nesting an inner try/catch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: log direct signup success in TUI path Matches the non-TUI helper (bin.ts:501) which logs the same message. Users who explicitly pass --signup --email get no confirmation in the TUI flow otherwise. Addresses review feedback on #165. * chore: warn when signup userInfo lacks env with apiKey after retries Complements the `has env with api key` property on the `agentic signup attempted` Amplitude event (#197) — the event gives us the cohort signal, this log gives per-session context when a user hits the misleading `no_stored_credentials` downstream. Addresses review feedback on #165. * refactor: centralize zone resolution (follow-up to #165) (#208) * refactor: add resolveZone helper + unit tests Single source of truth for zone resolution. Not yet wired to any call site — follows as dead code so reviewers can evaluate correctness in isolation before seeing the wiring. Also excludes .claude/ worktrees from ESLint (flat config) to fix a pre-existing hook breakage caused by stale worktree directories being picked up by eslint.config.mjs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: centralize zone resolution, enforce session.region-is-intent invariant Wire runDirectSignupIfRequested, resolveCredentials, and the TUI OAuth path to the shared resolveZone helper. Drop the two session.region = zone cache-writes that blurred the intent/effective-zone distinction. Add a JSDoc invariant to WizardSession.region. Behavior note: resolveCredentials now honors --region when session.region is set, closing a latent bug where the flag was silently dropped for returning users with a differing stored zone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: sweep remaining direct session.region reads in zone contexts Post-review cleanup of Task 2: resolveEnvironmentSelection and the fire-and-forget hydration block in bin.ts were both reading session.region directly as an effective zone. Both now call resolveZone. The bin.ts hydration guard moves from session.region to session.credentials — which is the real intent (hydrate after credential resolution succeeded), and avoids silently skipping hydration for returning agent-mode users whose session.region is null because resolveCredentials no longer cache-writes it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: assert zone-resolution agreement across call sites Property 1: resolveZone returns the highest-priority present signal (or the fallback) across arbitrary session inputs. Property 2: all three production call sites produce the same effective zone given the same session. Today that's trivially true — this test guards against future drift if anyone reintroduces a bespoke chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(zone-resolution): post-review cleanup - Comment the always-true if(zone) guard in resolveCredentials so future readers don't mistake it for a meaningful check. - Drop unused fallback field from property 2 scenario generator. - Soften the session.region JSDoc invariant: split into WRITE (strict) and READ (softer) sections, noting that TUI screens currently read the field directly as a safe-by-flow-order exception. Follow-up will migrate them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(zone-resolution): post-review polish - Drop tautological if(zone) guard in resolveCredentials now that resolveZone is total; body unindents but otherwise unchanged. - Type session.region as AmplitudeZone | null (was CloudRegion | null); removes the redundant cast in resolveZone tier 1 and in 5 TUI screens (LoginScreen, OutroScreen, SlackScreen, DataSetupScreen, DataIngestionCheckScreen) that no longer need the widening cast. - Trim Tier 4 comment in zone-resolution.ts to match the terse style of the other tier comments. - Narrow eslint ignore from '.claude/**' to '.claude/worktrees/**' so future .claude/ content isn't silently skipped from lint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * revert(zone-resolution): restore if(zone) guard in resolveCredentials Undo the guard removal from d7b902d. The guard is tautological (resolveZone is total), but removing it reindents ~340 lines, which muddies review of this PR. Cleaner to drop it in a follow-up that can be reviewed in isolation. Other parts of d7b902d (session.region typed AmplitudeZone, trimmed comment, narrower eslint ignore) are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(zone-resolution): sweep TUI screens onto resolveZone Five screens (LoginScreen, OutroScreen, DataSetupScreen, DataIngestionCheckScreen, SlackScreen) were still doing their own two-tier 'session.region ?? "us"' zone derivation. These ran after RegionSelect so they were safe today, but they bypassed the centralized helper and would silently drop to 'us' if a new entry path ever skipped RegionSelect. All six sites now call resolveZone(session, DEFAULT_AMPLITUDE_ZONE). Functionally equivalent when region is set; more robust when it isn't (falls through ampli.json Zone and storedUser.zone before defaulting). Closes the TUI-screen follow-up that was deferred from earlier commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(zone-resolution): inline region in SlackScreen, drop redundant zone alias After resolveZone returns a concrete AmplitudeZone, the 'const zone = region' alias was pure noise. Inline 'region' at the two downstream call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(zone-resolution): use resolved zone in hydration-block analytics identify Bugbot-flagged missed migration: after the hydration guard moved from session.region to session.credentials, the analytics.identifyUser call at bin.ts:1010 was still reading session.region directly — which can be null for returning agent-mode users whose zone came from storedUser. Switch to the local 'zone' variable (already resolved via resolveZone) so analytics receives the concrete region consistently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(zone-resolution): route remaining analytics/API region reads through resolveZone Three more sites in the same class as the bugbot finding: - src/ui/tui/store.ts:setCredentials — analytics.identifyUser and analytics.wizardCapture('auth complete') were reading session.region directly. Safe today (RegionSelect gates TUI OAuth entry) but inconsistent with the invariant. Resolved via resolveZone once at the top and reused for both calls. - src/ui/tui/components/ConsoleView.tsx (/whoami handler) — dropped the session.region truthy gate (was silently skipping hydration for returning agent-mode users whose zone came from storedUser), and routed both the fetchAmplitudeUser call and the analytics.identifyUser property through resolveZone. Test updated: setCredentials auth-complete assertion now expects 'us' (the DEFAULT_AMPLITUDE_ZONE fallback) rather than null, matching the new total-function behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(zone-resolution): migrate all pendingAuthCloudRegion chains to resolveZone Seven sites were still doing bespoke (session.region ?? session.pendingAuthCloudRegion ?? 'us') chains. All now delegate to resolveZone: - bin.ts:250 — agent/classic --project-name create. Strictly improves direct-signup: flagless EU signup with --project-name now picks up storedUser.zone (written by direct-signup) instead of defaulting to 'us'. - src/lib/agent-runner.ts:232 — agent-mode cloudRegion derivation. - src/ui/tui/store.ts:635 — writeAmpliConfig zone. - src/ui/tui/screens/AuthScreen.tsx — three sites (env-key selection, API-key fallback, Start Over re-fetch). Dep-array cleanup for the now-unread pendingAuthCloudRegion. - src/ui/tui/screens/CreateProjectScreen.tsx:73 — create-project zone. In every reachable state, session.region is set (from flag, RegionSelect, or /region slash) or storedUser.zone is persisted (from a prior session or just-written direct-signup), so resolveZone returns the correct zone without needing pendingAuthCloudRegion as a tier. The narrow OAuth-complete-before-storedUser-written window where it was uniquely useful is TUI-only, and RegionSelect always runs before those screens in the TUI flow. Closes the documented pre-ramp follow-up. Every production zone-reading site in the codebase now routes through resolveZone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(zone-resolution): widen intent allowlist, swap drift test for grep guard Self-review on #208 surfaced three docstring/test findings: 1. The `session.region` write invariant at `src/lib/wizard-session.ts` listed five intent-bearing write sources but `store.ts:setOAuthComplete` and `agent-runner.ts:190` also write the field from OAuth-derived cloudRegion. The code was conforming in practice — signing into an EU account IS regional intent — but the docstring said otherwise. Fold OAuth-derived zone into the allowlist explicitly so future readers don't have to squint at the contradiction. 2. The read-guidance paragraph still said "TUI screens are currently allowed to read session.region directly... a follow-up will migrate them." That follow-up landed in d0d59ca/8b165ec/a364855/b2a6b4e. Rewrite it to describe the actual post-refactor state: the only legitimate direct reads now are display/debug, checkpoint persistence, and the pre-OAuth RegionSelect gate in bin.ts. 3. The "all three production call sites agree on the effective zone" property test was tautological — it called `resolveZone` three times with identical args and asserted the results matched. A reintroduced `session.region ?? 'us'` chain at a call site would pass it cleanly because the test never imported the call sites. Replace with `zone-resolution.invariants.test.ts`: grep-based guard that (a) forbids `session.region ?? <zone>` fallback chains outside resolveZone, (b) forbids the pre-refactor `pendingAuthCloudRegion ?? session.region` pattern, and (c) restricts direct `session.region` reads to an explicit file allowlist covering the legitimate post-refactor cases. This actually catches the drift the old test was trying to prevent. The remaining property test (tier-ordering correctness) is kept as-is — it's a real property, not tautological. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(zone-resolution): drop dead pendingAuthCloudRegion field The field was declared on `WizardSession` and written in two places (`buildSession` initializer, `setOAuthComplete`) but never read. Grep across `src/` + `bin.ts` confirms zero read sites — the zone data consumed downstream flows through `session.region` (auto-set from the same OAuth cloudRegion) and the resolveZone chain's Tier 3 storedUser lookup. Keep `setOAuthComplete`'s `cloudRegion` parameter — it still drives the auto-set of `session.region` for users whose zone is detected via OAuth, so RegionSelect can be skipped. Type it directly as `CloudRegion | null` instead of borrowing the removed field's type via indexed access. Also tightens the comment on the auto-set branch to reference the region field invariant without naming `session.region` in prose (the drift-guard test searches for that string; prose mentions counted as reads). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(zone-resolution): collapse redundant stored-user tier branches The helper had two tiers walking the stored user: // Tier 3: real stored user's home zone. if (storedUser && storedUser.id !== 'pending' && storedUser.zone != null) { return storedUser.zone; } // Tier 4: pending stored user's zone. if (storedUser && storedUser.id === 'pending' && storedUser.zone != null) { return storedUser.zone; } `getStoredUser` returns at most one record, so the two predicates are mutually exclusive on the same value and both branches return `storedUser.zone` — the tier distinction existed only in comments. A pending user mid-SUSI has the same home-zone semantics as a real user restored from `~/.ampli.json`; there's no fall-through or fallback between them to preserve. Collapse to one branch and document the bifurcation-that-wasn't in a comment so the next person reading the pending vs. real distinction elsewhere doesn't assume this helper treats them differently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(zone-resolution): drop dead BDD writes, rename stale test Self-review triage surfaced two test-only residues left behind by the earlier zone-resolution sweep. - Two Cucumber step files still assigned `session.pendingAuthCloudRegion` in their Before hooks, even though the field was removed from `WizardSession` in 65c77e7. The writes survived because cucumber.mjs uses `tsx/cjs` (transpile-only, no type-check) — the dead assignments compiled but had no effect. Adjacent `session.region = 'us'` already covers the intent the assignments were trying to express. - The unit test at `credential-resolution.test.ts:173` was named "returns early without touching region when no user is stored" — accurate before this PR, stale after. Post-refactor, `resolveZone` is total, so `resolveCredentials` enters the function body rather than returning early; the only reason the `region === null` assertion still holds is that `session.region` is no longer mutated from this path. Renamed to describe what the test actually asserts. No behavior change; tests-only. Surfaced in the PR #208 self-review triage. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: remove wizard-direct-signup feature flag gate The direct-signup gate is moving server-side — the provisioning endpoint will decide who is eligible, so the client doesn't need its own Amplitude Experiment check. Removing the client-side flag: - drops FLAG_DIRECT_SIGNUP from src/lib/feature-flags.ts along with the AMPLITUDE_WIZARD_FORCE_DIRECT_SIGNUP dev-override branch in isFlagEnabled - removes the isFlagEnabled gate at the top of performSignupOrAuth; the remaining short-circuit on missing email/fullName is kept - drops the initFeatureFlags() call in runDirectSignupIfRequested in bin.ts (it only existed so non-interactive modes could evaluate FLAG_DIRECT_SIGNUP before calling into performSignupOrAuth) - deletes src/lib/__tests__/feature-flags.test.ts (every test targeted the removed flag) and cleans the per-test isFlagEnabled mock scaffolding out of the signup-or-auth tests The rest of the feature-flags module (FLAG_LLM_ANALYTICS, FLAG_AGENT_ANALYTICS, initFeatureFlags in the TUI path) is untouched. With this change, whenever --signup --email --full-name are all provided, the wizard POSTs to the provisioning endpoint; the server decides whether to return an oauth code or requires_auth, and the existing null-return / fallback behavior in each mode is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(telemetry): emit browser_fallback_after_signup for TUI post-signup edge case When direct signup succeeds in the TUI path but the wrapper's internal fetchUserWithProvisioningRetry fails, performSignupOrAuth persists a pending sentinel and returns { userInfo: null, tokens: real }. The TUI caller at bin.ts:1244 then retries fetchAmplitudeUser externally; if that also fails, the catch at 1251 unconditionally opens browser OAuth via performAmplitudeAuth({ forceFresh: true }) — treating fresh signup tokens as if they were expired. The code-level guard (re-throw when tokens came from a fresh signup, matching Bugbot's autofix in discussion_r3126264914) is deferred — provisioning lag is not expected in practice, and --signup doesn't promise headless auth (backend eligibility / existing-email both legitimately trigger browser fallback). So the edge case is theoretical rather than a likely production path. This commit lands the observability half so we can decide whether to invest in the code fix based on actual canary signal: - User-facing info log at the catch: "Account created, but user data is still being provisioned. Opening browser to complete sign-in…" Explains the transition so the user isn't confused by a browser opening seconds after "Direct signup succeeded." - New `browser_fallback_after_signup` status on the existing `agentic signup attempted` event. Distinguishes this rare edge case from a primary-path browser OAuth (which never fires the event). Canary dashboards can now measure how often the path actually hits. The `signupTokensObtained` boolean gates both — only set when performSignupOrAuth returned fresh tokens in this run, so the new log and telemetry never fire on the normal expired-token browser fallback. trackSignupAttempt import hoisted out of the signup block so the catch can reach it without a second dynamic import. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(telemetry): persist tokens before emitting signup attempt event performSignupOrAuth used to fire trackSignupAttempt ('success' or 'user_fetch_failed') before calling storeToken. If storeToken threw (disk full, permission error, atomic-rename failure), the exception propagated to the caller's outer catch which would also emit trackSignupAttempt with status 'wrapper_exception' — producing two agentic signup attempted events for a single attempt and corrupting the telemetry funnel denominator. Swap the order: storeToken runs first, telemetry emits only after persistence succeeds. A storeToken throw now collapses cleanly into the caller's wrapper_exception event, so every attempt emits exactly one telemetry event. Probability of storeToken failing is low in practice (uses atomicWriteJSON under the hood with 0o600 perms), but correct telemetry semantics are cheap to guarantee and more important once the feature rolls out broadly. Addresses Bugbot finding at #discussion_r3127608845. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(signup): require explicit --zone for non-TUI direct signup The provisioning backend does not route across regions — POSTing an EU-intending email to the US endpoint silently provisions the account in the US data center, with data-residency implications. Previously, non-TUI modes (agent/CI/classic) running `--signup` with no stored state, no project config Zone, and no way to set a region would fall through resolveZone's fallback and default to `'us'` — wrong for any first-time EU user. Add an explicit `--zone us|eu` CLI flag that pre-populates `session.region`, and require that some zone signal exist before the direct-signup request goes out. When none is present, fail loudly with `AUTH_REQUIRED` and a clear message pointing at `--zone`, rather than silently sending the request to the US endpoint. Changes: - `zone-resolution.ts` — extract `tryResolveZone` that returns null when no explicit signal is present. `resolveZone` is unchanged in shape (still total) and now delegates to `tryResolveZone ?? fallback`. Callers that need definite regional intent can use `tryResolveZone` and treat null as "ask the user." - `wizard-session.ts` — add `zone` to `CliArgsSchema` and `buildSession`. When set, pre-populates `session.region` so RegionSelect is skipped in the TUI flow and `tryResolveZone` Tier 1 returns the explicit zone. - `bin.ts` — add `--zone` global flag (`choices: ['us', 'eu']`). Thread through `buildSessionFromOptions`. In `runDirectSignupIfRequested`, use `tryResolveZone` and exit AUTH_REQUIRED when null instead of silently defaulting. - Tests for `tryResolveZone`, the `--zone` → `session.region` wiring, and regression coverage for the pre-existing `resolveZone` behavior. Addresses Bugbot finding at #discussion_r3127199434. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(signup): use canonical AmplitudeZone type, drop dead conditional Small follow-up cleanup on a222198 surfaced by /simplify review: - `bin.ts` and `src/lib/wizard-session.ts` were re-stringifying `'us' | 'eu'` in three places (runtime cast, buildSession arg type). `AmplitudeZone = keyof typeof AMPLITUDE_ZONE_SETTINGS` already exists in constants.ts and is the canonical name used elsewhere. Adding a third literal union widened drift surface: if a zone is ever added, three sites would need to be updated in lockstep. Replaced the new sites with `AmplitudeZone` so constants.ts stays the single source. - The `region` field in `buildSession` was `parsed.success ? validated.zone ?? null : args.zone ?? null`. Since `validated` is `parsed.success ? parsed.data : args` a few lines up and the zone zod schema does no transformation (only enum validation), both branches reduce to `validated.zone ?? null`. Unlike `signupEmail`, where the strict-reject branch is load-bearing (a malformed email would otherwise bypass zod's `.email()` check), zone has no such safety property — the conditional was dead. Collapsed it, trimmed the accompanying 5-line comment to one line (the prior version rehashed the commit body of a222198 verbatim). No behavior change. Build + 1139 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(cli): align on --region, keep --zone as alias User-facing terminology across the wizard is consistently "region" (RegionSelect screen, /region slash command, `wizard region` subcommand, log messages, error strings, help text). The one outlier was the CLI flag: the pre-existing `wizard login --zone` and the new `--region` flag introduced in a222198 for non-TUI signup both sent users to a "data center region" per their descriptions, but wore different flag names. Normalize to `--region` as the canonical flag name on both call sites, and keep `--zone` as a yargs alias so any scripts using the pre-existing `wizard login --zone` or the a222198-era `--region` / `--zone` continue to work. - bin.ts: default `$0` command owns the scoped `region` option (rather than a global option that collided with the `login` subcommand override). Both definitions carry `alias: 'zone'`, producing a single clean `--region, --zone` row in `--help` output instead of the duplicated choices yargs generated when the flag was defined twice. login handler now reads `argv.region` (yargs populates the alias mirror too). - wizard-session.ts: `CliArgsSchema` field + `buildSession` arg renamed `zone` -> `region`. The session's own `region` field was already the destination, so the rename just aligns the inbound argument name with it. - shell-completions.ts (zsh + bash): advertise `--region` as the primary flag, keep `--zone` tab-completing for scripted users. - Tests: update `buildSession({ region: 'eu' })` call sites and the describe block titles. Same behavior, renamed inputs. Intentionally NOT changed: - Analytics event property `region` (pre-existing; already aligns). - `ampli.json` stored `Zone` field (file-format convention). - Internal `AmplitudeZone` type + `resolveZone` / `tryResolveZone` (not user-facing; internal implementation can keep the shorter term). Addresses terminology-conflation concern raised during PR #165 review — user-facing surface is now uniformly "region." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(telemetry): scope signup catch so onSuccess throws don't double-count runDirectSignupIfRequested's outer try/catch previously wrapped both performSignupOrAuth AND the onSuccess callback (classic mode's resolveCredentials). When signup succeeded, performSignupOrAuth internally emitted `trackSignupAttempt` with `success` or `user_fetch_failed` before returning; if onSuccess then threw, the outer catch emitted a second `trackSignupAttempt` with `wrapper_exception` — producing two `agentic signup attempted` events for a single attempt. d1774fc addressed this pattern inside signup-or-auth.ts (moved storeToken before telemetry so a storeToken throw collapsed cleanly into the caller's wrapper_exception). The outer scope in bin.ts re-introduced the same double-event pattern through onSuccess — that fix is landed here. Refactor: split the single try/catch into two narrower blocks. 1. Wrap only performSignupOrAuth. On throw: emit wrapper_exception, log, and return early. No further code runs after a wrapper throw. 2. On successful (non-null) signup, log the success message and wrap the onSuccess call in its own try/catch. An onSuccess throw now logs a dedicated "post-signup handling failed" warning but does NOT re-emit telemetry — performSignupOrAuth's internal event is the authoritative record of the attempt. After this change, every direct-signup attempt emits exactly one `agentic signup attempted` event regardless of where post-signup plumbing fails. Addresses Bugbot finding at #discussion_r3127860816. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(cli): exercise real control flow in wrapper_exception test The `wrapper_exception` test at `cli.test.ts:874` omitted `--region`, which means `tryResolveZone` in `runDirectSignupIfRequested` returned null and `process.exit(AUTH_REQUIRED)` fired before `performSignupOrAuth` was ever called. Because the test harness mocks `process.exit` as a no-op, execution fell through and passed `zone: null` to `performSignupOrAuth` — a path that cannot occur in production, since real `process.exit` halts. The assertion that `wrapper_exception` fires was passing for the wrong reason: it was reaching the performSignupOrAuth throw via mock-enabled fall-through, not via the documented non-TUI signup control flow. Add `--region us` to the args so the test exercises the same path production takes. Add an inline comment explaining why, so a future reader doesn't delete the flag thinking it's boilerplate. Addresses Bugbot finding at #discussion_r3127825312. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(tui): memoize resolveZone in SlackScreen to avoid per-render disk I/O SlackScreen's `region = resolveZone(store.session, DEFAULT_AMPLITUDE_ZONE)` call sat in the component body, so every re-render hit two synchronous disk reads (`readAmpliConfig` on `ampli.json` and `getStoredUser` on `~/.ampli.json`). The screen drives several state transitions (`phase`, `openedUrl`) via `useState` + `setTimeout`, and also re-renders on any session update via `useWizardStore`. Prior to the zone-resolution consolidation in #208, the read was `session.region ?? 'us'` — a pure property access with no I/O. The consolidation made it correct in more edge cases but introduced a needless perf regression on this hot path. Wrap the call in `useMemo` keyed on the inputs that can actually change within this screen's lifetime: `session.region` and `session.installDir`. `resolveZone`'s remaining inputs (`ampli.json` Zone, stored user zone) are stable once RegionSelect / auth have completed, so treating them as invariant for the memo's lifetime is safe. DataSetupScreen also calls `resolveZone` but it does so inside a `useEffect`, so no fix is needed there — the effect runs at most once per session. Update the drift-guard allowlist to permit the `session.region` read that now appears only in the `useMemo` dep array — same rationale as the existing AuthScreen entry. Addresses Bugbot finding at #discussion_r3127801032. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(tui): extract useResolvedZone hook, apply to CreateProjectScreen 7bab7c7 added an inline useMemo in SlackScreen to avoid per-render disk I/O from resolveZone. Audit of all other resolveZone call sites in the TUI tree surfaced one more render-body leak: CreateProjectScreen.tsx:74 has the same pattern. (The remaining sites — AuthScreen, OutroScreen, DataSetupScreen, DataIngestionCheckScreen, store methods — all call resolveZone inside useEffect or event handlers, so they fire at most once per invocation and aren't per-render.) Extract the memoization into a shared hook `useResolvedZone` under `src/ui/tui/hooks/` and apply it to both render-body consumers (SlackScreen + CreateProjectScreen). Same memo mechanics as the inline version, same staleness tradeoff (deps track session.region + installDir only; the Tier 2/3 disk values are treated as stable for a screen's lifetime, which they are after RegionSelect / auth have completed). DRYs the pattern and documents the staleness assumption in one place with guidance on when to hoist instead. Drift-guard allowlist: replace the per-screen SlackScreen entry with a single allowlist entry for the hook file. Screens no longer read session.region directly — they consume the hook's already-resolved AmplitudeZone return value. Imports cleaned up in both screens: DEFAULT_AMPLITUDE_ZONE, resolveZone, and the AmplitudeZone type alias are no longer needed locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tui): eliminate render-path disk I/O via readDisk option on resolveZone Prior commit 9f8b030 DRYed the per-render `useMemo` into a shared `useResolvedZone` hook — but the underlying staleness + I/O problem persisted: the hook still called the full `resolveZone` chain on each memo miss, and the memo's dep array (`session.region`, `session.installDir`) does not capture Tier 2/3's disk-backed inputs. So the hook was either doing redundant disk reads or silently returning stale zones on the rare cases when the backing files mutated. Caught by reviewer feedback: useMemo over an impure function with incomplete deps isn't a real fix. Actual fix: separate disk-reading from tier-walking at the `resolveZone` level rather than papering over it at the hook. - `resolveZone(session, fallback, { readDisk?: boolean })` — new option. When `false`, only Tier 1 (`session.region`) is consulted and the fallback wins if it's null. Tiers 2 and 3 (ampli.json, stored user) are skipped entirely; no disk reads, no cache, no staleness. Default remains `true` so every existing call site preserves full-chain semantics. - `useResolvedZone` now calls `resolveZone(..., { readDisk: false })` and memos on just `session.region`. Zero I/O per render, zero staleness. Safe because every consumer (SlackScreen, CreateProjectScreen) runs after RegionSelect / auth has populated `session.region` — Tier 1 is the authoritative answer. - Updated the hook docstring to document the `readDisk: false` contract and to steer future hook consumers toward hoisting + prop threading if they ever need a pre-RegionSelect consumer, rather than re-adding disk reads to the hot path. Tests: three new cases on `resolveZone` — `readDisk: false` returns `session.region` when set without touching mocks; returns fallback when `session.region` is null even when Tier 2/3 would have matched; default (no options) preserves full-chain behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(signup): share email regex between yargs coerce and zod schema Previously the `--email` CLI flag had two independent validators that could disagree: yargs used a custom regex (`/^[^\s@]+@[^\s@]+\.[^\s@]+$/`) in its `coerce` handler, and `CliArgsSchema` used zod's `.email()`. zod's built-in is stricter on some forms (IDN domains, certain quoted-local-part shapes). An email that passed yargs but failed zod would cause the entire CliArgsSchema `safeParse` to fail, and the `parsed.success ? ... : null` guard in `buildSession` would silently null out BOTH `signupEmail` AND `signupFullName`. Direct signup would no-op with only a generic "[wizard] Invalid CLI args" console warning — nothing pointing at the email as the culprit. Fix: extract `EMAIL_REGEX` as a single exported constant in `constants.ts` and import it from both layers. - `constants.ts` — new `EMAIL_REGEX` export with a docstring explaining it is THE source of truth for CLI email validation and why both layers must agree. - `bin.ts` — yargs `coerce` for `--email` imports `EMAIL_REGEX` statically (no more ad-hoc inline regex). - `wizard-session.ts` — `CliArgsSchema.signupEmail` uses `.regex(EMAIL_REGEX, 'Invalid email')` instead of `.email()`. Both layers now accept/reject exactly the same set of strings. - `cli.test.ts` — the `../lib/constants` vi.mock needed the new export surfaced so bin.ts's yargs coerce sees the regex at test time; two tests ("accepts --email/--full-name" and "wrapper_exception") were silently timing out because `EMAIL_REGEX` was undefined in the mock and the coerce threw. Addresses Bugbot finding at #discussion_r3127917514. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(run): include signup fields + region in runWizard Args type The `Args` type for `runWizard` declared only a subset of the CLI flags flowing through this PR's surface: missing `signupEmail`, `signupFullName`, and `region`. `runWizard` has a fallback branch that rebuilds a session from `Args` when no `session` parameter is provided (run.ts:52-66). Today every call site passes a pre-built session so the fallback doesn't fire, but the type being structurally incomplete meant a future caller that forgets to pre-build (or a refactor that routes through `Args` only) would silently drop signup fields with no type error — direct signup would no-op and the bug would only surface as missing telemetry. Add the three fields to `Args` and thread them through the `buildSession` call in the fallback branch. Closes the latent data-loss class of bug permanently. Addresses Bugbot finding at #discussion_r3127917521. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(session): guard region against zod parse failure like signup fields `buildSession` applies a `parsed.success ? validated.X ?? null : null` guard to `signupEmail` and `signupFullName` so a zod-rejected value cannot slip through via the raw-args fallback. The `region` field added in a222198 missed the same guard — it used `validated.region ?? null` unconditionally, meaning if any zod validation elsewhere in `CliArgsSchema` failed, `validated` would fall back to raw `args` and `region` could receive a value that bypassed `z.enum(['us', 'eu'])`. yargs' `choices: ['us', 'eu']` constraint protects the CLI path, so this was a latent rather than live bug — but programmatic callers of `buildSession` (tests, future routes, internal rebuilds) don't get yargs validation and could pass a bogus region. Aligning with the existing signup-field pattern closes the gap and makes the invariant uniform across all zod-validated fields. Addresses Bugbot finding at #discussion_r3128170204. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: use AGENTIC_SIGNUP_ATTEMPTED_EVENT constant instead of hardcoded string `signup-or-auth.ts` exports `AGENTIC_SIGNUP_ATTEMPTED_EVENT` as the single source of truth for the event name, but nine test assertions across `cli.test.ts` and `signup-or-auth.test.ts` hardcoded the literal `'agentic signup attempted'`. A future rename would update the constant but leave the tests silently passing against the old string, hiding the drift until a data consumer noticed. Import the constant and use it everywhere. Same assertions, just routed through the constant so a rename propagates automatically. No behavior change. Addresses Bugbot finding at #discussion_r3128170199. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(signup): wipe prior account entries on signup success (#217) * fix(signup): wipe prior account entries on signup success Direct signup now uses a new `replaceStoredUser()` helper instead of the additive `storeToken()`. Before writing the new user entry, it removes every existing `User-*` / `User[*]-*` key in ~/.ampli.json, preserving other config keys. Why: `~/.ampli.json` can hold many user entries, but every reader (`getStoredUser()`, called from every auth path) returns the first real user it finds in insertion order. The direct-signup path writes non-destructively via `storeToken`, which means signup-over-existing- user leaves the new account stranded behind the old one — the next wizard run silently logs in as the old user and ignores the freshly issued signup tokens. Narrow fix at the signup success path (not inside `storeToken` itself): OAuth and token-refresh callers legitimately write under an existing user's key and should not wipe a coexisting `ampli` CLI session that was installed for a different account. Only signup expresses the "this account replaces any prior one" intent. Also clears lingering `{ id: 'pending' }` sentinels from prior failed signup user-fetch fallbacks — those already accumulated in the file and `getStoredUser` skips them, but they're cruft worth sweeping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(ampli-settings): extract isUserKey helper, trim comments The User-key predicate (`k.startsWith('User-') || k.startsWith('User[')`) appeared in three call sites after the recent signup-sweep work — `getStoredUser`, `getStoredToken`, and the new `replaceStoredUser`. Rule of three met: extract a module-private `isUserKey()` helper so the sites can't drift in what they consider a "User entry." Also trims four recently-added comment blocks that drifted past the repo convention ("don't explain WHAT the code does; don't narrate the change"): - `EMAIL_REGEX` JSDoc (constants.ts): 11 lines → 3 - `replaceStoredUser` JSDoc (ampli-settings.ts): 7 lines → 1 - `region:` inline comment (wizard-session.ts): 6 lines → 2 (full reasoning already exists above on `signupEmail`) - call-site comment in signup-or-auth.ts: 10 lines → 3 (the replaceStoredUser-vs-storeToken rationale is now carried by the function name plus the one-line JSDoc) No behavior change. Surfaced by `/simplify` after landing commit 91845eb. * chore(ampli-settings): log wipe details, pin no-spread invariant Follow-up to the review on PR #217: - `replaceStoredUser` now emits a `debug` log listing the `User-*` keys it deletes (count + keys — keys are non-sensitive; tokens stay out). This makes "I signed up but the wizard logged me in as someone else" and the inverse "my ampli CLI session disappeared" diagnosable from the session log alone, without needing a live repro. The log is skipped when nothing was wiped to keep noise down on the common case. - A one-line comment at the new-entry assignment documents why the write doesn't spread an existing entry (`storeToken` does; this function deliberately doesn't, because the preceding loop has already deleted every `User-*` key). Pins the invariant so a future edit that reorders or removes the sweep doesn't silently drop fields. No behavior change for callers. Existing tests (ampli-settings 33, signup-or-auth 14) pass unchanged; the debug log isn't asserted on because it isn't part of the observable contract. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(zone): skip disk I/O in hot-path zone resolution `resolveZone(session, fallback)` defaults to `readDisk: true`, which on each call walks the full resolution chain: Tier 1: session.region (in-memory) Tier 2: readAmpliConfig (synchronous fs.readFile on ampli.json) Tier 3: getStoredUser (synchronous fs.readFile on ~/.ampli.json) Both the Data Ingestion check screen and the Data Setup screen run after the wizard's RegionSelect gate, which is declaratively enforced by the flow pipeline — those screens cannot mount until the resolved region is populated on the session. Tier 1 is therefore authoritative at these call sites, and Tiers 2/3 are pure waste. Three call sites were affected: - DataIngestionCheckScreen.checkIngestion(): polled every 30s while the screen is active, performing two synchronous disk reads per tick. - DataIngestionCheckScreen render body (x2): coaching-tip branches that re-evaluate on every render. The screen re-renders every 5s from a lastChecked/elapsedSeconds timer and on every store subscription tick, so these were the hottest path of the bunch. - DataSetupScreen mount-only useEffect: one-shot on mount, low impact but still unnecessary given the flow invariant. The polled and effect sites pass `{ readDisk: false }` explicitly. The render-body sites switch to the shared `useResolvedZone` hook, which memoizes on `session.region` and internally calls `resolveZone` with `readDisk: false` — matching the existing pattern in SlackScreen and CreateProjectScreen. No behavior change: by flow invariant, Tier 1 already had the same answer Tiers 2/3 would have produced. * refactor(zone): make resolveZone readDisk option required Previously `resolveZone(session, fallback)` defaulted to `readDisk: true`, so callers silently opted into two synchronous disk reads (`readAmpliConfig` + `getStoredUser`) on every invocation unless they remembered to pass `{ readDisk: false }`. That made hot-path regressions invisible in review: the 30s-poll + JSX-render-body issue fixed in the prior commit had existed unflagged since the hot-path option was added. Remove the default and require the options arg at every call site. Each caller now makes an explicit, reviewable choice: - `readDisk: true` for pre-RegionSelect paths where disk tiers must be consulted (bin.ts CLI arg parsing, credential resolution, agent-runner entry, store mutation paths, Auth/Login screens, /whoami console). - `readDisk: false` for post-RegionSelect callers that can assert Tier 1 is authoritative (OutroScreen, DataIngestionCheckScreen checkIngestion, DataSetupScreen effect, and the existing useResolvedZone hook). All existing call sites are translated to preserve current behavior — this is a contract change, not a behavior change. New reviewers will now see the disk-read cost at every call site and be prompted to justify it. Tests updated to pass the option explicitly. * fix(direct-signup): surface 4xx client errors with HTTP meaning validateStatus passes all <500 responses through to schema parsing. When the provisioning body doesn't match RedirectSchema/ErrorSchema/OAuthCodeSchema, the fallthrough framed every non-match as "Unexpected response (status)" — which reads as a shape mismatch, not an HTTP client error. An upstream proxy or rate limiter returning 429 with a non-ErrorSchema body is the concrete case: the user saw "Unexpected response (429)" instead of a rate-limit narrative they could act on. 4xx from a WAF, CDN, or infra layer had the same misleading framing. Now: after all three schemas fail to match, branch on status — 429 gets a rate-limit message, other 4xx gets "Provisioning failed with HTTP XXX", and only truly unexpected 2xx shapes keep the "Unexpected response" wording. Mirrors the pattern already used at the token-exchange step (line 162). Addresses review comment on PR #165: #165 (comment) * fix(zone): drop redundant readDisk at TUI auth call site The `await` on `session.region !== null` a few lines above this call site guarantees Tier 1 is populated by the time we call `resolveZone`, so `readDisk: true` causes two synchronous disk reads (`readAmpliConfig` + `getStoredUser`) whose results are always discarded when Tier 1 short-circuits. Matches the pre-refactor behavior on main, which did no disk I/O at this point. Also removes the inline "Pre-auth: session.region may be unset at this point" comment — that justification was true at a site earlier in the flow but is incorrect here; leaving it would mislead future readers. Flagged by BugBot on PR #165. The stale-capture half of the review (captured `zone` not re-read if user changes region mid-flow) is pre-existing on main and left out of this change. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
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
2 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.
AMP-00000
Summary
replaceStoredUser(user, token)helper insrc/utils/ampli-settings.tsthat removes everyUser-*/User[*]-*key in~/.ampli.jsonbefore writing the new entry (non-user config keys are preserved).src/utils/signup-or-auth.ts) now uses it instead ofstoreTokenon the success path.Why
~/.ampli.jsonwrites are additive (storeTokenis non-destructive) but reads are effectively single-user — every caller goes throughgetStoredUser(), which returns the first realUser-*entry it finds in insertion order. That asymmetry is pre-existing, but direct signup is the first code path that programmatically creates a new account without the user going through an explicit `/logout` first.Concrete failure mode: user is logged in as alice → runs `amplitude-wizard --signup --email bob@… --full-name Bob` (agent/CI/classic) → signup succeeds and writes bob's entry → file now holds both alice and bob → next wizard run silently resolves back to alice (older insertion), bob's just-issued tokens are stranded dead weight. Signup appeared to succeed but the user is still logged in as alice.
The fix is intentionally narrow. Only the signup success path calls `replaceStoredUser`. OAuth and token-refresh callers stay on `storeToken` — they legitimately operate on the current user and shouldn't silently wipe a coexisting `ampli` CLI session that belongs to a different account. Only signup carries the "this account replaces any prior one" intent.
As a bonus, the sweep also clears lingering `{ id: 'pending' }` sentinels from earlier failed post-signup user-fetches. `getStoredUser()` already skipped them, but they were cruft accumulating in the file.
Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Changes credential persistence on the direct-signup path to delete all existing
User-*entries before writing the new account, which could affect users with multiple stored sessions if invoked unexpectedly. Scope is limited to signup, with added tests, but it touches auth/token storage behavior.Overview
Direct signup now replaces the stored Ampli account instead of appending to
~/.ampli.json: a newreplaceStoredUser()helper wipes all existingUser-*/User[zone]-*entries (includingpendingsentinels) while preserving non-user config keys, then writes the new user+token entry.performSignupOrAuthswitches fromstoreTokentoreplaceStoredUser(keeping the “persist before telemetry” ordering), and unit tests are expanded/updated to cover the new wipe-and-replace semantics and updated mocks.Reviewed by Cursor Bugbot for commit f71dfcd. Bugbot is set up for automated code reviews on this repo. Configure here.