ci: GitHub Actions workflow, Node pin, and version-drift guard - #13
Merged
Conversation
Three jobs run in parallel on pull_request and push to main:
Typecheck, lint & format — node pin guard, typecheck, lint, format:check
Next build — no env block; verified nothing calls getConfig()
at build time (every route is dynamic)
Tests (unit, then e2e) — Postgres service, migrate, vitest, playwright
Unit and e2e share one database (docs/ops.md; playwright.config.ts sets
workers: 1), so they are sequential steps of a single job and can never
overlap. The Postgres service publishes host port 5433 and names its database
authgd_test, so the TEST_DATABASE_URL default already baked into
tests/helpers/db.ts and playwright.config.ts is correct with no override — CI
uses the same URL developers do.
Migrations run as an explicit step before the tests. tests/helpers/db.ts
migrates itself, but e2e/helpers.ts only TRUNCATEs; verified that e2e against
an unmigrated database fails with `relation "account" does not exist`, so
without this step e2e would pass only as a side effect of the unit tests
having run first.
Node is pinned to 22 in .nvmrc to match the Dockerfile, with engines >=22 in
package.json. scripts/check-node-version.sh asserts the Dockerfile, .nvmrc and
engines agree and runs first in CI, so a partial bump fails the PR instead of
surfacing at deploy time.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Comment |
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
…cks (#16) #14 added an http_service check on /api/health, which makes a claim I wrote in #12 false: docs/ops.md said "fly.toml defines no health checks". It now does. The conclusion was right for the wrong reason, and the wrong reason is the dangerous part — a reader who checks fly.toml now sees a health check and reasonably concludes config errors are covered. They are not: /api/health never calls getConfig(). getDb() reads process.env.DATABASE_URL directly (src/db/index.ts) and checkLiveness only runs `select 1` (src/services/health.ts). With SYNC_MODE unset the endpoint returns 200 and the machine stays in rotation while every real page 500s. Replaced the paragraph with a per-component table that says exactly which layer catches what, and names /api/health's 200 as the trap. Also switched the example to `fly secrets set --stage`, which avoids restarting machines for a value the running code does not yet read. Separately, notes .nvmrc and scripts/check-node-version.sh from #13 alongside the existing engines/.npmrc mention, so a future Node bump knows all three pins must agree.
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
EVE SSO rejects the fake client id in .env.example, so there was no way to browse a running dev app as a real account — least of all an admin. The seeding machinery existed but was e2e-only: pinned to the test database and port 3111, and insert-only, because e2e gets a truncated database before every test and never had to survive a second run. npm run db:seed upsert; safe to re-run npm run db:seed -- --reset TRUNCATE first, for a clean slate Seeds six accounts — an admin with two alts, a flygd member with an alt, blue, green, plus cryo and tier_locked so the admin pages have those states to render — and prints a session cookie for each. IDEMPOTENT BY DEFAULT, which is the whole design. Character ids are fixed (a reserved 91_000_0xx block, clear of e2e's 90_000_0xx counter so the two can never collide) and every row is upserted, so a second run converges instead of dying on a duplicate primary key. The account and its main character are upserted in ONE transaction because account_main_character_fk is DEFERRABLE INITIALLY DEFERRED and only checked at commit. Both paths write rows — --reset destructively, the default by adding fixture accounts — and a dev seed has no legitimate remote use, so the script refuses any DATABASE_URL that is not provably loopback. ALLOW_REMOTE_SEED=1 overrides. The host is lower-cased first: `postgres:` is a NON-SPECIAL URL scheme, so WHATWG preserves host case (`http:` would not), and without that postgres://LOCALHOST/... is refused — which would train developers to reach for the override to work around a false rejection. Deliberately NOT shared with e2e/helpers.ts or tests/helpers/seed.ts. The repo already runs two independent seed helpers with different contracts; this is a third. Unifying them would need one helper parameterised for insert-only AND upsert, which costs more than the ~20 duplicated lines and puts the e2e suite at risk to make dev nicer. The session hashing is NOT duplicated — createSession from src/services/session.ts is the real one. Also removes a duplicate "engines" key in package.json: main carried both ">=22" (#13) and ">=22.9" (#12). JSON.parse keeps the last, so it resolved correctly by luck of ordering; a tool taking the first would have read a floor below the one fly.toml's release command depends on. Stage 3 of docs/superpowers/specs/2026-08-03-local-dev-setup.md
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
* feat: dev seed script with paste-able session cookies EVE SSO rejects the fake client id in .env.example, so there was no way to browse a running dev app as a real account — least of all an admin. The seeding machinery existed but was e2e-only: pinned to the test database and port 3111, and insert-only, because e2e gets a truncated database before every test and never had to survive a second run. npm run db:seed upsert; safe to re-run npm run db:seed -- --reset TRUNCATE first, for a clean slate Seeds six accounts — an admin with two alts, a flygd member with an alt, blue, green, plus cryo and tier_locked so the admin pages have those states to render — and prints a session cookie for each. IDEMPOTENT BY DEFAULT, which is the whole design. Character ids are fixed (a reserved 91_000_0xx block, clear of e2e's 90_000_0xx counter so the two can never collide) and every row is upserted, so a second run converges instead of dying on a duplicate primary key. The account and its main character are upserted in ONE transaction because account_main_character_fk is DEFERRABLE INITIALLY DEFERRED and only checked at commit. Both paths write rows — --reset destructively, the default by adding fixture accounts — and a dev seed has no legitimate remote use, so the script refuses any DATABASE_URL that is not provably loopback. ALLOW_REMOTE_SEED=1 overrides. The host is lower-cased first: `postgres:` is a NON-SPECIAL URL scheme, so WHATWG preserves host case (`http:` would not), and without that postgres://LOCALHOST/... is refused — which would train developers to reach for the override to work around a false rejection. Deliberately NOT shared with e2e/helpers.ts or tests/helpers/seed.ts. The repo already runs two independent seed helpers with different contracts; this is a third. Unifying them would need one helper parameterised for insert-only AND upsert, which costs more than the ~20 duplicated lines and puts the e2e suite at risk to make dev nicer. The session hashing is NOT duplicated — createSession from src/services/session.ts is the real one. Also removes a duplicate "engines" key in package.json: main carried both ">=22" (#13) and ">=22.9" (#12). JSON.parse keeps the last, so it resolved correctly by luck of ordering; a tool taking the first would have read a floor below the one fly.toml's release command depends on. Stage 3 of docs/superpowers/specs/2026-08-03-local-dev-setup.md * docs: testing EVE SSO and Discord linking over a tunnel (#21) Stage 4, documentation only. The seeded cookie from Stage 3 covers most dev work; this covers the case it cannot — changing the login or character-link flows themselves, which needs the real providers to redirect back to your machine. Every concrete claim was verified against the code rather than described from memory: - Redirect URIs are string-concatenated, not URL-joined, so a trailing slash on APP_BASE_URL yields `//auth/eve/callback` and z.string().url() accepts it silently — it surfaces much later as an unexplained redirect mismatch. - One EVE entry covers both flows: /auth/eve/login and /auth/eve/link both call buildEveAuthorizeUrl, so they share /auth/eve/callback. - EVE sends redirect_uri only on authorize; exchangeEveCode sends grant_type, code and code_verifier. Discord sends it TWICE — authorize and token exchange — and they must match. Changing APP_BASE_URL mid-flow therefore breaks Discord linking while EVE login keeps working, which reads like a Discord outage and is not. - Once APP_BASE_URL is https the session cookie is issued Secure, so the browser will not return it over http://localhost — you appear logged out no matter how often you log in. Browse the tunnel origin. Recommends .env.local for the override: it wins over .env, .env* is gitignored apart from .env.example, and switching back is deleting one file. * fix: address CodeRabbit review on #20 Seven findings, all verified against the code first. Two were real bugs: - The non-local refusal message called new URL(cfg.databaseUrl).host to name the host — but that branch is reached BY unparseable URLs, so it threw a TypeError and printed a stack trace instead of the guidance. Confirmed: `new URL("not a url")` throws. Now degrades to "unparseable DATABASE_URL". - The entry-point guard was process.argv[1]?.endsWith("seed-dev.ts"). Rename the file or emit it as .js and main() silently stops running: the command exits 0 having seeded nothing. Now compares resolved paths against fileURLToPath(import.meta.url). The truncation table list was duplicated between scripts/seed-dev.ts and tests/helpers/db.ts. Drift there is silent — a new table missing from one copy leaves stale rows after --reset and fails no test. Both now import src/db/tables.ts, and a new test asserts the list matches information_schema, so adding a table without registering it breaks a test instead of leaking rows. Test fix: the session-reissue assertion compared two Sets for inequality, which passes if only one of six cookies changed. Now asserts full disjointness. Docs: the cookie step said the "value" is the printed string, but the script prints a full name=value assignment and devtools has separate fields — pasting the whole line fails to authenticate. The documented id range said 91_000_0xx while alts use 91_000_1xx; corrected to the reserved 91_000_000-91_999_999 range. And "later file first" was a confusing way to describe env precedence — now states that Next applies its own .env.local-wins rule while the tsx scripts get the same result from flag order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 2 of the tooling work. Adds CI, pins Node, and guards the pin against drift.
Jobs
Three jobs run in parallel on
pull_requestand onpushtomain:typecheck,lint,format:checknpm run buildnpm test→npm run test:e2eDecisions
Unit and e2e are serialized. They share one database (
docs/ops.md;playwright.config.tssetsworkers: 1), so they are sequential steps of a single job rather than separate jobs — they can never overlap. Chosen over giving them separate databases.CI uses the same database URL developers do. The Postgres service publishes host port
5433and names its databaseauthgd_test, so theTEST_DATABASE_URLdefault already baked intotests/helpers/db.tsandplaywright.config.tsis correct with no env override.docker-compose.dev.ymlcreates that database via an init script, which a service container cannot mount; naming it directly is equivalent for CI, which never needs the non-testauthgddatabase.Playwright config is left alone. GitHub Actions sets
CI=true, which makesreuseExistingServerfalse, so its self-contained :3111 dev server and env block work as written.Migrations run as an explicit step.
tests/helpers/db.tsmigrates itself, bute2e/helpers.tsonlyTRUNCATEs. Verified that e2e against an unmigrated database fails withrelation "account" does not exist— so without this step, e2e would pass only as a side effect of the unit tests having run first. That implicit ordering is now removed.The build job has no env block. Verified by building with
DATABASE_URLandTOKEN_ENCRYPTION_KEYunset:getConfig()validates lazily per request and every route is dynamic (ƒ), so nothing calls it at build time.Node pin.
.nvmrcis22to match the Dockerfile;package.jsongetsengines: { node: ">=22" }.scripts/check-node-version.shasserts the Dockerfile,.nvmrc, andenginesagree, and runs first in CI so a partial bump fails the PR rather than surfacing at deploy time.enginesis a>=22floor rather than a hard22.xso it does not emitEBADENGINEon newer local Node versions; the exact pin lives in.nvmrcand the guard script is what actually prevents drift.Verification
All run locally:
Drift guard, all three paths:
Full CI sequence against a clean, isolated database:
Suggested branch protection
Require all three checks.
Tests (unit, then e2e)is only 6 e2e tests in ~13s and covers the session-cookie and admin-guard paths, so the flake risk is low relative to the coverage; revisit if the suite grows. Also worth enabling "require branches to be up to date before merging" — the checks are fast and it prevents semantic conflicts between concurrently merged PRs.🤖 Generated with Claude Code