ReviewOS

also looking at this

stacks/commshq

test(auth): pin the browser session contract, and gate the deploy

#14
Merged glennmichael123 wants to merge feat/auth-framework-session into main
4 files +400 -8
10 days ago

Groundwork for migrating CommsHQ off its hand-rolled auth actions (a66ffc2) onto the framework browser-session policy that landed upstream as stacksjs/stacks#2795. No auth behavior changes here — this is the safety net that has to exist before the overrides can be deleted.

1. Gate the deploy on the test suite

The production job ran lint, typecheck and test nowhere, so a push whose tests fail still deployed. Not hypothetical: on aac396460 the CI workflow's test job failed while the Deploy workflow succeeded — same sha, same minute — and production shipped.

The suite now runs inside the deploy job rather than as a needs: on the CI workflow, because needs: only reaches jobs in the same file and lint/typecheck/test live in ci.yml. Adding needs: there would have looked like a gate in review and done nothing.

Placed after the install and before the build, so a red suite aborts before the artifact exists and well before the deploy key is prepared. Costs ~2.5s against a ~3 minute deploy.

2. Pin the browser session contract

Until now the suite had 21 test files and not one touched auth. Eighteen new tests assert both sides at once — the app's current helpers and the framework functions that will replace them — so each must keep passing after the swap.

Three carry the weight:

  • The collapse guard. rememberedLifetime defaults to the baseline, so deleting the overrides without adding a browserSession config block silently turns "keep me signed in for 30 days" into 7 days. No error, no other failing test, no signal until someone is logged out 23 days early.
  • Same-number stamping. The cookie Max-Age must come from the lifetime issuance reported, and authCookieForBrowserSession throws rather than invent one.
  • The Secure trap. shouldSecureAuthCookie({url: 'commshq.localhost'}) returns false, and config/app.ts:15 is exactly url: env.APP_URL ?? 'commshq.localhost'. If APP_URL is ever absent or fails to decrypt, we ship a 30-day session cookie with no Secure. The migration must set config.auth.cookie.secure explicitly.

Every framework call takes an explicit config object, so no assertion depends on which .env the suite loads.

Verification

  • Suite: 128 → 146 pass, 0 fail
  • Lint: 0 errors, 0 warnings across 1374 files
  • Typecheck: clean

What comes next

Steps 3-6 of the migration, each separately reviewable: a real-login deploy smoke test, resyncing the vendored defaults from 0.74.59 to 0.74.66, adding the browserSession config (behavior-neutral while the overrides still run), and only then deleting app/Actions/Auth/* together with a regenerated storage/framework/auto-imports/actions.ts — the router reads that registry first with no existence check, so deleting the files without regenerating it turns every sign-in into a 500.

4 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.