chore(chrome-extension-native-host): split writeFrameAndExit + rewrite history-narrating docstrings

clients/chrome-extension-native-host/src/__tests__/index.test.ts

@@ -1,24 +1,20 @@
 /**
- * Subprocess regression tests for the Chrome native messaging helper —
- * targeting the two P1 bugs surfaced by the PR #24159 self-review:
+ * Subprocess regression tests for the Chrome native messaging helper.
  *
- *   - **Gap 2**: the unauthorized-origin branch must terminate the
- *     process synchronously and never POST `/v1/browser-extension-pair`.
- *     The previous `writeFrameAndExit` returned a forever-pending Promise
- *     after queuing the write, which meant `await`-ing it yielded the
- *     event loop and let the stdin listener run before the helper
- *     actually exited.
+ * These tests spawn the compiled helper binary and verify:
  *
- *   - **Gap 3**: the helper must surface the `guardianId` field returned
- *     by `/v1/browser-extension-pair` in its `token_response` frame.
- *     Earlier versions destructured only `{ token, expiresAt }` and
- *     silently dropped the field, causing `bootstrapLocalToken()` on the
- *     consumer side to reject the frame as malformed.
+ *   1. Unauthorized chrome-extension origins terminate with exit code 1
+ *      BEFORE any HTTP request is made to /v1/browser-extension-pair.
+ *      (The helper installs its stdin listener only after the origin
+ *      allowlist check, so unauthorized callers cannot inject frames.)
  *
- * These tests are intentionally separate from `integration.test.ts` so
- * the regression coverage is colocated with the bugs it pins down — if
- * either gap regresses, the failure points directly at the responsible
- * scenario rather than getting buried in the broader integration suite.
+ *   2. Authorized origins forward the pair endpoint's token, expiresAt,
+ *      and guardianId fields verbatim into the native-messaging
+ *      token_response frame.
+ *
+ *   3. A pair endpoint response missing guardianId causes the helper to
+ *      exit non-zero and emit an error frame, preventing a malformed
+ *      token from reaching the extension's bootstrap path.
  *
  * The suite skips gracefully when `dist/index.js` is missing so cold
  * checkouts don't break CI.
@@ -133,10 +129,9 @@ interface HelperRunResult {
 /**
  * Spawn the helper as a subprocess via `Bun.spawn`, write the framed
  * `request_token` payload to its stdin, and collect stdout / stderr /
- * exit code with a hard upper bound on wall clock time. We deliberately
- * use `Bun.spawn` here (rather than node:child_process as the e2e test
- * does) so the test surface matches the PR plan and so we get
- * `proc.exited` as a clean Promise.
+ * exit code with a hard upper bound on wall-clock time. Uses
+ * `Bun.spawn` (instead of `node:child_process`) for `proc.exited` as a
+ * clean Promise that integrates with the bun:test runner.
  */
 async function runHelper(options: {
   extensionOrigin: string;
@@ -202,7 +197,7 @@ async function runHelper(options: {
 // Tests
 // ---------------------------------------------------------------------------
 
-describe("native host — Gap 2 / Gap 3 regression coverage", () => {
+describe("native host — subprocess regression coverage", () => {
   let pair: MockPairServer | null = null;
 
   beforeAll(() => {
@@ -226,17 +221,17 @@ describe("native host — Gap 2 / Gap 3 regression coverage", () => {
     return;
   }
 
-  test("Gap 2: unauthorized origin halts before contacting the pair endpoint", async () => {
+  test("unauthorized origin halts before contacting the pair endpoint", async () => {
     const srv = pair!;
     srv.requests.length = 0;
 
     const result = await runHelper({
       extensionOrigin: DISALLOWED_ORIGIN,
       assistantPort: srv.port,
       // Pre-write a request_token frame so that if the unauthorized
-      // branch ever falls through to the stdin listener (the Gap 2
-      // bug), the helper would observe a frame and POST the pair
-      // endpoint. Both of those side effects are asserted below.
+      // branch ever falls through to the stdin listener, the helper
+      // would observe a frame and POST the pair endpoint. Both of
+      // those side effects are asserted below.
       stdinBytes: encodeFrame({ type: "request_token" }),
       timeoutMs: 1000,
     });
@@ -252,14 +247,14 @@ describe("native host — Gap 2 / Gap 3 regression coverage", () => {
     expect(frame.type).toBe("error");
     expect(frame.message).toBe("unauthorized_origin");
 
-    // The critical Gap 2 invariant: the helper must NOT have POSTed
-    // anything to /v1/browser-extension-pair. If the unauthorized
-    // branch falls through and the stdin listener runs, the mock
-    // server's `requests` array will contain at least one entry.
+    // The critical invariant: the helper must NOT have POSTed anything
+    // to /v1/browser-extension-pair. If the unauthorized branch falls
+    // through and the stdin listener runs, the mock server's
+    // `requests` array will contain at least one entry.
     expect(srv.requests).toHaveLength(0);
   });
 
-  test("Gap 3: authorized origin forwards guardianId in the token_response frame", async () => {
+  test("authorized origin forwards guardianId in the token_response frame", async () => {
     const srv = pair!;
     srv.requests.length = 0;
     srv.nextResponseBody = () => ({
@@ -296,7 +291,7 @@ describe("native host — Gap 2 / Gap 3 regression coverage", () => {
     expect(srv.requests[0]!.body).toEqual({ extensionOrigin: ALLOWED_ORIGIN });
   });
 
-  test("Gap 3: missing guardianId in the pair response is rejected with an error frame", async () => {
+  test("missing guardianId in the pair response is rejected with an error frame", async () => {
     const srv = pair!;
     srv.requests.length = 0;
     // Mock returns a body without `guardianId`. The helper's

clients/chrome-extension-native-host/src/index.ts

@@ -25,9 +25,11 @@
  * The helper deliberately does NOT persist tokens — the extension is
  * responsible for storing the returned token in `chrome.storage.local`.
  *
- * See PR 11 (`/v1/browser-extension-pair` endpoint) and PR 12 (native
- * messaging host manifest + macOS installer wiring) for the rest of the
- * pairing flow.
+ * The pairing flow as a whole consists of: (a) this helper, (b) the
+ * assistant-side `/v1/browser-extension-pair` endpoint that mints the
+ * capability token, and (c) the macOS installer wiring that drops the
+ * compiled binary alongside the native-messaging host manifest Chrome
+ * reads to resolve `com.vellum.daemon`.
  */
 
 import { readFileSync } from "node:fs";
@@ -133,48 +135,80 @@ function parseExtensionId(origin: string | undefined): string | null {
 }
 
 /**
- * Write a single native-messaging frame to stdout and synchronously
- * terminate the process.
+ * Writes a native-messaging frame to stdout and terminates the process
+ * synchronously. The exit code is the authoritative signal to Chrome;
+ * the frame body is best-effort. Use this for error paths (unauthorized
+ * origin, malformed requests) where Chrome only needs to observe a
+ * non-zero exit and any frame-body truncation is acceptable.
  *
- * Chrome native messaging hosts communicate over a pipe-backed stdout.
- * Earlier versions of this helper used the callback form of
- * `process.stdout.write()` to defer `process.exit()` until the buffer had
- * been handed off, but that introduced a subtle bug: the function had to
- * return a forever-pending Promise to satisfy the `never` return type,
- * meaning callers that awaited it would yield control of the event loop
- * before the process terminated. In the unauthorized-origin path that
- * meant the stdin listener could be installed and start handling frames
- * before the helper actually exited.
- *
- * The synchronous form below avoids that hazard entirely:
- * `process.exit()` is itself `never`, so the function legitimately never
- * returns and there is no event-loop tick between the write and the
- * exit. Any partial flushing concerns are accepted as a trade-off — the
- * exit code is the authoritative signal to Chrome regardless of whether
- * the frame body made it across the pipe.
+ * Typed `never` because `process.exit()` never returns, which lets
+ * callers treat this as an unconditional terminator with no event-loop
+ * tick between the write and the exit.
  */
-function writeFrameAndExit(payload: unknown, exitCode: number): never {
+function writeErrorFrameAndExit(payload: unknown, exitCode: number): never {
   try {
     process.stdout.write(encodeFrame(payload));
   } catch {
-    // ignore — exit code is the authoritative signal
+    // Ignore — exit code is the authoritative signal here.
   }
   process.exit(exitCode);
 }
 
+/**
+ * Writes a native-messaging frame to stdout and terminates the process
+ * only after libuv has flushed the write to the pipe. Use this for
+ * success paths (e.g., `token_response`) where Chrome needs the full
+ * frame body to drive the extension's pairing flow.
+ *
+ * The callback form of `process.stdout.write()` fires once the buffer
+ * has been handed off to the kernel, so awaiting the returned Promise
+ * guarantees the frame made it across the pipe before the process
+ * exits. This matters on pipe-backed stdout (Chrome native messaging)
+ * where a sync `process.exit()` can terminate before libuv finishes
+ * flushing a large-enough frame — most visibly on Windows.
+ *
+ * The Promise never resolves: the callback always ends in
+ * `process.exit(exitCode)`, so from the caller's perspective an `await`
+ * on this function is a terminator. A defensive 5-second safety timeout
+ * rejects if the callback somehow never fires; the timer is `.unref()`ed
+ * so it cannot keep the event loop alive on its own.
+ */
+function writeFrameAndExitAsync(
+  payload: unknown,
+  exitCode: number,
+): Promise<never> {
+  return new Promise<never>((_, reject) => {
+    process.stdout.write(encodeFrame(payload), () => {
+      // Best-effort: exit with the intended code even if the callback
+      // reports a write error. Chrome will observe a disconnect on the
+      // pipe and report the error through its native-messaging UI.
+      process.exit(exitCode);
+    });
+    const safety = setTimeout(() => {
+      reject(
+        new Error(
+          "writeFrameAndExitAsync timed out waiting for stdout flush",
+        ),
+      );
+    }, 5000);
+    safety.unref?.();
+  });
+}
+
 /**
  * Emit an `error` frame and exit with a non-zero status. Also logs the
  * underlying message to stderr so an operator running the binary by hand
  * (or a Chrome extension developer inspecting the host's stderr stream)
  * can see what went wrong.
  *
- * Uses `writeFrameAndExit` so the error frame is written to stdout before
- * the process terminates. `writeFrameAndExit` handles its own write
- * failures internally, so no additional try/catch is needed here.
+ * Uses `writeErrorFrameAndExit` so the error frame is written to stdout
+ * before the process terminates. `writeErrorFrameAndExit` handles its
+ * own write failures internally, so no additional try/catch is needed
+ * here.
  */
 function fail(message: string, code = 1): never {
   process.stderr.write(`vellum-chrome-native-host: ${message}\n`);
-  writeFrameAndExit({ type: "error", message }, code);
+  writeErrorFrameAndExit({ type: "error", message }, code);
 }
 
 /**
@@ -257,11 +291,15 @@ async function main(): Promise<void> {
     process.stderr.write(
       `vellum-chrome-native-host: unauthorized_origin (got ${extensionOrigin ?? "<none>"})\n`,
     );
-    writeFrameAndExit({ type: "error", message: "unauthorized_origin" }, 1);
-    // Defense-in-depth: even though writeFrameAndExit calls process.exit
-    // synchronously and is typed `never`, an explicit `return` here
-    // guarantees we never fall through to the stdin listener setup below
-    // if a future refactor accidentally makes writeFrameAndExit async.
+    writeErrorFrameAndExit(
+      { type: "error", message: "unauthorized_origin" },
+      1,
+    );
+    // Defense-in-depth: even though writeErrorFrameAndExit calls
+    // process.exit synchronously and is typed `never`, an explicit
+    // `return` here guarantees we never fall through to the stdin
+    // listener setup below if a future refactor accidentally makes the
+    // helper async.
     return;
   }
 
@@ -305,7 +343,7 @@ async function main(): Promise<void> {
             extensionOrigin!,
             process.argv,
           );
-          writeFrameAndExit(
+          await writeFrameAndExitAsync(
             { type: "token_response", token, expiresAt, guardianId },
             0,
           );