From d584d2a6f8b8422dfe2f46981e1f3fb696cb3434 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Tue, 3 Nov 2026 20:13:43 -0600 Subject: [PATCH 01/10] fix(migrate): address findings from the pre-release smoke test - Report users created without an extra or unverified email or phone Clerk refused once the user existed: the summary lists them, `--json` adds them to `result.warnings`, the user's line records `refused`, and `runs ` shows them under warnings. Before, only users.ndjson said so. - A secret key Clerk refuses (401) stops the import with Clerk's error, rather than a stand-in instance that sent the run checks off course. A run undone in full no longer blocks a stand-in import with "did not confirm which instance". - Replace a gateway's HTML error page with "Clerk returned HTTP with no error details" in import, undo and runs breakdowns. - A refused dry run says so after the checks, with the --allow-partial command (`refused: true` in --json). - Explain the APP_ID placeholder in fix commands (`checks.placeholders` in --json). - `--json` carries the notes the human summary adds to Clerk's messages, such as the development-instance user limit (`result.notes`). Co-Authored-By: Claude Opus 5.5 --- .changeset/migrate-final-testing-fixes.md | 5 + .../cli-core/src/commands/migrate/README.md | 31 ++++-- .../src/commands/migrate/import-users.test.ts | 47 +++++++++ .../src/commands/migrate/import-users.ts | 27 +++-- .../src/commands/migrate/lib/checks.ts | 5 +- .../src/commands/migrate/lib/failures.test.ts | 27 +++++ .../src/commands/migrate/lib/failures.ts | 12 ++- .../src/commands/migrate/lib/run-store.ts | 5 + .../src/commands/migrate/lib/target.test.ts | 23 +++++ .../src/commands/migrate/lib/target.ts | 9 +- .../cli-core/src/commands/migrate/run.test.ts | 99 +++++++++++++++++++ packages/cli-core/src/commands/migrate/run.ts | 77 +++++++++++++-- .../src/commands/migrate/runs.test.ts | 43 ++++++++ .../cli-core/src/commands/migrate/runs.ts | 28 ++++++ .../cli-core/src/commands/migrate/types.ts | 6 ++ 15 files changed, 412 insertions(+), 32 deletions(-) create mode 100644 .changeset/migrate-final-testing-fixes.md create mode 100644 packages/cli-core/src/commands/migrate/lib/failures.test.ts diff --git a/.changeset/migrate-final-testing-fixes.md b/.changeset/migrate-final-testing-fixes.md new file mode 100644 index 000000000..d9ba29e89 --- /dev/null +++ b/.changeset/migrate-final-testing-fixes.md @@ -0,0 +1,5 @@ +--- +"clerk": patch +--- + +Report users that `clerk migrate import` created without an email or phone Clerk refused, in the summary, `--json` and `clerk migrate runs `, and fix several misleading messages: a mistyped secret key now gets Clerk's invalid-key error, a refused dry run says so, a gateway's HTML error page no longer fills the error breakdown, the `APP_ID` placeholder in fix commands is explained, and `--json` carries the same notes the human summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. diff --git a/packages/cli-core/src/commands/migrate/README.md b/packages/cli-core/src/commands/migrate/README.md index 3c806407d..c126d7650 100644 --- a/packages/cli-core/src/commands/migrate/README.md +++ b/packages/cli-core/src/commands/migrate/README.md @@ -807,7 +807,9 @@ instance ID; the latest matching import run decides what happens: Clerk cannot name the instance (`GET /v1/instance` failed, often from rate limiting right after a large import) and a run of this file exists under a real instance ID, it exits 2 rather than starting over: try again, or pass -`--new-run`. A continue whose run changed while it waited at the prompt, from +`--new-run`. A run undone in full holds no users, so it doesn't count. A key +Clerk refuses (`401`) is not a failed lookup: the import stops with Clerk's +error. A continue whose run changed while it waited at the prompt, from an undo or another continue, exits 2 too, with nothing written. A continued run also finishes what the last one left open: @@ -913,7 +915,9 @@ store: …`), passwords a source had to drop, emails Clerk refuses (malformed, Any reject stops the import, and it exits 2 with the command that adds `--allow-partial`. With `--allow-partial`, the rest import and each reject is recorded as `skipped` with its reason. `--dry-run` exits 2 when the real run -would be refused, and 0 otherwise. +would be refused, and 0 otherwise. A refused dry run says so after the checks, +with the `--allow-partial` command when any user passes (`refused: true` in +`--json`). ``` Checks @@ -936,7 +940,8 @@ Or change the instance instead Each fix names its instance with `--instance`, so it changes the instance the import targets, whatever the key's source. A key from `--secret-key` or `CLERK_SECRET_KEY` names no app, so its fix reads `--app APP_ID`: fill in the -app that owns the instance. When Clerk could not name the +app that owns the instance. The checks say so beneath the fixes, and `--json` +adds `checks.placeholders` (`{ "APP_ID": … }`). When Clerk could not name the instance (a `key_…` fallback ID), the fix points at the Dashboard instead: in `--json` it carries `url` in place of `command`. @@ -958,9 +963,12 @@ users already signed up on the source platform. Only the first verified email and phone go on `POST /v1/users`. Every additional verified identifier, and every unverified one, is attached afterwards with its own request, ahead of any create still queued, and backs -off on a `429` like the create. A refusal there is logged and the user still -counts as imported — a duplicate secondary email should not undo an otherwise -successful user. An attach with no answer stays `pending` for a re-run. +off on a `429` like the create. A refusal there does not fail the user — a +duplicate secondary email should not undo an otherwise successful user — but +it is a warning: the summary lists the user under "Imported without an email or +phone Clerk refused", by kind and Clerk's reason (`result.warnings` in +`--json`), and the user's line records it under `refused`, which `runs ` +reports too. An attach with no answer stays `pending` for a re-run. The first phone gets the same treatment when Clerk refuses it — a country the instance does not support, or a number that is not E.164 — and the user has an @@ -1001,7 +1009,8 @@ that predates the field leaves a development instance at Users that do exceed the limit come back in the error breakdown as `You have reached your limit of N users`, annotated with what a development -instance can do about it. The first refusal stops the import: the users it +instance can do about it (`result.notes` in `--json`, which carries every such +note the summary prints). The first refusal stops the import: the users it never sent are counted under "Not sent" (`result.notSent` in `--json`, and `counts.notSent` in `run.json`), written when the run finishes. A run stopped with Ctrl-C never finishes, so it can't count them, and `runs ` offers its @@ -1025,9 +1034,11 @@ clerk migrate runs --json | `--runs-dir ` | Read runs from somewhere else | It prints the runs folder first. The listing shows each run's ID, date, kind, -status, target, file and counts. `runs ` adds the error breakdown and the -users that failed, were skipped, or are `creating` with no answer to confirm the -create (`unconfirmed` in `--json`), with the path to the full record, then what +status, target, file and counts. `runs ` adds the error breakdown, the +warnings for users imported without an email or phone Clerk refused +(`warnings` and `importedWithout` in `--json`), and the users that failed, were +skipped, or are `creating` with no answer to confirm the create (`unconfirmed` +in `--json`), with the path to the full record, then what to run next: for an interrupted or partial import, the import that continues it, with the flags it ran with, then the undo. The continue is left out when importing the file again might not reach this run: a newer run of the same file diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index ccc30f1a8..2362d7ebf 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -434,6 +434,53 @@ describe("importUsers", () => { expect(lines[1]).toMatchObject({ status: "created", clerkId: "user_created" }); expect(lines[1]?.error).toContain("Failed to add additional email b@x.dev"); expect(lines[1]).not.toHaveProperty("pending"); + // Imported without it, which the summary and `runs` report as a warning. + expect(lines[1]?.refused).toEqual(["additional email: that email is taken"]); + expect([...summary.droppedIdentifiers]).toEqual([["additional email: that email is taken", 1]]); + }); + + test("counts each user once per reason Clerk refused its identifiers", async () => { + stub((url) => + url.endsWith("/v1/email_addresses") + ? clerkError(422, "that email is taken") + : ok("user_created"), + ); + + const summary = await importUsers({ + users: [user({ email: ["a@x.dev", "b@x.dev", "c@x.dev"] })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)?.refused).toEqual(["additional email: that email is taken"]); + expect([...summary.droppedIdentifiers]).toEqual([["additional email: that email is taken", 1]]); + }); + + test("keeps an earlier refusal when a continue attaches the rest", async () => { + stub(() => ok("user_created")); + + const summary = await importUsers({ + users: [], + attachOnly: [ + { + sourceId: "u1", + clerkId: "user_created", + status: "created", + error: "Failed to add additional email b@x.dev: that email is taken", + refused: ["additional email: that email is taken"], + pending: [{ kind: "phone", value: "+15555550100", verified: false }], + }, + ], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)?.refused).toEqual(["additional email: that email is taken"]); + expect(lines.at(-1)).not.toHaveProperty("pending"); + // Counted by the run that saw the refusal, not again by the continue. + expect(summary.droppedIdentifiers.size).toBe(0); }); // A Ctrl-C mid-backoff should not wait out the pause, nor send the attach. diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index eb4bde001..e7abb75b1 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -251,7 +251,7 @@ async function attachIdentifier( ctx: CreateContext, clerkUserId: string, { kind, value, verified }: PendingIdentifier, -): Promise<{ note?: string; pending?: boolean }> { +): Promise<{ note?: string; refused?: string; pending?: boolean }> { const path = kind === "email" ? "/v1/email_addresses" : "/v1/phone_numbers"; const body = kind === "email" @@ -281,8 +281,12 @@ async function attachIdentifier( return {}; } catch (error) { if (error instanceof NotSentError || outcomeUnknown(error)) return { pending: true }; - const label = `${verified ? "additional" : "unverified"} ${kind} ${value}`; - return { note: `Failed to add ${label}: ${(error as Error).message}` }; + const kindLabel = `${verified ? "additional" : "unverified"} ${kind}`; + const { message } = failureOf(error); + return { + note: `Failed to add ${kindLabel} ${value}: ${message}`, + refused: `${kindLabel}: ${normalizeErrorMessage(message)}`, + }; } } @@ -290,18 +294,20 @@ async function attachIdentifier( * Attaches each identifier. Extra identifiers are best-effort: a duplicate * secondary email should not undo a user who was otherwise imported. * - * @returns A note per identifier Clerk refused, and those still pending. + * @returns A note per identifier Clerk refused, why by kind, and those still + * pending. */ async function attachAll( ctx: CreateContext, clerkUserId: string, identifiers: PendingIdentifier[], -): Promise<{ notes: string[]; pending: PendingIdentifier[] }> { +): Promise<{ notes: string[]; refused: string[]; pending: PendingIdentifier[] }> { const results = await Promise.all( identifiers.map(async (identifier) => attachIdentifier(ctx, clerkUserId, identifier)), ); return { notes: results.flatMap((result) => (result.note ? [result.note] : [])), + refused: results.flatMap((result) => (result.refused ? [result.refused] : [])), pending: identifiers.filter((_, index) => results[index]?.pending), }; } @@ -496,6 +502,7 @@ export async function importUsers(options: ImportUsersOptions): Promise(); + const droppedIdentifiers = new Map(); // A line that cannot be written halts the import: a user created with no // record is beyond both `undo` and a re-run. The error is rethrown once @@ -559,14 +566,21 @@ export async function importUsers(options: ImportUsersOptions): Promise { - const { error: _error, pending: _pending, ...base } = line; + const { error: _error, pending: _pending, refused: earlier = [], ...base } = line; const attached = await attachAll(ctx, base.clerkId ?? "", toAttach); const error = [...notes, ...attached.notes].join("; "); + // Imported, but without what Clerk refused: a warning for the summary, + // counted once per user and reason. A continue keeps the earlier ones. + for (const reason of new Set(attached.refused)) { + droppedIdentifiers.set(reason, (droppedIdentifiers.get(reason) ?? 0) + 1); + } + const refused = [...new Set([...earlier, ...attached.refused])]; // A second line, which wins as the latest, adds what happened on the way. if (toAttach.length > 0 || error) { record({ ...base, ...(error ? { error } : {}), + ...(refused.length > 0 ? { refused } : {}), ...(attached.pending.length > 0 ? { pending: attached.pending } : {}), }); } @@ -716,6 +730,7 @@ export async function importUsers(options: ImportUsersOptions): Promise diff --git a/packages/cli-core/src/commands/migrate/lib/failures.test.ts b/packages/cli-core/src/commands/migrate/lib/failures.test.ts new file mode 100644 index 000000000..b2f36711d --- /dev/null +++ b/packages/cli-core/src/commands/migrate/lib/failures.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, test } from "bun:test"; +import { BapiError } from "../../../lib/errors.ts"; +import { failureOf } from "./failures.ts"; + +describe("failureOf", () => { + test("keeps Clerk's own message", () => { + const error = new BapiError( + 422, + JSON.stringify({ + errors: [{ code: "form_identifier_exists", message: "That email is taken." }], + }), + new Headers(), + ); + expect(failureOf(error)).toEqual({ message: "That email is taken.", code: "422" }); + }); + + // A gateway's error page fills a terminal and says nothing a summary can use. + test.each([ + ["an HTML page", "\n\n500 Server Error"], + ["a doctype", "Bad gateway"], + ])("replaces %s with the status", (_, body) => { + expect(failureOf(new BapiError(500, body, new Headers()))).toEqual({ + message: "Clerk returned HTTP 500 with no error details", + code: "500", + }); + }); +}); diff --git a/packages/cli-core/src/commands/migrate/lib/failures.ts b/packages/cli-core/src/commands/migrate/lib/failures.ts index c6dbbf4ff..2e66805b9 100644 --- a/packages/cli-core/src/commands/migrate/lib/failures.ts +++ b/packages/cli-core/src/commands/migrate/lib/failures.ts @@ -10,10 +10,14 @@ import { RateLimitExceededError } from "./retry.ts"; export function failureOf(error: unknown): { message: string; code: string } { if (error instanceof RateLimitExceededError) return { message: error.message, code: "429" }; const apiError = error as BapiError; - return { - message: apiError.longMessage ?? apiError.message ?? "Unknown error", - code: String(apiError.status ?? "unknown"), - }; + const code = String(apiError.status ?? "unknown"); + const message = apiError.longMessage ?? apiError.message ?? "Unknown error"; + // A proxy's or gateway's error page, not Clerk's JSON: its markup says + // nothing a summary could use, and fills a terminal. + if (/^\s*<(!doctype|html|head|body)\b/i.test(message)) { + return { message: `Clerk returned HTTP ${code} with no error details`, code }; + } + return { message, code }; } /** Counts a failure under its normalized message, the way every summary groups them. */ diff --git a/packages/cli-core/src/commands/migrate/lib/run-store.ts b/packages/cli-core/src/commands/migrate/lib/run-store.ts index 29fc68dd5..6e613f436 100644 --- a/packages/cli-core/src/commands/migrate/lib/run-store.ts +++ b/packages/cli-core/src/commands/migrate/lib/run-store.ts @@ -127,6 +127,11 @@ export type UserLine = { * run attaches them; a later `created` line without it means they are done. */ pending?: PendingIdentifier[]; + /** + * On a `created` line: the extra identifiers Clerk refused once the user + * existed, by kind and Clerk's reason. The user is imported without them. + */ + refused?: string[]; /** Why a user was skipped. */ reason?: string; error?: string; diff --git a/packages/cli-core/src/commands/migrate/lib/target.test.ts b/packages/cli-core/src/commands/migrate/lib/target.test.ts index f67becee0..20fc42ee3 100644 --- a/packages/cli-core/src/commands/migrate/lib/target.test.ts +++ b/packages/cli-core/src/commands/migrate/lib/target.test.ts @@ -161,6 +161,29 @@ describe("fetchInstanceIdentity", () => { expect(await fetchInstanceIdentity("sk_test_a")).toEqual(first); expect((await fetchInstanceIdentity("sk_test_b")).instanceId).not.toBe(first.instanceId); }); + + // A stand-in would send the import's run checks after an instance that + // does not matter, with advice that leads back to the same refusal. + test("throws Clerk's error for a key Clerk refuses", async () => { + globalThis.fetch = (async () => + Response.json( + { + errors: [ + { + code: "clerk_key_invalid", + message: "Secret Key invalid", + long_message: "The provided Clerk Secret Key is invalid.", + }, + ], + }, + { status: 401 }, + )) as unknown as typeof fetch; + + await expect(fetchInstanceIdentity("sk_test_bad")).rejects.toMatchObject({ + status: 401, + code: "clerk_key_invalid", + }); + }); }); describe("resolveClerkTarget --instance", () => { diff --git a/packages/cli-core/src/commands/migrate/lib/target.ts b/packages/cli-core/src/commands/migrate/lib/target.ts index 9925a3854..a437f64aa 100644 --- a/packages/cli-core/src/commands/migrate/lib/target.ts +++ b/packages/cli-core/src/commands/migrate/lib/target.ts @@ -14,7 +14,7 @@ import { bapiRequest } from "../../../lib/bapi.ts"; import { dim } from "../../../lib/color.ts"; import { resolveBapiSecretKey } from "../../../lib/bapi-command.ts"; import { INSTANCE_ALIASES, resolveAppContext } from "../../../lib/config.ts"; -import { throwUsageError } from "../../../lib/errors.ts"; +import { ApiError, throwUsageError } from "../../../lib/errors.ts"; import { ENV_VAR_SOURCE_SUFFIX, resolveKeylessTarget, @@ -180,7 +180,8 @@ export function laterCarry(runsDir: string | undefined, target: RunTarget): stri * Falls back to a hash of the key when `GET /v1/instance` cannot be read: the * same key always addresses the same instance, so it still tells two * instances apart, and nothing is sent anywhere. A 429 is retried first: it is - * most likely straight after a large import, which is when `undo` runs. + * most likely straight after a large import, which is when `undo` runs. A key + * Clerk refuses (401) throws Clerk's error instead. */ export async function fetchInstanceIdentity(secretKey: string): Promise { const fallbackEnv = detectInstanceType(secretKey) === "prod" ? "production" : "development"; @@ -202,6 +203,10 @@ export async function fetchInstanceIdentity(secretKey: string): Promise { expect(created()).toHaveLength(0); expect(listRuns(runsDir())).toHaveLength(0); expect(captured.err).toContain("Dry run: nothing was written."); + // Said in words, not only by the exit code. + expect(Bun.stripANSI(captured.err)).toContain( + "The import would refuse: 1 user would be rejected. Fix them, or pass --allow-partial to import the other 2 users.", + ); expect(process.exitCode).toBe(2); }); + test("--dry-run --json says the import would be refused", async () => { + fs.writeFileSync( + path.join(workDir, "export.json"), + JSON.stringify([...export2, { id: "u3" }]), + ); + + await run({ ...baseOptions, dryRun: true, json: true }); + + expect(JSON.parse(captured.out)).toMatchObject({ dryRun: true, refused: true }); + }); + test("--dry-run exits 0 when nothing would be rejected", async () => { await run({ ...baseOptions, dryRun: true }); @@ -1502,6 +1517,16 @@ describe("run", () => { expect(captured.err).toContain( `clerk config patch --app APP_ID --instance ins_1 --json '{"auth_email":{"required_for_sign_up":false}}'`, ); + // A key names no app, so the placeholder is explained. + expect(Bun.stripANSI(captured.err)).toContain( + "Replace APP_ID with the app's ID: `clerk apps list` shows it.", + ); + + captured.clear(); + await run({ ...baseOptions, dryRun: true, json: true }); + expect(JSON.parse(captured.out)).toMatchObject({ + checks: { placeholders: { APP_ID: "the app's ID: `clerk apps list` shows it" } }, + }); }); // Reserved meets the requirement, so neither the reject nor the fix applies. @@ -1805,6 +1830,8 @@ describe("run", () => { failed: 1, notSent: 1, stopReason: "You have reached your limit of 100 users.", + // What the human summary adds: an agent reads only this. + notes: [expect.stringContaining("The user limit is a development-instance quota.")], }, }); }); @@ -1892,6 +1919,56 @@ describe("run", () => { }); }); + // Created, then an extra email Clerk refused: imported, but without it. + describe("an email Clerk refuses once the user exists", () => { + const TAKEN = "That email address is taken. Please try another."; + + beforeEach(() => { + fs.writeFileSync( + path.join(workDir, "export.json"), + JSON.stringify([ + { id: "u1", primary_email_address: "a@x.dev", unverified_email_addresses: "b@x.dev" }, + ]), + ); + const clerk = globalThis.fetch; + globalThis.fetch = (async (input: string | URL | Request, init?: RequestInit) => + new URL(input.toString()).pathname === "/v1/email_addresses" + ? Response.json( + { + errors: [{ code: "form_identifier_exists", message: "taken", long_message: TAKEN }], + }, + { status: 422 }, + ) + : clerk(input, init)) as typeof fetch; + }); + + test("is counted in the summary", async () => { + await run(baseOptions); + const err = Bun.stripANSI(captured.err); + expect(err).toContain("Failed: 0"); + expect(err).toContain( + `Imported without an email or phone Clerk refused:\n 1 user: unverified email: ${TAKEN}`, + ); + }); + + test("is a warning in --json", async () => { + await run({ ...baseOptions, json: true }); + expect(JSON.parse(captured.out)).toMatchObject({ + result: { + created: 1, + failed: 0, + errors: [], + warnings: [ + { + warning: `imported without an email or phone Clerk refused: unverified email: ${TAKEN}`, + count: 1, + }, + ], + }, + }); + }); + }); + describe("continuing an earlier run", () => { test("a complete run is not imported again", async () => { await run(baseOptions); @@ -2556,6 +2633,28 @@ describe("run", () => { expect(listRuns(runsDir())).toHaveLength(2); }); + // An undone run holds no users, so it cannot be the instance's: the + // import starts over rather than asking for --new-run. + test("starts over when the only run of this file Clerk can't place was undone", async () => { + await run(baseOptions); + const [first] = listRuns(runsDir()); + const undoRun = startRun(runsDir(), { + kind: "undo", + target: { instanceId: "ins_1" }, + undoes: first!.id, + }); + undoRun.append({ sourceId: "u1", status: "deleted", clerkId: "user_u1" }); + undoRun.append({ sourceId: "u2", status: "deleted", clerkId: "user_u2" }); + undoRun.finish(); + + requests = []; + stubClerk({ instanceDown: true }); + await run(baseOptions); + + expect(created()).toEqual(["u1", "u2"]); + expect(listRuns(runsDir()).filter((record) => record.kind === "import")).toHaveLength(2); + }); + // Fixing the row a partial run skipped changes the file, so its run meets // the users the first one created: the refusal says which run that was. test("a changed file's refusal names the earlier run that created the users it rejects", async () => { diff --git a/packages/cli-core/src/commands/migrate/run.ts b/packages/cli-core/src/commands/migrate/run.ts index 7cb522091..aab7fb8bf 100644 --- a/packages/cli-core/src/commands/migrate/run.ts +++ b/packages/cli-core/src/commands/migrate/run.ts @@ -46,7 +46,12 @@ import { confirm } from "../../lib/prompts.ts"; import { interruptedExitCode } from "../../lib/signals.ts"; import { withGutter, withSpinner } from "../../lib/spinner.ts"; import { importUsers, splitIdentifiers } from "./import-users.ts"; -import { ALREADY_IMPORTED, checkImport, type ImportChecks } from "./lib/checks.ts"; +import { + ALREADY_IMPORTED, + APP_ID_PLACEHOLDER, + checkImport, + type ImportChecks, +} from "./lib/checks.ts"; import { fetchInstanceSettings, fetchUserCount } from "./lib/clerk-config.ts"; import { readEnvelope, type ExportEnvelope } from "./lib/export-file.ts"; import { @@ -246,6 +251,15 @@ export function explainErrors(errors: Iterable, instanceType: InstanceTy return notes; } +/** Every message a summary groups, for {@link explainErrors}. */ +function explainedErrors(summary: ImportSummary): string[] { + return [ + ...summary.errorBreakdown.keys(), + ...summary.droppedPhones.keys(), + ...summary.droppedIdentifiers.keys(), + ]; +} + // --- Settling the file, source and target ---------------------------------- /** @@ -571,7 +585,9 @@ function findResume( (record) => record.kind === "import" && record.file?.sha256 === match.sha256 && - isClerkInstanceId(record.target.instanceId), + isClerkInstanceId(record.target.instanceId) && + // An undone run holds no users, so starting over beside it is safe. + !undoneInFull(runsDir, record), ); if (unconfirmed) { throwUsageError( @@ -658,9 +674,20 @@ function printChecks(checks: ImportChecks): void { log.info(` ${fix.label}`); log.info(dim(` ${fix.command ?? fix.url}`)); } + // A key names no app, so the commands carry a placeholder to fill in. + if (hasAppIdPlaceholder(checks)) { + log.info( + dim(` Replace ${APP_ID_PLACEHOLDER} with the app's ID: \`clerk apps list\` shows it.`), + ); + } } } +/** Whether a fix command names the app with {@link APP_ID_PLACEHOLDER}. */ +function hasAppIdPlaceholder(checks: ImportChecks): boolean { + return checks.fixes.some((fix) => fix.command?.includes(` --app ${APP_ID_PLACEHOLDER} `)); +} + function checksJson(checks: ImportChecks) { return { total: checks.total, @@ -671,6 +698,9 @@ function checksJson(checks: ImportChecks) { skippedReasons: checks.skippedReasons, warnings: checks.warnings, fixes: checks.fixes, + ...(hasAppIdPlaceholder(checks) + ? { placeholders: { [APP_ID_PLACEHOLDER]: "the app's ID: `clerk apps list` shows it" } } + : {}), ...(checks.quota ? { quota: checks.quota } : {}), settingsUnavailable: checks.settingsUnavailable, }; @@ -717,8 +747,14 @@ function formatSummary( lines.push(` ${plural(count, "user")}: ${reason}`); } } - const explained = [...summary.errorBreakdown.keys(), ...summary.droppedPhones.keys()]; - for (const note of explainErrors(explained, instanceType)) lines.push("", note); + // The same for an extra email or phone Clerk refused once the user existed. + if (summary.droppedIdentifiers.size > 0) { + lines.push("", bold("Imported without an email or phone Clerk refused:")); + for (const [reason, count] of summary.droppedIdentifiers) { + lines.push(` ${plural(count, "user")}: ${reason}`); + } + } + for (const note of explainErrors(explainedErrors(summary), instanceType)) lines.push("", note); lines.push("", dim(`Run ${record.id}: ${runFolder}`)); return lines; } @@ -1325,10 +1361,23 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi if (options.dryRun) { setTelemetryStage("migrate_dry_run"); - if (options.json) preview({ dryRun: true }); + if (options.json) preview({ dryRun: true, ...(refused ? { refused: true } : {}) }); else { log.blank(); log.info(dim("Dry run: nothing was written.")); + // Said in words too, or the gutter would close on "Done". + if (refused) { + const nonePass = checks.importable.length === 0 && attachOnly.length === 0; + log.warn( + `The import would refuse: ${plural(checks.rejects.length, "user")} would be rejected. ` + + (nonePass + ? "Fix them first: with every user rejected, --allow-partial would import nobody." + : `Fix them, or pass --allow-partial to import the other ${plural(checks.importable.length + attachOnly.length, "user")}.`), + ); + if (!nonePass) { + setNextSteps([commandFor(options, input.fromExport, ["--allow-partial", "--yes"])]); + } + } } // The exit code says what the real run would do. if (refused) { @@ -1541,6 +1590,7 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi failed: 0, notSent: 0, droppedPhones: new Map(), + droppedIdentifiers: new Map(), validationFailed: 0, errorBreakdown: new Map(), }; @@ -1576,10 +1626,19 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi ...(summary.stopReason ? { stopReason: summary.stopReason } : {}), skipped: checks.rejects.length + checks.skipped.length + withoutPassword.length, errors: [...summary.errorBreakdown].map(([error, count]) => ({ error, count })), - warnings: [...summary.droppedPhones].map(([reason, count]) => ({ - warning: `imported without their phone: ${reason}`, - count, - })), + warnings: [ + ...[...summary.droppedPhones].map(([reason, count]) => ({ + warning: `imported without their phone: ${reason}`, + count, + })), + ...[...summary.droppedIdentifiers].map(([reason, count]) => ({ + warning: `imported without an email or phone Clerk refused: ${reason}`, + count, + })), + ], + // What the human summary adds to Clerk's messages: whether the + // operator can act on them, and how. + notes: explainErrors(explainedErrors(summary), limits.instanceType), }, }, null, diff --git a/packages/cli-core/src/commands/migrate/runs.test.ts b/packages/cli-core/src/commands/migrate/runs.test.ts index e9be4cd45..3348d60ed 100644 --- a/packages/cli-core/src/commands/migrate/runs.test.ts +++ b/packages/cli-core/src/commands/migrate/runs.test.ts @@ -172,6 +172,49 @@ describe("runs ", () => { expect(err).toContain("1 user: bad gateway (a re-run checks)"); }); + // Imported, so not failed, but without an email or phone Clerk refused once + // the user existed: only `users.ndjson` said so before. + test("warns about users imported without an email or phone Clerk refused", async () => { + fs.writeFileSync(usersFile(), "[]"); + const run = startRun(runsDir, { + kind: "import", + target: TARGET, + source: "clerk", + file: { path: usersFile(), sha256: sha256File(usersFile()) }, + }); + run.append({ sourceId: "a", status: "created", clerkId: "user_a" }); + run.append({ + sourceId: "a", + status: "created", + clerkId: "user_a", + error: "Failed to add unverified phone +15555550100: That phone number is taken.", + refused: ["unverified phone: That phone number is taken."], + }); + const record = run.finish(); + + await runs(record.id, { runsDir, json: true }); + expect(JSON.parse(captured.out)).toMatchObject({ + run: { state: "complete", counts: { created: 1 } }, + errors: [], + warnings: [ + { + warning: + "imported without an email or phone Clerk refused: unverified phone: That phone number is taken.", + count: 1, + }, + ], + importedWithout: [{ sourceId: "a" }], + }); + + captured.clear(); + await runs(record.id, { runsDir }); + const err = Bun.stripANSI(captured.err); + expect(err).toContain( + "1 user: imported without an email or phone Clerk refused: unverified phone: That phone number is taken.", + ); + expect(err).toContain("Imported without an email or phone Clerk refused (1)"); + }); + // A stopped import's unsent users have no lines: only the count says so. test("shows the users a stopped import never sent, in the list and the run", async () => { fs.writeFileSync(usersFile(), "[]"); diff --git a/packages/cli-core/src/commands/migrate/runs.ts b/packages/cli-core/src/commands/migrate/runs.ts index d8b68dc28..7346b8796 100644 --- a/packages/cli-core/src/commands/migrate/runs.ts +++ b/packages/cli-core/src/commands/migrate/runs.ts @@ -112,6 +112,21 @@ function errorBreakdown(lines: UserLine[]): { error: string; count: number }[] { return [...counts].map(([error, count]) => ({ error, count })).sort((a, b) => b.count - a.count); } +/** Users imported without an email or phone Clerk refused, by kind and reason. */ +function warningBreakdown(lines: UserLine[]): { warning: string; count: number }[] { + const counts = new Map(); + for (const line of lines) { + if (line.status !== "created") continue; + for (const reason of line.refused ?? []) counts.set(reason, (counts.get(reason) ?? 0) + 1); + } + return [...counts] + .map(([reason, count]) => ({ + warning: `imported without an email or phone Clerk refused: ${reason}`, + count, + })) + .sort((a, b) => b.count - a.count); +} + /** * The import that continues this run: its file (or export run) and source, * with the flags that reach the same runs folder and instance, ending in @@ -237,7 +252,11 @@ function showJson(runsDir: string, record: RunRecord, carry: string[]) { run: { ...record, state }, next: nextCommands(runsDir, record, state, carry, lines), errors: errorBreakdown(lines), + warnings: warningBreakdown(lines), failed: lines.filter((line) => line.status === "failed"), + importedWithout: lines.filter( + (line) => line.status === "created" && (line.refused?.length ?? 0) > 0, + ), skipped: lines.filter((line) => line.status === "skipped"), unconfirmed: lines.filter((line) => line.status === "creating"), }; @@ -289,7 +308,16 @@ function printRun(runsDir: string, record: RunRecord, carry: string[]): void { } } + if (view.warnings.length > 0) { + log.blank(); + log.info(bold("Warnings")); + for (const { warning, count } of view.warnings) { + log.info(` ${yellow(String(count))} ${count === 1 ? "user" : "users"}: ${warning}`); + } + } + printUsers("Failed", view.failed, usersFile); + printUsers("Imported without an email or phone Clerk refused", view.importedWithout, usersFile); printUsers( "Not confirmed (Clerk may have created them; a continue checks)", view.unconfirmed, diff --git a/packages/cli-core/src/commands/migrate/types.ts b/packages/cli-core/src/commands/migrate/types.ts index eaf3746bb..b415ecbb9 100644 --- a/packages/cli-core/src/commands/migrate/types.ts +++ b/packages/cli-core/src/commands/migrate/types.ts @@ -63,6 +63,12 @@ export type ImportSummary = { * count as imported, so these are warnings, not failures. */ droppedPhones: Map; + /** + * Users created without an extra or unverified email or phone that Clerk + * refused once the user existed, by kind and Clerk's reason. Imported, so + * warnings too. + */ + droppedIdentifiers: Map; validationFailed: number; errorBreakdown: Map; }; From 3551c11e786d97e8d625a4f8865867067db1b9b1 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Tue, 3 Nov 2026 20:21:43 -0600 Subject: [PATCH 02/10] fix(migrate): only call an attach refused when Clerk refused the identifier A 401 says the key stopped working, and an exhausted 429 says nothing about the email or phone, so neither is recorded as `refused` or warned about. Also lists `refused` among the users.ndjson fields, splits the `runs ` README sentence, and rewrites the changeset in short sentences. Co-Authored-By: Claude Opus 5.5 --- .changeset/migrate-final-testing-fixes.md | 2 +- .../cli-core/src/commands/migrate/README.md | 20 +++++++++---------- .../src/commands/migrate/import-users.test.ts | 20 +++++++++++++++++++ .../src/commands/migrate/import-users.ts | 12 +++++++---- 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/.changeset/migrate-final-testing-fixes.md b/.changeset/migrate-final-testing-fixes.md index d9ba29e89..97ac8c394 100644 --- a/.changeset/migrate-final-testing-fixes.md +++ b/.changeset/migrate-final-testing-fixes.md @@ -2,4 +2,4 @@ "clerk": patch --- -Report users that `clerk migrate import` created without an email or phone Clerk refused, in the summary, `--json` and `clerk migrate runs `, and fix several misleading messages: a mistyped secret key now gets Clerk's invalid-key error, a refused dry run says so, a gateway's HTML error page no longer fills the error breakdown, the `APP_ID` placeholder in fix commands is explained, and `--json` carries the same notes the human summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. +`clerk migrate import` now warns about users it created without an email or phone Clerk refused, in the summary, `--json` and `clerk migrate runs `. A mistyped secret key gets Clerk's invalid-key error. A refused dry run says so and prints the `--allow-partial` command. A gateway's HTML error page no longer fills the error breakdown. The checks explain how to replace `APP_ID` in fix commands, and `--json` carries the notes the summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. diff --git a/packages/cli-core/src/commands/migrate/README.md b/packages/cli-core/src/commands/migrate/README.md index c126d7650..7d90f5ef4 100644 --- a/packages/cli-core/src/commands/migrate/README.md +++ b/packages/cli-core/src/commands/migrate/README.md @@ -139,11 +139,11 @@ leaves `.gitignore` alone. Each run is a folder named for its ID, `YYYYMMDD-HHmmss-xxxx`: -| File | Contents | -| -------------- | -------------------------------------------------------------------------------------------------------------------------------------- | -| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | -| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending` or `passwordDropped` when present | -| `lock` | The PID of the process writing the run, while it runs | +| File | Contents | +| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | +| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | +| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending`, `refused` or `passwordDropped` when present | +| `lock` | The PID of the process writing the run, while it runs | On a `failed` line, `code` is the HTTP status, such as `422` or `429`, or `unknown`. On a `skipped` line it gives the reason in machine form: @@ -1034,11 +1034,11 @@ clerk migrate runs --json | `--runs-dir ` | Read runs from somewhere else | It prints the runs folder first. The listing shows each run's ID, date, kind, -status, target, file and counts. `runs ` adds the error breakdown, the -warnings for users imported without an email or phone Clerk refused -(`warnings` and `importedWithout` in `--json`), and the users that failed, were -skipped, or are `creating` with no answer to confirm the create (`unconfirmed` -in `--json`), with the path to the full record, then what +status, target, file and counts. `runs ` adds the error breakdown and the +users that failed, were skipped, or are `creating` with no answer to confirm the +create (`unconfirmed` in `--json`). It also warns about users imported without +an email or phone Clerk refused (`warnings` and `importedWithout` in `--json`). +Then it prints the path to the full record, and what to run next: for an interrupted or partial import, the import that continues it, with the flags it ran with, then the undo. The continue is left out when importing the file again might not reach this run: a newer run of the same file diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index 2362d7ebf..db479cffa 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -439,6 +439,26 @@ describe("importUsers", () => { expect([...summary.droppedIdentifiers]).toEqual([["additional email: that email is taken", 1]]); }); + // A key Clerk stopped accepting says nothing about the email itself. + test("notes an attach the key was refused for, without calling it refused", async () => { + stub((url) => + url.endsWith("/v1/email_addresses") + ? clerkError(401, "The provided Clerk Secret Key is invalid.") + : ok("user_created"), + ); + + const summary = await importUsers({ + users: [user({ email: ["a@x.dev", "b@x.dev"] })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)?.error).toContain("Failed to add additional email b@x.dev"); + expect(lines.at(-1)).not.toHaveProperty("refused"); + expect(summary.droppedIdentifiers.size).toBe(0); + }); + test("counts each user once per reason Clerk refused its identifiers", async () => { stub((url) => url.endsWith("/v1/email_addresses") diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index e7abb75b1..c82c9a9aa 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -283,10 +283,14 @@ async function attachIdentifier( if (error instanceof NotSentError || outcomeUnknown(error)) return { pending: true }; const kindLabel = `${verified ? "additional" : "unverified"} ${kind}`; const { message } = failureOf(error); - return { - note: `Failed to add ${kindLabel} ${value}: ${message}`, - refused: `${kindLabel}: ${normalizeErrorMessage(message)}`, - }; + const note = `Failed to add ${kindLabel} ${value}: ${message}`; + // Only Clerk's answer about the identifier itself is a refusal: a key it + // no longer accepts, or a 429 that outlasted the retries, says nothing + // about the email or phone. + const refusal = error instanceof BapiError && error.status !== 401; + return refusal + ? { note, refused: `${kindLabel}: ${normalizeErrorMessage(message)}` } + : { note }; } } From e627f68db30c495bdc15c5ff616eaf4bfcdb7511 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Tue, 3 Nov 2026 20:24:52 -0600 Subject: [PATCH 03/10] fix(migrate): leave an attach pending when the failure isn't about the identifier A 401 or a 429 that outlasted its retries says nothing about the email or phone, so the attach stays pending and a re-run finishes it, rather than being dropped with only a note. Also types `createUser`'s response and adds the Oxford commas the README and changeset were missing. Co-Authored-By: Claude Opus 5.5 --- .changeset/migrate-final-testing-fixes.md | 2 +- .../cli-core/src/commands/migrate/README.md | 10 +++++----- .../src/commands/migrate/import-users.test.ts | 4 ++-- .../src/commands/migrate/import-users.ts | 17 ++++++++--------- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/.changeset/migrate-final-testing-fixes.md b/.changeset/migrate-final-testing-fixes.md index 97ac8c394..319abdc32 100644 --- a/.changeset/migrate-final-testing-fixes.md +++ b/.changeset/migrate-final-testing-fixes.md @@ -2,4 +2,4 @@ "clerk": patch --- -`clerk migrate import` now warns about users it created without an email or phone Clerk refused, in the summary, `--json` and `clerk migrate runs `. A mistyped secret key gets Clerk's invalid-key error. A refused dry run says so and prints the `--allow-partial` command. A gateway's HTML error page no longer fills the error breakdown. The checks explain how to replace `APP_ID` in fix commands, and `--json` carries the notes the summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. +`clerk migrate import` now warns about users it created without an email or phone Clerk refused, in the summary, `--json`, and `clerk migrate runs `. A mistyped secret key gets Clerk's invalid-key error. A refused dry run says so and prints the `--allow-partial` command. A gateway's HTML error page no longer fills the error breakdown. The checks explain how to replace `APP_ID` in fix commands, and `--json` carries the notes the summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. diff --git a/packages/cli-core/src/commands/migrate/README.md b/packages/cli-core/src/commands/migrate/README.md index 7d90f5ef4..361658b38 100644 --- a/packages/cli-core/src/commands/migrate/README.md +++ b/packages/cli-core/src/commands/migrate/README.md @@ -139,11 +139,11 @@ leaves `.gitignore` alone. Each run is a folder named for its ID, `YYYYMMDD-HHmmss-xxxx`: -| File | Contents | -| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | -| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | -| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending`, `refused` or `passwordDropped` when present | -| `lock` | The PID of the process writing the run, while it runs | +| File | Contents | +| -------------- | -------------------------------------------------------------------------------------------------------------------------------------------------- | +| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | +| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending`, `refused`, or `passwordDropped` when present | +| `lock` | The PID of the process writing the run, while it runs | On a `failed` line, `code` is the HTTP status, such as `422` or `429`, or `unknown`. On a `skipped` line it gives the reason in machine form: diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index db479cffa..cb6f5cfb9 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -440,7 +440,7 @@ describe("importUsers", () => { }); // A key Clerk stopped accepting says nothing about the email itself. - test("notes an attach the key was refused for, without calling it refused", async () => { + test("leaves an attach the key was refused for pending, not refused", async () => { stub((url) => url.endsWith("/v1/email_addresses") ? clerkError(401, "The provided Clerk Secret Key is invalid.") @@ -454,7 +454,7 @@ describe("importUsers", () => { record, }); - expect(lines.at(-1)?.error).toContain("Failed to add additional email b@x.dev"); + expect(lines.at(-1)?.pending).toEqual([{ kind: "email", value: "b@x.dev", verified: true }]); expect(lines.at(-1)).not.toHaveProperty("refused"); expect(summary.droppedIdentifiers.size).toBe(0); }); diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index c82c9a9aa..6f2eca55f 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -281,16 +281,15 @@ async function attachIdentifier( return {}; } catch (error) { if (error instanceof NotSentError || outcomeUnknown(error)) return { pending: true }; + // A key Clerk stopped accepting, or a 429 that outlasted the retries, says + // nothing about the email or phone: left pending, a re-run attaches it. + if (!(error instanceof BapiError) || error.status === 401) return { pending: true }; const kindLabel = `${verified ? "additional" : "unverified"} ${kind}`; const { message } = failureOf(error); - const note = `Failed to add ${kindLabel} ${value}: ${message}`; - // Only Clerk's answer about the identifier itself is a refusal: a key it - // no longer accepts, or a 429 that outlasted the retries, says nothing - // about the email or phone. - const refusal = error instanceof BapiError && error.status !== 401; - return refusal - ? { note, refused: `${kindLabel}: ${normalizeErrorMessage(message)}` } - : { note }; + return { + note: `Failed to add ${kindLabel} ${value}: ${message}`, + refused: `${kindLabel}: ${normalizeErrorMessage(message)}`, + }; } } @@ -376,7 +375,7 @@ async function createUser( const notes: string[] = []; let phoneRefusal: string | undefined; let attachLater: string[] = []; - let response; + let response: Awaited>; try { response = await create(body); } catch (error) { From 3be2c83e970ba849231f9bea764fc1b486abb108 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Tue, 3 Nov 2026 20:25:37 -0600 Subject: [PATCH 04/10] fix(migrate): restore createUser's untyped response, which tsc needs Typing it made tsc report a use before assignment on a path it can't follow. Co-Authored-By: Claude Opus 5.5 --- packages/cli-core/src/commands/migrate/import-users.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index 6f2eca55f..8ddd856d5 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -375,7 +375,7 @@ async function createUser( const notes: string[] = []; let phoneRefusal: string | undefined; let attachLater: string[] = []; - let response: Awaited>; + let response; try { response = await create(body); } catch (error) { From a60f7b4f58786428cbeb1a0d7e424cd8529cb3ea Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Tue, 3 Nov 2026 20:34:07 -0600 Subject: [PATCH 05/10] fix(migrate): leave an attach pending on a 403 that isn't about the phone A 403 can mean the key isn't permitted, which says nothing about the email or phone, so the attach stays pending for a re-run. A country Clerk doesn't support (`unsupported_country_code`) is still a refusal of the phone. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/migrate/import-users.test.ts | 45 +++++++++++++++++++ .../src/commands/migrate/import-users.ts | 10 +++-- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index cb6f5cfb9..560a2da87 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -459,6 +459,51 @@ describe("importUsers", () => { expect(summary.droppedIdentifiers.size).toBe(0); }); + test("leaves an attach pending on a 403 that isn't about the phone", async () => { + stub((url) => + url.endsWith("/v1/email_addresses") + ? clerkError(403, "You are not authorized to perform this request.") + : ok("user_created"), + ); + + const summary = await importUsers({ + users: [user({ email: ["a@x.dev", "b@x.dev"] })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)?.pending).toEqual([{ kind: "email", value: "b@x.dev", verified: true }]); + expect(summary.droppedIdentifiers.size).toBe(0); + }); + + // A country Clerk doesn't support is about the phone, so it is a refusal. + test("counts a 403 for the phone's country as refused", async () => { + const country = "Phone numbers from this country (Netherlands) are currently not supported."; + stub((url) => + url.endsWith("/v1/phone_numbers") + ? new Response( + JSON.stringify({ + errors: [ + { code: "unsupported_country_code", message: "unsupported", long_message: country }, + ], + }), + { status: 403 }, + ) + : ok("user_created"), + ); + + const summary = await importUsers({ + users: [user({ email: ["a@x.dev"], unverifiedPhoneNumbers: ["+31612345678"] })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)).not.toHaveProperty("pending"); + expect([...summary.droppedIdentifiers]).toEqual([[`unverified phone: ${country}`, 1]]); + }); + test("counts each user once per reason Clerk refused its identifiers", async () => { stub((url) => url.endsWith("/v1/email_addresses") diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index 8ddd856d5..c14ae5366 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -281,9 +281,13 @@ async function attachIdentifier( return {}; } catch (error) { if (error instanceof NotSentError || outcomeUnknown(error)) return { pending: true }; - // A key Clerk stopped accepting, or a 429 that outlasted the retries, says - // nothing about the email or phone: left pending, a re-run attaches it. - if (!(error instanceof BapiError) || error.status === 401) return { pending: true }; + // A key Clerk stopped accepting or doesn't permit, or a 429 that outlasted + // the retries, says nothing about the email or phone: left pending, a + // re-run attaches it. A country Clerk doesn't support is about the phone. + const aboutTheKey = + error instanceof BapiError && + (error.status === 401 || (error.status === 403 && error.code !== "unsupported_country_code")); + if (!(error instanceof BapiError) || aboutTheKey) return { pending: true }; const kindLabel = `${verified ? "additional" : "unverified"} ${kind}`; const { message } = failureOf(error); return { From 2aa694eaac5b0dab18e4486819cf049eac6ef859 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Wed, 4 Nov 2026 14:08:33 -0600 Subject: [PATCH 06/10] fix(migrate): report dropped phones in runs , and match the refused dry run to the real one A first phone Clerk refused on the create now lands on the user's line as `phoneRefused`, so `runs ` warns about it the way the import summary does. A refused dry run now gives the real refusal's advice and next steps, including the earlier run's undo or continue. The README also says which attach errors stay pending. Co-Authored-By: Claude Opus 5.5 --- .changeset/migrate-final-testing-fixes.md | 2 +- .../cli-core/src/commands/migrate/README.md | 25 ++-- .../src/commands/migrate/import-users.test.ts | 2 + .../src/commands/migrate/import-users.ts | 7 +- .../src/commands/migrate/lib/run-store.ts | 5 + .../cli-core/src/commands/migrate/run.test.ts | 27 ++++- packages/cli-core/src/commands/migrate/run.ts | 112 +++++++++--------- .../src/commands/migrate/runs.test.ts | 40 +++++++ .../cli-core/src/commands/migrate/runs.ts | 20 ++-- 9 files changed, 162 insertions(+), 78 deletions(-) diff --git a/.changeset/migrate-final-testing-fixes.md b/.changeset/migrate-final-testing-fixes.md index 319abdc32..7503ce576 100644 --- a/.changeset/migrate-final-testing-fixes.md +++ b/.changeset/migrate-final-testing-fixes.md @@ -2,4 +2,4 @@ "clerk": patch --- -`clerk migrate import` now warns about users it created without an email or phone Clerk refused, in the summary, `--json`, and `clerk migrate runs `. A mistyped secret key gets Clerk's invalid-key error. A refused dry run says so and prints the `--allow-partial` command. A gateway's HTML error page no longer fills the error breakdown. The checks explain how to replace `APP_ID` in fix commands, and `--json` carries the notes the summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. +`clerk migrate import` now warns about users it created without an email or phone Clerk refused, in the summary, `--json`, and `clerk migrate runs `. A mistyped secret key gets Clerk's invalid-key error. A refused dry run says so and gives the same next steps as the real run. A gateway's HTML error page no longer fills the error breakdown. The checks explain how to replace `APP_ID` in fix commands, and `--json` carries the notes the summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. diff --git a/packages/cli-core/src/commands/migrate/README.md b/packages/cli-core/src/commands/migrate/README.md index 361658b38..cff526e16 100644 --- a/packages/cli-core/src/commands/migrate/README.md +++ b/packages/cli-core/src/commands/migrate/README.md @@ -139,11 +139,11 @@ leaves `.gitignore` alone. Each run is a folder named for its ID, `YYYYMMDD-HHmmss-xxxx`: -| File | Contents | -| -------------- | -------------------------------------------------------------------------------------------------------------------------------------------------- | -| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | -| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending`, `refused`, or `passwordDropped` when present | -| `lock` | The PID of the process writing the run, while it runs | +| File | Contents | +| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | +| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending`, `refused`, `phoneRefused`, or `passwordDropped` when present | +| `lock` | The PID of the process writing the run, while it runs | On a `failed` line, `code` is the HTTP status, such as `422` or `429`, or `unknown`. On a `skipped` line it gives the reason in machine form: @@ -916,8 +916,9 @@ Any reject stops the import, and it exits 2 with the command that adds `--allow-partial`. With `--allow-partial`, the rest import and each reject is recorded as `skipped` with its reason. `--dry-run` exits 2 when the real run would be refused, and 0 otherwise. A refused dry run says so after the checks, -with the `--allow-partial` command when any user passes (`refused: true` in -`--json`). +with the advice and commands the real run would give: the `--allow-partial` +command when any user passes, and the earlier run's undo (or continue) when it +created some of the rejected users (`refused: true` in `--json`). ``` Checks @@ -968,13 +969,16 @@ duplicate secondary email should not undo an otherwise successful user — but it is a warning: the summary lists the user under "Imported without an email or phone Clerk refused", by kind and Clerk's reason (`result.warnings` in `--json`), and the user's line records it under `refused`, which `runs ` -reports too. An attach with no answer stays `pending` for a re-run. +reports too. An attach with no answer stays `pending` for a re-run, and so +does one whose error says nothing about the identifier: a `401`, a `403` other +than `unsupported_country_code`, or a `429` that outlasts the retries. The first phone gets the same treatment when Clerk refuses it — a country the instance does not support, or a number that is not E.164 — and the user has an email: the create is retried without the phone. The user counts as imported, and the summary lists them under "Imported without their phone", by Clerk's -reason (`result.warnings` in `--json`). +reason (`result.warnings` in `--json`). The user's line records the reason +under `phoneRefused`, which `runs ` reports too. With [reserved](#verified-vs-unverified-identifiers) phones on the create, the refused one may be any of them, so the verified phone is retried alone first. @@ -1037,7 +1041,8 @@ It prints the runs folder first. The listing shows each run's ID, date, kind, status, target, file and counts. `runs ` adds the error breakdown and the users that failed, were skipped, or are `creating` with no answer to confirm the create (`unconfirmed` in `--json`). It also warns about users imported without -an email or phone Clerk refused (`warnings` and `importedWithout` in `--json`). +their phone, or an email or phone Clerk refused (`warnings` and +`importedWithout` in `--json`). Then it prints the path to the full record, and what to run next: for an interrupted or partial import, the import that continues it, with the flags it ran with, then the undo. The continue is left out when diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index 560a2da87..7631999f4 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -852,6 +852,8 @@ describe("importUsers", () => { expect(lines.at(-1)?.error).toContain( `Failed to add phone +31612345678: ${clerkErr.long_message}`, ); + // On the line too, so `runs ` warns about it as the summary does. + expect(lines.at(-1)).toMatchObject({ status: "created", phoneRefused: clerkErr.long_message }); }); test("drops a refused phone's statuses with it", async () => { diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index c14ae5366..aed4dbf9a 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -684,11 +684,13 @@ export async function importUsers(options: ImportUsersOptions): Promise 0 ? { ...line, pending: toAttach } : line); - if (created.phoneRefusal) { - const reason = normalizeErrorMessage(created.phoneRefusal); - droppedPhones.set(reason, (droppedPhones.get(reason) ?? 0) + 1); - } + if (phoneRefused) droppedPhones.set(phoneRefused, (droppedPhones.get(phoneRefused) ?? 0) + 1); await finishUser(line, toAttach, [...created.notes, ...retries]); successful++; processed++; diff --git a/packages/cli-core/src/commands/migrate/lib/run-store.ts b/packages/cli-core/src/commands/migrate/lib/run-store.ts index 6e613f436..afef0d00a 100644 --- a/packages/cli-core/src/commands/migrate/lib/run-store.ts +++ b/packages/cli-core/src/commands/migrate/lib/run-store.ts @@ -132,6 +132,11 @@ export type UserLine = { * existed, by kind and Clerk's reason. The user is imported without them. */ refused?: string[]; + /** + * On a `created` line: Clerk's reason for refusing the first phone on the + * create, which was retried without it. The user is imported without it. + */ + phoneRefused?: string; /** Why a user was skipped. */ reason?: string; error?: string; diff --git a/packages/cli-core/src/commands/migrate/run.test.ts b/packages/cli-core/src/commands/migrate/run.test.ts index 2b09c8885..4afe82da3 100644 --- a/packages/cli-core/src/commands/migrate/run.test.ts +++ b/packages/cli-core/src/commands/migrate/run.test.ts @@ -1470,7 +1470,7 @@ describe("run", () => { expect(captured.err).toContain("Dry run: nothing was written."); // Said in words, not only by the exit code. expect(Bun.stripANSI(captured.err)).toContain( - "The import would refuse: 1 user would be rejected. Fix them, or pass --allow-partial to import the other 2 users.", + "The import would refuse: 1 user would be rejected. Fix them, or pass --allow-partial to import the rest and record them as skipped.", ); expect(process.exitCode).toBe(2); }); @@ -2681,6 +2681,31 @@ describe("run", () => { expect(created()).toEqual([]); }); + // A dry run predicts the real refusal, the earlier run's undo and all. + test("a changed file's refused dry run names the earlier run and its undo", async () => { + stubClerk({ failing: new Set(["u2"]) }); + await run(baseOptions); + const [first] = listRuns(runsDir()); + fs.writeFileSync( + path.join(workDir, "export.json"), + JSON.stringify([export2[0], { ...export2[1], primary_email_address: "fixed@x.dev" }]), + ); + requests = []; + process.exitCode = 0; + stubClerk({ existing: [{ id: "user_u1", external_id: "u1" }] }); + captured.clear(); + + await run({ ...baseOptions, dryRun: true }); + + const err = Bun.stripANSI(captured.err); + expect(err).toContain( + `The import would refuse: 1 user would be rejected. 1 of them was created by run ${first!.id}, from an earlier version of this file.`, + ); + expect(err).toContain(`clerk migrate undo ${first!.id} --secret-key `); + expect(process.exitCode).toBe(2); + expect(created()).toEqual([]); + }); + test("a changed file is a new run", async () => { await run(baseOptions); fs.writeFileSync( diff --git a/packages/cli-core/src/commands/migrate/run.ts b/packages/cli-core/src/commands/migrate/run.ts index aab7fb8bf..b0f9c95ea 100644 --- a/packages/cli-core/src/commands/migrate/run.ts +++ b/packages/cli-core/src/commands/migrate/run.ts @@ -1359,36 +1359,8 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi if (!options.json) printChecks(checks); - if (options.dryRun) { - setTelemetryStage("migrate_dry_run"); - if (options.json) preview({ dryRun: true, ...(refused ? { refused: true } : {}) }); - else { - log.blank(); - log.info(dim("Dry run: nothing was written.")); - // Said in words too, or the gutter would close on "Done". - if (refused) { - const nonePass = checks.importable.length === 0 && attachOnly.length === 0; - log.warn( - `The import would refuse: ${plural(checks.rejects.length, "user")} would be rejected. ` + - (nonePass - ? "Fix them first: with every user rejected, --allow-partial would import nobody." - : `Fix them, or pass --allow-partial to import the other ${plural(checks.importable.length + attachOnly.length, "user")}.`), - ); - if (!nonePass) { - setNextSteps([commandFor(options, input.fromExport, ["--allow-partial", "--yes"])]); - } - } - } - // The exit code says what the real run would do. - if (refused) { - process.exitCode = 2; - declareSoftExitOutcome("error", "migrate_checks_rejected"); - } - return; - } - - if (refused) { - if (options.json) preview({ refused: true }); + // What a refused import says to do next, which the dry run predicts. + const refusalAdvice = () => { const earlier = earlierRunHolding(runsDir, target.instanceId, checks, sha256); const undoEarlier = earlier ? undoCommand(earlier.id, carry) : undefined; const earlierRun = @@ -1423,35 +1395,65 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi `undo run ${earlier.id} first (\`${undoEarlier}\`) to import the whole file again`, ].join(", or ") : ""; + const advice = earlier + ? `${earlier.count} of them ${earlier.count === 1 ? "was" : "were"} created by run ${earlier.id}, from ${earlier.sameFile ? "this file" : "an earlier version of this file"}. ` + + (continueEarlier + ? `Drop --new-run to continue run ${earlier.id}. Or ${ways}.` + : `${ways[0]!.toUpperCase()}${ways.slice(1)}.`) + : nonePass + ? "Fix them: with every user rejected, --allow-partial would import nobody." + : "Fix them, or pass --allow-partial to import the rest and record them as skipped."; + const steps = [ + ...(continueEarlier + ? [{ command: continueEarlier, description: `Continue run ${earlier!.id}` }] + : []), + ...(nonePass + ? [] + : [ + { + command: commandFor(options, input.fromExport, ["--allow-partial", "--yes"]), + description: "Import the users that pass", + }, + ]), + ...(undoEarlier + ? [{ command: undoEarlier, description: `Undo run ${earlier!.id} first` }] + : []), + ]; + return { advice, steps }; + }; + + if (options.dryRun) { + setTelemetryStage("migrate_dry_run"); + if (options.json) preview({ dryRun: true, ...(refused ? { refused: true } : {}) }); + else { + log.blank(); + log.info(dim("Dry run: nothing was written.")); + // Said in words too, or the gutter would close on "Done". + if (refused) { + const { advice, steps } = refusalAdvice(); + log.warn( + `The import would refuse: ${plural(checks.rejects.length, "user")} would be rejected. ${advice}`, + ); + if (steps.length > 0) setNextSteps(steps.map((step) => step.command)); + } + } + // The exit code says what the real run would do. + if (refused) { + process.exitCode = 2; + declareSoftExitOutcome("error", "migrate_checks_rejected"); + } + return; + } + + if (refused) { + if (options.json) preview({ refused: true }); + const { advice, steps } = refusalAdvice(); throwUsageError( `${plural(checks.rejects.length, "user")} would be rejected, so nothing was imported.` + - `${rejectSummary(checks, options.json)} ` + - (earlier - ? `${earlier.count} of them ${earlier.count === 1 ? "was" : "were"} created by run ${earlier.id}, from ${earlier.sameFile ? "this file" : "an earlier version of this file"}. ` + - (continueEarlier - ? `Drop --new-run to continue run ${earlier.id}. Or ${ways}.` - : `${ways[0]!.toUpperCase()}${ways.slice(1)}.`) - : nonePass - ? "Fix them: with every user rejected, --allow-partial would import nobody." - : "Fix them, or pass --allow-partial to import the rest and record them as skipped."), + `${rejectSummary(checks, options.json)} ${advice}`, undefined, undefined, - [ - ...(continueEarlier - ? [{ command: continueEarlier, description: `Continue run ${earlier!.id}` }] - : []), - ...(nonePass - ? [] - : [ - { - command: commandFor(options, input.fromExport, ["--allow-partial", "--yes"]), - description: "Import the users that pass", - }, - ]), - ...(undoEarlier - ? [{ command: undoEarlier, description: `Undo run ${earlier!.id} first` }] - : []), - ], + steps, ); } diff --git a/packages/cli-core/src/commands/migrate/runs.test.ts b/packages/cli-core/src/commands/migrate/runs.test.ts index 3348d60ed..6bdb0b433 100644 --- a/packages/cli-core/src/commands/migrate/runs.test.ts +++ b/packages/cli-core/src/commands/migrate/runs.test.ts @@ -215,6 +215,46 @@ describe("runs ", () => { expect(err).toContain("Imported without an email or phone Clerk refused (1)"); }); + // Clerk refused the first phone on the create, retried without it: the + // import's summary warned, so `runs ` has to as well. + test("warns about users imported without their phone", async () => { + fs.writeFileSync(usersFile(), "[]"); + const run = startRun(runsDir, { + kind: "import", + target: TARGET, + source: "clerk", + file: { path: usersFile(), sha256: sha256File(usersFile()) }, + }); + run.append({ + sourceId: "a", + status: "created", + clerkId: "user_a", + error: "Failed to add phone +31612345678: Phone numbers from this country are not supported.", + phoneRefused: "Phone numbers from this country are not supported.", + }); + const record = run.finish(); + + await runs(record.id, { runsDir, json: true }); + expect(JSON.parse(captured.out)).toMatchObject({ + warnings: [ + { + warning: + "imported without their phone: Phone numbers from this country are not supported.", + count: 1, + }, + ], + importedWithout: [{ sourceId: "a" }], + }); + + captured.clear(); + await runs(record.id, { runsDir }); + const err = Bun.stripANSI(captured.err); + expect(err).toContain( + "1 user: imported without their phone: Phone numbers from this country are not supported.", + ); + expect(err).toContain("Imported without an email or phone Clerk refused (1)"); + }); + // A stopped import's unsent users have no lines: only the count says so. test("shows the users a stopped import never sent, in the list and the run", async () => { fs.writeFileSync(usersFile(), "[]"); diff --git a/packages/cli-core/src/commands/migrate/runs.ts b/packages/cli-core/src/commands/migrate/runs.ts index 7346b8796..bb5fdb94d 100644 --- a/packages/cli-core/src/commands/migrate/runs.ts +++ b/packages/cli-core/src/commands/migrate/runs.ts @@ -112,18 +112,22 @@ function errorBreakdown(lines: UserLine[]): { error: string; count: number }[] { return [...counts].map(([error, count]) => ({ error, count })).sort((a, b) => b.count - a.count); } -/** Users imported without an email or phone Clerk refused, by kind and reason. */ +/** + * Users imported without their first phone, or an email or phone Clerk + * refused, by reason: worded as the import's `result.warnings`. + */ function warningBreakdown(lines: UserLine[]): { warning: string; count: number }[] { const counts = new Map(); + const add = (warning: string) => counts.set(warning, (counts.get(warning) ?? 0) + 1); for (const line of lines) { if (line.status !== "created") continue; - for (const reason of line.refused ?? []) counts.set(reason, (counts.get(reason) ?? 0) + 1); + if (line.phoneRefused) add(`imported without their phone: ${line.phoneRefused}`); + for (const reason of line.refused ?? []) { + add(`imported without an email or phone Clerk refused: ${reason}`); + } } return [...counts] - .map(([reason, count]) => ({ - warning: `imported without an email or phone Clerk refused: ${reason}`, - count, - })) + .map(([warning, count]) => ({ warning, count })) .sort((a, b) => b.count - a.count); } @@ -255,7 +259,9 @@ function showJson(runsDir: string, record: RunRecord, carry: string[]) { warnings: warningBreakdown(lines), failed: lines.filter((line) => line.status === "failed"), importedWithout: lines.filter( - (line) => line.status === "created" && (line.refused?.length ?? 0) > 0, + (line) => + line.status === "created" && + (line.phoneRefused !== undefined || (line.refused?.length ?? 0) > 0), ), skipped: lines.filter((line) => line.status === "skipped"), unconfirmed: lines.filter((line) => line.status === "creating"), From 98e2290a661ecf37b7c2dc7c8d1e50b3c87572ef Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Wed, 4 Nov 2026 15:05:43 -0600 Subject: [PATCH 07/10] fix(migrate): address Roy's second review of the final-testing fixes - An attach Clerk answers `form_identifier_exists` looks the user up first: when the user already holds it, an earlier attach with no answer landed, so it counts as attached, not refused. A failed lookup leaves it pending. - A refused first phone's reason goes on the retry's `creating` line too, so a continue that adopts the user keeps `phoneRefused` and its note. - A created user's first line carries its notes, so a stop before the second line doesn't lose why the phone was dropped. - A refused `--dry-run --json` carries the real refusal's `advice` and `steps`. - `runs ` lists those users under "Imported without an email or phone", and both commands share the warning prefixes. - Tests for a 429 attach that outlasts the retries, the taken-by-this-user lookup, the adopted phone refusal, and the SMS note on a refused attach. Co-Authored-By: Claude Opus 5.5 --- .../cli-core/src/commands/migrate/README.md | 13 +- .../src/commands/migrate/import-users.test.ts | 210 +++++++++++++++++- .../src/commands/migrate/import-users.ts | 132 ++++++++--- .../src/commands/migrate/lib/failures.ts | 5 + .../src/commands/migrate/lib/run-store.ts | 1 + .../cli-core/src/commands/migrate/run.test.ts | 59 ++++- packages/cli-core/src/commands/migrate/run.ts | 15 +- .../src/commands/migrate/runs.test.ts | 4 +- .../cli-core/src/commands/migrate/runs.ts | 8 +- 9 files changed, 401 insertions(+), 46 deletions(-) diff --git a/packages/cli-core/src/commands/migrate/README.md b/packages/cli-core/src/commands/migrate/README.md index cff526e16..ed57773c2 100644 --- a/packages/cli-core/src/commands/migrate/README.md +++ b/packages/cli-core/src/commands/migrate/README.md @@ -918,7 +918,9 @@ recorded as `skipped` with its reason. `--dry-run` exits 2 when the real run would be refused, and 0 otherwise. A refused dry run says so after the checks, with the advice and commands the real run would give: the `--allow-partial` command when any user passes, and the earlier run's undo (or continue) when it -created some of the rejected users (`refused: true` in `--json`). +created some of the rejected users. In `--json` the preview carries +`refused: true`, with `advice` (the real refusal's sentence) and `steps` (its +commands, each a `command` and `description`). ``` Checks @@ -971,14 +973,18 @@ phone Clerk refused", by kind and Clerk's reason (`result.warnings` in `--json`), and the user's line records it under `refused`, which `runs ` reports too. An attach with no answer stays `pending` for a re-run, and so does one whose error says nothing about the identifier: a `401`, a `403` other -than `unsupported_country_code`, or a `429` that outlasts the retries. +than `unsupported_country_code`, or a `429` that outlasts the retries. An +attach Clerk refuses as taken (`form_identifier_exists`) looks the user up with +`GET /v1/users/{user_id}` first: when the user already holds it, an earlier attach +with no answer landed, and it counts as attached. The first phone gets the same treatment when Clerk refuses it — a country the instance does not support, or a number that is not E.164 — and the user has an email: the create is retried without the phone. The user counts as imported, and the summary lists them under "Imported without their phone", by Clerk's reason (`result.warnings` in `--json`). The user's line records the reason -under `phoneRefused`, which `runs ` reports too. +under `phoneRefused`, which `runs ` reports too. The retry's `creating` +line carries it as well, so a continue that adopts the user keeps it. With [reserved](#verified-vs-unverified-identifiers) phones on the create, the refused one may be any of them, so the verified phone is retried alone first. @@ -1563,6 +1569,7 @@ A run that finishes part-way exits nonzero without throwing, and carries a code | `POST` | `/v1/users` | `migrate import` — creates each user | | `POST` | `/v1/email_addresses` | `migrate import` — attaches additional emails | | `POST` | `/v1/phone_numbers` | `migrate import` — attaches additional phones | +| `GET` | `/v1/users/{user_id}` | `migrate import` — an attach refused as taken checks whether the user already holds it | | `GET` | `/v1/users?limit=&offset=&order_by=+created_at` | `migrate export clerk` — pages the whole instance, oldest first, 500 at a time | | `GET` | `/v1/users?limit=&starting_after=&order_by=+created_at` | `migrate export clerk` — pages the whole instance, oldest first, 500 at a time, each page from the last user of the one before | | `GET` | `/v1/users/count` | `migrate import` — headroom against the instance's user limit | diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index 7631999f4..8c1625888 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -591,6 +591,136 @@ describe("importUsers", () => { expect(lines.at(-1)).not.toHaveProperty("pending"); }); + // Rate limited past the retries says nothing about the email: a re-run + // attaches it, so it is no refusal. + test("keeps an attach pending when its 429 outlasts the retries", async () => { + stub((url) => + url.endsWith("/v1/email_addresses") + ? clerkError(429, "slow down", { "retry-after": "1" }) + : ok("user_created"), + ); + + const summary = await importUsers({ + users: [user({ email: ["a@x.dev", "b@x.dev"] })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)).toMatchObject({ + status: "created", + pending: [{ kind: "email", value: "b@x.dev", verified: true }], + }); + expect(lines.at(-1)).not.toHaveProperty("refused"); + expect(summary.droppedIdentifiers.size).toBe(0); + }, 20_000); + + const taken = () => + new Response( + JSON.stringify({ + errors: [ + { + code: "form_identifier_exists", + message: "That email address is taken. Please try another.", + long_message: "That email address is taken. Please try another.", + }, + ], + }), + { status: 422 }, + ); + const userWith = (emails: string[]) => + new Response( + JSON.stringify({ + id: "user_created", + email_addresses: emails.map((email_address) => ({ email_address })), + }), + { status: 200 }, + ); + + // An earlier attach with no answer landed: the continue's retry finds the + // email taken, by this user, and that is no refusal. + test("counts an attach as done when the email is taken by this user", async () => { + stub((url) => { + if (url.endsWith("/v1/email_addresses")) return taken(); + if (url.endsWith("/v1/users/user_found")) return userWith(["a@x.dev", "B@x.dev"]); + return ok("user_created"); + }); + + const summary = await importUsers({ + users: [], + attachOnly: [ + { + sourceId: "u1", + status: "created", + clerkId: "user_found", + pending: [{ kind: "email", value: "b@x.dev", verified: true }], + }, + ], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)).toMatchObject({ status: "created", clerkId: "user_found" }); + expect(lines.at(-1)).not.toHaveProperty("refused"); + expect(lines.at(-1)).not.toHaveProperty("pending"); + expect(summary.droppedIdentifiers.size).toBe(0); + }); + + test("counts an attach as refused when someone else holds the email", async () => { + stub((url) => { + if (url.endsWith("/v1/email_addresses")) return taken(); + if (url.endsWith("/v1/users/user_found")) return userWith(["a@x.dev"]); + return ok("user_created"); + }); + + const summary = await importUsers({ + users: [], + attachOnly: [ + { + sourceId: "u1", + status: "created", + clerkId: "user_found", + pending: [{ kind: "email", value: "b@x.dev", verified: true }], + }, + ], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)?.refused).toEqual([ + "additional email: That email address is taken. Please try another.", + ]); + expect(summary.droppedIdentifiers.size).toBe(1); + }); + + test("keeps a taken attach pending when the user can't be looked up", async () => { + stub((url) => { + if (url.endsWith("/v1/email_addresses")) return taken(); + if (url.endsWith("/v1/users/user_found")) return clerkError(503, "unavailable"); + return ok("user_created"); + }); + + await importUsers({ + users: [], + attachOnly: [ + { + sourceId: "u1", + status: "created", + clerkId: "user_found", + pending: [{ kind: "email", value: "b@x.dev", verified: true }], + }, + ], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)?.pending).toEqual([{ kind: "email", value: "b@x.dev", verified: true }]); + expect(lines.at(-1)).not.toHaveProperty("refused"); + }); + test("keeps an attach with no answer pending, for a continued run", async () => { stub((url) => url.endsWith("/v1/email_addresses") ? clerkError(503, "unavailable") : ok("user_created"), @@ -856,6 +986,79 @@ describe("importUsers", () => { expect(lines.at(-1)).toMatchObject({ status: "created", phoneRefused: clerkErr.long_message }); }); + // The retry without the phone went out with no answer: the continue that + // adopts the user still knows the phone was lost. + test("keeps a refused phone's reason when the user is adopted on a continue", async () => { + const reason = "Phone numbers from this country (Netherlands) are currently not supported."; + stub((url, attempt) => + url.endsWith("/v1/users") + ? attempt === 1 + ? new Response( + JSON.stringify({ + errors: [{ code: "unsupported_country_code", message: "x", long_message: reason }], + }), + { status: 403 }, + ) + : clerkError(503, "unavailable") + : ok("user_created"), + ); + + await importUsers({ + users: [user({ phone: "+31612345678" })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + expect(allLines.at(-1)).toMatchObject({ status: "creating", phoneRefused: reason }); + + lines = []; + stub(() => ok("user_created")); + const summary = await importUsers({ + users: [user({ phone: "+31612345678" })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + adopted: new Map([["u1", "user_found"]]), + adoptedPhoneRefused: new Map([["u1", allLines.at(-1)!.phoneRefused!]]), + }); + + expect(lines.at(-1)).toMatchObject({ + status: "created", + clerkId: "user_found", + phoneRefused: reason, + error: `Failed to add phone: ${reason}`, + }); + expect([...summary.droppedPhones]).toEqual([[reason, 1]]); + }); + + // A stop between the user's two lines keeps the first: it says why too. + test("puts a refused phone's note on the user's first line", async () => { + stub((url, attempt) => + url.endsWith("/v1/users") && attempt === 1 + ? new Response( + JSON.stringify({ + errors: [{ code: "unsupported_country_code", message: "x", long_message: "No NL." }], + }), + { status: 403 }, + ) + : ok("user_created"), + ); + + await importUsers({ + users: [user({ email: ["a@x.dev", "b@x.dev"], phone: "+31612345678" })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines[0]).toMatchObject({ + status: "created", + phoneRefused: "No NL.", + error: "Failed to add phone +31612345678: No NL.", + pending: [{ kind: "email", value: "b@x.dev", verified: true }], + }); + }); + test("drops a refused phone's statuses with it", async () => { stub((url, attempt) => url.endsWith("/v1/users") && attempt === 1 @@ -1348,9 +1551,10 @@ describe("importUsers", () => { expect(summary).toMatchObject({ successful: 1, failed: 0 }); expect(performance.now() - started).toBeGreaterThanOrEqual(900); expect(requests.filter((r) => r.url.endsWith("/v1/users"))).toHaveLength(2); - expect(lines).toHaveLength(2); - expect(lines[1]).toMatchObject({ status: "created", clerkId: "user_ok" }); - expect(lines[1]?.error).toContain("Rate limit hit (429)"); + // One line, the retry's note on it: nothing was left to attach. + expect(lines).toHaveLength(1); + expect(lines[0]).toMatchObject({ status: "created", clerkId: "user_ok" }); + expect(lines[0]?.error).toContain("Rate limit hit (429)"); }); // The instance is over its limit for every user, so the others wait too. diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index aed4dbf9a..0633088b9 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -190,6 +190,21 @@ function isPhoneRefusal(error: unknown): error is BapiError { ); } +/** True when the user Clerk returned already has this email or phone. */ +function userHolds(user: unknown, kind: PendingIdentifier["kind"], value: string): boolean { + const record = (user ?? {}) as { + email_addresses?: { email_address?: string }[]; + phone_numbers?: { phone_number?: string }[]; + }; + if (kind === "email") { + const wanted = value.toLowerCase(); + return (record.email_addresses ?? []).some( + (entry) => entry.email_address?.toLowerCase() === wanted, + ); + } + return (record.phone_numbers ?? []).some((entry) => entry.phone_number === value); +} + /** A request a stop kept from going out: neither a failure nor unknown. */ class NotSentError extends Error {} @@ -244,8 +259,10 @@ export function pendingIdentifiers( /** * Attaches one extra identifier, backing off on a 429. * - * @returns A note when Clerk refused it, `pending` when nothing says whether - * it attached. Never throws: the user itself was already created. + * @returns A note and the `refused` reason when Clerk refused the identifier. + * `pending` when nothing says whether it attached, or when the error is + * about the key or the rate limit, not the identifier. Never throws: the + * user itself was already created. */ async function attachIdentifier( ctx: CreateContext, @@ -261,26 +278,39 @@ async function attachIdentifier( // A Ctrl-C or a halt ends the wait for a slot or a backoff, and the attach is // left pending for a re-run. The quota does not: the user exists already. const stop = AbortSignal.any([interruptSignal(), ctx.halt]); - try { - await retryOn429( + const send = async (method: "GET" | "POST", requestPath: string, requestBody?: string) => + retryOn429( async () => ctx.schedule( async () => { if (stop.aborted) throw new NotSentError(); return bapiRequest({ - method: "POST", - path, + method, + path: requestPath, secretKey: ctx.secretKey, - body: JSON.stringify(body), + ...(requestBody ? { body: requestBody } : {}), }); }, { first: true, stop }, ), { signal: stop, onRetry: ({ delaySeconds }) => ctx.schedule.pause(delaySeconds * 1000) }, ); + try { + await send("POST", path, JSON.stringify(body)); return {}; } catch (error) { if (error instanceof NotSentError || outcomeUnknown(error)) return { pending: true }; + // Taken may mean taken by this user: an earlier attach with no answer, or + // one a stopped run never wrote down, landed. Its retry is no refusal. + if (error instanceof BapiError && error.code === "form_identifier_exists") { + try { + const held = await send("GET", `/v1/users/${encodeURIComponent(clerkUserId)}`); + if (userHolds(held.body, kind, value)) return {}; + } catch { + // No answer to say it attached: a re-run tries again. + return { pending: true }; + } + } // A key Clerk stopped accepting or doesn't permit, or a 429 that outlasted // the retries, says nothing about the email or phone: left pending, a // re-run attaches it. A country Clerk doesn't support is about the phone. @@ -340,16 +370,19 @@ async function createUser( user: User, identifiers: Identifiers, skipPasswordRequirement: boolean, - /** Called as each attempt goes out, with what it sends. */ - sending: (body: Record) => void, + /** + * Called as each attempt goes out, with what it sends and, on a retry + * without the phone, Clerk's reason the user would be imported without it. + */ + sending: (body: Record, phoneRefusal?: string) => void, ): Promise { // A Ctrl-C or a full instance hands the slot on to queued creates; none // of them is sent, and none waits for a paced turn to find that out. - const create = async (body: Record) => + const create = async (body: Record, phoneRefusal?: string) => ctx.schedule( async () => { if (ctx.stop.aborted) throw new NotSentError(); - sending(body); + sending(body, phoneRefusal); return bapiRequest({ method: "POST", path: "/v1/users", @@ -393,12 +426,30 @@ async function createUser( } = body; let refusal = error; let dropped = sent as string[]; + // Which reserved phone was refused is unknown when there are several, or + // when the verified one was refused alone: each is attached on its own once + // the user exists, unverified, so only a refused one is lost. Worked out + // before each retry too, so its line says what a create cut off would lose. + const settle = (left: string[], why: BapiError) => { + const reservedLeft = left.filter((phone) => phone !== identifiers.primaryPhone); + const later = statuses && (why !== error || reservedLeft.length > 1) ? reservedLeft : []; + const lost = left.filter((phone) => !later.includes(phone)); + return { + later, + lost, + reason: lost.length > 0 ? (why.longMessage ?? why.message) : undefined, + }; + }; // With reserved phones on the create, any one of them may be the refused // one: the verified phone alone goes first, so it is not lost with them. if (statuses && identifiers.primaryPhone) { + const withPrimary = dropped.filter((phone) => phone !== identifiers.primaryPhone); try { - response = await create({ ...withoutPhone, phone_number: [identifiers.primaryPhone] }); - dropped = dropped.filter((phone) => phone !== identifiers.primaryPhone); + response = await create( + { ...withoutPhone, phone_number: [identifiers.primaryPhone] }, + settle(withPrimary, error).reason, + ); + dropped = withPrimary; } catch (retryError) { if (!isPhoneRefusal(retryError)) throw retryError; refusal = retryError; @@ -406,19 +457,13 @@ async function createUser( } if (!response) { if (!body.email_address) throw refusal; - response = await create(withoutPhone); - } - // Which reserved phone was refused is unknown when there are several, or - // when the verified one was refused alone: each is attached on its own once - // the user exists, unverified, so only a refused one is lost. - const reservedDropped = dropped.filter((phone) => phone !== identifiers.primaryPhone); - if (statuses && (refusal !== error || reservedDropped.length > 1)) { - attachLater = reservedDropped; - dropped = dropped.filter((phone) => !attachLater.includes(phone)); + response = await create(withoutPhone, settle(dropped, refusal).reason); } - if (dropped.length > 0) { - phoneRefusal = refusal.longMessage ?? refusal.message; - notes.push(`Failed to add phone ${dropped.join(", ")}: ${phoneRefusal}`); + const settled = settle(dropped, refusal); + attachLater = settled.later; + if (settled.reason) { + phoneRefusal = settled.reason; + notes.push(`Failed to add phone ${settled.lost.join(", ")}: ${phoneRefusal}`); } } @@ -467,6 +512,11 @@ export type ImportUsersOptions = { * the user is adopted, unverified, as they would have been. */ adoptedPending?: Map; + /** + * Clerk's reason for refusing an adopted user's first phone, from its + * `creating` line: the create that landed was the retry without it. + */ + adoptedPhoneRefused?: Map; /** Allow users that carry no password. */ skipPasswordRequirement?: boolean; /** Create the identifiers the source never verified as reserved. */ @@ -495,6 +545,7 @@ export async function importUsers(options: ImportUsersOptions): Promise(), adoptedReserved = new Set(), adoptedPending = new Map(), + adoptedPhoneRefused = new Map(), skipPasswordRequirement = true, reserveUnverified = false, validationFailed = 0, @@ -562,6 +613,7 @@ export async function importUsers(options: ImportUsersOptions): Promise 0 || error) { + // With nothing to attach, the first line already says it all. + if (toAttach.length > 0) { record({ ...base, ...(error ? { error } : {}), @@ -593,6 +646,20 @@ export async function importUsers(options: ImportUsersOptions): Promise { + const phoneRefusal = adoptedPhoneRefused.get(sourceId); + return { + clerkUserId, + notes: phoneRefusal ? [`Failed to add phone: ${phoneRefusal}`] : [], + attachLater: adoptedPending.get(sourceId) ?? [], + ...(phoneRefusal ? { phoneRefusal } : {}), + }; + }; + const processUser = async (user: User): Promise => { const retries: string[] = []; const identifiers = splitIdentifiers(user); @@ -609,10 +676,10 @@ export async function importUsers(options: ImportUsersOptions): Promise - createUser(ctx, user, identifiers, skipPasswordRequirement, (body) => { + createUser(ctx, user, identifiers, skipPasswordRequirement, (body, phoneRefusal) => { // A retry after a refused phone sends without the reserved // ones. Cut off before its answer, the user would be adopted // as reserved, those phones never attached. @@ -624,6 +691,7 @@ export async function importUsers(options: ImportUsersOptions): Promise 0 ? { pending: leftOut.map((value) => ({ @@ -700,7 +768,13 @@ export async function importUsers(options: ImportUsersOptions): Promise 0 ? { ...line, pending: toAttach } : line); + // Its notes too: a run stopped before the next line would lose them. + const firstNotes = [...created.notes, ...retries].join("; "); + record({ + ...line, + ...(firstNotes ? { error: firstNotes } : {}), + ...(toAttach.length > 0 ? { pending: toAttach } : {}), + }); if (phoneRefused) droppedPhones.set(phoneRefused, (droppedPhones.get(phoneRefused) ?? 0) + 1); await finishUser(line, toAttach, [...created.notes, ...retries]); successful++; diff --git a/packages/cli-core/src/commands/migrate/lib/failures.ts b/packages/cli-core/src/commands/migrate/lib/failures.ts index 2e66805b9..d727523be 100644 --- a/packages/cli-core/src/commands/migrate/lib/failures.ts +++ b/packages/cli-core/src/commands/migrate/lib/failures.ts @@ -39,3 +39,8 @@ export function normalizeErrorMessage(message: string): string { return `[${names.map((name) => `"${name}"`).join(" ")}]`; }); } + +/** How `result.warnings` and `runs ` word a user imported without their first phone. */ +export const WITHOUT_PHONE_WARNING = "imported without their phone: "; +/** How they word a user imported without an extra email or phone Clerk refused. */ +export const WITHOUT_IDENTIFIER_WARNING = "imported without an email or phone Clerk refused: "; diff --git a/packages/cli-core/src/commands/migrate/lib/run-store.ts b/packages/cli-core/src/commands/migrate/lib/run-store.ts index afef0d00a..c0d648b74 100644 --- a/packages/cli-core/src/commands/migrate/lib/run-store.ts +++ b/packages/cli-core/src/commands/migrate/lib/run-store.ts @@ -135,6 +135,7 @@ export type UserLine = { /** * On a `created` line: Clerk's reason for refusing the first phone on the * create, which was retried without it. The user is imported without it. + * On a `creating` line, the retry's: a continue that adopts the user keeps it. */ phoneRefused?: string; /** Why a user was skipped. */ diff --git a/packages/cli-core/src/commands/migrate/run.test.ts b/packages/cli-core/src/commands/migrate/run.test.ts index 4afe82da3..b9140f71c 100644 --- a/packages/cli-core/src/commands/migrate/run.test.ts +++ b/packages/cli-core/src/commands/migrate/run.test.ts @@ -1483,7 +1483,18 @@ describe("run", () => { await run({ ...baseOptions, dryRun: true, json: true }); - expect(JSON.parse(captured.out)).toMatchObject({ dryRun: true, refused: true }); + // The real run's advice and commands, for an agent to act on. + expect(JSON.parse(captured.out)).toMatchObject({ + dryRun: true, + refused: true, + advice: "Fix them, or pass --allow-partial to import the rest and record them as skipped.", + steps: [ + { + command: expect.stringContaining("--allow-partial --yes"), + description: "Import the users that pass", + }, + ], + }); }); test("--dry-run exits 0 when nothing would be rejected", async () => { @@ -1866,6 +1877,52 @@ describe("run", () => { }); }); + // An unverified phone attached after the create, refused for its country: + // the user imports without it, and the summary explains the blocklist. + describe("an attached phone Clerk refuses for its country", () => { + const COUNTRY = + "Phone numbers from this country (France) are currently not supported. For more information, please contact support."; + + beforeEach(() => { + fs.writeFileSync( + path.join(workDir, "export.json"), + JSON.stringify([ + { id: "u1", primary_email_address: "a@x.dev", unverified_phone_numbers: "+33612345678" }, + ]), + ); + const clerk = globalThis.fetch; + globalThis.fetch = (async (input: string | URL | Request, init?: RequestInit) => { + if (init?.method === "POST" && new URL(input.toString()).pathname === "/v1/phone_numbers") { + return Response.json( + { + errors: [ + { code: "unsupported_country_code", message: "unsupported", long_message: COUNTRY }, + ], + }, + { status: 422 }, + ); + } + return clerk(input, init); + }) as typeof fetch; + }); + + test("adds the SMS note to the summary", async () => { + await run(baseOptions); + const err = Bun.stripANSI(captured.err); + expect(err).toContain("Failed: 0"); + expect(err).toContain("Imported without an email or phone Clerk refused:"); + expect(err).toContain("Development instances block SMS"); + }); + + test("adds the SMS note to result.notes in --json", async () => { + await run({ ...baseOptions, json: true }); + const { result } = JSON.parse(captured.out) as { result: { notes: string[] } }; + expect(result.notes.some((note) => note.includes("Development instances block SMS"))).toBe( + true, + ); + }); + }); + describe("a phone Clerk refuses", () => { const COUNTRY = "Phone numbers from this country (France) are currently not supported. For more information, please contact support."; diff --git a/packages/cli-core/src/commands/migrate/run.ts b/packages/cli-core/src/commands/migrate/run.ts index b0f9c95ea..454341e13 100644 --- a/packages/cli-core/src/commands/migrate/run.ts +++ b/packages/cli-core/src/commands/migrate/run.ts @@ -112,6 +112,7 @@ import { getSource, resolveSource, sourceKeys } from "./sources/registry.ts"; import type { ImportSummary, User } from "./types.ts"; import { promptForFile, promptForFirebaseHashConfig, promptForSource } from "./wizard.ts"; import { recordMigrateSource, recordMigrateTarget } from "./telemetry.ts"; +import { WITHOUT_IDENTIFIER_WARNING, WITHOUT_PHONE_WARNING } from "./lib/failures.ts"; import { declareSoftExitOutcome, setTelemetryStage } from "../../lib/telemetry.ts"; import { login } from "../auth/login.ts"; import { link } from "../link/index.ts"; @@ -1152,10 +1153,14 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi const inFlight: string[] = []; const inFlightReserved = new Set(); const inFlightPending = new Map(); + const inFlightPhoneRefused = new Map(); if (continued) { for (const line of latestUserLines(runsDir, continued.id).values()) { if (line.status === "creating") inFlight.push(line.sourceId); if (line.status === "creating" && line.reserved) inFlightReserved.add(line.sourceId); + if (line.status === "creating" && line.phoneRefused) { + inFlightPhoneRefused.set(line.sourceId, line.phoneRefused); + } if (line.status === "creating" && line.pending?.length) { inFlightPending.set( line.sourceId, @@ -1424,8 +1429,9 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi if (options.dryRun) { setTelemetryStage("migrate_dry_run"); - if (options.json) preview({ dryRun: true, ...(refused ? { refused: true } : {}) }); - else { + if (options.json) { + preview({ dryRun: true, ...(refused ? { refused: true, ...refusalAdvice() } : {}) }); + } else { log.blank(); log.info(dim("Dry run: nothing was written.")); // Said in words too, or the gutter would close on "Done". @@ -1581,6 +1587,7 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi adopted, adoptedReserved: inFlightReserved, adoptedPending: inFlightPending, + adoptedPhoneRefused: inFlightPhoneRefused, skipPasswordRequirement: !options.requirePassword, reserveUnverified, progress, @@ -1630,11 +1637,11 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi errors: [...summary.errorBreakdown].map(([error, count]) => ({ error, count })), warnings: [ ...[...summary.droppedPhones].map(([reason, count]) => ({ - warning: `imported without their phone: ${reason}`, + warning: `${WITHOUT_PHONE_WARNING}${reason}`, count, })), ...[...summary.droppedIdentifiers].map(([reason, count]) => ({ - warning: `imported without an email or phone Clerk refused: ${reason}`, + warning: `${WITHOUT_IDENTIFIER_WARNING}${reason}`, count, })), ], diff --git a/packages/cli-core/src/commands/migrate/runs.test.ts b/packages/cli-core/src/commands/migrate/runs.test.ts index 6bdb0b433..8d840ffaf 100644 --- a/packages/cli-core/src/commands/migrate/runs.test.ts +++ b/packages/cli-core/src/commands/migrate/runs.test.ts @@ -212,7 +212,7 @@ describe("runs ", () => { expect(err).toContain( "1 user: imported without an email or phone Clerk refused: unverified phone: That phone number is taken.", ); - expect(err).toContain("Imported without an email or phone Clerk refused (1)"); + expect(err).toContain("Imported without an email or phone (1)"); }); // Clerk refused the first phone on the create, retried without it: the @@ -252,7 +252,7 @@ describe("runs ", () => { expect(err).toContain( "1 user: imported without their phone: Phone numbers from this country are not supported.", ); - expect(err).toContain("Imported without an email or phone Clerk refused (1)"); + expect(err).toContain("Imported without an email or phone (1)"); }); // A stopped import's unsent users have no lines: only the count says so. diff --git a/packages/cli-core/src/commands/migrate/runs.ts b/packages/cli-core/src/commands/migrate/runs.ts index bb5fdb94d..e72fd8e16 100644 --- a/packages/cli-core/src/commands/migrate/runs.ts +++ b/packages/cli-core/src/commands/migrate/runs.ts @@ -12,7 +12,7 @@ import { throwUsageError } from "../../lib/errors.ts"; import { recordMigrateSource, recordMigrateTarget } from "./telemetry.ts"; import { quoteArg } from "../../lib/json-body.ts"; import { log } from "../../lib/log.ts"; -import { tallyError } from "./lib/failures.ts"; +import { tallyError, WITHOUT_IDENTIFIER_WARNING, WITHOUT_PHONE_WARNING } from "./lib/failures.ts"; import { hasRetryableUsers, latestUserLines, @@ -121,9 +121,9 @@ function warningBreakdown(lines: UserLine[]): { warning: string; count: number } const add = (warning: string) => counts.set(warning, (counts.get(warning) ?? 0) + 1); for (const line of lines) { if (line.status !== "created") continue; - if (line.phoneRefused) add(`imported without their phone: ${line.phoneRefused}`); + if (line.phoneRefused) add(`${WITHOUT_PHONE_WARNING}${line.phoneRefused}`); for (const reason of line.refused ?? []) { - add(`imported without an email or phone Clerk refused: ${reason}`); + add(`${WITHOUT_IDENTIFIER_WARNING}${reason}`); } } return [...counts] @@ -323,7 +323,7 @@ function printRun(runsDir: string, record: RunRecord, carry: string[]): void { } printUsers("Failed", view.failed, usersFile); - printUsers("Imported without an email or phone Clerk refused", view.importedWithout, usersFile); + printUsers("Imported without an email or phone", view.importedWithout, usersFile); printUsers( "Not confirmed (Clerk may have created them; a continue checks)", view.unconfirmed, From 5fa43a3787c3e6e201b8f0472e4fb304e909e286 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Wed, 4 Nov 2026 17:13:14 -0600 Subject: [PATCH 08/10] fix(migrate): address Roy's third review of the final-testing fixes - An email the instance's allowlist or blocklist refuses (`not_allowed_access`) is a refusal, not a key problem left pending. - A taken phone counts as attached when the user holds it in E.164, whatever the source wrote; emails are trimmed. - A lone reserved phone a retry already counts as refused isn't also pending, so a continue that adopts the user warns about it once. - An attach left pending on a Clerk error notes why, once across continues, and `runs ` lists those users. - `runs ` drops its warnings once the run is undone in full. - A refused dry run prints its next steps in agent mode too, and a real refusal's `--json` carries `advice` and `steps`. - A key Clerk rejects names where it came from, with Clerk's long message. - Tests for the continue's phone wiring, the dry run's next steps, and a 403 for the country; README and changeset brought up to date. Co-Authored-By: Claude Opus 5.5 --- .changeset/migrate-final-testing-fixes.md | 2 +- .../cli-core/src/commands/migrate/README.md | 51 +++-- .../src/commands/migrate/export/clerk.ts | 8 +- .../src/commands/migrate/import-users.test.ts | 179 +++++++++++++++++- .../src/commands/migrate/import-users.ts | 75 ++++++-- .../src/commands/migrate/lib/run-store.ts | 8 + .../src/commands/migrate/lib/target.test.ts | 8 +- .../src/commands/migrate/lib/target.ts | 22 ++- .../cli-core/src/commands/migrate/run.test.ts | 99 ++++++++-- packages/cli-core/src/commands/migrate/run.ts | 21 +- .../src/commands/migrate/runs.test.ts | 41 +++- .../cli-core/src/commands/migrate/runs.ts | 18 +- 12 files changed, 439 insertions(+), 93 deletions(-) diff --git a/.changeset/migrate-final-testing-fixes.md b/.changeset/migrate-final-testing-fixes.md index 7503ce576..4f9a86e2f 100644 --- a/.changeset/migrate-final-testing-fixes.md +++ b/.changeset/migrate-final-testing-fixes.md @@ -2,4 +2,4 @@ "clerk": patch --- -`clerk migrate import` now warns about users it created without an email or phone Clerk refused, in the summary, `--json`, and `clerk migrate runs `. A mistyped secret key gets Clerk's invalid-key error. A refused dry run says so and gives the same next steps as the real run. A gateway's HTML error page no longer fills the error breakdown. The checks explain how to replace `APP_ID` in fix commands, and `--json` carries the notes the summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. +`clerk migrate import` now warns about users it created without an email or phone Clerk refused, in the summary, `--json`, and `clerk migrate runs `, which also warns about users imported without their first phone. An attach that fails on the key or a `429` that outlasts the retries stays pending for a re-run, with a note saying why, and one Clerk calls taken counts as attached when the user already holds it. A secret key Clerk rejects gets Clerk's error and says where the key came from. A refused dry run says so and gives the same next steps as the real run. A run undone in full no longer blocks an import when Clerk can't name the instance. A gateway's HTML error page no longer fills the error breakdown. The checks explain how to replace `APP_ID` in fix commands, and `--json` carries the notes the summary prints. `clerk migrate` is still experimental and needs `CLERK_EXPERIMENTAL=migrate`. diff --git a/packages/cli-core/src/commands/migrate/README.md b/packages/cli-core/src/commands/migrate/README.md index ed57773c2..408af375e 100644 --- a/packages/cli-core/src/commands/migrate/README.md +++ b/packages/cli-core/src/commands/migrate/README.md @@ -139,11 +139,11 @@ leaves `.gitignore` alone. Each run is a folder named for its ID, `YYYYMMDD-HHmmss-xxxx`: -| File | Contents | -| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | -| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending`, `refused`, `phoneRefused`, or `passwordDropped` when present | -| `lock` | The PID of the process writing the run, while it runs | +| File | Contents | +| -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `run.json` | Kind, status, start and finish times, the target, the source, the file and its sha256, an import's flags, and the counts | +| `users.ndjson` | One line per user outcome: `sourceId`, `clerkId`, `status`, and `reason`, `error`, `code`, `pending`, `reserved`, `refused`, `phoneRefused`, or `passwordDropped` when present | +| `lock` | The PID of the process writing the run, while it runs | On a `failed` line, `code` is the HTTP status, such as `422` or `429`, or `unknown`. On a `skipped` line it gives the reason in machine form: @@ -154,9 +154,10 @@ asked to skip. A user's status is `creating`, `created`, `failed`, `skipped`, `deleted` or `exported`. The last line for each `sourceId` wins. A `429` retry, an extra -email or phone that did not attach, a first phone Clerk refused (which the -summary also counts), and a validation failure all land in the line's `error` -field. +email or phone Clerk refused, a first phone Clerk refused (which the summary +also counts), and a validation failure all land in the line's `error` field. So +does why an attach was left `pending`, when Clerk said: "Not added yet", which a +continue replaces rather than repeats. `creating` is written as a user's `POST /v1/users` goes out. It stays the latest status when no answer says whether the create landed: an abort, a network error, @@ -779,8 +780,10 @@ the flag: it continues this run and creates them. A skip for the file's own data something to retry: fix the file, which makes a new run. `--json` returns `{ target, run, resume, checks, result }`. When a run stops -before importing, it carries one of `dryRun: true`, `refused: true`, -`consent: "required"` or `nothingToImport: true` in place of `result`, and a +before importing, it carries `dryRun: true`, `refused: true`, +`consent: "required"` or `nothingToImport: true` in place of `result` (a +refused dry run carries both `dryRun` and `refused`, and a refusal carries +`advice` and `steps`), and a file already imported in full returns `alreadyImported: true`. With `--require-password`, `withoutPassword` counts the users it left out before the checks, so on a first import `checks.total` plus it is the file's size. A @@ -808,9 +811,10 @@ Clerk cannot name the instance (`GET /v1/instance` failed, often from rate limiting right after a large import) and a run of this file exists under a real instance ID, it exits 2 rather than starting over: try again, or pass `--new-run`. A run undone in full holds no users, so it doesn't count. A key -Clerk refuses (`401`) is not a failed lookup: the import stops with Clerk's -error. A continue whose run changed while it waited at the prompt, from -an undo or another continue, exits 2 too, with nothing written. +Clerk refuses (`401`) is not a failed lookup: the import stops, naming where the +key came from and Clerk's error, and so do `migrate undo` and +`migrate export clerk`. A continue whose run changed while it waited at the +prompt, from an undo or another continue, exits 2 too, with nothing written. A continued run also finishes what the last one left open: @@ -973,10 +977,13 @@ phone Clerk refused", by kind and Clerk's reason (`result.warnings` in `--json`), and the user's line records it under `refused`, which `runs ` reports too. An attach with no answer stays `pending` for a re-run, and so does one whose error says nothing about the identifier: a `401`, a `403` other -than `unsupported_country_code`, or a `429` that outlasts the retries. An -attach Clerk refuses as taken (`form_identifier_exists`) looks the user up with -`GET /v1/users/{user_id}` first: when the user already holds it, an earlier attach -with no answer landed, and it counts as attached. +than `unsupported_country_code` or `not_allowed_access` (the instance's +allowlist or blocklist), or a `429` that outlasts the retries. Those note why +on the line, and `runs ` lists them. An attach Clerk refuses as taken +(`form_identifier_exists`) looks the user up with `GET /v1/users/{user_id}` +first: when the user already holds it, an earlier attach with no answer landed, +and it counts as attached. When the lookup gets no answer, the attach stays +`pending`. The first phone gets the same treatment when Clerk refuses it — a country the instance does not support, or a number that is not E.164 — and the user has an @@ -1048,7 +1055,8 @@ status, target, file and counts. `runs ` adds the error breakdown and the users that failed, were skipped, or are `creating` with no answer to confirm the create (`unconfirmed` in `--json`). It also warns about users imported without their phone, or an email or phone Clerk refused (`warnings` and -`importedWithout` in `--json`). +`importedWithout` in `--json`), unless the run was undone in full, and lists +users with identifiers left to attach and why (`leftToAttach`). Then it prints the path to the full record, and what to run next: for an interrupted or partial import, the import that continues it, with the flags it ran with, then the undo. The continue is left out when @@ -1419,12 +1427,15 @@ refused; pass `--new-run` to change modes. A user a run adopts keeps the mode its first create recorded (`reserved` on its `creating` line). When Clerk refused a phone and the retry went out without the reserved phones, that line lists them as `pending`, and a continue that adopts the user attaches them, -unverified (one Clerk refuses again fails with a note). +unverified. One Clerk refuses again is a warning, recorded under `refused`. A +lone reserved phone the retry already counts as refused is under `phoneRefused` +instead, and isn't sent again. A reserved identifier is checked for duplicates like a verified one, since it goes on the user's create. If an instance user, or an earlier record in the file that passes the checks, already holds it, the whole user is rejected. -Without reserving, only that address fails to attach, with a note. If that +Without reserving, only that address fails to attach: the user is imported +with a warning, and the address is recorded under `refused`. If that rejects users you want, import without reserving. ### Firebase hash parameters diff --git a/packages/cli-core/src/commands/migrate/export/clerk.ts b/packages/cli-core/src/commands/migrate/export/clerk.ts index fd2bdc3de..8e4507e9c 100644 --- a/packages/cli-core/src/commands/migrate/export/clerk.ts +++ b/packages/cli-core/src/commands/migrate/export/clerk.ts @@ -280,15 +280,15 @@ export async function exportClerk(options: ExportClerkOptions): Promise { }); await withGutter("Exporting users from Clerk", async () => { - const identity = await fetchInstanceIdentity(source.secretKey); - // A key picks the instance on its own and ignores --instance, so the two - // must agree, as on import: otherwise this reads the wrong user pool under - // the requested instance's name. const keyFrom = options.secretKey ? KEY_SOURCE.secretKeyFlag : !options.app && process.env.CLERK_SECRET_KEY ? KEY_SOURCE.envVar : undefined; + const identity = await fetchInstanceIdentity(source.secretKey, keyFrom ?? source.target); + // A key picks the instance on its own and ignores --instance, so the two + // must agree, as on import: otherwise this reads the wrong user pool under + // the requested instance's name. if (keyFrom) assertInstanceFlagMatches({ instance: options.instance }, keyFrom, identity); const keySource = keyFrom ?? source.target; const target = { diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index 8c1625888..c3b775107 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -456,9 +456,44 @@ describe("importUsers", () => { expect(lines.at(-1)?.pending).toEqual([{ kind: "email", value: "b@x.dev", verified: true }]); expect(lines.at(-1)).not.toHaveProperty("refused"); + // Why it is still pending, which `runs ` shows. + expect(lines.at(-1)?.error).toBe( + "Not added yet: additional email b@x.dev: The provided Clerk Secret Key is invalid.", + ); expect(summary.droppedIdentifiers.size).toBe(0); }); + // The continue tries the attach again: its note replaces the earlier one. + test("notes a pending attach once across continues", async () => { + stub((url) => + url.endsWith("/v1/email_addresses") + ? clerkError(401, "The provided Clerk Secret Key is invalid.") + : ok("user_created"), + ); + + await importUsers({ + users: [], + attachOnly: [ + { + sourceId: "u1", + status: "created", + clerkId: "user_found", + refused: ["additional email: taken"], + error: + "Failed to add additional email c@x.dev: taken; Not added yet: additional email b@x.dev: The provided Clerk Secret Key is invalid.", + pending: [{ kind: "email", value: "b@x.dev", verified: true }], + }, + ], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)?.error).toBe( + "Failed to add additional email c@x.dev: taken; Not added yet: additional email b@x.dev: The provided Clerk Secret Key is invalid.", + ); + }); + test("leaves an attach pending on a 403 that isn't about the phone", async () => { stub((url) => url.endsWith("/v1/email_addresses") @@ -504,6 +539,38 @@ describe("importUsers", () => { expect([...summary.droppedIdentifiers]).toEqual([[`unverified phone: ${country}`, 1]]); }); + // The instance's allowlist or blocklist is about the email, not the key. + test("counts a 403 for an email the instance doesn't allow as refused", async () => { + const blocked = "b@x.dev is not allowed to access this application."; + stub((url) => + url.endsWith("/v1/email_addresses") + ? new Response( + JSON.stringify({ + errors: [ + { + code: "not_allowed_access", + message: "Access not allowed.", + long_message: blocked, + }, + ], + }), + { status: 403 }, + ) + : ok("user_created"), + ); + + const summary = await importUsers({ + users: [user({ email: ["a@x.dev", "b@x.dev"] })], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)).not.toHaveProperty("pending"); + expect(lines.at(-1)?.error).toBe(`Failed to add additional email b@x.dev: ${blocked}`); + expect([...summary.droppedIdentifiers]).toEqual([[`additional email: ${blocked}`, 1]]); + }); + test("counts each user once per reason Clerk refused its identifiers", async () => { stub((url) => url.endsWith("/v1/email_addresses") @@ -667,6 +734,44 @@ describe("importUsers", () => { expect(summary.droppedIdentifiers.size).toBe(0); }); + // Clerk stores the phone in E.164, whatever the source wrote. + test.each([ + ["+15555550100", "+15555550100", false], + ["+1 555-555-0100", "+15555550100", false], + ["+15555550100", "+15555550199", true], + ])("a taken phone %s, the user holding %s, refused: %s", async (source, held, refused) => { + stub((url) => { + if (url.endsWith("/v1/phone_numbers")) return taken(); + if (url.endsWith("/v1/users/user_found")) { + return Response.json({ + id: "user_found", + email_addresses: [], + phone_numbers: [{ phone_number: held }], + }); + } + return ok("user_created"); + }); + + const summary = await importUsers({ + users: [], + attachOnly: [ + { + sourceId: "u1", + status: "created", + clerkId: "user_found", + pending: [{ kind: "phone", value: source, verified: false }], + }, + ], + secretKey: "sk_test_x", + limits: LIMITS, + record, + }); + + expect(lines.at(-1)).not.toHaveProperty("pending"); + expect(lines.at(-1)?.refused?.length ?? 0).toBe(refused ? 1 : 0); + expect(summary.droppedIdentifiers.size).toBe(refused ? 1 : 0); + }); + test("counts an attach as refused when someone else holds the email", async () => { stub((url) => { if (url.endsWith("/v1/email_addresses")) return taken(); @@ -1125,13 +1230,13 @@ describe("importUsers", () => { expect(requests[1]?.body).toMatchObject({ phone_number: ["+15555550100"] }); expect(requests[1]?.body).not.toHaveProperty("phone_number_identification_status"); - // Recorded before the retry goes out, so a continue can attach them. + // Landed, the verified phone alone says the reserved one was refused: + // recorded before the retry goes out, so a continue knows it is lost, + // not left to attach. const creating = allLines.filter((line) => line.status === "creating"); expect(creating[0]).not.toHaveProperty("pending"); - expect(creating[1]).toMatchObject({ - reserved: true, - pending: [{ kind: "phone", value: "+31612345678", verified: false }], - }); + expect(creating[1]).toMatchObject({ reserved: true, phoneRefused: "bad phone" }); + expect(creating[1]).not.toHaveProperty("pending"); expect(lines.at(-1)?.error).toContain("Failed to add phone +31612345678:"); expect(lines.at(-1)?.error).not.toContain("+15555550100"); }); @@ -1140,6 +1245,7 @@ describe("importUsers", () => { // what the attempt reserved and left out, or the user is adopted as if // nothing was reserved and the left-out phones are never attached. test("keeps reserved and the left-out phones when the retried create gets no answer", async () => { + const left = ["+31612345678", "+31612345679"]; stub((url, attempt) => !url.endsWith("/v1/users") ? ok("idn_1") @@ -1160,20 +1266,79 @@ describe("importUsers", () => { ); const summary = await importUsers({ - users: [withPhones()], + users: [withPhones({ unverifiedPhoneNumbers: left })], secretKey: "sk_test_x", limits: LIMITS, record, reserveUnverified: true, }); + // Either reserved phone may be the refused one: both are attached later. expect(summary.failed).toBe(1); expect(allLines.at(-1)).toMatchObject({ status: "creating", reserved: true, - pending: [{ kind: "phone", value: "+31612345678", verified: false }], + pending: left.map((value) => ({ kind: "phone", value, verified: false })), error: expect.stringContaining("a re-run checks"), }); + expect(allLines.at(-1)).not.toHaveProperty("phoneRefused"); + }); + + // One reserved phone, refused, is lost whichever create landed: a continue + // that adopts the user warns about it once and doesn't send it again. + test.each([ + ["with a verified phone", { phone: "+15555550100" }], + ["without a verified phone", { phone: undefined }], + ])("counts a lone reserved phone as lost once, %s", async (_label, fields) => { + stub((url, attempt) => + !url.endsWith("/v1/users") + ? ok("idn_1") + : attempt === 1 + ? new Response( + JSON.stringify({ + errors: [ + { + code: "form_param_format_invalid", + message: "bad phone", + meta: { param_name: "phone_number" }, + }, + ], + }), + { status: 422 }, + ) + : new Response("upstream", { status: 503 }), + ); + const target = withPhones(fields); + + await importUsers({ + users: [target], + secretKey: "sk_test_x", + limits: LIMITS, + record, + reserveUnverified: true, + }); + const creating = allLines.at(-1)!; + expect(creating).toMatchObject({ status: "creating", phoneRefused: "bad phone" }); + expect(creating).not.toHaveProperty("pending"); + + lines = []; + requests = []; + stub(() => ok("idn_1")); + const summary = await importUsers({ + users: [target], + secretKey: "sk_test_x", + limits: LIMITS, + record, + reserveUnverified: true, + adopted: new Map([["u1", "user_found"]]), + adoptedReserved: new Set(["u1"]), + adoptedPending: new Map(), + adoptedPhoneRefused: new Map([["u1", creating.phoneRefused!]]), + }); + + expect(requests.some((request) => request.url.endsWith("/v1/phone_numbers"))).toBe(false); + expect([...summary.droppedPhones]).toEqual([["bad phone", 1]]); + expect(summary.droppedIdentifiers.size).toBe(0); }); test("keeps reserved when a reserved create gets no answer", async () => { diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index 0633088b9..a05e87947 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -197,14 +197,24 @@ function userHolds(user: unknown, kind: PendingIdentifier["kind"], value: string phone_numbers?: { phone_number?: string }[]; }; if (kind === "email") { - const wanted = value.toLowerCase(); + const wanted = value.trim().toLowerCase(); return (record.email_addresses ?? []).some( (entry) => entry.email_address?.toLowerCase() === wanted, ); } - return (record.phone_numbers ?? []).some((entry) => entry.phone_number === value); + // Clerk stores phones in E.164; the source may carry spaces, dashes or parens. + const digits = (phone: string) => phone.replace(/\D/g, ""); + return (record.phone_numbers ?? []).some( + (entry) => digits(entry.phone_number ?? "") === digits(value), + ); } +/** + * Starts the note on an attach left pending for a re-run, which tries it again: + * a continue drops the earlier one rather than repeating it. + */ +const NOT_ADDED_YET = "Not added yet: "; + /** A request a stop kept from going out: neither a failure nor unknown. */ class NotSentError extends Error {} @@ -261,8 +271,9 @@ export function pendingIdentifiers( * * @returns A note and the `refused` reason when Clerk refused the identifier. * `pending` when nothing says whether it attached, or when the error is - * about the key or the rate limit, not the identifier. Never throws: the - * user itself was already created. + * about the key or the rate limit, not the identifier: then with a note + * saying why, when Clerk gave one. Never throws: the user itself was already + * created. */ async function attachIdentifier( ctx: CreateContext, @@ -313,13 +324,19 @@ async function attachIdentifier( } // A key Clerk stopped accepting or doesn't permit, or a 429 that outlasted // the retries, says nothing about the email or phone: left pending, a - // re-run attaches it. A country Clerk doesn't support is about the phone. + // re-run attaches it. A country Clerk doesn't support, or an email the + // instance's allowlist or blocklist refuses, is about the identifier. const aboutTheKey = error instanceof BapiError && - (error.status === 401 || (error.status === 403 && error.code !== "unsupported_country_code")); - if (!(error instanceof BapiError) || aboutTheKey) return { pending: true }; + (error.status === 401 || + (error.status === 403 && + error.code !== "unsupported_country_code" && + error.code !== "not_allowed_access")); const kindLabel = `${verified ? "additional" : "unverified"} ${kind}`; const { message } = failureOf(error); + if (!(error instanceof BapiError) || aboutTheKey) { + return { pending: true, note: `${NOT_ADDED_YET}${kindLabel} ${value}: ${message}` }; + } return { note: `Failed to add ${kindLabel} ${value}: ${message}`, refused: `${kindLabel}: ${normalizeErrorMessage(message)}`, @@ -331,8 +348,8 @@ async function attachIdentifier( * Attaches each identifier. Extra identifiers are best-effort: a duplicate * secondary email should not undo a user who was otherwise imported. * - * @returns A note per identifier Clerk refused, why by kind, and those still - * pending. + * @returns A note per identifier Clerk refused or left pending with a reason, + * why by kind for those refused, and those still pending. */ async function attachAll( ctx: CreateContext, @@ -349,6 +366,14 @@ async function attachAll( }; } +/** What a create retried without a refused phone leaves out for good. */ +type PhoneRetry = { + /** The phones the user is imported without. */ + lost: string[]; + /** Clerk's reason, when any phone is lost. */ + reason: string | undefined; +}; + type CreatedUser = { clerkUserId: string; notes: string[]; @@ -372,17 +397,18 @@ async function createUser( skipPasswordRequirement: boolean, /** * Called as each attempt goes out, with what it sends and, on a retry - * without the phone, Clerk's reason the user would be imported without it. + * without the phone, the phones the user would be imported without and + * Clerk's reason. */ - sending: (body: Record, phoneRefusal?: string) => void, + sending: (body: Record, retry?: PhoneRetry) => void, ): Promise { // A Ctrl-C or a full instance hands the slot on to queued creates; none // of them is sent, and none waits for a paced turn to find that out. - const create = async (body: Record, phoneRefusal?: string) => + const create = async (body: Record, retry?: PhoneRetry) => ctx.schedule( async () => { if (ctx.stop.aborted) throw new NotSentError(); - sending(body, phoneRefusal); + sending(body, retry); return bapiRequest({ method: "POST", path: "/v1/users", @@ -430,7 +456,7 @@ async function createUser( // when the verified one was refused alone: each is attached on its own once // the user exists, unverified, so only a refused one is lost. Worked out // before each retry too, so its line says what a create cut off would lose. - const settle = (left: string[], why: BapiError) => { + const settle = (left: string[], why: BapiError): PhoneRetry & { later: string[] } => { const reservedLeft = left.filter((phone) => phone !== identifiers.primaryPhone); const later = statuses && (why !== error || reservedLeft.length > 1) ? reservedLeft : []; const lost = left.filter((phone) => !later.includes(phone)); @@ -447,7 +473,7 @@ async function createUser( try { response = await create( { ...withoutPhone, phone_number: [identifiers.primaryPhone] }, - settle(withPrimary, error).reason, + settle(withPrimary, error), ); dropped = withPrimary; } catch (retryError) { @@ -457,7 +483,7 @@ async function createUser( } if (!response) { if (!body.email_address) throw refusal; - response = await create(withoutPhone, settle(dropped, refusal).reason); + response = await create(withoutPhone, settle(dropped, refusal)); } const settled = settle(dropped, refusal); attachLater = settled.later; @@ -679,19 +705,22 @@ export async function importUsers(options: ImportUsersOptions): Promise - createUser(ctx, user, identifiers, skipPasswordRequirement, (body, phoneRefusal) => { + createUser(ctx, user, identifiers, skipPasswordRequirement, (body, retry) => { // A retry after a refused phone sends without the reserved // ones. Cut off before its answer, the user would be adopted - // as reserved, those phones never attached. + // as reserved, those phones never attached. One the retry + // already counts as lost would only be refused again. const sentPhones = new Set((body.phone_number as string[] | undefined) ?? []); const leftOut = reserved - ? identifiers.unverifiedPhones.filter((phone) => !sentPhones.has(phone)) + ? identifiers.unverifiedPhones.filter( + (phone) => !sentPhones.has(phone) && !retry?.lost.includes(phone), + ) : []; attempt = { sourceId: user.userId, status: "creating", ...(reserved ? { reserved: true } : {}), - ...(phoneRefusal ? { phoneRefused: normalizeErrorMessage(phoneRefusal) } : {}), + ...(retry?.reason ? { phoneRefused: normalizeErrorMessage(retry.reason) } : {}), ...(leftOut.length > 0 ? { pending: leftOut.map((value) => ({ @@ -785,7 +814,11 @@ export async function importUsers(options: ImportUsersOptions): Promise { // Its earlier error names identifiers Clerk refused; the new line keeps it. - await finishUser(line, line.pending ?? [], line.error ? [line.error] : []); + // The pending ones are tried again, and noted again if still pending. + const earlier = (line.error ?? "") + .split("; ") + .filter((note) => note && !note.startsWith(NOT_ADDED_YET)); + await finishUser(line, line.pending ?? [], earlier); // Created by this run's earlier attempt, as an adopted user was: the bar, // the summary and `result.created` all count it. successful++; diff --git a/packages/cli-core/src/commands/migrate/lib/run-store.ts b/packages/cli-core/src/commands/migrate/lib/run-store.ts index c0d648b74..967a95bb6 100644 --- a/packages/cli-core/src/commands/migrate/lib/run-store.ts +++ b/packages/cli-core/src/commands/migrate/lib/run-store.ts @@ -497,6 +497,14 @@ export function undosOf(runs: RunRecord[], importId: string): RunRecord[] { return runs.filter((record) => record.kind === "undo" && record.undoes === importId); } +/** True when a run's users are gone: it is marked undone, or an undo of it finished. */ +export function undoneInFull(runsDir: string, record: RunRecord): boolean { + return ( + runState(runsDir, record) === "undone" || + undosOf(listRuns(runsDir), record.id).some((undo) => runState(runsDir, undo) === "complete") + ); +} + /** * The undo of this import that has not finished, if any. `runs` is the * caller's `listRuns`, when it has one already. diff --git a/packages/cli-core/src/commands/migrate/lib/target.test.ts b/packages/cli-core/src/commands/migrate/lib/target.test.ts index 20fc42ee3..b4d8d8a17 100644 --- a/packages/cli-core/src/commands/migrate/lib/target.test.ts +++ b/packages/cli-core/src/commands/migrate/lib/target.test.ts @@ -179,9 +179,11 @@ describe("fetchInstanceIdentity", () => { { status: 401 }, )) as unknown as typeof fetch; - await expect(fetchInstanceIdentity("sk_test_bad")).rejects.toMatchObject({ - status: 401, - code: "clerk_key_invalid", + await expect(fetchInstanceIdentity("sk_test_bad", "--secret-key")).rejects.toMatchObject({ + code: "auth_required", + message: expect.stringContaining( + "Clerk rejected the secret key from --secret-key (401): The provided Clerk Secret Key is invalid.", + ), }); }); }); diff --git a/packages/cli-core/src/commands/migrate/lib/target.ts b/packages/cli-core/src/commands/migrate/lib/target.ts index a437f64aa..99257c34e 100644 --- a/packages/cli-core/src/commands/migrate/lib/target.ts +++ b/packages/cli-core/src/commands/migrate/lib/target.ts @@ -14,7 +14,7 @@ import { bapiRequest } from "../../../lib/bapi.ts"; import { dim } from "../../../lib/color.ts"; import { resolveBapiSecretKey } from "../../../lib/bapi-command.ts"; import { INSTANCE_ALIASES, resolveAppContext } from "../../../lib/config.ts"; -import { ApiError, throwUsageError } from "../../../lib/errors.ts"; +import { ApiError, CliError, ERROR_CODE, throwUsageError } from "../../../lib/errors.ts"; import { ENV_VAR_SOURCE_SUFFIX, resolveKeylessTarget, @@ -181,9 +181,14 @@ export function laterCarry(runsDir: string | undefined, target: RunTarget): stri * same key always addresses the same instance, so it still tells two * instances apart, and nothing is sent anywhere. A 429 is retried first: it is * most likely straight after a large import, which is when `undo` runs. A key - * Clerk refuses (401) throws Clerk's error instead. + * Clerk refuses (401) throws instead, naming where the key came from. + * + * @param keySource - Where the key came from, such as `--secret-key`. */ -export async function fetchInstanceIdentity(secretKey: string): Promise { +export async function fetchInstanceIdentity( + secretKey: string, + keySource?: string, +): Promise { const fallbackEnv = detectInstanceType(secretKey) === "prod" ? "production" : "development"; try { const { body } = await retryOn429(async () => @@ -206,7 +211,14 @@ export async function fetchInstanceIdentity(secretKey: string): Promise { const secretKey = await resolveBapiSecretKey(options); const { instance: named, ...source } = await describeKeySource(options); - const fetched = await fetchInstanceIdentity(secretKey); + const fetched = await fetchInstanceIdentity(secretKey, source.keySource); // The Platform API or the profile already named the instance: recorded // instead of a stand-in, a run's commands can reach it with `--instance`. // Named by ID, its environment is unknown: the key prefix only guesses it. diff --git a/packages/cli-core/src/commands/migrate/run.test.ts b/packages/cli-core/src/commands/migrate/run.test.ts index b9140f71c..c8a19b826 100644 --- a/packages/cli-core/src/commands/migrate/run.test.ts +++ b/packages/cli-core/src/commands/migrate/run.test.ts @@ -15,6 +15,7 @@ import path from "node:path"; import { _setConfigDir, setProfile } from "../../lib/config.ts"; import { type CliError, EXIT_CODE } from "../../lib/errors.ts"; import * as plapi from "../../lib/plapi.ts"; +import { _resetMode, setMode } from "../../mode.ts"; import { captureTelemetryPayload, credentialStoreStubs, @@ -1475,6 +1476,26 @@ describe("run", () => { expect(process.exitCode).toBe(2); }); + // The real run's next step: printed by the gutter for a person, and + // straight to the log for an agent, whose gutter drops it. + test.each([["human"], ["agent"]] as const)( + "a refused --dry-run gives the --allow-partial command (%s)", + async (mode) => { + fs.writeFileSync( + path.join(workDir, "export.json"), + JSON.stringify([...export2, { id: "u3" }]), + ); + setMode(mode); + try { + await run({ ...baseOptions, dryRun: true }); + } finally { + _resetMode(); + } + + expect(Bun.stripANSI(captured.err)).toMatch(/→ .*--allow-partial --yes/); + }, + ); + test("--dry-run --json says the import would be refused", async () => { fs.writeFileSync( path.join(workDir, "export.json"), @@ -1768,7 +1789,13 @@ describe("run", () => { JSON.stringify([...export2, { id: "u3" }]), ); expect(await exitCodeOf(run({ ...baseOptions, json: true }))).toBe(EXIT_CODE.USAGE); - expect(JSON.parse(captured.out)).toMatchObject({ run: null, refused: true }); + // The dry run's advice and commands, on stdout for an agent to act on. + expect(JSON.parse(captured.out)).toMatchObject({ + run: null, + refused: true, + advice: "Fix them, or pass --allow-partial to import the rest and record them as skipped.", + steps: [{ command: expect.stringContaining("--allow-partial --yes") }], + }); expect(created()).toHaveLength(0); }); @@ -1899,7 +1926,7 @@ describe("run", () => { { code: "unsupported_country_code", message: "unsupported", long_message: COUNTRY }, ], }, - { status: 422 }, + { status: 403 }, ); } return clerk(input, init); @@ -1988,15 +2015,24 @@ describe("run", () => { ]), ); const clerk = globalThis.fetch; - globalThis.fetch = (async (input: string | URL | Request, init?: RequestInit) => - new URL(input.toString()).pathname === "/v1/email_addresses" - ? Response.json( - { - errors: [{ code: "form_identifier_exists", message: "taken", long_message: TAKEN }], - }, - { status: 422 }, - ) - : clerk(input, init)) as typeof fetch; + globalThis.fetch = (async (input: string | URL | Request, init?: RequestInit) => { + const { pathname } = new URL(input.toString()); + if (pathname === "/v1/email_addresses") { + return Response.json( + { errors: [{ code: "form_identifier_exists", message: "taken", long_message: TAKEN }] }, + { status: 422 }, + ); + } + // The lookup a taken attach makes: this user holds only its primary. + if (init?.method === "GET" && /^\/v1\/users\/user_[^/]+$/.test(pathname)) { + return Response.json({ + id: pathname.split("/").pop(), + email_addresses: [{ email_address: "a@x.dev" }], + phone_numbers: [], + }); + } + return clerk(input, init); + }) as typeof fetch; }); test("is counted in the summary", async () => { @@ -2337,6 +2373,47 @@ describe("run", () => { ]); }); + // The retry without a refused phone was cut off: the continue that adopts + // the user reads the reason from that `creating` line, and still warns. + test("an adopted user keeps the phone its cut-off retry went without", async () => { + const reason = "Phone numbers from this country (Netherlands) are currently not supported."; + fs.writeFileSync( + path.join(workDir, "export.json"), + JSON.stringify([ + { id: "u1", primary_email_address: "a@x.dev" }, + { id: "u2", primary_email_address: "b@x.dev", primary_phone_number: "+31612345678" }, + ]), + ); + stubClerk({ failing: new Set(["u2"]) }); + await run(baseOptions); + const [first] = listRuns(runsDir()); + fs.appendFileSync( + path.join(runsDir(), first!.id, "users.ndjson"), + `${JSON.stringify({ sourceId: "u2", status: "creating", phoneRefused: reason })}\n`, + ); + interrupt(first!.id); + + requests = []; + process.exitCode = 0; + captured.clear(); + stubClerk({ + existing: [ + { id: "user_found", external_id: "u2", private_metadata: { clerkMigrateRun: first!.id } }, + ], + }); + await run(baseOptions); + + expect(created()).toEqual([]); + expect(latestUserLines(runsDir(), first!.id).get("u2")).toMatchObject({ + status: "created", + clerkId: "user_found", + phoneRefused: reason, + }); + expect(Bun.stripANSI(captured.err)).toContain( + `Imported without their phone:\n 1 user: ${reason}`, + ); + }); + // A reserved create with no answer: its latest line has to keep `reserved`, // or the continue adopts the user as unreserved and attaches the email // Clerk already holds reserved. diff --git a/packages/cli-core/src/commands/migrate/run.ts b/packages/cli-core/src/commands/migrate/run.ts index 454341e13..7afa0e0fb 100644 --- a/packages/cli-core/src/commands/migrate/run.ts +++ b/packages/cli-core/src/commands/migrate/run.ts @@ -87,6 +87,7 @@ import { openUndoOf, patchRun, importsOfFile, + undoneInFull, undosOf, withAttempt, } from "./lib/run-store.ts"; @@ -504,17 +505,6 @@ function sameFileAndInstance( ); } -/** True when a run's users are gone: it is marked undone, or an undo of it finished. */ -function undoneInFull(runsDir: string, record: RunRecord): boolean { - return ( - runState(runsDir, record) === "undone" || - listRuns(runsDir).some( - (undo) => - undo.kind === "undo" && undo.undoes === record.id && runState(runsDir, undo) === "complete", - ) - ); -} - /** How this import relates to earlier runs of the same file, source and instance. */ type ResumeCase = | { kind: "new" } @@ -1440,7 +1430,12 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi log.warn( `The import would refuse: ${plural(checks.rejects.length, "user")} would be rejected. ${advice}`, ); - if (steps.length > 0) setNextSteps(steps.map((step) => step.command)); + if (steps.length > 0) { + const commands = steps.map((step) => step.command); + setNextSteps(commands); + // With no terminal, the gutter drops what setNextSteps gets. + printAgentNextSteps(commands); + } } } // The exit code says what the real run would do. @@ -1452,8 +1447,8 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi } if (refused) { - if (options.json) preview({ refused: true }); const { advice, steps } = refusalAdvice(); + if (options.json) preview({ refused: true, advice, steps }); throwUsageError( `${plural(checks.rejects.length, "user")} would be rejected, so nothing was imported.` + `${rejectSummary(checks, options.json)} ${advice}`, diff --git a/packages/cli-core/src/commands/migrate/runs.test.ts b/packages/cli-core/src/commands/migrate/runs.test.ts index 8d840ffaf..81662ffca 100644 --- a/packages/cli-core/src/commands/migrate/runs.test.ts +++ b/packages/cli-core/src/commands/migrate/runs.test.ts @@ -153,6 +153,7 @@ describe("runs ", () => { sourceId: "b", status: "created", clerkId: "user_b", + error: "Not added yet: additional email b2@x.dev: Unauthorized request", pending: [{ kind: "email", value: "b2@x.dev", verified: true }], }); const record = run.finish(); @@ -162,6 +163,7 @@ describe("runs ", () => { run: { state: "partial", counts: { creating: 1, pending: 1 } }, errors: [{ error: "bad gateway (a re-run checks)", count: 1 }], unconfirmed: [{ sourceId: "a" }], + leftToAttach: [{ sourceId: "b" }], }); captured.clear(); @@ -170,6 +172,9 @@ describe("runs ", () => { expect(err).toContain("1 created, 1 left to attach, 1 not confirmed"); expect(err).toContain("Not confirmed (Clerk may have created them; a continue checks) (1)"); expect(err).toContain("1 user: bad gateway (a re-run checks)"); + // And why each extra is still unattached. + expect(err).toContain("Identifiers left to attach (1)"); + expect(err).toContain("b Not added yet: additional email b2@x.dev: Unauthorized request"); }); // Imported, so not failed, but without an email or phone Clerk refused once @@ -212,7 +217,7 @@ describe("runs ", () => { expect(err).toContain( "1 user: imported without an email or phone Clerk refused: unverified phone: That phone number is taken.", ); - expect(err).toContain("Imported without an email or phone (1)"); + expect(err).toContain("Imported without an email or phone Clerk refused (1)"); }); // Clerk refused the first phone on the create, retried without it: the @@ -252,7 +257,39 @@ describe("runs ", () => { expect(err).toContain( "1 user: imported without their phone: Phone numbers from this country are not supported.", ); - expect(err).toContain("Imported without an email or phone (1)"); + expect(err).toContain("Imported without an email or phone Clerk refused (1)"); + }); + + // Undo writes its `deleted` lines to its own run: the import's still end on + // `created`, for users that no longer exist. + test("warns about nothing once the run is undone in full", async () => { + fs.writeFileSync(usersFile(), "[]"); + const run = startRun(runsDir, { + kind: "import", + target: TARGET, + source: "clerk", + file: { path: usersFile(), sha256: sha256File(usersFile()) }, + }); + run.append({ + sourceId: "a", + status: "created", + clerkId: "user_a", + error: "Failed to add unverified phone +15555550100: That phone number is taken.", + refused: ["unverified phone: That phone number is taken."], + }); + const record = run.finish(); + const undo = startRun(runsDir, { kind: "undo", target: TARGET, undoes: record.id }); + undo.append({ sourceId: "a", status: "deleted", clerkId: "user_a" }); + undo.finish(); + + await runs(record.id, { runsDir, json: true }); + expect(JSON.parse(captured.out)).toMatchObject({ warnings: [], importedWithout: [] }); + + captured.clear(); + await runs(record.id, { runsDir }); + const err = Bun.stripANSI(captured.err); + expect(err).not.toContain("Warnings"); + expect(err).not.toContain("Imported without"); }); // A stopped import's unsent users have no lines: only the count says so. diff --git a/packages/cli-core/src/commands/migrate/runs.ts b/packages/cli-core/src/commands/migrate/runs.ts index e72fd8e16..2410fe5bf 100644 --- a/packages/cli-core/src/commands/migrate/runs.ts +++ b/packages/cli-core/src/commands/migrate/runs.ts @@ -18,6 +18,7 @@ import { latestUserLines, listRuns, importsOfFile, + undoneInFull, undosOf, readRun, resolveRunsDir, @@ -251,18 +252,22 @@ function printList(runsDir: string, records: RunRecord[]): void { function showJson(runsDir: string, record: RunRecord, carry: string[]) { const lines = [...latestUserLines(runsDir, record.id).values()]; const state = runState(runsDir, record); + // The undo's `deleted` lines are its own run's: these users are gone, so + // nothing they were imported without is worth a warning. + const created = undoneInFull(runsDir, record) + ? [] + : lines.filter((line) => line.status === "created"); return { runsDir, run: { ...record, state }, next: nextCommands(runsDir, record, state, carry, lines), errors: errorBreakdown(lines), - warnings: warningBreakdown(lines), + warnings: warningBreakdown(created), failed: lines.filter((line) => line.status === "failed"), - importedWithout: lines.filter( - (line) => - line.status === "created" && - (line.phoneRefused !== undefined || (line.refused?.length ?? 0) > 0), + importedWithout: created.filter( + (line) => line.phoneRefused !== undefined || (line.refused?.length ?? 0) > 0, ), + leftToAttach: created.filter((line) => (line.pending?.length ?? 0) > 0), skipped: lines.filter((line) => line.status === "skipped"), unconfirmed: lines.filter((line) => line.status === "creating"), }; @@ -323,7 +328,8 @@ function printRun(runsDir: string, record: RunRecord, carry: string[]): void { } printUsers("Failed", view.failed, usersFile); - printUsers("Imported without an email or phone", view.importedWithout, usersFile); + printUsers("Imported without an email or phone Clerk refused", view.importedWithout, usersFile); + printUsers("Identifiers left to attach", view.leftToAttach, usersFile); printUsers( "Not confirmed (Clerk may have created them; a continue checks)", view.unconfirmed, From bc388db9edb2888c93132c98bd0de39c2fe89989 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Wed, 4 Nov 2026 17:27:34 -0600 Subject: [PATCH 09/10] fix(migrate): group refusals without the identifier, and word an HTML 401 by status - A refusal's summary key drops the email or phone Clerk's message names, so `not_allowed_access` refusals group as one warning instead of one per user. - A 401 whose body is a proxy's HTML page reads "Clerk returned HTTP 401 with no error details" instead of the markup. - Spread the modes into `test.each`, per the testing rule. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/migrate/import-users.test.ts | 5 ++++- .../cli-core/src/commands/migrate/import-users.ts | 9 ++++++++- .../src/commands/migrate/lib/target.test.ts | 14 ++++++++++++++ .../cli-core/src/commands/migrate/lib/target.ts | 3 ++- packages/cli-core/src/commands/migrate/run.test.ts | 3 ++- 5 files changed, 30 insertions(+), 4 deletions(-) diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index c3b775107..2a9e3ca8e 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -568,7 +568,10 @@ describe("importUsers", () => { expect(lines.at(-1)).not.toHaveProperty("pending"); expect(lines.at(-1)?.error).toBe(`Failed to add additional email b@x.dev: ${blocked}`); - expect([...summary.droppedIdentifiers]).toEqual([[`additional email: ${blocked}`, 1]]); + // Grouped by reason, without the address: one line for every blocked user. + expect([...summary.droppedIdentifiers]).toEqual([ + ["additional email: is not allowed to access this application.", 1], + ]); }); test("counts each user once per reason Clerk refused its identifiers", async () => { diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index a05e87947..53a114be7 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -209,6 +209,12 @@ function userHolds(user: unknown, kind: PendingIdentifier["kind"], value: string ); } +/** The message with the identifier it names replaced by `` or ``. */ +function withoutValue(message: string, value: string, kind: PendingIdentifier["kind"]): string { + const escaped = value.trim().replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + return escaped ? message.replace(new RegExp(escaped, "gi"), `<${kind}>`) : message; +} + /** * Starts the note on an attach left pending for a re-run, which tries it again: * a continue drops the earlier one rather than repeating it. @@ -339,7 +345,8 @@ async function attachIdentifier( } return { note: `Failed to add ${kindLabel} ${value}: ${message}`, - refused: `${kindLabel}: ${normalizeErrorMessage(message)}`, + // Without the value, so users refused for one reason group as one. + refused: `${kindLabel}: ${normalizeErrorMessage(withoutValue(message, value, kind))}`, }; } } diff --git a/packages/cli-core/src/commands/migrate/lib/target.test.ts b/packages/cli-core/src/commands/migrate/lib/target.test.ts index b4d8d8a17..d577c5fec 100644 --- a/packages/cli-core/src/commands/migrate/lib/target.test.ts +++ b/packages/cli-core/src/commands/migrate/lib/target.test.ts @@ -186,6 +186,20 @@ describe("fetchInstanceIdentity", () => { ), }); }); + + // A proxy's page, not Clerk's JSON: its markup says nothing worth printing. + test("words a 401 with an HTML body by its status", async () => { + globalThis.fetch = (async () => + new Response("401 Authorization Required", { + status: 401, + })) as unknown as typeof fetch; + + const error = (await fetchInstanceIdentity("sk_test_bad", "--secret-key").catch( + (thrown: unknown) => thrown, + )) as Error; + expect(error.message).toContain("Clerk returned HTTP 401 with no error details"); + expect(error.message).not.toContain(""); + }); }); describe("resolveClerkTarget --instance", () => { diff --git a/packages/cli-core/src/commands/migrate/lib/target.ts b/packages/cli-core/src/commands/migrate/lib/target.ts index 99257c34e..4c3d76586 100644 --- a/packages/cli-core/src/commands/migrate/lib/target.ts +++ b/packages/cli-core/src/commands/migrate/lib/target.ts @@ -24,6 +24,7 @@ import { log } from "../../../lib/log.ts"; import { isHuman } from "../../../mode.ts"; import { detectInstanceType } from "./instance.ts"; import { retryOn429 } from "./retry.ts"; +import { failureOf } from "./failures.ts"; import { type CarryOptions, carryFlags } from "./carry-flags.ts"; import type { RunTarget } from "./run-store.ts"; @@ -214,7 +215,7 @@ export async function fetchInstanceIdentity( if (error instanceof ApiError && error.status === 401) { const from = keySource ? ` from ${keySource}` : ""; throw new CliError( - `Clerk rejected the secret key${from} (401): ${error.longMessage ?? error.message}\n` + + `Clerk rejected the secret key${from} (401): ${failureOf(error).message}\n` + "Check the key, or copy a new one from the API keys page in the Clerk Dashboard.", { code: ERROR_CODE.AUTH_REQUIRED }, ); diff --git a/packages/cli-core/src/commands/migrate/run.test.ts b/packages/cli-core/src/commands/migrate/run.test.ts index c8a19b826..844d857a2 100644 --- a/packages/cli-core/src/commands/migrate/run.test.ts +++ b/packages/cli-core/src/commands/migrate/run.test.ts @@ -1478,7 +1478,8 @@ describe("run", () => { // The real run's next step: printed by the gutter for a person, and // straight to the log for an agent, whose gutter drops it. - test.each([["human"], ["agent"]] as const)( + const MODES = ["human", "agent"] as const; + test.each([...MODES])( "a refused --dry-run gives the --allow-partial command (%s)", async (mode) => { fs.writeFileSync( From eed0ae12cd2145827749b113c65fa73c48809240 Mon Sep 17 00:00:00 2001 From: Michael Novotny Date: Thu, 5 Nov 2026 00:42:35 -0600 Subject: [PATCH 10/10] refactor(migrate): address Roy's nits on the final-testing fixes - One `adoptedLines` map of `creating` lines replaces the `adoptedReserved`, `adoptedPending` and `adoptedPhoneRefused` options. - `RegExp.escape`, no spread for an unset body, notes and the key source each built once, and a one-line warning loop without a status check that never skipped anything. Co-Authored-By: Claude Opus 5.5 --- .../src/commands/migrate/export/clerk.ts | 4 +- .../src/commands/migrate/import-users.test.ts | 21 ++++++--- .../src/commands/migrate/import-users.ts | 45 +++++++++---------- packages/cli-core/src/commands/migrate/run.ts | 29 ++++-------- .../cli-core/src/commands/migrate/runs.ts | 5 +-- 5 files changed, 46 insertions(+), 58 deletions(-) diff --git a/packages/cli-core/src/commands/migrate/export/clerk.ts b/packages/cli-core/src/commands/migrate/export/clerk.ts index 8e4507e9c..d4f68a790 100644 --- a/packages/cli-core/src/commands/migrate/export/clerk.ts +++ b/packages/cli-core/src/commands/migrate/export/clerk.ts @@ -285,12 +285,12 @@ export async function exportClerk(options: ExportClerkOptions): Promise { : !options.app && process.env.CLERK_SECRET_KEY ? KEY_SOURCE.envVar : undefined; - const identity = await fetchInstanceIdentity(source.secretKey, keyFrom ?? source.target); + const keySource = keyFrom ?? source.target; + const identity = await fetchInstanceIdentity(source.secretKey, keySource); // A key picks the instance on its own and ignores --instance, so the two // must agree, as on import: otherwise this reads the wrong user pool under // the requested instance's name. if (keyFrom) assertInstanceFlagMatches({ instance: options.instance }, keyFrom, identity); - const keySource = keyFrom ?? source.target; const target = { platform: "clerk", env: identity.env, diff --git a/packages/cli-core/src/commands/migrate/import-users.test.ts b/packages/cli-core/src/commands/migrate/import-users.test.ts index 2a9e3ca8e..8fed5efd8 100644 --- a/packages/cli-core/src/commands/migrate/import-users.test.ts +++ b/packages/cli-core/src/commands/migrate/import-users.test.ts @@ -1014,7 +1014,7 @@ describe("importUsers", () => { await importUsers({ users: [user({ unverifiedEmailAddresses: ["c@x.dev"] })], adopted: new Map([["u1", "user_found"]]), - adoptedReserved: new Set(["u1"]), + adoptedLines: new Map([["u1", { sourceId: "u1", status: "creating", reserved: true }]]), secretKey: "sk_test_x", limits: LIMITS, record, @@ -1033,8 +1033,17 @@ describe("importUsers", () => { user({ unverifiedEmailAddresses: ["c@x.dev"], unverifiedPhoneNumbers: ["+31612345678"] }), ], adopted: new Map([["u1", "user_found"]]), - adoptedReserved: new Set(["u1"]), - adoptedPending: new Map([["u1", ["+31612345678"]]]), + adoptedLines: new Map([ + [ + "u1", + { + sourceId: "u1", + status: "creating", + reserved: true, + pending: [{ kind: "phone", value: "+31612345678", verified: false }], + }, + ], + ]), secretKey: "sk_test_x", limits: LIMITS, record, @@ -1127,7 +1136,7 @@ describe("importUsers", () => { limits: LIMITS, record, adopted: new Map([["u1", "user_found"]]), - adoptedPhoneRefused: new Map([["u1", allLines.at(-1)!.phoneRefused!]]), + adoptedLines: new Map([["u1", allLines.at(-1)!]]), }); expect(lines.at(-1)).toMatchObject({ @@ -1334,9 +1343,7 @@ describe("importUsers", () => { record, reserveUnverified: true, adopted: new Map([["u1", "user_found"]]), - adoptedReserved: new Set(["u1"]), - adoptedPending: new Map(), - adoptedPhoneRefused: new Map([["u1", creating.phoneRefused!]]), + adoptedLines: new Map([["u1", creating]]), }); expect(requests.some((request) => request.url.endsWith("/v1/phone_numbers"))).toBe(false); diff --git a/packages/cli-core/src/commands/migrate/import-users.ts b/packages/cli-core/src/commands/migrate/import-users.ts index 53a114be7..fb5e42861 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -211,7 +211,7 @@ function userHolds(user: unknown, kind: PendingIdentifier["kind"], value: string /** The message with the identifier it names replaced by `` or ``. */ function withoutValue(message: string, value: string, kind: PendingIdentifier["kind"]): string { - const escaped = value.trim().replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const escaped = RegExp.escape(value.trim()); return escaped ? message.replace(new RegExp(escaped, "gi"), `<${kind}>`) : message; } @@ -305,7 +305,7 @@ async function attachIdentifier( method, path: requestPath, secretKey: ctx.secretKey, - ...(requestBody ? { body: requestBody } : {}), + body: requestBody, }); }, { first: true, stop }, @@ -534,22 +534,15 @@ export type ImportUsersOptions = { */ adopted?: Map; /** - * The adopted users whose create reserved their unverified identifiers, from - * its `creating` line. The rest had them attached unverified, whatever this - * run's `reserveUnverified` says. + * Source ID → the `creating` line of each adopted user's create, which says + * how it was made: `reserved` when it reserved the unverified identifiers + * (the rest had them attached unverified, whatever this run's + * `reserveUnverified` says), `pending` for reserved phones a retry after a + * refused phone left out (attached once the user is adopted, unverified, as + * they would have been), and `phoneRefused` when the create that landed was + * the retry without the first phone. */ - adoptedReserved?: Set; - /** - * Reserved phones an adopted user's create left out, from its `creating` - * line: the retry after a refused phone sends without them. Attached once - * the user is adopted, unverified, as they would have been. - */ - adoptedPending?: Map; - /** - * Clerk's reason for refusing an adopted user's first phone, from its - * `creating` line: the create that landed was the retry without it. - */ - adoptedPhoneRefused?: Map; + adoptedLines?: Map; /** Allow users that carry no password. */ skipPasswordRequirement?: boolean; /** Create the identifiers the source never verified as reserved. */ @@ -576,9 +569,7 @@ export async function importUsers(options: ImportUsersOptions): Promise(), - adoptedReserved = new Set(), - adoptedPending = new Map(), - adoptedPhoneRefused = new Map(), + adoptedLines = new Map(), skipPasswordRequirement = true, reserveUnverified = false, validationFailed = 0, @@ -684,11 +675,14 @@ export async function importUsers(options: ImportUsersOptions): Promise { - const phoneRefusal = adoptedPhoneRefused.get(sourceId); + const creating = adoptedLines.get(sourceId); + const phoneRefusal = creating?.phoneRefused; return { clerkUserId, notes: phoneRefusal ? [`Failed to add phone: ${phoneRefusal}`] : [], - attachLater: adoptedPending.get(sourceId) ?? [], + attachLater: (creating?.pending ?? []) + .filter((entry) => entry.kind === "phone") + .map((entry) => entry.value), ...(phoneRefusal ? { phoneRefusal } : {}), }; }; @@ -701,7 +695,7 @@ export async function importUsers(options: ImportUsersOptions): Promise 0 || identifiers.unverifiedPhones.length > 0); let sent = false; @@ -805,14 +799,15 @@ export async function importUsers(options: ImportUsersOptions): Promise 0 ? { pending: toAttach } : {}), }); if (phoneRefused) droppedPhones.set(phoneRefused, (droppedPhones.get(phoneRefused) ?? 0) + 1); - await finishUser(line, toAttach, [...created.notes, ...retries]); + await finishUser(line, toAttach, notes); successful++; processed++; progress(); diff --git a/packages/cli-core/src/commands/migrate/run.ts b/packages/cli-core/src/commands/migrate/run.ts index 7afa0e0fb..1ef19529c 100644 --- a/packages/cli-core/src/commands/migrate/run.ts +++ b/packages/cli-core/src/commands/migrate/run.ts @@ -1140,23 +1140,12 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi const plannedLines = continued ? readUserLines(runsDir, continued.id).length : undefined; const done = new Map(); const attachOnly: UserLine[] = []; - const inFlight: string[] = []; - const inFlightReserved = new Set(); - const inFlightPending = new Map(); - const inFlightPhoneRefused = new Map(); + // Each create with no answer, by its `creating` line: what it reserved, + // left out and lost decides how an adopted user is finished. + const inFlight = new Map(); if (continued) { for (const line of latestUserLines(runsDir, continued.id).values()) { - if (line.status === "creating") inFlight.push(line.sourceId); - if (line.status === "creating" && line.reserved) inFlightReserved.add(line.sourceId); - if (line.status === "creating" && line.phoneRefused) { - inFlightPhoneRefused.set(line.sourceId, line.phoneRefused); - } - if (line.status === "creating" && line.pending?.length) { - inFlightPending.set( - line.sourceId, - line.pending.filter((entry) => entry.kind === "phone").map((entry) => entry.value), - ); - } + if (line.status === "creating") inFlight.set(line.sourceId, line); if (line.status !== "created" || !line.clerkId) continue; done.set(line.sourceId, line.clerkId); if (line.pending?.length) attachOnly.push(line); @@ -1170,7 +1159,7 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi if (continued) { const found = await findInFlight({ runId: continued.id, - sourceIds: inFlight, + sourceIds: [...inFlight.keys()], secretKey, schedule, }); @@ -1315,7 +1304,9 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi adoptedClerkIds: new Set(adopted.values()), adoptedSourceIds: new Set(adopted.keys()), // Their `creating` line says how the create ran, not this run's flag. - reservedSourceIds: new Set([...adopted.keys()].filter((id) => inFlightReserved.has(id))), + reservedSourceIds: new Set( + [...adopted.keys()].filter((id) => inFlight.get(id)?.reserved === true), + ), spinner, }), ); @@ -1580,9 +1571,7 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi runId: run.record.id, attachOnly, adopted, - adoptedReserved: inFlightReserved, - adoptedPending: inFlightPending, - adoptedPhoneRefused: inFlightPhoneRefused, + adoptedLines: inFlight, skipPasswordRequirement: !options.requirePassword, reserveUnverified, progress, diff --git a/packages/cli-core/src/commands/migrate/runs.ts b/packages/cli-core/src/commands/migrate/runs.ts index 2410fe5bf..c73126b33 100644 --- a/packages/cli-core/src/commands/migrate/runs.ts +++ b/packages/cli-core/src/commands/migrate/runs.ts @@ -121,11 +121,8 @@ function warningBreakdown(lines: UserLine[]): { warning: string; count: number } const counts = new Map(); const add = (warning: string) => counts.set(warning, (counts.get(warning) ?? 0) + 1); for (const line of lines) { - if (line.status !== "created") continue; if (line.phoneRefused) add(`${WITHOUT_PHONE_WARNING}${line.phoneRefused}`); - for (const reason of line.refused ?? []) { - add(`${WITHOUT_IDENTIFIER_WARNING}${reason}`); - } + for (const reason of line.refused ?? []) add(`${WITHOUT_IDENTIFIER_WARNING}${reason}`); } return [...counts] .map(([warning, count]) => ({ warning, count }))