chore: remove --classic mode#558
Conversation
The wizard had four mode branches: agent, CI, classic, and TUI.
Classic — interactive prompts without the rich Ink TUI — was a legacy
fallback for users who couldn't run the TUI; in practice the TUI works
everywhere classic worked, and we no longer need to maintain a parallel
prompt-based UX.
Removing classic collapses the mode handler to three branches and lets
us drop a couple of small bits of code that only existed to serve it:
- The public `--classic` yargs option in `default.ts` and the hidden
`AMPLITUDE_WIZARD_CLASSIC` env-var passthrough in `bin.ts`.
- The entire `else if (options.classic ...)` branch in the default
command handler (~30 lines), including its bespoke
`runDirectSignupIfRequested` onSuccess hook that called
`resolveCredentials({ requireOrgId: false })`.
- The `onSuccess` callback parameter on `runDirectSignupIfRequested`
itself — agent and CI never used it, so the helper is now a
straightforward fire-and-return.
- `!options.classic && AMPLITUDE_WIZARD_CLASSIC !== '1'` guards in the
agent-detect TTY check (no longer needed once the branch is gone).
Comment-only updates touch a handful of files where the mode tally
read "agent / CI / classic / TUI" — now "agent / CI / TUI".
Verified: pnpm tsc --noEmit, pnpm exec vitest run --pool=forks
--maxWorkers=1 (187 files / 2826 tests), pnpm lint all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
bugbot run |
kelsonpw
left a comment
There was a problem hiding this comment.
Review
Nice cleanup — removing a legacy mode is the right call and the shape (drop the branch, drop the onSuccess callback that only existed for it, comment-sweep the survivors) is clean.
Main concern: bin.ts shadow removal is likely a regression
The doc comment at bin.ts:465-468 says these hidden shadows exist so .env('AMPLITUDE_WIZARD') + .strict() don't crash on env-var injection. The benchmark-file comment further down (~554-558) is even more explicit: "Without these shadows, .env('AMPLITUDE_WIZARD') auto-maps them to argv keys that .strict() then rejects — crashing every command (including --help) the moment a developer has one of them exported in their shell."
Dropping the classic shadow means a user who currently has AMPLITUDE_WIZARD_CLASSIC=1 in their shell rc — which any prior classic-mode user plausibly does — will hit Unknown argument: classic on every wizard invocation. That contradicts the PR description's last (unchecked) smoke test, which expects the env var to silently fall through to TUI/agent.
Two options:
- Keep the shadow with a comment like
// Backwards-compat: --classic is removed but the env var lingers in users' rc files; this no-op shadow keeps yargs.strict() from crashing them.All call sites already ignoreoptions.classicpost-PR. - Actually run that smoke test (
AMPLITUDE_WIZARD_CLASSIC=1 pnpm try). If it crashes, go with option 1.
Worth adding a CLI test that asserts a stale AMPLITUDE_WIZARD_CLASSIC=1 env doesn't crash strict-mode parsing — nothing currently covers it.
Quiet behavior change worth noting in the description
src/commands/default.ts:246: pre-PR, a non-TTY env with AMPLITUDE_WIZARD_CLASSIC=1 would fall into the classic branch. Post-PR, the env var is no longer consulted there, so the same env auto-promotes to agent mode via isNonInteractiveEnvironment(). Probably the intended outcome, but anyone scripting around that env var will start getting NDJSON instead of pretty prompts.
Minor / nits
-
src/commands/helpers.ts:705— the deleted comment block ("Scope this catch narrowly so anonSuccessthrow below cannot re-emit telemetry...") captured a real invariant aboutwrapper_exceptionnot being double-emitted. WithonSuccessgone the comment is moot, but consider keeping a one-liner — e.g.// Narrow catch: only the wrapper itself; performSignupOrAuth has already emitted its own success/failure telemetry.Otherwise a future refactor won't know why the catch is so tight. -
src/lib/agent-runner.ts:746-747—TUI fallbackis a bit ambiguous as a phrase. Worth specifying which fallback (e.g. "the rare TUI-startup-failure fallback that drops into agent-runner viagetOrAskForProjectData", matching CLAUDE.md's language). -
src/commands/helpers.ts— onceonSuccessis gone, both remaining callers ofrunDirectSignupIfRequested(default.ts:279and:312) pass the same'cached-token resolution'string. ThefallbackLabelparameter is effectively a constant now. Could be inlined in a follow-up — fine to leave as a knob in case CI vs agent diverge later.
Tests
- 187 files / 2826 tests pass per the PR description ✅
- No coverage for the strict-mode env-var graceful-fallthrough claim — see point 1.
Verdict
Low-risk in spirit; the bin.ts shadow removal is the one place this could bite real users. Keep the shadow or verify the smoke before merge.
|
Following up on the top-level review concern about the Concrete fixRestore the shadow as a no-op in 'install-dir': {
hidden: true,
describe: 'internal: AMPLITUDE_WIZARD_INSTALL_DIR env-var passthrough',
type: 'string',
},
// Backwards-compat: --classic was removed but the env var lingers in
// users' rc files; this no-op shadow keeps yargs.strict() from rejecting
// AMPLITUDE_WIZARD_CLASSIC when .env('AMPLITUDE_WIZARD') auto-maps it.
classic: {
hidden: true,
describe: 'internal: AMPLITUDE_WIZARD_CLASSIC env-var passthrough (no-op)',
type: 'boolean',
},
// The `apply` subcommand spawns a child wizard process and passes theOptional: pin the contract with a one-linerWherever the existing CLI tests already shell out to the wizard binary: it('does not crash strict mode on stale AMPLITUDE_WIZARD_CLASSIC=1 env', () => {
const result = spawnSync('node', [BIN, '--help'], {
env: { ...process.env, AMPLITUDE_WIZARD_CLASSIC: '1' },
});
expect(result.status).toBe(0);
expect(result.stderr.toString()).not.toMatch(/Unknown argument/);
});Belt-and-suspenders, but locks in the invariant so the next person dropping a shadow doesn't repeat it. (Side note: I tried to leave this as an inline |
|
Honestly i don't really care if the CLI breaks if you pass --classic, i doubt anyone uses it. Feel free to ignore. |
Main's #558 (`chore: remove --classic mode`) deleted the `--classic` flag and `AMPLITUDE_WIZARD_CLASSIC` env-var entirely. The classic-mode workaround in `buildSessionFromOptions` (added in PR #535's bugbot fix to avoid stripping signup flags in classic-mode TTY runs) is now reading flags that no longer exist — `Boolean(options.classic)` and `process.env.AMPLITUDE_WIZARD_CLASSIC === '1'` are always false, so the gate is dead code. Drop the gate and the two tests that exercised it. The remaining three tests (TUI / CI / agent mode) still cover the live behavior: TUI strips signup flags, non-interactive modes honor them. 2911 / 2911 tests pass; tsc clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat: parse needs_information arm in direct-signup
The agentic provisioning endpoint can return a `needs_information` response
when the email belongs to a new user but the request didn't include the
fields the server needs to create the account (today: `full_name`). This
arm lives at amplitude/javascript: server/packages/thunder/src/lib/
agentic-provisioning/wizard/signup.ts and is gated by the
AgenticWizardNewUserSignup feature flag.
Until now the wizard parsed only the oauth / requires_auth / error arms
and reported `needs_information` as "Unexpected response" — fail-closed,
not actionable.
Changes:
- `NeedsInformationSchema` parses `{ type: 'needs_information',
needs_information: { type: 'object', properties, required: [...] } }`.
`properties` is preserved as opaque (record of unknown) so future
field additions don't fail-closed the parse; only `required` is
consumed.
- `DirectSignupResult` gains `{ kind: 'needs_information',
requiredFields: string[] }`. Doc-comment guidance for callers updated.
- `DirectSignupInput.fullName` is now optional. The request body conditionally
includes `full_name` only when set, matching the server's
`full_name: z.string().min(1).optional()` schema. Sending an empty
string would either be rejected or silently accepted depending on
coercion — neither is what we want.
- Tests cover the new arm, the email-only POST shape, and the
full-name-included POST shape.
This is purely additive in production: `signup-or-auth.ts` always passes
`fullName`, so the new branches are unreachable until that wrapper is
updated to support email-only probing in a follow-up PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): handle needs_information arm in signup-or-auth wrapper
Adding `needs_information` to the `DirectSignupResult` union (prior
commit) breaks type narrowing in `performSignupOrAuth`, which previously
only filtered out `requires_redirect` and `error` before reaching for
`result.tokens`. Without this fix, `tsc --noEmit` errs at the success-arm
property accesses.
The wrapper today is single-shot and can't collect missing fields — it's
called from non-TUI paths where both `email` and `fullName` are gated
upstream by the caller. Reaching the `needs_information` branch from
this wrapper means the wizard sent a valid `full_name` but the server
still asked for more (a future-field case the wrapper isn't equipped
to handle). Treat it as a redirect-style fallback: log, emit a
`needs_information` telemetry status, return null. The TUI path that
handles `needs_information` properly — collect the missing field via a
screen and re-POST — comes in a follow-up PR (server-driven field
collection).
Also adds `'needs_information'` to `SignupAttemptStatus` so this
outcome surfaces distinctly from `requires_redirect` in the agentic
signup attempted event.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(signup): clarify TUI-vs-non-interactive scope of signup CLI flags
The signup-related CLI flags `--auth-onboarding`, `--email`, and
`--accept-tos` were originally designed for non-interactive (`--ci` /
`--agent`) modes — there's no TUI to render the Intro picker, signup-email
screen, or ToS screen, so the flags carry that information instead.
In interactive TUI runs they're functionally inert today: the Intro
picker overwrites whatever `--auth-onboarding` set; the EmailCaptureScreen
silently pre-fills from `--email` and the ToS screen silently pre-accepts
from `--accept-tos`, both bypassing user-facing confirmation steps that
exist for a reason. `--help` doesn't signal any of this — every flag
appears in one flat Options block with no mode scope.
This PR cleans both up:
1. Tighten the `--help` describe text so each flag explicitly says
"(ignored in interactive TUI; …)". `--auth-onboarding` says the Intro
menu is canonical; `--email` says the email screen always renders;
`--accept-tos` says the ToS screen always renders.
2. Drop the three flags at session-build time when the resolved
execution mode is `interactive`. `buildSession` gains an optional
`executionMode` parameter and a single doorstep override that strips
`authOnboardingPath` / `authOnboarding` / `signup` /
`accountCreationFlow` / `signupEmail` / `acceptTos` before any
downstream resolver sees them. Every later read site (zod parse,
`resolveAuthOnboardingPathFromArgs`, default assignment) treats them
as unset.
3. `--full-name` is intentionally NOT gated. Pre-fill is just metadata
and bypasses no confirmation step — power users can keep using
`--full-name "Jane Doe"` to skip the name screen in TUI.
4. `buildSessionFromOptions` (the per-command entry point) resolves the
mode via `resolveMode` and threads `executionMode` through to
`buildSession`. The per-command callers (`apply`, `default`, `region`,
`mcp`, etc.) inherit the gating automatically.
5. When `executionMode` is omitted, `buildSession` preserves the legacy
"honor every flag" contract so existing callers (TUI store init,
tests, anything not yet routed through `buildSessionFromOptions`)
keep working unchanged.
Tests cover all four arms (interactive ignores each flag individually,
ci/agent honor every flag, omitted-mode preserves legacy behavior, and
`--full-name` is honored in interactive). 47 wizard-session tests pass;
2834 unit tests + 100 BDD scenarios green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): preserve signup flags in classic mode
Classic mode (--classic / AMPLITUDE_WIZARD_CLASSIC=1) is interactive in
the prompt sense — it uses terminal prompts via @clack/prompts — but it
does NOT have the Ink TUI screens (Intro picker / EmailCaptureScreen /
ToSScreen) that own signup-flag UX in real TUI mode. Classic also has
its own direct-signup call site at default.ts:348 that depends on
--auth-onboarding / --email / --full-name populating the session.
`resolveMode` knows nothing about --classic, so it sees a TTY without
--ci/--yes/--force/--agent and classifies that as 'interactive'. The
prior commit on this branch then made `buildSession` strip
authOnboardingPath / signupEmail / acceptTos in interactive mode —
silently regressing classic-mode direct signup. The classic branch's
runDirectSignupIfRequested call would no-op because
accountCreationProvisioningInputsReady checks all three of the now-
stripped fields, and the wizard would fall through to performAmplitudeAuth
(browser OAuth) — defeating the point of passing --auth-onboarding
create-account --email X --full-name Y in the first place.
Fix: in `buildSessionFromOptions`, detect --classic / AMPLITUDE_WIZARD_CLASSIC
and omit `executionMode` so `buildSession` falls through to its
legacy "honor every flag" path. Real TUI mode keeps the gating; classic,
ci, agent all keep their flags.
The abstraction in the prior commit assumed `executionMode === 'interactive'`
was a proxy for "the Ink TUI owns the signup screens" — which holds for
real TUI mode but not classic. Either the classic-detection at the call
site (this commit) or a finer ExecutionMode union (e.g. add 'classic')
would solve it; the call-site approach is minimal and keeps the existing
ExecutionMode contract unchanged.
New test file `src/commands/__tests__/build-session-from-options.test.ts`
covers the four-axis matrix (classic flag, classic env var, real TUI,
ci, agent) so this can't regress silently again. Caught by Cursor Bugbot
on PR #535.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(signup): semantic mode scoping in --help describe text
The describe text on `--auth-onboarding` and `--email` previously phrased
TUI gating as a parenthetical ("...ignored in interactive TUI; the menu
is canonical"). That reads as an exception to a default of "supported
everywhere", which is the wrong mental model — these flags exist
specifically for non-interactive entry points.
Rewrite as a positive enumeration of supported modes followed by an
explicit "ignored otherwise" so --help readers get the scope at a
glance:
Supported in --agent, --ci, and --classic modes; ignored otherwise.
`--accept-tos` is intentionally left as-is (its TUI-only ToS screen
behavior is materially different from the other two flags' behavior, so
the bespoke wording earns its keep). `--full-name` also unchanged — it's
honored in every mode so the supported/ignored framing doesn't apply.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(signup): tighten --help describe text for signup CLI flags
Refine the describe text on --auth-onboarding, --email, and --accept-tos
so each flag's intended scope is the focal point rather than the
gating exception:
- --auth-onboarding: phrase as "intended for use in --agent and --ci
modes; interactive modes prompt for selection" — clearer that the
Intro picker is the canonical interactive UX.
- --email: name the modes it's meant for inline ("in --agent or --ci
mode") instead of a trailing supported/ignored clause.
- --accept-tos: drop the parenthetical about TUI-only ToS rendering;
the inline mode scope already conveys it.
No behavior change. Just shorter, more direct help text.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(signup): wrapper discriminated union + ceremony session fields
Foundation for server-driven signup field collection in the TUI. No
user-visible behavior change yet — wires up the data model and the
contract the SigningUpScreen will consume.
**Wrapper refactor (`src/utils/signup-or-auth.ts`):**
`performSignupOrAuth` previously returned `T | null`, conflating four
distinct outcomes — success, redirect, error, and "not enough input
provided". Callers that wanted to distinguish them had to look at side
effects (telemetry events) or guess. The new shape is a discriminated
union mirroring `performDirectSignup`:
| { kind: 'success'; idToken; accessToken; ...; userInfo; dashboardUrl }
| { kind: 'needs_information'; requiredFields: string[] }
| { kind: 'redirect' }
| { kind: 'error'; message: string }
The wrapper now also accepts a missing `fullName` and probes the server
with email-only — required for the TUI's two-POST ceremony where the
first POST asks "is this user new?" and the server's needs_information
response decides what to collect next. Non-TUI callers (CI / agent /
classic) still gate on both being present upstream and never hit the
needs_information arm in practice.
`wrapper_exception` is now a distinct telemetry status (was previously
collapsed into `signup_error`), letting orchestrators distinguish a
thrown network/transport failure from the server's clean error arm.
**Caller updates:**
- `src/commands/default.ts` — TUI auth task narrows on `kind === 'success'`
and extracts the four token fields explicitly. needs_information /
redirect / error all fall through to the existing OAuth path with no
behavior change vs. today's null return.
- `src/commands/helpers.ts::runDirectSignupIfRequested` — same narrowing,
with a slightly more informative log message that names the non-success
arm.
**Session fields (`src/lib/wizard-session.ts`):**
Three new fields on `WizardSession` for the upcoming SigningUpScreen
ceremony:
- `signupRequiredFields: string[] | null` — set by SigningUpScreen on
`needs_information`; collection screens render iff their field key is
present.
- `signupAuth: { idToken; accessToken; refreshToken; zone; userInfo;
dashboardUrl } | null` — set on the success arm; drives the auth
task gate (next commit).
- `signupAbandoned: boolean` — set on redirect / error; releases the
auth gate to fall through to OAuth.
Plus matching setters / a single `resetSignupCeremony` resetter on
`WizardStore` for back-nav from collection screens.
**Tests:**
Existing signup-or-auth tests updated to assert the new discriminated
union (`result.kind === 'redirect'` instead of `result === null`, etc.)
plus a new test covering the email-only probe path. cli.test.ts mock
default switched from `vi.fn()` (returns undefined) to
`vi.fn().mockResolvedValue({ kind: 'error' })` so existing test paths
that don't override the mock still narrow safely.
2842/2842 unit tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(signup): server-driven field collection screens + flow rewire
Replaces the legacy EmailCaptureScreen + always-on ToSScreen with a
three-screen ceremony driven by the agentic provisioning endpoint's
needs_information response arm.
**Why**
Today's signup flow asks for email + name + ToS up front, then POSTs.
That means an existing customer (whose email is already on file) is
forced to provide name and ToS before the server can tell them "you
need to log in via the browser instead" — wasted prompts. The server
already knows whether agentic signup is happening based on email alone;
this PR moves the ceremony to match.
**Flow shape**
SignupEmail — collect email
↓
SigningUp — POST email-only; route on response
↓ ↓ ↓
oauth needs_information requires_auth / error
(success) (server asked for (existing user OR
more) server rejected)
↓ ↓ ↓
Auth ToS → SignupFullName → SigningUp (re-POST) → Auth
↓
(fall through to
browser OAuth via
signupAbandoned)
The router's flow-pipeline predicates implement the routing — there's
no imperative branching, no MAX_POSTS counter. SigningUp's `show`
predicate goes false the moment needs_information arrives (because
required fields are unsatisfied), the router walks to the collection
screens, and SigningUp re-shows once the fields are filled. Returning
users hit redirect → signupAbandoned → fall through, never seeing ToS
or the name prompt.
**New screens** (`src/ui/tui/screens/`)
- `SignupEmailScreen.tsx` — passive: TextInput → store.setSignupEmail.
Replaces EmailCaptureScreen's first step. Validates email format,
shows error inline. Esc rewinds to Welcome.
- `SigningUpScreen.tsx` — coordinator: only screen with network I/O.
useAsyncEffect calls performSignupOrAuth, then writes ONE of
`signupAuth` / `signupRequiredFields` / `signupAbandoned` based on
the discriminated-union response. fullName is included in the POST
only when ToS is accepted (avoids creating an account before the
user confirms ToS).
- `SignupFullNameScreen.tsx` — passive: TextInput → store.setSignupFullName.
Renders only when `signupRequiredFields` includes 'full_name' AND
the session doesn't already have a name (e.g. from --full-name).
Esc clears email + ceremony state so the user can correct a typo.
**Flow rewire** (`src/ui/tui/flows.ts`)
Replaces the EmailCapture + ToS create-account block with four entries
(SignupEmail / SigningUp / ToS / SignupFullName) and matching show /
isComplete / revert predicates. ToS now gates on `signupRequiredFields
!== null` so it ONLY renders after the server confirmed agentic signup.
Screen enum gains `SignupEmail`, `SigningUp`, `SignupFullName`. The
old `EmailCapture` enum value and screen file are removed.
**Auth task gate** (`src/commands/helpers.ts::isAuthTaskGateReady`)
Was: gates on tosAccepted on the create-account path.
Now: gates on `signupAuth !== null || signupAbandoned`. The auth task
runs concurrently with the screens, so without this gate it would race
SigningUpScreen and open a browser OAuth tab while the screen-driven
POST is still in flight. New unit tests cover both released arms
(signupAuth captured, ceremony abandoned to OAuth) plus the blocked
state.
**JourneyStepper** stays accurate — Auth-section screen list updated to
include the three new screens.
**BDD scenarios** updated in `features/02-wizard-flow.feature` to
reflect the new ceremony order. Added a new step
`the signup probe returns needs_information for "<field>"` that
mirrors what SigningUpScreen writes on that response arm. 100/100
scenarios pass.
**Tests**
- 2844 unit tests pass (router + flow-invariants + auth-gate + screens)
- 100 BDD scenarios pass
- Typecheck + lint clean
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(signup): router resolution coverage for the four ceremony shapes
11 new router tests pinning down each step of the create-account
ceremony's flow shapes:
Shape 1 (no flags) : SignupEmail → SigningUp → ToS →
SignupFullName → SigningUp → Auth
Shape 2 (--email only) : SigningUp → ToS → SignupFullName →
SigningUp → Auth
Shape 3 (email + full_name) : SigningUp (probe) → ToS →
SigningUp (success) → Auth
Shape 4 (requires_redirect) : SigningUp → Auth (browser OAuth via
signupAbandoned)
Each `it` exercises ONE router resolution along the path so a
predicate-level regression (wrong show condition, missing isComplete
arm) fails one specific test instead of fanning out across the whole
flow. Plus a sign-in-path negative test asserting none of the new
screens render when authOnboardingPath !== 'create_account'.
These complement the existing flow-invariants property tests (which
cover global properties like "router never resolves SignupEmail when
authOnboardingPath is sign_in") with concrete step-level pins.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): skip TUI auth task's wrapper call after ceremony abandon
After PR 3, `isAuthTaskGateReady` releases on either `signupAuth !== null`
or `signupAbandoned` (so a SigningUpScreen abandon to OAuth doesn't leave
the auth task waiting forever). On the abandon path the auth task body's
direct-signup gate at default.ts:761-766 then sees:
isCreateAccountOnboarding ✓
signupEmail ✓ (set by SignupEmailScreen)
signupFullName ✓ (set by SignupFullNameScreen)
!signupTokensObtained ✓ (no tokens — the screen abandoned)
…and re-POSTs the same args that SigningUpScreen just got redirect/error
on. Same response, falls through to OAuth, but burns an extra round-trip
and double-counts the telemetry event for the attempt.
Practical impact is bounded — the user's outcome is the same — but it's
a clean regression to fix. Add `!s.signupAbandoned` to the gate so the
abandon path goes straight to the OAuth fallback below.
Caught by Cursor Bugbot on PR #539. The architectural cleanup the smell
points at (auth task should read `signupAuth` directly instead of using
`signupTokensObtained` as a proxy + reading from disk) is deferred —
this fix is the minimal correct change matching the surrounding style.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): correct needs_information schema shape to match server
The agentic provisioning endpoint wraps the JSON-Schema payload in an
extra `schema` field. The earlier zod schema put `type` / `properties` /
`required` directly under `needs_information`, so every probe POST
fell through to the generic "unrecognized response shape" error and
silently routed users to OAuth — the same symptom reported live: a
new email gets `needs_information`, but the user is sent to the
redirect link instead of a name prompt.
Verified via direct curl against `https://app.amplitude.com/t/agentic/signup/v1`
on 2026-05-06:
{
"type": "needs_information",
"needs_information": {
"schema": {
"type": "object",
"properties": { "full_name": { ... } },
"required": ["full_name"]
}
}
}
Update `NeedsInformationSchema` to expect the `schema` wrapper, and
update the read site to drill `parsedNeeds.data.needs_information.schema.required`.
Test fixtures updated to mirror the real shape (an inline comment now
points at the curl-verified date so future drift gets caught).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(signup): pin properties-value opacity in needs_information schema
Document and lock the wire contract: each `properties` value in the
needs_information JSON-Schema is opaque to the wizard. The server today
emits `{ type, description }` per property, but `description` is purely
cosmetic and may be removed (or any other inner field added) without
coordination — we only consume `required`.
The schema already uses `z.unknown()` for property values, so the
behavior is correct today. This commit:
1. Extends the schema's leading comment to call out the opacity
contract explicitly, with a "do not tighten without re-verifying
against the live server" guard.
2. Adds a regression test (`accepts properties values with or without
optional metadata`) that parses a response with `{type}` (no
description) AND an entirely empty `{}` property value. Pins the
looseness so a future "tighten the inner shape" change can't
accidentally break wire compatibility — the server could remove
`description` tomorrow and the wizard would still parse.
22/22 direct-signup tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): bind ceremony reset to setSignupEmail(null); guard analytics on clears
Two related back-nav holes addressed in one setter rewrite:
**1. Stale ceremony state across back-nav (the bigger one)**
The signup ceremony has shared state — `signupRequiredFields`,
`signupAuth`, `signupAbandoned` — that's set by `SigningUpScreen` (from
the server response) and consumed by multiple downstream entries' `show`
predicates (`ToS`, `SignupFullName`, the re-show of `SigningUp`).
The per-entry `revert` callbacks only un-did the field that *they* set.
None cleared the shared ceremony state. Concrete failure mode:
1. User on `ToS`, presses Esc → router walks back to `SignupEmail`,
calls its revert → `signupEmail` set to null.
2. `signupRequiredFields` stays `['full_name']` from the prior probe.
3. User retypes any email → `SigningUp.show` is false because
the gate `(signupRequiredFields === null || tosAccepted === true)`
fails (tosAccepted got reset between steps).
4. Router lands the user back on `ToS` without a fresh probe POST —
consuming the stale `needs_information` against a possibly
different email.
Bind the ceremony reset to `setSignupEmail(null)` itself: any path
that rewinds the user back to an empty email state automatically
invalidates the prior probe. The previous `resetSignupCeremony`
helper is removed (its only caller was `SignupFullNameScreen`'s Esc
handler, which now gets the reset for free).
**2. False "captured" analytics on back-nav (Cursor Bugbot finding)**
PR #539 widened `setSignupEmail` and `setSignupFullName` to accept
`null` so revert handlers could clear the values. The setters fired
`'signup email captured'` / `'signup full name captured'` analytics
events unconditionally — including on `null` clears with `{ 'has email':
false }` payloads. That polluted the top-of-funnel "captured" metric
with non-capture events (every Esc-out emitted one).
Fix: guard the analytics calls behind `value !== null`. Drop the
now-always-true `'has email'` / `'has name'` properties at the same
time — they were defensive payloads that became meaningless once the
event only fires on positive captures.
**Tests**
5 new store tests pin the contract:
- `setSignupEmail(string)` fires analytics; `setSignupEmail(null)` does not.
- `setSignupEmail(null)` resets `signupRequiredFields`, `signupAuth`,
`signupAbandoned`.
- Same shape for `setSignupFullName`.
2861 unit tests pass (was 2856 + 5 new); typecheck + lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): make ToS / SignupFullName reverts return false when skipped
Closes the abandon-path back-nav trap. Walks the user past entries
that were "complete because they were skipped" rather than firing a
no-op revert that stops the walk on a state change that doesn't exist.
**The bug**
After `requires_redirect → signupAbandoned=true → browser OAuth`, the
user is on `Auth`. Pressing Esc:
1. Walk back from Auth-1 (`SignupFullName`).
2. `SignupFullName.isComplete` returns true via the
`signupRequiredFields === null` arm — it was skipped because the
server never asked for `full_name`.
3. `SignupFullName.revert` fires → `setSignupFullName(null)`. But
signupFullName was never set; the call is a no-op.
4. Revert returns void (truthy) → router stops walking.
5. Forward resolve: SignupFullName.show is false; everything else
skips back to `Auth`.
User pressed Esc and the screen didn't change. `ToS.revert` had the
same shape (`tosAccepted` was never set, `resetToS` was a no-op,
walk stopped on a phantom revert).
**The fix**
Both reverts now return `false` when `isComplete` is satisfied via
the "screen was skipped" arms — there's nothing to undo, so the
back-walk should continue.
- `ToS.revert`: returns false when `signupRequiredFields === null`
OR `tosAccepted === null`.
- `SignupFullName.revert`: returns false when
`signupRequiredFields === null` OR
`!signupRequiredFields.includes('full_name')` OR
`signupFullName === null`.
In the abandon path, both return false in sequence; the walk reaches
`SignupEmail`, whose revert clears email + ceremony state (per the
prior fix), and forward resolve correctly lands the user on
`SignupEmail` with everything reset.
In the success path, `SignupFullName.revert` IS meaningful
(signupFullName was set), so the walk stops there. The user lands on
SignupFullName and re-types — known leaky because the server-side
account already exists, but at least back-nav is responsive. Tracked
separately.
**Tests**
Two new router tests:
- `Esc on Auth in the abandon path walks past skipped signup screens
to SignupEmail` — the regression that motivated this fix.
- `Esc on Auth in the success path walks back to SignupFullName` —
pins the known-leaky case so we notice if it changes.
Stub store extended to mirror the production `setSignupEmail(null)`
ceremony-reset contract so the test's back-walk produces a faithful
forward-resolve.
2863/2863 unit tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): backToWelcome must clear ceremony state alongside signupEmail
`backToWelcome` writes `signupEmail = null` via raw `$session.setKey` —
bypassing `setSignupEmail(null)`'s ceremony-reset contract that commit
2e521d16 bound to the setter. Result: a user mid-ceremony who hits Esc
back to Welcome leaves stale `signupRequiredFields` / `signupAuth` /
`signupAbandoned` on the session.
Failure mode:
1. User submits email; server returns needs_information; session has
signupRequiredFields=['full_name'].
2. User Esc's back to Welcome.
3. User restarts with a fresh email — but signupRequiredFields is
still set, so SigningUp.show is false and ToS lands directly with
a stale required-fields cache. Or worse: if the user's already
hit the success arm (signupAuth populated), the auth gate
releases on stale tokens.
`backToWelcome` can't *call* `setSignupEmail(null)` here because it
batches multiple raw setKey writes and emits one change at the end —
calling the setter mid-batch would emit twice. Inline the ceremony
reset directly, with a comment pointing back to `setSignupEmail`'s
contract so the two stay in sync.
The existing test (`store.test.ts:933`) didn't assert any of the new
ceremony fields, so it passed despite the broken contract. Extended
to cover (a) the standard mid-ceremony Esc-back case and (b) the
edge case where signupAuth is populated before backToWelcome fires.
152 store tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(signup): remove EmailCaptureScreen vestige
PR #539 deleted EmailCaptureScreen.tsx but left behind:
- `WizardStore.markEmailCaptureComplete` / `resetEmailCapture` — the
setter/resetter pair the deleted screen called. No production
callers remain; only the BDD step defs and the legacy
`backToWelcome` write referenced them.
- `WizardSession.emailCaptureComplete` — a boolean that was the
isComplete signal for the old EmailCapture FlowEntry. The new flow
gates on `signupEmail !== null` directly (`SignupEmail.isComplete`
in flows.ts). The field is now write-only — never read.
- `'email capture complete'` and `'back navigation' { to:
'email-capture' }` analytics events emitted by those setters.
- BDD step defs in `wizard-flow.steps.ts` set `emailCaptureComplete =
true` after Then-actions and asserted on it in `the signup flow
should switch to regular auth`. Tautological under the new gate;
replaced with a meaningful `signupEmail === null` assertion.
Removing all four lets the next reader navigate the create-account
section without false leads. The `'tosAccepted'` clears in
`backToWelcome` stay — they're still load-bearing.
`signupTokensObtained` doc-comment refreshed: it used to point at
`EmailCaptureScreen`; now points at the SigningUp ceremony with a
note on why we keep it as a flag separate from `signupAuth`.
2864 unit tests pass; 100 BDD scenarios pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(signup): drop empty wizardCapture args + tighten --full-name describe
**Empty `{}` props on `wizardCapture`**
`SignupEmailScreen` and `SignupFullNameScreen` Esc handlers fired
`analytics.wizardCapture('signup ... screen back', {})` — the empty
object adds nothing the API needs. Other call sites in this codebase
omit the second arg when there are no properties; matching that style.
**`--full-name` describe text**
Pre-PR-535 the describe said "requires --auth-onboarding create-account".
Post-PR-535 `--auth-onboarding` is silently ignored in interactive TUI
(the Intro menu is canonical) — so the "requires" framing is false on
that path. Rewrite to acknowledge both:
Required in --agent or --ci mode; in interactive TUI it pre-fills
the name screen as a metadata-only shortcut.
Mirrors the language used on `--email` and `--accept-tos` post-PR-535.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(default): document auth-task gate's settle dependency
The TUI auth task's `await isAuthTaskGateReady` now depends on
`SigningUpScreen` writing `signupAuth` or `signupAbandoned`. That's a
non-obvious data dependency — the auth task lives in default.ts and
the screen lives in src/ui/tui/screens/, separated by 1500+ lines of
infrastructure.
Add an inline block explaining the three settle paths
(success / non-success / timeout-then-abandon) and the load-bearing
invariant: don't remove `REQUEST_TIMEOUT_MS` from `direct-signup.ts`
without re-thinking this gate. A raw async-effect with no timeout
would let the gate hang forever on a network hang, with no way for
the user to force a settle short of `/exit`.
No behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(signup): sync flows.md to the server-driven ceremony
CLAUDE.md calls out flows.md as source of truth for the wizard's UX
and asks for it to be updated *first* on flow changes. PR #539
shipped the new ceremony screens and shipped the code-side rewire,
but the docs side was missed. This catches up.
**Mermaid diagram (flows.md + wizard-flow.mmd):**
Replaced the EmailCapture → ToS → Auth chain with the
server-driven path:
SignupEmail
↓
SigningUp (probe POST)
↓
┌──────────┬──────────────────┬─────────┬─────────────────┐
│ oauth │ requires_auth │ error │ needs_information │
↓ ↓ ↓ ↓
Auth Auth Auth ToS → SignupFullName → SigningUp (re-POST) → Auth
The diagram makes explicit that ToS and SignupFullName only render
on the `needs_information` arm — the regression behind PR #539's
existence (forcing ToS+name on existing-user redirects).
**Esc / back-nav table:**
Rewrote per-screen rows for the four new ceremony screens.
SignupFullName / ToS rows note the "rewinds to SignupEmail with
ceremony state cleared" behavior bound by `setSignupEmail(null)`.
Added a SigningUp row noting the `revert: () => false` transparent
walk-past contract. Added an "invariants" subsection capturing:
- `setSignupEmail(null)` clears all ceremony state.
- `SignupFullName.revert` / `ToS.revert` return `false` on the
"screen was skipped" arms.
- `SigningUpScreen` is the only signup screen with network I/O.
**Side-effect updates:**
`pnpm flows` regenerated three other .mmd files where flows.md had
drifted (data-setup-flow title, framework-detection-flow plan-step
rewrite, susi-flow ampli-migration wording). Including them here
because they're "sync the diagrams to the prose" output of the same
command — leaving them stale would just mean the next person who
runs `pnpm flows` ships them with their unrelated PR.
The renderer also spat out a misnamed `back-navigation.mmd` that
was a near-duplicate of `top-level-commands.mmd` (the renderer's
section detector matched my new "Back navigation" subsection
header). Deleted — bug to track separately if it recurs.
`pnpm flows` re-runnable cleanly. Tests + BDD still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: drop --classic-mode workaround now that --classic is gone
Main's #558 (`chore: remove --classic mode`) deleted the `--classic`
flag and `AMPLITUDE_WIZARD_CLASSIC` env-var entirely. The
classic-mode workaround in `buildSessionFromOptions` (added in PR
#535's bugbot fix to avoid stripping signup flags in classic-mode
TTY runs) is now reading flags that no longer exist —
`Boolean(options.classic)` and `process.env.AMPLITUDE_WIZARD_CLASSIC
=== '1'` are always false, so the gate is dead code.
Drop the gate and the two tests that exercised it. The remaining
three tests (TUI / CI / agent mode) still cover the live behavior:
TUI strips signup flags, non-interactive modes honor them.
2911 / 2911 tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): treat needs_information shapes other than ['full_name'] as terminal
The wizard's TUI ceremony has exactly one collection screen
(`SignupFullNameScreen`). The flow predicates and the post-response
plumbing assume the server's `needs_information.schema.required` is
either empty (probe-only) or the literal `['full_name']`. Any other
shape — an unknown field name, a mix, or an empty array — drops the
user into a UI freeze on the SigningUp spinner: the every() in
`SigningUp.show` returns true for unknown fields (the ternary's
else-branch is `true` for anything not equal to `'full_name'`), so
the screen re-mounts and re-POSTs once more, then sits forever
because no collection screen is showable for a field we don't know.
Today's blast radius: low — server only sends `['full_name']`. But
every wizard release in the wild is one server change away from
silently freezing standard-mode runs. This commit makes the
contract explicit at the wrapper layer.
**Behavior**
`performSignupOrAuth` now validates the `requiredFields` array
shape against an allowlist (currently `['full_name']`). On match,
behavior is unchanged. On any other shape — including extra
fields, missing fields, empty array, substituted field — the
wrapper returns `{ kind: 'error', message: ... }` and emits a new
`needs_information_unsupported` telemetry status. The screen's
existing `error` handling routes the user to the OAuth fallback via
`signupAbandoned`, the same path it takes on `requires_redirect`.
The schema layer (`direct-signup.ts`) stays intentionally loose —
`z.unknown()` for property values, `min(1)` on `required` to reject
malformed empties at the wire layer. Wrapper handles the semantic
"do we know how to act on this" gate.
**Why a distinct telemetry status?**
`needs_information_unsupported` is separate from `signup_error` so
the wire-contract drift is visible in the funnel. If this status
starts firing in production, it's a hard signal that the server has
added a required field the wizard doesn't yet handle — one
dashboard query reveals the drift before users start filing tickets.
**Tests**
Five new wrapper tests pin the contract:
- 4× `it.each` covering unknown-only / mixed / empty / substituted
shapes — assert error return + `needs_information_unsupported`
telemetry.
- 1× passes-through test for the supported `['full_name']` shape —
asserts unchanged behavior + regular `needs_information`
telemetry.
20/20 signup-or-auth tests pass. 2916/2916 unit tests pass overall.
Typecheck + lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(signup): move needs_information shape gate to the zod schema
Last commit's wrapper-level allowlist worked but split the wire-shape
contract across two layers — `direct-signup.ts` parsed any well-formed
`needs_information` arm; `signup-or-auth.ts` then re-validated the
`required` content against `['full_name']`. Cleaner to enforce in one
place: the schema.
**The change:**
`NeedsInformationSchema.required` gains a `.refine()` constraining it
to exactly `SUPPORTED_REQUIRED` (currently `['full_name']`). Anything
else fails the parse. To preserve the distinct telemetry signal —
"server changed contract" vs. "server returned garbage" —
`direct-signup.ts` now peeks at `response.data.type` after the parse
chain and, if it's `'needs_information'`, returns
`{ kind: 'error', code: 'unsupported_required_shape', message: ... }`
instead of falling through to the generic "Unexpected response"
error. The wrapper maps that code to
`needs_information_unsupported` telemetry.
**Why this shape:**
- The schema is now the single source of truth for the supported
`required` shape. To extend (e.g. add a `SignupCompanyScreen`),
update `SUPPORTED_REQUIRED` once. The wrapper has nothing to know.
- Schema's overall contract stays "loose by design" everywhere it
matters — `properties` values remain `z.unknown()`, the JSON-Schema
`description` field stays optional. The refine targets only the
semantic axis where the wizard's UX is actually opinionated.
- Type-aware fall-through means a real malformed response (e.g. body
doesn't even have `type`) still surfaces as plain "Unexpected
response" → `signup_error` telemetry, distinct from the contract-
drift signal.
**Tests**
- `direct-signup.test.ts`: 5 new schema-rejection cases (table) plus
one positive shape pin. All exercise the real schema via MSW.
- `signup-or-auth.test.ts`: prior table-driven wrapper cases collapse
to two — one mapping `code: 'unsupported_required_shape'` →
`needs_information_unsupported` telemetry, one verifying generic
errors stay on `signup_error`.
2919/2919 unit tests pass. Typecheck + lint clean. Wrapper drops 22
lines; schema-layer adds 19 lines + the refine.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): thread AbortSignal through performSignupOrAuth → axios
`SigningUpScreen.useAsyncEffect`'s `signal` was checked AFTER the
wrapper returned, but the wrapper itself didn't accept or honor the
signal — so the in-flight axios POST kept running after unmount, and
on the success arm `replaceStoredUser` wrote tokens to disk even when
the user had already navigated away. Net: `/exit` mid-POST left the
user "signed in" on the next launch (Intro picker hid the
create-account option) without their having completed the ceremony.
**Plumbing**
- `DirectSignupInput` and `SignupOrAuthInput` gain an optional
`signal: AbortSignal`.
- `performDirectSignup` passes it to both `axios.post` calls
(provisioning + token exchange) so axios cancels in-flight requests
on abort.
- After each axios call, an `if (input.signal?.aborted)` guard returns
`{ kind: 'error', code: 'aborted' }` so we don't continue to token
exchange / persistence after a cancelled provisioning POST, and
don't continue to persistence after a cancelled token exchange.
- `performSignupOrAuth` adds a final guard immediately before
`replaceStoredUser` — covers the window between the last network
call and the disk write. Returns `{ kind: 'error', message:
'aborted' }`, which the screen routes through its existing abandon
arm to OAuth fallback.
- `SigningUpScreen.useAsyncEffect` passes its `signal` to
`performSignupOrAuth`. The existing `if (signal.aborted) return`
guard on the screen side stays as a backstop.
**Why three guard points, not one**
The wrapper does three things in sequence: provisioning POST, token
exchange POST, user-info fetch. A guard between each ensures the
wrapper can't slip past abort to a downstream step. The
`replaceStoredUser` guard is the most important — it's the
disk-leaking step.
**No new abort code in `performAmplitudeAuth`**
OAuth fallback path is initiated externally (browser opens, user
interacts) — there's no in-flight `await` to cancel from the screen
side.
2919 / 2919 unit tests pass. Typecheck + lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(signup): drop emailCaptureComplete orphans + TRANSITION_GROUPS doc rot
**emailCaptureComplete cleanup**
Three test files still spread `emailCaptureComplete` into session
overrides: `auth-gate.test.ts:64`,
`session-checkpoint-events.test.ts:106`,
`AuthScreen.coaching.test.tsx:54,85`. The field was removed from
`WizardSession` in commit `e5df61b3`; tsc didn't catch the orphans
because `tsconfig.json` excludes `src/**/__tests__/**/*`.
The auth-gate test "blocks create-account runs that have only
completed email capture" was the most concerning — its name described
behavior the new ceremony doesn't have a notion of, and the assertion
was load-bearing on a now-nonexistent gate condition. Renamed to
"blocks create-account runs while ToS is unaccepted" — that case is
still real (a user entering create-account without `--accept-tos`
should hold the gate until they accept), and the test now exercises
it cleanly.
**TRANSITION_GROUPS doc rot**
`SigningUpScreen.tsx:104-107` claimed "the TRANSITION_GROUPS map in
App.tsx collapses the dissolve between SignupEmail/FullName/SigningUp."
No such map exists — the closed PR #234 mentioned it but the
implementation didn't ship; I copied the comment forward by accident.
Each screen swap is a discrete dissolve today, which is fine for our
purposes. Drop the misleading sentence.
2919/2919 tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(signup): single _resetCeremonyKeys() helper for ceremony reset
`setSignupEmail(null)` and `backToWelcome` were both inlining the
same ceremony-reset writes — `signupRequiredFields` / `signupAuth` /
`signupAbandoned` — with a comment in `backToWelcome` saying "keep
this in sync with `setSignupEmail`." That's exactly the fragile
pattern the original review flagged.
Extract `_resetCeremonyKeys()` as a private method on `WizardStore`.
Both call sites use it. The "keep in sync" comment goes away because
there's nothing left to sync.
**Defensive scope expansion**
The helper also clears `signupFullName` and `tosAccepted` — the
inputs the ceremony fed. Originally I argued the bug-as-described
(stale `signupFullName` after Esc-back leaks into the next probe)
isn't reachable because `TextInput`'s draft state doesn't lift to
session without an explicit submit. That's still true today.
But the broader "ceremony reset = whole ceremony" semantic is
consistent with the comment we already ship, costs ~3 lines, and
covers a class of future regressions: any change that adds
`onChange={(v) => setSignupFullName(v)}` for live persistence would
otherwise activate the leak. `tosAccepted` has a related
stale-but-load-bearing risk — `SigningUpScreen` only includes
`full_name` in the POST when `tosAccepted === true`, so a stale
`true` after a back-out would change POST semantics on the next
forward pass.
Cheap insurance, and it makes the contract uniform.
**Tests**
Existing `setSignupEmail(null) resets ceremony state` test renamed
and extended to assert that `signupFullName` and `tosAccepted` are
also cleared. The defensive coverage is documented inline so a
future reader sees why we clear "more than the strictly-needed"
fields.
2919/2919 tests pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(signup): pin abort-signal contract for cancellation path
Two regression tests for the abort plumbing landed in the prior
commit:
1. **`aborting the signal pre-call returns error without persisting
tokens`** — pins the load-bearing guard inside
`performSignupOrAuth`: even if the success arm of
`performDirectSignup` resolves (mocked) and the user-info fetch
succeeds (mocked), an aborted signal must skip
`replaceStoredUser`. Without that, the disk-write happens behind
the back of an unmounted screen and the user lands as "signed in"
on the next launch despite explicitly cancelling.
2. **`threads signal through to performDirectSignup`** — pins the
wire-level pass-through. Without this, the screen's
`useAsyncEffect` AbortController couldn't actually cancel the
axios POSTs on unmount; we'd be relying on a guard that the screen
never reaches because `performSignupOrAuth` blocks on a
non-cancelable promise.
Together they cover both halves of the abort contract: the
network-cancel half (signal reaches axios) and the persistence-skip
half (signal blocks `replaceStoredUser`). A regression in either
half re-introduces the disk-leak failure mode flagged on the review.
2921/2921 unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(signup): exhaustive switch on PerformSignupOrAuthResult arms
Reviewer's pushback was sharper than my original counter — the
`rate_limited / retryAfter` example surfaced a failure mode I'd
dismissed: TS catches new arms whose properties get accessed in
existing code (the success-arm `result.tokens` access), but does NOT
catch new arms that should have bespoke handling and instead silently
route to the deny-by-default fallback. The "easy to add later"
framing was weak — "later" is "after a contributor adds an arm and
nobody notices the semantics drift."
The cost asymmetry tilts in favor of `assertNever`:
- Now: ~3-5 lines per call site, plus a one-line helper.
- Later: rediscover every consumer of the union and re-verify each
was correct.
`assertNever` doesn't change the deny-by-default behavior — every
existing call site still routes non-success arms to OAuth fallback.
What it does is force every call site to *list* the arms it's
fallback-ing. When the union grows, TypeScript points at each one.
**Changes**
New `src/utils/assert-never.ts` — bog-standard exhaustiveness helper
with a runtime throw as defense-in-depth.
Three call sites restructured to exhaustive switch on
`PerformSignupOrAuthResult.kind`:
- `SigningUpScreen.tsx` — was an if-chain ending in an unguarded
`setSignupAbandoned(true)` for "everything else." Now lists
`redirect | error` explicitly with `default: assertNever`.
- `default.ts` (TUI auth task) — was `if (success) {...}` with a
"fall through to OAuth" comment. Now lists each non-success arm
in a single combined case with the same fall-through behavior,
plus the assertNever default.
- `helpers.ts::runDirectSignupIfRequested` (CI/agent path) — was
`if (kind !== 'success') return;` (negation, so a new arm would
auto-route to fallback without TS noticing). Now exhaustive.
Behavior is unchanged. The compile-time check is the upgrade.
2918+/2918+ tests pass (3 unrelated oauth-server failures from a
stale port-listener process not connected to this change). Typecheck
clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: prettier --write on signup-or-auth.ts and assert-never.ts
CI's `pnpm lint:prettier` flagged two files. Husky's pre-commit hook
runs `prettier --write` on staged files but appears to have missed
these on the prior commits — likely the new-file path on
`assert-never.ts`, with the cascading import edit on
`signup-or-auth.ts` carrying the same drift. No code change; pure
whitespace/formatting per the project's prettier config.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): abandon on needs_information for an already-satisfied field
If the server returns `needs_information` with a field the wizard has
already populated (e.g. re-requesting `full_name` after we sent
`full_name`), the ceremony can't make progress:
1. SigningUpScreen mounts with `useAsyncEffect` deps
`[email, fullName]`.
2. Effect runs, POSTs, server returns
`needs_information: ['full_name']`.
3. Handler calls `setSignupRequiredFields(['full_name'])` — but
`signupRequiredFields` isn't a useAsyncEffect dep.
4. React re-renders. SigningUp.show stays true (no unmount).
useAsyncEffect deps unchanged. No re-fire.
5. User wedges on the spinner forever; only escape is Ctrl+C.
Today this only fires as a server bug (the schema's `.refine()`
already rejects unsupported `required` shapes, so the only way to
reach this path is the server requesting the same supported shape it
just received). Probability low; failure mode bad enough that the
one-branch guard is worth the cost.
**Fix**
In the `needs_information` arm, check whether every requested field
is already populated in the session. If yes, the wrapper can't make
progress — log, abandon to OAuth, route the user to browser auth.
This is the same fall-through `unsupported_required_shape` already
uses; we just generalize the "I don't know what to do with this" set
to include "you asked for something I gave you."
The check only knows about `full_name` today (the single supported
field), matching the schema's allowlist. If `SUPPORTED_REQUIRED`
grows, this list grows alongside.
Caught by reviewer subagent on PR #539.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): include signupTokensObtained in _resetCeremonyKeys
`_resetCeremonyKeys` (PR #539's helper consolidating ceremony reset
across `setSignupEmail(null)` and `backToWelcome`) was clearing five
fields but missing `signupTokensObtained`. `backToWelcome` cleared
it explicitly; `setSignupEmail(null)` didn't.
Failure mode: user signs up successfully on attempt 1
(`signupTokensObtained=true`) → backs out via an Esc that walks
through `setSignupEmail(null)` → `signupTokensObtained` stays true →
next forward pass hits `default.ts:807` (the post-success hydration
branch), reads tokens from `~/.ampli.json`, and silently re-signs the
user in as the prior account even though they explicitly rewound to
start fresh.
The fix is to clear `signupTokensObtained` inside the helper so all
ceremony-reset paths land in the same state. The redundant explicit
write in `backToWelcome` (post-helper-call) becomes unnecessary —
removed.
Existing test extended: the `setSignupEmail(null) resets the entire
ceremony as one unit` case now pre-seeds `signupTokensObtained=true`
and asserts it's cleared post-call. Comment explains why this field
matters operationally so a future reader doesn't drop it as
"redundant flag."
Caught by reviewer subagent on PR #539. Related to MCP-245
(architectural cleanup to remove `signupTokensObtained` entirely in
favor of `signupAuth`-as-discriminator); this commit is the targeted
fix while that broader refactor stays scoped to its own work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(default): delete dead post-gate signup wrapper call in TUI
PR #539's auth-task gate (`isAuthTaskGateReady`) requires
`signupAuth !== null || signupAbandoned` before releasing on the
create-account path. That guarantees, by the time the TUI auth-task
block runs:
- Success: `signupAuth` set AND `signupTokensObtained=true` (set
synchronously by SigningUpScreen alongside `signupAuth`).
- Abandon: `signupAbandoned=true`.
The block at `default.ts:746-806` then gated on
`!s.signupTokensObtained && !s.signupAbandoned` — false on both axes
in either gated state — so it could never fire in TUI mode. Pure
dead code: ~60 lines of unreachable switch-with-assertNever, a
redundant `performSignupOrAuth` call (SigningUpScreen already made
it), and the only remaining caller of `assertNever` in this file.
CI / agent / classic modes have their own signup path
(`runDirectSignupIfRequested` in `helpers.ts`), so deleting this
block doesn't affect them.
**What's left**
The `else if (s.signupTokensObtained)` branch survives — it's the
actual hydration path that reads tokens from disk and avoids
opening a spurious browser OAuth on a fresh-install-dir success.
Promoted from `else if` to `if` since the prior block is gone.
Comment updated: it referenced the deleted `EmailCaptureScreen`
("EmailCaptureScreen already called replaceStoredUser…"); now points
at the live writer (`SigningUpScreen settled the ceremony…`).
`trackSignupAttempt` import moved out of the deleted block (still
used by the `browser_fallback_after_signup` telemetry call further
down). `assertNever` import dropped — no other callers in this file.
**Why this works as a deletion, not an "add a comment" preservation**
If a future change weakens `isAuthTaskGateReady` (e.g. allows the
gate to release before SigningUpScreen settles), the dead block
becomes live again — and would re-introduce the redundant POST that
PR #539's bugbot fix `caa57a30` was specifically about preventing.
Better to delete now and force the next change author to think
through the gate semantics, rather than leave a dormant trap.
Caught by reviewer subagent on PR #539 (issues #3 and #4 — dead
block + stale comment).
2909/2909 unit tests pass (3 unrelated oauth-server failures from a
stale port-listener process). Typecheck + prettier clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(signup): fold markSignupTokensObtained into setSignupAuth
The TUI auth-task gate (`isAuthTaskGateReady`) releases on
`signupAuth !== null`; the post-gate hydration branch in `default.ts`
reads `signupTokensObtained` to decide whether to pull tokens from
disk vs. open browser OAuth. Both fields are part of the same logical
"ceremony settled successfully" event — but they were two separate
calls (`setSignupAuth(...)` followed by
`store.markSignupTokensObtained()`), with `setSignupMagicLinkUrl`
sandwiched in between.
That ordering quietly relied on event-loop semantics: nanostores
notifies subscribers synchronously, so the gate-listener could in
principle fire its microtask continuation between the two writes and
observe `signupAuth` set with `signupTokensObtained` still false —
then open browser OAuth even though valid tokens were already in
hand. The shape was a footgun waiting for a refactor.
Fold the `signupTokensObtained=true` write into `setSignupAuth` so
both fields land in the same synchronous setKey + emitChange block.
Add a test pinning the contract: `setSignupAuth(non-null)` flips
`signupTokensObtained=true`; `setSignupAuth(null)` leaves it false
(so a ceremony reset can't accidentally re-open the gate).
* refactor(signup): funnel switchToLogin through _resetCeremonyKeys
`switchToLogin` was inlining a subset of the ceremony reset
(`signupEmail = null`, `signupFullName = null`) but missing the rest:
`signupRequiredFields`, `signupAuth`, `signupAbandoned`, `tosAccepted`,
`signupTokensObtained`. Today nothing in the SignIn path reads those
fields, so the omission is latent — but it's the same shape of bug
that prompted `_resetCeremonyKeys` in the first place. The moment
someone adds a new ceremony field (or wires a sign-in code path that
reads one of the existing ones), this drifts.
Funnel the cleanup through the shared helper so any new ceremony
field added to `_resetCeremonyKeys` is automatically cleared on the
signup→login switch too. Pin the contract with a test that pre-seeds
the full ceremony state and asserts every field clears.
* docs(signup): tighten ceremony comments and auth-gate test names
Bundle of small wording fixes flagged in peer review of PR #539. No
behavior change; everything here is comments + test names.
* SignupFullName Esc handler: the previous comment said "Esc rewinds
to the email screen so the user can correct a typo" — but
`setSignupEmail(null)` clears the email entirely (it doesn't
preserve the prior value), and crucially also resets the rest of
the ceremony state via `_resetCeremonyKeys`. Reword to reflect both
effects so a future reader doesn't think Esc is a soft rewind.
* `auth-gate.test.ts`: two tests asserted the gate blocks "until ToS
is accepted" / "while ToS is unaccepted", but the gate itself no
longer checks `tosAccepted` (PR-539 moved the predicate to "signup
ceremony has settled" — `signupAuth !== null || signupAbandoned`).
Rename both to make explicit that the *ceremony-unsettled* check is
what holds the gate, and document the pre-PR-539 history in a
shared describe-block comment so the redundancy with "ceremony in
flight" is intentional and explained.
* `default.ts` post-gate hydration branch: comment referenced the
removed `markSignupTokensObtained` setter. Rewrite to point at the
new atomic write inside `setSignupAuth`.
* `_resetCeremonyKeys` `signupTokensObtained` reset: cross-link to
`setSignupAuth` so a reader landing on the reset side knows where
the forward-direction write lives (and why it's folded together).
* `flows.ts` SigningUp `revert: () => false`: expand the comment to
explain WHY there's no in-band undo (the in-flight POST may have
created or abandoned the account on the server) and HOW back-nav
still works (walks past to a screen whose own revert clears the
inputs and resets the ceremony).
* test(signup): pin direct-signup inner abort guards
The two `if (input.signal?.aborted)` checks inside `performDirectSignup`
(post-provisioning, post-token-exchange) only had transitive coverage
through the wrapper-level abort tests. If a future refactor splits the
token-exchange step out or moves the wrapper's persistence gate, the
"skip parsing on abort" behavior could regress without any test failure.
Stubs `axios.post` to deterministically land the abort between the
await resolving and the post-await guard — MSW operates below axios
and can't synchronize that window. One test per guard, asserting the
function returns `{kind:'error', code:'aborted'}` and (for the first
guard) that token exchange never fires.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(signup): switch+assertNever in performSignupOrAuth wrapper
Convert the if-chain on result.kind to a switch with default:
assertNever(result). TS narrowing already protects the wrapper from a
silent fall-through into the success block (the success path reads
result.tokens, which would fail typecheck on any new arm), but the
explicit pattern matches what every other consumer of
PerformSignupOrAuthResult / DirectSignupResult uses (SigningUpScreen,
runDirectSignupIfRequested). The wrapper is the closest layer to the
wire and the most worth-protecting site for an exhaustiveness check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): hydrate signupUserInfo from session.signupAuth
The auth task declared signupUserInfo as `const null`, making the
`if (signupUserInfo)` skip-fetch branch unreachable. The wrapper had
already fetched the user profile (with provisioning retry) and
SigningUpScreen mirrored it onto session.signupAuth.userInfo, but
the auth task never read it — falling through to a redundant
fetchAmplitudeUser call on every successful signup happy path.
Read from tui.store.session.signupAuth?.userInfo so the skip-fetch
branch becomes live: one fewer RTT per successful signup, and the
inline storeToken (already in the else branch) gets correctly skipped
since the wrapper's replaceStoredUser covered it. When the wrapper's
fetch failed (provisioning lag exhausted retries),
signupAuth.userInfo is null and we fall through to the probe path —
same behavior as before.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(signup): note intentional signal omission in non-TUI helper
runDirectSignupIfRequested doesn't pass `signal` to performSignupOrAuth
because CI / agent / classic modes have no in-band cancellation
surface — no Esc handler, no unmount lifecycle, nothing to thread
through. Add a short note at the call site so the omission doesn't
read as an oversight when the TUI's SigningUpScreen passes a signal a
few files away.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(signup): fold signupEmail into _resetCeremonyKeys
Every existing caller of `_resetCeremonyKeys` (setSignupEmail,
switchToLogin, backToWelcome) was already clearing signupEmail
explicitly around the call — the helper's "callers handle email"
contract was a footgun waiting to be tripped by a future caller that
forgot the matched line. Fold the clear into the helper so the
ceremony reset is a single atomic operation.
setSignupEmail's null branch now goes only through the helper (no
redundant double-write); the non-null branch still does its own
setKey + analytics capture. switchToLogin and backToWelcome shed
their explicit clears. The backToWelcome doc comment that listed
specific keys is replaced with a pointer to the helper as the
source of truth — listing keys inline created drift risk every
time a new ceremony field was added.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): wipe ceremony state on /region
setRegionForced cleared every other zone-scoped key (credentials,
OAuth intermediates, org/project selection, framework state, etc.)
but left the signup ceremony untouched. signupAuth.zone is pinned
to the old region and signupRequiredFields is a cached snapshot
from the old zone's probe POST — both would silently leak into the
next forward pass on the new zone, steering the user through the
wrong field-collection screens or short-circuiting the
needs_information probe entirely.
Funnel through the shared `_resetCeremonyKeys` helper alongside the
existing zone-scoped clears so any future ceremony field gets reset
automatically. New store test seeds all seven ceremony keys
(signupEmail, signupFullName, tosAccepted, signupRequiredFields,
signupAuth, signupAbandoned, signupTokensObtained) and asserts they
clear after setRegionForced — the canary for the contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): wipe ceremony state on auth→region back-nav
resetAuthForRegionChange clears every other zone-scoped key
(credentials, pendingOrgs, selected*, etc.) but skipped the signup
ceremony. Same zone-scoping reasoning as setRegionForced:
signupAuth.zone is pinned to the old region and signupRequiredFields
cached the old zone's probe response — both would silently leak
across the region change.
Dormant today: tracing the back-nav path from Auth → RegionSelect on
the create-account flow shows that the per-screen reverts of the
signup screens already clear individual fields before this revert
fires, so no current reader observes a stale signupAuth here. The
fix preserves the invariant that every reset path goes through
_resetCeremonyKeys — adding a new caller (or changing the back-nav
order) won't have to remember the matched-line contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(signup): walk past collection screens on back-nav after abandonment
When the second POST errored after ToS+name collection,
signupAbandoned=true while signupRequiredFields, tosAccepted, and
signupFullName were all populated. Esc on Auth would land on
SignupFullName, the user types a new name, the next forward pass
skips SigningUp (its show predicate gates on !signupAbandoned), and
the user is dumped back on Auth without any retry — a confusing
dead-end where typing a name accomplishes nothing.
Make ToS.revert and SignupFullName.revert return false when
signupAbandoned is true so the back-walk continues to
SignupEmail.revert. SignupEmail.revert calls setSignupEmail(null),
which funnels through _resetCeremonyKeys and clears the whole
ceremony — including signupAbandoned — giving the user a clean
restart.
Stub fix: makeStubStore's setSignupEmail(null) was missing
signupFullName / tosAccepted / signupTokensObtained from its mutate
patch, …
Summary
The wizard had four mode branches: agent, CI, classic, and TUI. Classic — interactive prompts without the rich Ink TUI — was a legacy fallback we no longer need. Removing it collapses the mode handler to three branches and lets us drop a couple of small bits of code that only existed to serve it.
Changes
--classicyargs option (src/commands/default.ts) and the hiddenAMPLITUDE_WIZARD_CLASSICenv-var passthrough shadow (bin.ts).else if (options.classic ...)branch in the default command handler (~30 lines), including its bespokerunDirectSignupIfRequestedonSuccesshook that calledresolveCredentials({ requireOrgId: false }).onSuccesscallback parameter onrunDirectSignupIfRequested— agent and CI never used it, so the helper is now a straightforward fire-and-return.!options.classic && AMPLITUDE_WIZARD_CLASSIC !== '1'guards from the agent-detect TTY check (no longer reachable once the branch is gone).account-creation-flow.ts,wizard-session.ts,zone-resolution.ts,orchestrator-context.ts,agent-runner.ts,store.ts,store.test.ts— the mode tally that read "agent / CI / classic / TUI" now reads "agent / CI / TUI".Net diff: 10 files, +24 / -92.
What's preserved
LoggingUIis kept — CI mode and thelogin/region/feedback/slack/mcpsubcommands still use it.getOrAskForProjectDatais kept — agent-runner still routes through it for CI mode and the rare TUI-startup-failure fallback.ExecutionModewas always'interactive' | 'ci' | 'agent'(no'classic'member), so no enum changes were needed.Test plan
pnpm tsc --noEmitpnpm exec vitest run --pool=forks --maxWorkers=1— 187 files / 2826 tests passingpnpm lintpnpm tryopens the TUI as beforepnpm try --agentemits NDJSON as beforepnpm try --ci --install-dir <fixture>runs non-interactively as beforeAMPLITUDE_WIZARD_CLASSIC=1 pnpm tryno longer routes to a removed branch (should now fall through to TUI / agent auto-detect, same as no flag)🤖 Generated with Claude Code
Note
Low Risk
Low risk: this mainly removes a legacy execution branch and related flags, with minimal behavioral impact beyond no longer supporting
--classic/AMPLITUDE_WIZARD_CLASSIC.Overview
Removes the legacy classic prompt-based mode from the wizard, collapsing the default command handler down to agent / CI / TUI only.
Drops the public
--classicflag and the hiddenAMPLITUDE_WIZARD_CLASSICenv passthrough, deletes the classic-mode execution branch, and simplifiesrunDirectSignupIfRequestedby removing its classic-only success callback. Updates comments/tests/docs references to reflect the new three-mode setup.Reviewed by Cursor Bugbot for commit 7275893. Bugbot is set up for automated code reviews on this repo. Configure here.