Skip to content

feat(signup): server-driven field collection and interactive prompts for --signup#220

Open
bird-m wants to merge 104 commits into
mainfrom
followup/signup-missing-fields
Open

feat(signup): server-driven field collection and interactive prompts for --signup#220
bird-m wants to merge 104 commits into
mainfrom
followup/signup-missing-fields

Conversation

@bird-m

@bird-m bird-m commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Replaces the all-or-nothing --signup flag contract with a server-driven field collection flow. When --signup is passed without all required fields, the wizard now collects them interactively instead of hard-failing. The backend can also request additional fields after the initial POST via a needs_information response, enabling a retry loop.

TUI mode

Three new screens slot into the SUSI flow after RegionSelect:

  • SignupFullNameScreen — prompts for full name when --signup-full-name is missing
  • SignupEmailScreen — prompts for email when --signup-email is missing
  • SigningUpScreen — performs the signup POST, handles needs_information responses by routing back to collection screens, and falls through to OAuth on requires_redirect

The flow pipeline in flows.ts gates each screen on whether the corresponding field is present in the session, so screens are skipped when flags are already provided.

Classic mode

New promptForMissingSignupFields helper uses @inquirer/prompts to collect missing region, signupFullName, or signupEmail in the classic (non-TUI) interactive path.

Agent / CI mode

Behavior unchanged — canPrompt: false preserves the existing hard-fail contract. Agent mode exits with AUTH_REQUIRED (exit code 3) and emits an auth_required NDJSON event when fields are missing.

Discriminated result types

performSignupOrAuth now returns a discriminated union (kind: 'success' | 'needs_information' | 'requires_redirect' | ...) instead of AmplitudeAuthResult | null, eliminating null-means-what ambiguity for callers.

State cleanup on fresh auth

New clearStaleProjectState utility wipes per-project state (API key store, .env.local, session checkpoint) when a new account is established, preventing credential inheritance across accounts. Called from direct signup success, OAuth completion, and logout.

Session extensions

WizardSession gains signup ceremony state: signupRequiredFields (server-requested fields), signupAuth (intermediate auth result), and signupAbandoned (requires_redirect fallthrough).

Test plan

  • Unit tests for all three new TUI screens (validation, session writes, dispatch)
  • Unit tests for promptForMissingSignupFields — all-missing / partial / no-op / email validator / name validator / trimming
  • Unit tests for clearStaleProjectState
  • Router parameterized cases for each signup field combination
  • Flow-invariants property tests (fast-check) — signup gate + Auth unreachable until fields filled
  • BDD scenarios for server-driven field collection (4 journey shapes: no flags, partial flags, all flags, requires_redirect)
  • BDD scenarios for interactive TUI prompts (no flags / partial flags / all flags)
  • Automated smoke of --agent --signup → exits with AUTH_REQUIRED (3)
  • Automated smoke of --ci --signup → exits non-zero with clear error
  • Manual smoke of pnpm try --signup (TUI) — verify screens render, submit advances
  • Manual smoke of pnpm try --classic --signup — verify inquirer prompts run in order

Out of scope

A "stored user will be cleared — continue?" confirmation before direct signup. Tracked as a follow-up.


Note

Medium Risk
Changes core signup/auth routing and token persistence behavior (new needs_information retry loop, new TUI screens, and per-project state wipes on fresh auth), which could affect onboarding flow and credential handling if any edge case is missed.

Overview
Enables a server-driven --signup flow where the provisioning endpoint can return needs_information and the wizard collects missing fields (full name/email) and retries, instead of treating missing inputs as an all-or-nothing failure.

Adds a dedicated TUI “signup ceremony” (SignupFullName, SignupEmail, SigningUp) with new session state (signupRequiredFields, signupAuth, signupAbandoned) and updates routing/gating so OAuth doesn’t race direct-signup and direct-signup results are reused rather than re-POSTed.

Refactors the direct-signup wrapper to return a discriminated union (success/needs_information/requires_redirect/error), adds optional interactive prompting for classic mode, and introduces clearStaleProjectState to wipe install-dir keyed auth/project artifacts on successful direct signup, fresh OAuth completion, and logout. Extensive new unit/property/BDD tests cover the new states and invariants.

Reviewed by Cursor Bugbot for commit 5bdadbe. Bugbot is set up for automated code reviews on this repo. Configure here.

bird-m and others added 30 commits April 20, 2026 15:49
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>
… 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
…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>
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.
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: 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>
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>
…up 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>
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>
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>
…onal

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>
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>
…ount

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>
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>
…k 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>
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>
…veZone

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>
# Conflicts:
#	src/utils/shell-completions.ts
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>
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>
`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>
…d 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

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>
# Conflicts:
#	src/lib/zone-resolution.ts
#	src/ui/tui/screens/DataIngestionCheckScreen.tsx
#	src/ui/tui/store.ts
`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.
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.
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)
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.
Adds two new Screen enum values and two flow entries in the SUSI
pipeline (Flow.Wizard), placed after RegionSelect and before Auth. The
`show` predicates gate the screens on `session.signup && <field> ===
null`, so they only render when --signup was passed without the
corresponding flag. `isComplete` advances past them once the session
field is written (by the screens themselves in follow-up tasks).

This commit intentionally leaves the screens themselves, store setters,
and screen-registry wiring for follow-up commits. `pnpm tsc --noEmit`
may report missing registry cases until those land — any other type
errors are regressions.
bird-m and others added 8 commits May 1, 2026 09:18
Symmetric counterpart to the signup-side wipe added earlier in this PR.
On a successful fresh-OAuth completion (browser-returned code → tokens
exchanged → about to persist via storeToken), wipe pre-existing
install-dir-keyed state so the new account doesn't inherit the prior
account's API key, workspace binding, or session checkpoint.

Without this, the bug class fixed by the signup-side wipe still leaks
through the OAuth path: a user with cached state for account A who logs
in to account B via the browser would have B's tokens persisted to
~/.ampli.json but A's API key still in keychain / .env.local / project
ampli.json — events would silently route into A's tenancy.

Earlier discussion in this PR considered a read-side validation
primitive instead (validate cached state belongs to the active session
on every credential read). The wipe-on-fresh-auth-completion design is
strictly simpler:

- Idempotent in the same-account case: clearing the cached key just
  forces one extra backend round-trip on the next read; user ends up
  in the same valid state.
- Correct in the different-account case: stale state is gone before
  the new tokens are persisted, so downstream credential resolution
  fetches the new account's key.

The wipe fires only on the fresh-OAuth path (after the browser
callback). The cached-token short-circuit at oauth.ts:238-261 returns
existing tokens unchanged — no auth event has occurred, the cached
project state belongs to that same user, no wipe needed.

`installDir` is now required on `performAmplitudeAuth`'s options. The
compiler caught all four call sites: setup-utils.ts:494 (classic mode
funnel), bin.ts:1390 (TUI main flow), bin.ts:1443 (TUI token-expired
retry), bin.ts:1770 (the `wizard login` subcommand). The deprecated
performOAuthFlow wrapper at oauth.ts:417 has zero callers; updated to
default installDir to process.cwd().

Not adding a unit test for the wipe call inside performAmplitudeAuth
in this commit: oauth.ts's internal helpers (startCallbackServer,
exchangeCodeForToken, opn browser interaction) aren't exported and
mocking them properly would require significantly more harness than
the one-line behavior change warrants. The compiler-required
installDir signature plus the parallel test coverage on
performSignupOrAuth's wipe (which shares the helper) cover the
contract sufficiently. Happy to add a focused oauth.test.ts in a
follow-up if reviewers want it.

Refs MCP-196.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use AMPLITUDE_WIZARD_CACHE_DIR + getCheckpointFile for checkpoint removal.
Replace obsolete keychain exec assertion with per-user cache API key clear.

Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve helpers auth gate: keep SigningUp ceremony gating on top of
accountCreationFlow + ToS. Align flows/AuthScreen and BDD steps with
accountCreationFlow (replaces session.signup on WizardSession).

Co-authored-by: Cursor <cursoragent@cursor.com>
…te wipe)

- Restore session.accountCreationFlow from CLI --signup for SigningUp gate
- Combine isCreateAccountOnboarding ToS check with signup ceremony settling
- Logout uses clearStaleProjectState (symmetric with post-signup wipe)
- cli.test wizard-session mock uses importOriginal + accountCreationFlow
- Split auth-gate tests: menu create-account vs --signup blocking

Co-authored-by: Cursor <cursoragent@cursor.com>
- Align flows/screen-registry with EmailCapture, ToS, accountCreationFlow
- Dedupe signup helpers and legacy s.signup flow entries in flows.ts
- direct-signup: needs_information + structured errors with optional code
- signup-or-auth: dashboardUrl on success, single SignupOrAuthOptions
- SigningUpScreen passes installDir; AuthScreen OAuth copy uses accountCreationFlow
- Tests and Cucumber steps updated for installDir and session flags

Co-authored-by: Cursor <cursoragent@cursor.com>
- Wire promptForMissingSignupFields via runDirectSignupIfRequested({ canPrompt })
- Use accountCreationFlow in signup-prompt; drop duplicate TUI flow entries
- Remove orphaned bin.ts signup helper (commands/helpers is canonical)

Co-authored-by: Cursor <cursoragent@cursor.com>
@kelsonpw kelsonpw changed the base branch from feat/direct-signup-v2 to main May 1, 2026 21:28
kelsonpw and others added 2 commits May 1, 2026 14:28
session.signup is not part of WizardSession; flows gate on accountCreationFlow.

Co-authored-by: Cursor <cursoragent@cursor.com>
kelsonpw and others added 4 commits May 1, 2026 14:33
…ecked project

Matches Vitest React screen tests excluded from tsconfig.json.

Co-authored-by: Cursor <cursoragent@cursor.com>
- signup-prompt tests: mock tryResolveZone (Tier 1 only) via correct module path;
  mockReset inquirer mocks between tests to avoid queued mock leakage
- Allowlist signup-prompt session.region assignment in zone drift guard
- Align signup-related tests with accountCreationFlow and signup-prompt zone API

Co-authored-by: Cursor <cursoragent@cursor.com>
…mail

- Skip EmailCapture/ToS menu gates when accountCreationFlow (--signup); gate
  ToS after both signup fields exist on direct signup
- Order SignupFullName before SignupEmail; hide collection when signupAbandoned
- Update direct-signup BDD + needs_information scenarios; simulate needs_information
  by clearing signupFullName in steps
- Adjust router unit tests for abandoned fallback and name-first resolution

Co-authored-by: Cursor <cursoragent@cursor.com>
@kelsonpw kelsonpw changed the title feat(signup): prompt for missing fields instead of requiring all flags feat(signup): server-driven field collection and interactive prompts for --signup May 1, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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: needs_information re-prompt path is unreachable dead code
    • Updated setSignupRequiredFields to null the matching session value (e.g. signupFullName for full_name) so the flow re-resolves back to the collection screen, and removed the dead unmet branch in SigningUpScreen that always evaluated to empty.

Create PR

Or push these changes by commenting:

@cursor push 278394f724
Preview (278394f724)
diff --git a/features/step-definitions/direct-signup-needs-information.steps.ts b/features/step-definitions/direct-signup-needs-information.steps.ts
--- a/features/step-definitions/direct-signup-needs-information.steps.ts
+++ b/features/step-definitions/direct-signup-needs-information.steps.ts
@@ -101,10 +101,10 @@
   'the first signup POST returns needs_information for {string}',
   function (field: string) {
     // Mirrors what SigningUpScreen does on a needs_information response
-    // with a known, unmet field: store.setSignupRequiredFields([...]).
+    // with a known, unmet field: store.setSignupRequiredFields([...]),
+    // which both records the requirement and nulls the matching session
+    // value so the flow re-resolves back to the collection screen.
     session.signupRequiredFields = [field];
-    // fieldPresentOnSession treats an existing signupFullName as satisfied;
-    // clear it so the router returns to SignupFullName like a fresh ask.
     if (field === 'full_name') {
       session.signupFullName = null;
     }

diff --git a/src/ui/tui/screens/SigningUpScreen.tsx b/src/ui/tui/screens/SigningUpScreen.tsx
--- a/src/ui/tui/screens/SigningUpScreen.tsx
+++ b/src/ui/tui/screens/SigningUpScreen.tsx
@@ -37,7 +37,7 @@
   performSignupOrAuth,
   trackSignupAttempt,
 } from '../../../utils/signup-or-auth.js';
-import { KNOWN_REQUIRED_FIELDS, fieldPresentOnSession } from '../flows.js';
+import { KNOWN_REQUIRED_FIELDS } from '../flows.js';
 
 interface SigningUpScreenProps {
   store: WizardStore;
@@ -117,18 +117,13 @@
             store.setSignupAbandoned(true);
             return;
           }
-          // Re-read the live session for the unmet-field check —
-          // a slash command (e.g. /region) could in principle have
-          // mutated session during the await, and we want the freshest
-          // view when deciding whether to abandon vs continue.
-          const live = store.session;
-          const unmet = result.requiredFields.filter(
-            (f) => !fieldPresentOnSession(live, f),
-          );
-          if (unmet.length === 0) {
-            store.setSignupAbandoned(true);
-            return;
-          }
+          // setSignupRequiredFields nulls the matching session values so
+          // the flow re-resolves back to the corresponding collection
+          // screen for a re-prompt. The SignupFullName predicate gates on
+          // `signupFullName === null`; without that clear, the session
+          // still carries the value we just sent, the predicate skips
+          // SignupFullName, and the router lands back on SigningUp with
+          // no way to gather a fresh value (-> infinite spinner).
           store.setSignupRequiredFields(result.requiredFields);
           return;
         }

diff --git a/src/ui/tui/store.ts b/src/ui/tui/store.ts
--- a/src/ui/tui/store.ts
+++ b/src/ui/tui/store.ts
@@ -616,6 +616,17 @@
   }
 
   setSignupRequiredFields(fields: string[]): void {
+    // Null out the previously-submitted value for any field the server is
+    // re-requesting so the flow re-resolves back to its collection screen
+    // (e.g. SignupFullName, whose `show` predicate gates on
+    // `signupFullName === null`). Without this, SigningUpScreen would
+    // record the requirement but the flow would skip the collection
+    // screen, leaving the user stuck on SigningUp with no way forward.
+    for (const field of fields) {
+      if (field === 'full_name') {
+        this.$session.setKey('signupFullName', null);
+      }
+    }
     this.$session.setKey('signupRequiredFields', fields);
     this.emitChange();
   }

You can send follow-ups to the cloud agent here.

Comment thread src/ui/tui/screens/SigningUpScreen.tsx
@kelsonpw

kelsonpw commented May 1, 2026

Copy link
Copy Markdown
Member

@cursor push 278394f

cursoragent and others added 2 commits May 1, 2026 22:17
When SigningUpScreen received a needs_information response, it computed
`unmet` against the current session — but the SignupFullName flow
predicate guarantees `signupFullName !== null` before SigningUp can
mount, so `unmet` was always empty and the screen always abandoned to
browser OAuth instead of re-prompting.

Encapsulate the clear inside `setSignupRequiredFields`: null out the
matching session value (`signupFullName` for `full_name`) so the flow
re-resolves back to the collection screen for a fresh prompt. The dead
`unmet` branch in SigningUpScreen is removed.

Applied via @cursor push command

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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: Missing field-already-sent guard causes infinite re-prompt loop
    • Imported and applied fieldPresentOnSession in the needs_information branch of SigningUpScreen so that when the server requests fields the session already carries, the screen now sets signupAbandoned instead of re-prompting in a loop.

Create PR

Or push these changes by commenting:

@cursor push eec3255bdd
Preview (eec3255bdd)
diff --git a/src/ui/tui/screens/SigningUpScreen.tsx b/src/ui/tui/screens/SigningUpScreen.tsx
--- a/src/ui/tui/screens/SigningUpScreen.tsx
+++ b/src/ui/tui/screens/SigningUpScreen.tsx
@@ -37,7 +37,7 @@
   performSignupOrAuth,
   trackSignupAttempt,
 } from '../../../utils/signup-or-auth.js';
-import { KNOWN_REQUIRED_FIELDS } from '../flows.js';
+import { KNOWN_REQUIRED_FIELDS, fieldPresentOnSession } from '../flows.js';
 
 interface SigningUpScreenProps {
   store: WizardStore;
@@ -117,6 +117,17 @@
             store.setSignupAbandoned(true);
             return;
           }
+          // If the server is asking for a field we already submitted,
+          // it's effectively rejecting our value. Re-prompting would
+          // just send the same value again and loop forever — bail to
+          // browser OAuth instead.
+          const allFieldsAlreadySent = result.requiredFields.every((f) =>
+            fieldPresentOnSession(s, f),
+          );
+          if (allFieldsAlreadySent) {
+            store.setSignupAbandoned(true);
+            return;
+          }
           // setSignupRequiredFields nulls the matching session values so
           // the flow re-resolves back to the corresponding collection
           // screen for a re-prompt. The SignupFullName predicate gates on

You can send follow-ups to the cloud agent here.

Comment thread src/ui/tui/screens/SigningUpScreen.tsx
@kelsonpw

kelsonpw commented May 1, 2026

Copy link
Copy Markdown
Member

@cursor push eec3255

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Auth task fires duplicate signup after SigningUpScreen succeeds
    • Added s.signupAuth === null to the inline signup guard in src/commands/default.ts so the auth task skips the duplicate performSignupOrAuth call when SigningUpScreen has already populated signupAuth.
  • ✅ Fixed: Missing installDir in dashboardUrl forwarding test case
    • Added the missing installDir: '/tmp/wizard-test' property to the performSignupOrAuth call in the dashboardUrl forwarding test so it satisfies the required SignupOrAuthInput shape.

Create PR

Or push these changes by commenting:

@cursor push cfb8cf9832
Preview (cfb8cf9832)
diff --git a/src/commands/default.ts b/src/commands/default.ts
--- a/src/commands/default.ts
+++ b/src/commands/default.ts
@@ -767,7 +767,8 @@
                 isCreateAccountOnboarding(s) &&
                 s.signupEmail &&
                 s.signupFullName &&
-                !s.signupTokensObtained
+                !s.signupTokensObtained &&
+                s.signupAuth === null
               ) {
                 const { performSignupOrAuth } = await import(
                   '../utils/signup-or-auth.js'

diff --git a/src/utils/__tests__/signup-or-auth.test.ts b/src/utils/__tests__/signup-or-auth.test.ts
--- a/src/utils/__tests__/signup-or-auth.test.ts
+++ b/src/utils/__tests__/signup-or-auth.test.ts
@@ -220,6 +220,7 @@
       email: 'ada@example.com',
       fullName: 'Ada Lovelace',
       zone: 'us',
+      installDir: '/tmp/wizard-test',
     });
 
     expect(result.kind).toBe('success');

You can send follow-ups to the cloud agent here.

Comment thread src/commands/default.ts Outdated
Comment thread src/utils/__tests__/signup-or-auth.test.ts
kelsonpw and others added 2 commits May 1, 2026 16:17
… POST

SigningUpScreen already runs performSignupOrAuth and sets session.signupAuth
on success; the auth task must not POST again (duplicate account →
requires_redirect → spurious OAuth). Also skip the inline POST when
signupAbandoned is set. Add missing installDir in dashboardUrl forwarding test.

Co-authored-by: Cursor <cursoragent@cursor.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: EmailCapture isComplete doesn't account for accountCreationFlow skip
    • Updated EmailCapture's isComplete to also return true when accountCreationFlow is set, so the --signup path is semantically marked complete and back-navigation traversal isn't blocked.

Create PR

Or push these changes by commenting:

@cursor push c565e46cc8
Preview (c565e46cc8)
diff --git a/src/ui/tui/flows.ts b/src/ui/tui/flows.ts
--- a/src/ui/tui/flows.ts
+++ b/src/ui/tui/flows.ts
@@ -175,7 +175,9 @@
         !s.accountCreationFlow &&
         !s.emailCaptureComplete,
       isComplete: (s) =>
-        !isCreateAccountOnboarding(s) || s.emailCaptureComplete,
+        !isCreateAccountOnboarding(s) ||
+        s.emailCaptureComplete ||
+        s.accountCreationFlow,
       revert: (store) => {
         if (!isCreateAccountOnboarding(store.session)) return false;
         store.resetEmailCapture();

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 5bdadbe. Configure here.

Comment thread src/ui/tui/flows.ts
show: (s) =>
isCreateAccountOnboarding(s) &&
!s.accountCreationFlow &&
!s.emailCaptureComplete,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EmailCapture isComplete doesn't account for accountCreationFlow skip

Low Severity

EmailCapture's isComplete is !isCreateAccountOnboarding(s) || s.emailCaptureComplete. For the --signup path, isCreateAccountOnboarding is true but emailCaptureComplete is never set (the screen is hidden by !s.accountCreationFlow). isComplete returns false — the entry relies solely on show returning false to skip it. While the router's show-false-skip prevents this from blocking, the isComplete returning false is semantically incorrect for the --signup flow and could break back-navigation logic, since revert traversal walks isComplete separately from show.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5bdadbe. Configure here.

Comment thread src/ui/tui/store.ts
}

setSignupEmail(email: string): void {
this.$session.setKey('signupEmail', email);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.$session.setKey('signupEmail', email);
this.$session.setKey('signupEmail', email.trim());

Comment thread src/ui/tui/flows.ts
s.signupRequiredFields.includes('full_name')),
isComplete: (s) => !s.accountCreationFlow || s.signupFullName !== null,
revert: (store) => {
if (!store.session.accountCreationFlow) return false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is okay, but reverting will send you all the way back to region select as-is. Potentially an area for follow up

@kelsonpw kelsonpw removed the request for review from a team May 18, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants