diff --git a/.changeset/migrate-final-testing-fixes.md b/.changeset/migrate-final-testing-fixes.md new file mode 100644 index 000000000..4f9a86e2f --- /dev/null +++ b/.changeset/migrate-final-testing-fixes.md @@ -0,0 +1,5 @@ +--- +"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 `, 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 3c806407d..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` 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 @@ -807,8 +810,11 @@ 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 -an undo or another continue, exits 2 too, with nothing written. +`--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, 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: @@ -913,7 +919,12 @@ 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 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. In `--json` the preview carries +`refused: true`, with `advice` (the real refusal's sentence) and `steps` (its +commands, each a `command` and `description`). ``` Checks @@ -936,7 +947,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,15 +970,28 @@ 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, and so +does one whose error says nothing about the identifier: a `401`, a `403` other +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 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. 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. @@ -1001,7 +1026,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 @@ -1027,7 +1053,11 @@ clerk migrate runs --json 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 +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`), 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 importing the file again might not reach this run: a newer run of the same file @@ -1397,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 @@ -1547,6 +1580,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/export/clerk.ts b/packages/cli-core/src/commands/migrate/export/clerk.ts index fd2bdc3de..d4f68a790 100644 --- a/packages/cli-core/src/commands/migrate/export/clerk.ts +++ b/packages/cli-core/src/commands/migrate/export/clerk.ts @@ -280,17 +280,17 @@ 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; - if (keyFrom) assertInstanceFlagMatches({ instance: options.instance }, keyFrom, identity); 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 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 ccc30f1a8..8fed5efd8 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,188 @@ 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]]); + }); + + // A key Clerk stopped accepting says nothing about the email itself. + 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.") + : 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(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") + ? 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]]); + }); + + // 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}`); + // 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 () => { + 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. @@ -479,6 +661,174 @@ 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); + }); + + // 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(); + 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"), @@ -664,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, @@ -683,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, @@ -740,6 +1099,81 @@ 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 }); + }); + + // 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"]]), + adoptedLines: new Map([["u1", allLines.at(-1)!]]), + }); + + 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 () => { @@ -808,13 +1242,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"); }); @@ -823,6 +1257,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") @@ -843,20 +1278,77 @@ 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"]]), + adoptedLines: new Map([["u1", creating]]), + }); + + 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 () => { @@ -1234,9 +1726,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 eb4bde001..fb5e42861 100644 --- a/packages/cli-core/src/commands/migrate/import-users.ts +++ b/packages/cli-core/src/commands/migrate/import-users.ts @@ -190,6 +190,37 @@ 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.trim().toLowerCase(); + return (record.email_addresses ?? []).some( + (entry) => entry.email_address?.toLowerCase() === wanted, + ); + } + // 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), + ); +} + +/** The message with the identifier it names replaced by `` or ``. */ +function withoutValue(message: string, value: string, kind: PendingIdentifier["kind"]): string { + const escaped = RegExp.escape(value.trim()); + 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. + */ +const NOT_ADDED_YET = "Not added yet: "; + /** A request a stop kept from going out: neither a failure nor unknown. */ class NotSentError extends Error {} @@ -244,14 +275,17 @@ 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: then with a note + * saying why, when Clerk gave one. Never throws: the user itself was already + * created. */ 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" @@ -261,28 +295,59 @@ 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), + 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 }; - const label = `${verified ? "additional" : "unverified"} ${kind} ${value}`; - return { note: `Failed to add ${label}: ${(error as Error).message}` }; + // 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, 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" && + 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}`, + // Without the value, so users refused for one reason group as one. + refused: `${kindLabel}: ${normalizeErrorMessage(withoutValue(message, value, kind))}`, + }; } } @@ -290,22 +355,32 @@ 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 or left pending with a reason, + * why by kind for those refused, 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), }; } +/** 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[]; @@ -327,16 +402,20 @@ 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, the phones the user would be imported without and + * Clerk's reason. + */ + 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) => + const create = async (body: Record, retry?: PhoneRetry) => ctx.schedule( async () => { if (ctx.stop.aborted) throw new NotSentError(); - sending(body); + sending(body, retry); return bapiRequest({ method: "POST", path: "/v1/users", @@ -380,12 +459,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): 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)); + 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), + ); + dropped = withPrimary; } catch (retryError) { if (!isPhoneRefusal(retryError)) throw retryError; refusal = retryError; @@ -393,19 +490,13 @@ async function createUser( } if (!response) { if (!body.email_address) throw refusal; - response = await create(withoutPhone); + response = await create(withoutPhone, settle(dropped, refusal)); } - // 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)); - } - 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}`); } } @@ -443,17 +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. - */ - 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. + * 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. */ - adoptedPending?: Map; + adoptedLines?: Map; /** Allow users that carry no password. */ skipPasswordRequirement?: boolean; /** Create the identifiers the source never verified as reserved. */ @@ -480,8 +569,7 @@ export async function importUsers(options: ImportUsersOptions): Promise(), - adoptedReserved = new Set(), - adoptedPending = new Map(), + adoptedLines = new Map(), skipPasswordRequirement = true, reserveUnverified = false, validationFailed = 0, @@ -496,6 +584,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 @@ -548,6 +637,7 @@ 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) { + // With nothing to attach, the first line already says it all. + if (toAttach.length > 0) { record({ ...base, ...(error ? { error } : {}), + ...(refused.length > 0 ? { refused } : {}), ...(attached.pending.length > 0 ? { pending: attached.pending } : {}), }); } }; + /** + * A user a stopped run's create made: only its extras are left. When that + * create was the retry without a refused phone, the phone is still lost. + */ + const adoptedUser = (clerkUserId: string, sourceId: string): CreatedUser => { + const creating = adoptedLines.get(sourceId); + const phoneRefusal = creating?.phoneRefused; + return { + clerkUserId, + notes: phoneRefusal ? [`Failed to add phone: ${phoneRefusal}`] : [], + attachLater: (creating?.pending ?? []) + .filter((entry) => entry.kind === "phone") + .map((entry) => entry.value), + ...(phoneRefusal ? { phoneRefusal } : {}), + }; + }; + const processUser = async (user: User): Promise => { const retries: string[] = []; const identifiers = splitIdentifiers(user); @@ -580,7 +695,7 @@ export async function importUsers(options: ImportUsersOptions): Promise 0 || identifiers.unverifiedPhones.length > 0); let sent = false; @@ -588,21 +703,25 @@ export async function importUsers(options: ImportUsersOptions): Promise - createUser(ctx, user, identifiers, skipPasswordRequirement, (body) => { + 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 } : {}), + ...(retry?.reason ? { phoneRefused: normalizeErrorMessage(retry.reason) } : {}), ...(leftOut.length > 0 ? { pending: leftOut.map((value) => ({ @@ -663,11 +782,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); - } - await finishUser(line, toAttach, [...created.notes, ...retries]); + // Its notes too: a run stopped before the next line would lose them. + const notes = [...created.notes, ...retries]; + const firstNotes = notes.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, notes); successful++; processed++; progress(); @@ -691,7 +816,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++; @@ -716,6 +845,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..d727523be 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. */ @@ -35,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 29fc68dd5..967a95bb6 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,17 @@ 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[]; + /** + * 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. */ reason?: string; error?: string; @@ -486,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 f67becee0..d577c5fec 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,45 @@ 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", "--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.", + ), + }); + }); + + // 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 9925a3854..4c3d76586 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, CliError, ERROR_CODE, throwUsageError } from "../../../lib/errors.ts"; import { ENV_VAR_SOURCE_SUFFIX, resolveKeylessTarget, @@ -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"; @@ -180,9 +181,15 @@ 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 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 () => @@ -202,6 +209,17 @@ 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 345fbe0e2..844d857a2 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, @@ -1468,9 +1469,56 @@ describe("run", () => { 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 rest and record them as skipped.", + ); 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. + const MODES = ["human", "agent"] as const; + test.each([...MODES])( + "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"), + JSON.stringify([...export2, { id: "u3" }]), + ); + + await run({ ...baseOptions, dryRun: true, json: 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 () => { await run({ ...baseOptions, dryRun: true }); @@ -1502,6 +1550,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. @@ -1732,7 +1790,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); }); @@ -1805,6 +1869,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.")], }, }); }); @@ -1839,6 +1905,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: 403 }, + ); + } + 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."; @@ -1892,6 +2004,65 @@ 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) => { + 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 () => { + 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); @@ -2203,6 +2374,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. @@ -2556,6 +2768,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 () => { @@ -2582,6 +2816,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 7cb522091..1ef19529c 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 { @@ -82,6 +87,7 @@ import { openUndoOf, patchRun, importsOfFile, + undoneInFull, undosOf, withAttempt, } from "./lib/run-store.ts"; @@ -107,6 +113,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"; @@ -246,6 +253,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 ---------------------------------- /** @@ -489,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" } @@ -571,7 +576,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 +665,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 +689,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 +738,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; } @@ -1113,19 +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(); + // 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.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); @@ -1139,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, }); @@ -1284,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, }), ); @@ -1323,23 +1345,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 }); - else { - log.blank(); - log.info(dim("Dry run: nothing was written.")); - } - // 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 = @@ -1374,35 +1381,71 @@ 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, ...refusalAdvice() } : {}) }); + } 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) { + 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. + if (refused) { + process.exitCode = 2; + declareSoftExitOutcome("error", "migrate_checks_rejected"); + } + return; + } + + if (refused) { + 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)} ` + - (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, ); } @@ -1528,8 +1571,7 @@ async function runImport(rawOptions: MigrateRunOptions, lock: ImportLock): Promi runId: run.record.id, attachOnly, adopted, - adoptedReserved: inFlightReserved, - adoptedPending: inFlightPending, + adoptedLines: inFlight, skipPasswordRequirement: !options.requirePassword, reserveUnverified, progress, @@ -1541,6 +1583,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 +1619,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: `${WITHOUT_PHONE_WARNING}${reason}`, + count, + })), + ...[...summary.droppedIdentifiers].map(([reason, count]) => ({ + warning: `${WITHOUT_IDENTIFIER_WARNING}${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..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,124 @@ 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 + // 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)"); + }); + + // 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)"); + }); + + // 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 d8b68dc28..c73126b33 100644 --- a/packages/cli-core/src/commands/migrate/runs.ts +++ b/packages/cli-core/src/commands/migrate/runs.ts @@ -12,12 +12,13 @@ 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, listRuns, importsOfFile, + undoneInFull, undosOf, readRun, resolveRunsDir, @@ -112,6 +113,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 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.phoneRefused) add(`${WITHOUT_PHONE_WARNING}${line.phoneRefused}`); + for (const reason of line.refused ?? []) add(`${WITHOUT_IDENTIFIER_WARNING}${reason}`); + } + return [...counts] + .map(([warning, count]) => ({ warning, 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 @@ -232,12 +249,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(created), failed: lines.filter((line) => line.status === "failed"), + 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"), }; @@ -289,7 +316,17 @@ 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("Identifiers left to attach", view.leftToAttach, 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; };