feat(cli): redesign the OAuth login callback page, and fix the seven defects around it

packages/petdex-cli/scripts/build-callback-fonts.ts

@@ -0,0 +1,182 @@
+/**
+ * Regenerates `src/cli-auth/lib/callback-fonts.ts`.
+ *
+ * Run from this package after the page's copy gains a character outside the
+ * subset: the page would otherwise render that one character in the system
+ * face, part-way through a sentence.
+ *
+ *   GEIST_PACKAGE_DIR=/path/to/unpacked/geist bun run scripts/build-callback-fonts.ts
+ *
+ * `GEIST_PACKAGE_DIR` is the `geist` npm package unpacked from its tarball
+ * (`npm pack geist && tar xzf geist-*.tgz`), version 1.7.2 at the time of
+ * writing. The subsetting itself needs Python with `fontTools` and `brotli`;
+ * point `PYTHON` at that interpreter if it is not the default `python3`.
+ * All of that is build-time only. The published CLI carries the finished
+ * base64 strings and never runs this, and it is deliberately not a package
+ * script so `bun run build` cannot depend on the tools being installed.
+ */
+import { execFileSync } from "node:child_process";
+import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+/** The `geist` package's variable fonts, unpacked from its npm tarball. */
+const GEIST_PACKAGE = process.env.GEIST_PACKAGE_DIR;
+if (!GEIST_PACKAGE) {
+  console.error("Set GEIST_PACKAGE_DIR to the unpacked `geist` npm package.");
+  process.exit(1);
+}
+
+/** Interpreter holding `fontTools` and `brotli`. */
+const PYTHON_BIN = process.env.PYTHON ?? "python3";
+
+const SANS_SOURCE = join(
+  GEIST_PACKAGE,
+  "dist/fonts/geist-sans/Geist-Variable.woff2",
+);
+const MONO_SOURCE = join(
+  GEIST_PACKAGE,
+  "dist/fonts/geist-mono/GeistMono-Variable.woff2",
+);
+
+/**
+ * Latin-1 plus the punctuation and combining marks the page can produce.
+ *
+ * Latin-1 in full rather than only the characters in today's copy: the detail
+ * line renders text from the OAuth error response, so an accented character
+ * outside this set would swap to the system face mid-sentence.
+ */
+const SANS_UNICODES = [
+  "U+0020-007E", // printable ASCII
+  "U+00A0-00FF", // Latin-1 supplement
+  "U+0131", // dotless i
+  "U+0152-0153", // OE, oe
+  "U+02BC", // modifier apostrophe
+  "U+02C6", // modifier circumflex
+  "U+02DA", // ring above
+  "U+02DC", // small tilde
+  "U+0300-0301",
+  "U+0303-0304",
+  "U+0308-0309",
+  "U+0323", // combining marks
+  "U+2013-2014", // en dash, em dash
+  "U+2018-201D", // curly quotes
+  "U+2026", // ellipsis
+  "U+2122", // trade mark
+].join(",");
+
+/** The mono face renders only the fixed `eyebrow` label, so ASCII is enough. */
+const MONO_UNICODES = "U+0020-007E";
+
+/** Weights the page uses: 400 body, 500 eyebrow, 600 headings. */
+const WEIGHT_RANGE = "400:600";
+
+/** Copyright, family, subfamily, unique, full, PostScript, OFL, licence URL. */
+const NAME_IDS = "0,1,2,3,4,6,13,14";
+
+/** The four features the page's Latin text needs; `*` would add about 20KB. */
+const LAYOUT_FEATURES = "ccmp,liga,kern,locl";
+
+/**
+ * Subset and clamp the weight axis in one pass.
+ *
+ * Both steps are needed and pyftsubset cannot clamp the axis itself, so the
+ * whole thing runs through fontTools' Python API rather than the CLI. The
+ * order matters: instancing first would leave the subsetter resolving
+ * variations for glyphs it is about to drop.
+ */
+const PYTHON = `
+import sys
+from fontTools.ttLib import TTFont
+from fontTools.varLib import instancer
+from fontTools import subset
+
+source, unicodes, features, name_ids, weight, out = sys.argv[1:7]
+font = TTFont(source)
+opts = subset.Options()
+opts.flavor = None
+opts.layout_features = features.split(",")
+opts.hinting = False
+opts.desubroutinize = True
+opts.name_IDs = [int(n) for n in name_ids.split(",")]
+opts.notdef_outline = True
+subsetter = subset.Subsetter(options=opts)
+subsetter.populate(unicodes=subset.parse_unicodes(unicodes))
+subsetter.subset(font)
+lo, hi = (int(v) for v in weight.split(":"))
+instancer.instantiateVariableFont(
+    font, {"wght": (lo, hi)}, inplace=True, updateFontNames=False
+)
+# fontTools stamps head.modified with the current time on save, which makes
+# the output differ on every run. Pin it to the source font's own created
+# value, which is fixed in the tarball.
+font["head"].modified = font["head"].created
+font.flavor = "woff2"
+font.save(out)
+
+# One difference survives that pin and cannot be removed: woff2's brotli
+# compression is not byte-stable across fontTools or brotli versions, so
+# head.checkSumAdjustment - a checksum over the compressed file - moves with
+# it. Everything else, glyphs included, is byte-identical, and the two files
+# render the same. Expect a non-empty diff on the head table when regenerating
+# a toolchain change; compare the glyph set, not the bytes, before treating it
+# as a real difference.
+`;
+
+const work = mkdtempSync(join(tmpdir(), "petdex-fonts-"));
+try {
+  const scriptPath = join(work, "subset.py");
+  writeFileSync(scriptPath, PYTHON);
+
+  const build = (source: string, unicodes: string, name: string): string => {
+    const out = join(work, `${name}.woff2`);
+    execFileSync(
+      PYTHON_BIN,
+      [
+        scriptPath,
+        source,
+        unicodes,
+        LAYOUT_FEATURES,
+        NAME_IDS,
+        WEIGHT_RANGE,
+        out,
+      ],
+      {
+        // fontTools walks glyph sets whose iteration order follows the hash
+        // seed, so the same input otherwise produces a different (equivalent)
+        // woff2 each run — and a diff that never settles. Pinning the seed makes
+        // the output reproducible.
+        env: { ...process.env, PYTHONHASHSEED: "0" },
+      },
+    );
+    return readFileSync(out).toString("base64");
+  };
+
+  // Join with ` +`: adjacent string literals are not concatenated in
+  // JavaScript the way they are in C, so a newline-separated list would bind
+  // only the first line and silently discard the rest of the font.
+  const wrap = (value: string) =>
+    (value.match(/.{1,96}/g) ?? []).map((line) => `    "${line}"`).join(" +\n");
+
+  const target = join(
+    import.meta.dir,
+    "..",
+    "src",
+    "cli-auth",
+    "lib",
+    "callback-fonts.ts",
+  );
+  // Keep the hand-written doc comment; replace only the two literals.
+  const previous = readFileSync(target, "utf8");
+  const header = previous.slice(0, previous.indexOf("export const"));
+
+  writeFileSync(
+    target,
+    `${header}export const GEIST_SANS_WOFF2_BASE64 =\n` +
+      `${wrap(build(SANS_SOURCE, SANS_UNICODES, "sans"))};\n\n` +
+      `export const GEIST_MONO_WOFF2_BASE64 =\n` +
+      `${wrap(build(MONO_SOURCE, MONO_UNICODES, "mono"))};\n`,
+  );
+} finally {
+  rmSync(work, { recursive: true, force: true });
+}

packages/petdex-cli/src/cli-auth/clerk-cli-auth.test.ts

@@ -0,0 +1,165 @@
+import { afterEach, describe, expect, test } from "bun:test";
+
+import { ClerkCliAuth } from "./clerk-cli-auth";
+import type { CredentialStore } from "./types";
+
+const ISSUER = "https://clerk.test";
+
+/** The token endpoint only parses JSON when the response says it is JSON. */
+function json(body: unknown, status = 200): Response {
+  return new Response(JSON.stringify(body), {
+    status,
+    headers: { "Content-Type": "application/json" },
+  });
+}
+
+/** In-memory store, so a login never touches the OS keychain. */
+function memoryStore(): CredentialStore {
+  const values = new Map<string, string>();
+  return {
+    get: async (key) => values.get(key) ?? null,
+    set: async (key, value) => {
+      values.set(key, value);
+    },
+    delete: async (key) => {
+      values.delete(key);
+    },
+  };
+}
+
+const realFetch = globalThis.fetch;
+const unhandled: unknown[] = [];
+const onUnhandled = (reason: unknown) => unhandled.push(reason);
+
+afterEach(() => {
+  globalThis.fetch = realFetch;
+  unhandled.length = 0;
+});
+
+/**
+ * Drive a full login against a fake issuer, and return the page the browser
+ * was served once the flow finished.
+ */
+async function runLogin(options: {
+  tokenStatus?: number;
+  userinfoStatus?: number;
+  store?: CredentialStore;
+  openBrowserThrows?: boolean;
+}) {
+  const store = options.store ?? memoryStore();
+  let captured: Promise<Response> | null = null;
+
+  const auth = new ClerkCliAuth({
+    clientId: "client-id",
+    issuer: ISSUER,
+    storage: store,
+    callbackPort: 0,
+    timeoutMs: 4000,
+    openBrowser: async (authorizeUrl: string) => {
+      if (options.openBrowserThrows) throw new Error("no browser available");
+      const url = new URL(authorizeUrl);
+      const redirectUri = url.searchParams.get("redirect_uri") as string;
+      const state = url.searchParams.get("state") as string;
+      const callback = new URL(redirectUri);
+      callback.searchParams.set("code", "auth-code");
+      callback.searchParams.set("state", state);
+      // Deliberately not awaited: like a real browser, this request stays
+      // open until the CLI has finished the exchange and answered it.
+      captured = fetch(callback.toString());
+    },
+  });
+
+  type FetchArgs = Parameters<typeof fetch>;
+  globalThis.fetch = (async (input: FetchArgs[0], init?: FetchArgs[1]) => {
+    const url = String(input);
+    if (url.startsWith(`${ISSUER}/oauth/token`)) {
+      if (options.tokenStatus && options.tokenStatus >= 400) {
+        return json({ error: "invalid_grant" }, options.tokenStatus);
+      }
+      return json({ access_token: "token", token_type: "Bearer" });
+    }
+    if (url.startsWith(`${ISSUER}/oauth/userinfo`)) {
+      if (options.userinfoStatus && options.userinfoStatus >= 400) {
+        return json({ error: "unavailable" }, options.userinfoStatus);
+      }
+      return json({ sub: "user_1" });
+    }
+    return realFetch(input, init);
+  }) as typeof fetch;
+
+  const outcome = await auth.login().then(
+    (value) => ({ ok: true as const, value }),
+    (error: Error) => ({ ok: false as const, error }),
+  );
+  const page: Response | null = captured
+    ? await (captured as Promise<Response>)
+    : null;
+  return { outcome, page };
+}
+
+describe("clerk cli auth login page", () => {
+  test("serves the success page only after the credentials are stored", async () => {
+    const store = memoryStore();
+    const { outcome, page } = await runLogin({ store });
+
+    expect(outcome.ok).toBe(true);
+    expect(page?.status).toBe(200);
+    const html = await page?.text();
+    expect(html).toContain('class="chip chip-success"');
+    expect(await store.get("tokens")).toContain("token");
+    expect(await store.get("user")).toContain("user_1");
+  });
+
+  test("shows a failure page when the code exchange fails", async () => {
+    // The regression this deferral exists for: the page used to say
+    // "signed in" while the terminal reported a failed exchange.
+    const { outcome, page } = await runLogin({ tokenStatus: 400 });
+
+    expect(outcome.ok).toBe(false);
+    expect(page?.status).toBe(500);
+    const html = await page?.text();
+    expect(html).toContain('class="chip chip-danger"');
+    expect(html).not.toContain('class="chip chip-success"');
+  });
+
+  test("shows a failure page when the userinfo call fails", async () => {
+    const { outcome, page } = await runLogin({ userinfoStatus: 500 });
+
+    expect(outcome.ok).toBe(false);
+    expect(page?.status).toBe(500);
+    expect(await page?.text()).toContain('class="chip chip-danger"');
+  });
+
+  test("shows a failure page when the credentials cannot be stored", async () => {
+    const failing: CredentialStore = {
+      get: async () => null,
+      set: async () => {
+        throw new Error("keychain unavailable");
+      },
+      delete: async () => {},
+    };
+    const { outcome, page } = await runLogin({ store: failing });
+
+    expect(outcome.ok).toBe(false);
+    expect(page?.status).toBe(500);
+    expect(await page?.text()).toContain('class="chip chip-danger"');
+  });
+
+  test("fails without an unhandled rejection when the browser cannot open", async () => {
+    process.on("unhandledRejection", onUnhandled);
+    try {
+      const { outcome, page } = await runLogin({ openBrowserThrows: true });
+      expect(outcome.ok).toBe(false);
+      expect((outcome as { error: Error }).error.message).toContain(
+        "Failed to open authorization URL",
+      );
+      // No callback was ever delivered, so there is no page to inspect.
+      expect(page).toBeNull();
+      // Let the rejection queue drain before asserting it stayed empty.
+      await Bun.sleep(20);
+      expect(unhandled).toEqual([]);
+    } finally {
+      process.off("unhandledRejection", onUnhandled);
+    }
+  });
+});

packages/petdex-cli/src/cli-auth/clerk-cli-auth.ts

@@ -145,7 +145,26 @@ export class ClerkCliAuth {
       });
       await this.setJson("user", user);
 
+      // Last, and only once every step that can still fail has succeeded.
+      // The browser is holding its connection open until this call, so
+      // answering earlier would report success for a login that then failed
+      // to save its credentials — the same mismatch this deferral exists to
+      // remove, just moved one step later.
+      server.respond({ kind: "success" });
       return { tokens, user };
+    } catch (error) {
+      // Also reached when the opener rejects after the browser has already
+      // delivered its callback, which is the case where only this call can
+      // tell the reader anything. A no-op if the response was already sent.
+      const reason = (error as { code?: unknown }).code;
+      server.respond({
+        kind: "error",
+        reason:
+          reason === "userinfo" || reason === "storage"
+            ? (`${reason}_failed` as const)
+            : "token_exchange_failed",
+      });
+      throw error;
     } finally {
       server.close();
     }
@@ -176,8 +195,18 @@ export class ClerkCliAuth {
   }
 
   async whoami(): Promise<UserInfo | null> {
+    // The cached profile is not evidence of a usable session: `logout` clears
+    // both records through Promise.allSettled, so a half-cleared store can
+    // leave the profile behind with no tokens next to it. Reporting that as
+    // "signed in" hides the problem until some later command 401s, which is
+    // the one place the user cannot act on it. Confirming the credential
+    // first turns a confusing failure into a prompt to sign in again.
     const cachedUser = await this.getJson<UserInfo>("user");
-    if (cachedUser) return cachedUser;
+    if (cachedUser) {
+      const accessToken = await this.getAccessToken();
+      if (!accessToken) return null;
+      return cachedUser;
+    }
 
     const accessToken = await this.getAccessToken();
     if (!accessToken) return null;