From 4ae70dbde329681265d44dd2b21ecfb871ec2dae Mon Sep 17 00:00:00 2001 From: Sourabh Choraria Date: Fri, 31 Jul 2026 21:43:57 +0100 Subject: [PATCH 1/2] fix(web-e2e): expect the auth origin the app actually uses, not the one we exported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The web e2e suite was UNRUNNABLE on any machine that had run `pnpm dev:secrets`, and had been since the harness was written. Reproduced deterministically here: 183 seconds, then next dev answered ... but /org/…/events never entered its route table The route was serving correctly within a second. It redirected to http://localhost:3001/login while the harness sat waiting for its own http://127.0.0.1:3199, so the readiness predicate returned false on every poll for the full three-minute budget and then blamed the route scan. The harness exported AUTH_BASE_URL to `next dev` via process.env and assumed that settled it. It does not: `getAuthBaseUrl()` resolves BINDING-FIRST (`workerEnv() ?? process.env`), and under `next dev` the binding comes from getPlatformProxy, which reads apps/web/.dev.vars — where the secrets manifest has always written AUTH_BASE_URL=http://localhost:3001. The app's precedence is correct; the harness's assumption was not. CI has no .dev.vars, so it took the fallback and stayed green — the inverse of the usual failure and exactly the class this repo's local-parity work exists to remove. The origin is now read from .dev.vars when present and falls back to the harness's isolated one otherwise, so CI behaviour is byte-for-byte unchanged. ⚠️ This does NOT explain the one CI failure. That remains unproven, and I am not claiming otherwise. What it does is make that failure mode self-diagnosing: a 307 to a DIFFERENT origin cannot be a partial route table — the catch-all rejects `org` and calls notFound(), so it emits 404 and never a redirect — so a redirect proves the route is serving and the EXPECTATION is wrong. The loop now says so immediately instead of polling it for 180 seconds. Verified by restoring the old behaviour behind the fix: it now fails in 4 seconds with "configuration mismatch, not a slow route scan", and the misleading "never entered its route table" message does not appear at all. With the fix in place: 25 passed in 31s. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae --- apps/web/playwright/deepest-route.test.ts | 70 ++++++++++++++++++++++- apps/web/playwright/deepest-route.ts | 42 ++++++++++++++ apps/web/playwright/global-setup.ts | 36 +++++++++++- 3 files changed, 145 insertions(+), 3 deletions(-) diff --git a/apps/web/playwright/deepest-route.test.ts b/apps/web/playwright/deepest-route.test.ts index d8200e98..a7a61e10 100644 --- a/apps/web/playwright/deepest-route.test.ts +++ b/apps/web/playwright/deepest-route.test.ts @@ -6,12 +6,14 @@ import { describe, expect, it } from "vitest"; import { ROUTE_TYPES_FILE, appRoutes, + authOriginMismatch, byDepthDesc, deepestAppRoute, - routeTypesInclude, - routeTypesListRoutes, + effectiveAuthOrigin, isRouteTableReady, probeUrlFor, + routeTypesInclude, + routeTypesListRoutes, urlSegments, } from "./deepest-route"; @@ -218,3 +220,67 @@ describe("isRouteTableReady", () => { expect(isRouteTableReady(302, `${AUTH}/login`, AUTH)).toBe(false); }); }); + +// ── The auth origin the probe must expect ──────────────────────────────────────────────────────────── +// The harness passed AUTH_BASE_URL to `next dev` via process.env and then demanded a 307 to that origin. +// But apps/web resolves it BINDING-FIRST (`getAuthBaseUrl`: workerEnv() ?? process.env), and under +// `next dev` the binding comes from getPlatformProxy — which reads `.dev.vars`. So on any machine that has +// run `pnpm dev:secrets`, the app redirected to http://localhost:3001 while the probe waited for +// http://127.0.0.1:3199, and the suite burned its whole 180s budget on a route that was serving correctly +// within a second. Green on CI (no `.dev.vars` there), dead on every developer machine — the inverse of +// the usual failure, and exactly the class this repo's local-parity work exists to remove. +describe("effectiveAuthOrigin", () => { + it("uses the origin the app will ACTUALLY resolve when .dev.vars sets one", () => { + expect( + effectiveAuthOrigin("AUTH_BASE_URL=http://localhost:3001\n", "http://127.0.0.1:3199"), + ).toBe("http://localhost:3001"); + }); + + it("falls back to the harness's isolated origin when there is no .dev.vars", () => { + // CI's case. Keeping the synthetic origin there preserves the property the comment relies on: an + // origin nothing else in the app points at, so nothing can wander off to a real auth server. + expect(effectiveAuthOrigin(null, "http://127.0.0.1:3199")).toBe("http://127.0.0.1:3199"); + }); + + it("ignores a blank or absent binding rather than expecting an empty origin", () => { + // `pnpm dev:secrets` writes an unconfigured key as `NAME=`; treating "" as configured would make the + // probe demand a redirect to the empty string and never match. + expect(effectiveAuthOrigin("AUTH_BASE_URL=\n", "http://127.0.0.1:3199")).toBe( + "http://127.0.0.1:3199", + ); + expect(effectiveAuthOrigin("OTHER=x\n", "http://127.0.0.1:3199")).toBe("http://127.0.0.1:3199"); + }); + + it("strips a trailing slash so the startsWith marker cannot miss", () => { + expect(effectiveAuthOrigin("AUTH_BASE_URL=http://localhost:3001/\n", "http://x")).toBe( + "http://localhost:3001", + ); + }); +}); + +// A 307 to the WRONG origin cannot be a partial route table: the catch-all rejects `org` and calls +// notFound(), so it emits 404 and never a redirect. Only the real route redirects — meaning the route is +// serving and the harness simply expects the wrong origin. Polling that for 180s and then blaming the +// scan is what cost this session an hour; it is a configuration mismatch and should say so at once. +describe("authOriginMismatch", () => { + it("recognises a 307 to a different origin as a config mismatch", () => { + expect(authOriginMismatch(307, "http://localhost:3001/login", "http://127.0.0.1:3199")).toBe( + true, + ); + }); + + it("is not triggered by the catch-all, which cannot redirect at all", () => { + expect(authOriginMismatch(404, null, "http://127.0.0.1:3199")).toBe(false); + expect(authOriginMismatch(200, null, "http://127.0.0.1:3199")).toBe(false); + }); + + it("is not triggered by the correct redirect", () => { + expect(authOriginMismatch(307, "http://127.0.0.1:3199/login", "http://127.0.0.1:3199")).toBe( + false, + ); + }); + + it("does not fire on a 307 with no Location, which is malformed rather than mismatched", () => { + expect(authOriginMismatch(307, null, "http://127.0.0.1:3199")).toBe(false); + }); +}); diff --git a/apps/web/playwright/deepest-route.ts b/apps/web/playwright/deepest-route.ts index 1f5d9f7f..1e458919 100644 --- a/apps/web/playwright/deepest-route.ts +++ b/apps/web/playwright/deepest-route.ts @@ -159,3 +159,45 @@ export function routeTypesInclude(text: string, route: readonly string[]): boole const want = `/${urlSegments(route).join("/")}`; return routeTypesListRoutes(text).includes(want); } + +/** + * The auth origin the app will ACTUALLY redirect to, given the `.dev.vars` this machine has. + * + * The probe's whole discriminator is "a 307 to the auth origin, which the catch-all cannot produce". That + * only works if the harness expects the origin the app really uses — and the harness does not get to + * choose. `getAuthBaseUrl()` resolves BINDING-FIRST (`workerEnv() ?? process.env`), and under `next dev` + * the binding comes from `getPlatformProxy`, which reads `apps/web/.dev.vars`. So a machine that has run + * `pnpm dev:secrets` redirects to http://localhost:3001 no matter what the harness puts in process.env. + * + * The symptom was maximally misleading: a route serving correctly within a second, reported after 180 + * seconds as "never entered its route table". Green on CI, where there is no `.dev.vars` at all, and dead + * on every developer machine. + * + * Falling back to the harness's own origin keeps CI isolated exactly as before — an origin nothing else in + * the app points at — so this widens nothing where the old assumption already held. + */ +export function effectiveAuthOrigin(devVars: string | null, fallback: string): string { + const line = (devVars ?? "").split("\n").find((l) => l.trim().startsWith("AUTH_BASE_URL=")); + const value = line ? line.slice(line.indexOf("=") + 1).trim() : ""; + // A blank is UNSET — `pnpm dev:secrets` writes an unconfigured key as `NAME=`, and demanding a redirect + // to the empty string would never match. + if (value === "") return fallback; + return value.replace(/\/+$/, ""); +} + +/** + * Is this response a CONFIGURATION mismatch rather than a route table still filling in? + * + * The catch-all `(app)/[...legacy]` rejects a first segment of `org` and calls `notFound()`, so a partial + * table yields 404 — never a redirect. A 307 therefore proves the real route is serving; if its Location + * points somewhere other than the origin the harness expects, the route is fine and the EXPECTATION is + * wrong. Continuing to poll that for the full boot budget and then reporting "never entered its route + * table" sends the reader through the wrong subsystem entirely, which is precisely what happened. + */ +export function authOriginMismatch( + status: number, + location: string | null, + authOrigin: string, +): boolean { + return status === 307 && location !== null && !location.startsWith(authOrigin); +} diff --git a/apps/web/playwright/global-setup.ts b/apps/web/playwright/global-setup.ts index babcfcd7..869937c9 100644 --- a/apps/web/playwright/global-setup.ts +++ b/apps/web/playwright/global-setup.ts @@ -1,4 +1,5 @@ import { spawn, type ChildProcess } from "node:child_process"; +import { readFileSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { resolve } from "node:path"; import { setTimeout as sleep } from "node:timers/promises"; @@ -11,7 +12,9 @@ import { setupSchema } from "../../../packages/db/test/migrate"; import { startEphemeralPostgres, type EphemeralPostgres } from "../../../packages/db/test/pg"; import { ROUTE_TYPES_FILE, + authOriginMismatch, deepestAppRoute, + effectiveAuthOrigin, isRouteTableReady, probeUrlFor, routeTypesInclude, @@ -124,7 +127,27 @@ async function seed(appUrl: string, ownerUrl: string): Promise { } /** Where the gate sends an unauthenticated request — see `AUTH_BASE_URL` below. */ -const AUTH_ORIGIN = "http://127.0.0.1:3199"; +const HARNESS_AUTH_ORIGIN = "http://127.0.0.1:3199"; + +/** + * The origin the probe must expect — read from this machine's `.dev.vars`, not assumed. + * + * apps/web resolves AUTH_BASE_URL binding-first, and under `next dev` the binding comes from + * getPlatformProxy reading `.dev.vars`. So the harness cannot impose its own origin via process.env, and + * assuming it could made the suite unrunnable on any machine that had run `pnpm dev:secrets` — see + * `effectiveAuthOrigin`. Resolved ONCE at module load: the file cannot change mid-run, and re-reading it + * per poll would be a second way for the loop to fail. + */ +const AUTH_ORIGIN = effectiveAuthOrigin( + (() => { + try { + return readFileSync(resolve(process.cwd(), ".dev.vars"), "utf8"); + } catch { + return null; // CI has none, which is the isolated case the fallback preserves + } + })(), + HARNESS_AUTH_ORIGIN, +); /** What the probe saw, so a timeout can report it instead of guessing. */ type ProbeResult = { ready: boolean; status: number; location: string | null }; @@ -273,6 +296,17 @@ async function startDevServer(appConnectionString: string): Promise Date: Fri, 31 Jul 2026 21:55:22 +0100 Subject: [PATCH 2/2] fix(web-e2e): parse .dev.vars properly, and compare origins not prefixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review found a real bug in the fix, and it was the same bug one level down: my hand-rolled parser would have silently reinstated the 180-second misdiagnosis it was written to remove. `scripts/dev-preflight.mjs` strips surrounding quotes when it reads the very same file. Mine did not, so `AUTH_BASE_URL="http://localhost:3001"` would have been read WITH its quotes, never matched the redirect, and produced exactly the three-minute "never entered its route table" this PR exists to eliminate. It now parses the way that parser does — skipping comments, matching the key exactly rather than by prefix, and stripping quotes. Verified end to end with a quoted value in a real .dev.vars: 25 passed in 33s. Also switched both predicates from `startsWith` to an ORIGIN comparison. `startsWith` cannot tell http://localhost:3001 from http://localhost:30010, so a genuinely wrong origin sharing a prefix would have read as correct — the readiness check would have accepted the wrong server, and the fast-fail would have missed the one case it exists for. A relative or unparseable Location is deliberately neither a match nor a mismatch: it means "keep polling". A false fast-fail trades a slow correct answer for a quick wrong one, which is the worse bargain of the two. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae --- apps/web/playwright/deepest-route.test.ts | 63 +++++++++++++++++++++++ apps/web/playwright/deepest-route.ts | 43 +++++++++++++--- 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/apps/web/playwright/deepest-route.test.ts b/apps/web/playwright/deepest-route.test.ts index a7a61e10..f823ec1f 100644 --- a/apps/web/playwright/deepest-route.test.ts +++ b/apps/web/playwright/deepest-route.test.ts @@ -284,3 +284,66 @@ describe("authOriginMismatch", () => { expect(authOriginMismatch(307, null, "http://127.0.0.1:3199")).toBe(false); }); }); + +// The parser must accept everything a real `.dev.vars` can hold, because anything it mishandles silently +// reinstates the 180s misdiagnosis this file exists to remove. `scripts/dev-preflight.mjs` strips +// surrounding quotes; a hand-rolled parser that did not would read `"http://localhost:3001"` — quotes +// included — and never match the redirect. +describe("effectiveAuthOrigin parses what .dev.vars actually contains", () => { + it("strips surrounding quotes, as the repo's own .dev.vars parser does", () => { + expect(effectiveAuthOrigin('AUTH_BASE_URL="http://localhost:3001"\n', "http://x")).toBe( + "http://localhost:3001", + ); + expect(effectiveAuthOrigin("AUTH_BASE_URL='http://localhost:3001'\n", "http://x")).toBe( + "http://localhost:3001", + ); + }); + + it("ignores a COMMENTED-OUT key rather than reading it as configuration", () => { + expect(effectiveAuthOrigin("# AUTH_BASE_URL=http://commented\n", "http://x")).toBe("http://x"); + }); + + it("is not fooled by a key that merely ends with the name", () => { + expect(effectiveAuthOrigin("NEXT_AUTH_BASE_URL=http://other\n", "http://x")).toBe("http://x"); + }); + + it("finds the key wherever it sits in the file", () => { + expect(effectiveAuthOrigin("A=1\nAUTH_BASE_URL=http://found\nB=2\n", "http://x")).toBe( + "http://found", + ); + }); + + it("strips a trailing slash from the FALLBACK too, not just the value", () => { + expect(effectiveAuthOrigin(null, "http://127.0.0.1:3199/")).toBe("http://127.0.0.1:3199"); + }); +}); + +// `startsWith` cannot distinguish http://localhost:3001 from http://localhost:30010, so a genuinely +// mismatched origin sharing a prefix would go unflagged — and fall back to the 180s timeout this PR is +// removing. Compare ORIGINS. +describe("origin comparison is exact, not prefix-based", () => { + it("does not treat a longer port as the expected origin", () => { + expect(isRouteTableReady(307, "http://localhost:30010/login", "http://localhost:3001")).toBe( + false, + ); + expect(authOriginMismatch(307, "http://localhost:30010/login", "http://localhost:3001")).toBe( + true, + ); + }); + + it("still accepts the genuine redirect", () => { + expect(isRouteTableReady(307, "http://localhost:3001/login", "http://localhost:3001")).toBe( + true, + ); + expect(authOriginMismatch(307, "http://localhost:3001/login", "http://localhost:3001")).toBe( + false, + ); + }); + + it("treats an unparseable or relative Location as 'keep polling', never as a mismatch", () => { + // Conservative on purpose: a false fast-fail would replace a slow correct answer with a fast wrong + // one, which is a worse trade than the timeout it avoids. + expect(authOriginMismatch(307, "/login", "http://localhost:3001")).toBe(false); + expect(isRouteTableReady(307, "/login", "http://localhost:3001")).toBe(false); + }); +}); diff --git a/apps/web/playwright/deepest-route.ts b/apps/web/playwright/deepest-route.ts index 1e458919..4fb5aece 100644 --- a/apps/web/playwright/deepest-route.ts +++ b/apps/web/playwright/deepest-route.ts @@ -122,7 +122,7 @@ export function isRouteTableReady( location: string | null, authOrigin: string, ): boolean { - return status === 307 && (location ?? "").startsWith(authOrigin); + return status === 307 && locationHasOrigin(location, authOrigin) === true; } // ── Waiting for the SCAN, not for a request ────────────────────────────────────────────────────────── @@ -177,12 +177,43 @@ export function routeTypesInclude(text: string, route: readonly string[]): boole * the app points at — so this widens nothing where the old assumption already held. */ export function effectiveAuthOrigin(devVars: string | null, fallback: string): string { - const line = (devVars ?? "").split("\n").find((l) => l.trim().startsWith("AUTH_BASE_URL=")); - const value = line ? line.slice(line.indexOf("=") + 1).trim() : ""; + // Parsed the way `scripts/dev-preflight.mjs` parses the same file, because anything this mishandles + // silently reinstates the 180-second misdiagnosis above. In particular a QUOTED value — which that + // parser strips and a naive one does not — would be read with its quotes and never match the redirect. + let value = ""; + for (const raw of (devVars ?? "").split("\n")) { + const line = raw.trim(); + if (line === "" || line.startsWith("#")) continue; // a commented key is not configuration + const at = line.indexOf("="); + if (at < 0 || line.slice(0, at).trim() !== "AUTH_BASE_URL") continue; // exact key, not a suffix match + value = line.slice(at + 1).trim(); + if (value.length >= 2 && /^(".*"|'.*')$/s.test(value)) value = value.slice(1, -1); + break; + } // A blank is UNSET — `pnpm dev:secrets` writes an unconfigured key as `NAME=`, and demanding a redirect // to the empty string would never match. - if (value === "") return fallback; - return value.replace(/\/+$/, ""); + return stripTrailingSlash(value === "" ? fallback : value); +} + +const stripTrailingSlash = (s: string): string => s.replace(/\/+$/, ""); + +/** + * Does `location` point at `origin`? Compared as ORIGINS, never as a string prefix. + * + * `startsWith` cannot tell http://localhost:3001 from http://localhost:30010, so a genuinely mismatched + * origin that happens to share a prefix would read as correct — the readiness predicate would accept the + * wrong server, and the fast-fail would miss the case it exists for. + * + * A relative or unparseable Location is NOT a match and NOT a mismatch: it means "keep polling". A false + * fast-fail would trade a slow correct answer for a quick wrong one, which is the worse bargain. + */ +export function locationHasOrigin(location: string | null, origin: string): boolean | null { + if (location === null) return null; + try { + return new URL(location).origin === new URL(origin).origin; + } catch { + return null; // relative or malformed — undecidable, so decide nothing + } } /** @@ -199,5 +230,5 @@ export function authOriginMismatch( location: string | null, authOrigin: string, ): boolean { - return status === 307 && location !== null && !location.startsWith(authOrigin); + return status === 307 && locationHasOrigin(location, authOrigin) === false; }