Stage 3: dev seed script with paste-able session cookies - #20
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 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 (6)
📝 WalkthroughWalkthroughThe pull request adds a development database seeder with reusable accounts and session cookies, tests its behavior, adds a ChangesDevelopment authentication workflows
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant db_seed as db:seed
participant seedDev
participant Database
Developer->>db_seed: Run development seed command
db_seed->>seedDev: Load environment and execute
seedDev->>Database: Upsert accounts and characters
seedDev->>Database: Revoke and recreate sessions
seedDev-->>Developer: Print session cookies
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ops.md`:
- Around line 393-399: Update the cookie setup instructions in the “To use one”
section to explicitly distinguish the cookie name from its value: tell
developers to enter only the text after `=` from the printed
`authgd_session=...` output in the value field, while keeping the cookie name as
`SESSION_COOKIE_NAME`.
- Around line 418-419: Update the character ID documentation in docs/ops.md to
describe the full reserved range as 91_000_000–91_999_999, while noting that
scripts/seed-dev.ts uses 91_000_0xx for mains and 91_000_1xx for alts and
e2e/helpers.ts starts at 90_000_001.
- Around line 439-441: Update the environment-file guidance in the relevant
documentation paragraph to explicitly state that Next.js gives .env.local
precedence over .env for npm run dev, and that the other scripts pass .env
before .env.local so Node applies the same override order. Remove the ambiguous
“later file first” wording while preserving the remaining .env.local workflow
guidance.
In `@scripts/seed-dev.ts`:
- Around line 83-90: The application table list is duplicated between
truncateAll and the test database helper. Add a shared APP_TABLES constant under
src/db, preserving the existing 11-table order and excluding pgboss, then update
truncateAll and the tests/helpers/db.ts truncation logic to import and reuse it
instead of maintaining local lists.
- Around line 183-190: The refusal branch in the seed flow crashes when
cfg.databaseUrl is unparseable because it reconstructs a URL directly. Update
the guard around isLocalDatabase to derive the host defensively, using a safe
parse with a fallback value, then preserve the refusal message and
process.exit(2) for both invalid and non-local URLs.
- Around line 226-232: Update the script-entry guard around main() to compare
the resolved process.argv[1] path with the resolved
fileURLToPath(import.meta.url) path, rather than checking for a seed-dev.ts
suffix. Preserve the existing main().catch error handling and ensure the guard
works after renaming or JavaScript emission.
In `@tests/seed-dev.test.ts`:
- Around line 87-92: Strengthen the assertions in the seedDev repeat-run test:
verify that no sessionId in second overlaps any sessionId in first, rather than
only comparing the Sets for inequality. Also assert that every session in second
is associated with a seeded account, using the existing session/account data and
identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 475d4863-839e-4c05-ae04-cf263ebc19ad
📒 Files selected for processing (4)
docs/ops.mdpackage.jsonscripts/seed-dev.tstests/seed-dev.test.ts
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
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.
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
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.
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
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.
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.
guarzo
force-pushed
the
worktree-local-dev-stage3
branch
from
August 3, 2026 20:46
2c483b1 to
7ac0057
Compare
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 3 of
docs/superpowers/specs/2026-08-03-local-dev-setup.md.The problem
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 it 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.Six accounts: an admin with two alts, a flygd member with an alt, blue, green, plus
cryoandtier_lockedso the admin pages have those states to render. Each prints a session cookie.Design
Idempotent by default, which is the whole point. Fixed character ids (a reserved
91_000_0xxblock, clear of e2e's90_000_0xxcounter so the two can never collide) plusonConflictDoUpdate, so a second run converges instead of dying on a duplicate primary key. Account and main character upsert in one transaction, becauseaccount_main_character_fkisDEFERRABLE INITIALLY DEFERREDand only checked at commit.Refuses a non-local
DATABASE_URL. Both paths write rows —--resetdestructively, the default by adding fixture accounts — and a dev seed has no legitimate remote use.ALLOW_REMOTE_SEED=1overrides. Note this is stricter than the approved decision, which only required the guard on--reset; easy to relax if you'd rather.Not shared with
e2e/helpers.tsortests/helpers/seed.ts. The repo already runs two independent seed helpers with different contracts (caller-supplied ids vs. auto-counter, both insert-only); this is a third (reserved block + upsert). Unifying them needs one helper parameterised for insert-only and upsert — more complexity than the ~20 duplicated lines, with the e2e suite as collateral. The session hashing is not duplicated:createSessionfromsrc/services/session.tsis the real one.Verification
The test that matters — does the printed cookie actually log you in?
Rerun, against a real database:
Guard, both directions, and
--resetproven by planting anaudit_logrow:Gates
npm run test:e2enot run locally — port 3111 was held by another workstream, and withCI=1Playwright errors rather than silently reusing their server. CI covers e2e on this PR.Review catch worth highlighting
isLocalDatabase()wrongly rejected mixed-case hosts.postgres:is a non-special URL scheme, so WHATWG preserves host case — unlikehttp::So
postgres://LOCALHOST:5433/...was refused, which would have trained developers to reach forALLOW_REMOTE_SEED=1to work around a false rejection — quietly defeating the guard. Fixed with an explicittoLowerCase(), plus a regression test. Also dropped a dead bare"::1"entry:new URL("postgres://[::1]/db").hostnamealways returns"[::1]", brackets included.Drive-by fix
package.jsoncarried twoengineskeys on main —">=22"(#13) and">=22.9"(#12).JSON.parsekeeps the last, so it resolved correctly by luck of ordering; a tool reading the first would have seen a floor below the onefly.toml's release command depends on. Removed the">=22".scripts/check-node-version.shstill passes:Dockerfile=22 .nvmrc=22 engines='>=22.9'.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation