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.

Author: guarzo

docs/ops.md

@@ -395,7 +395,9 @@ To use one:
 1. Open devtools on the app → **Application** → **Cookies** → the origin you are
    actually browsing (`http://localhost:3000` by default).
 2. Add a cookie whose **name** is your `SESSION_COOKIE_NAME` (default
-   `authgd_session`) and whose **value** is the printed string.
+   `authgd_session`). The **value** is only the text *after* the `=` — the
+   script prints a full `name=value` assignment, but devtools has separate
+   fields, and pasting the whole line into the value box fails to authenticate.
 3. **Path `/`.** Reload.
 
 Set it on the origin you browse, not on whatever `APP_BASE_URL` happens to say.
@@ -415,8 +417,9 @@ Two behaviors worth knowing:
   seed has no legitimate remote use. `ALLOW_REMOTE_SEED=1` overrides it
   deliberately.
 
-Character ids come from a reserved `91_000_0xx` block, chosen to sit clear of
-the `90_000_0xx` ids the e2e suite generates, so the two can never collide.
+Character ids come from the reserved **`91_000_000`–`91_999_999`** range
+(mains at `91_000_00x`, alts at `91_000_1xx`), chosen to sit clear of the
+`90_000_00x` ids `e2e/helpers.ts` generates, so the two can never collide.
 
 ### Real OAuth locally, over a tunnel
 
@@ -436,8 +439,11 @@ ngrok http 3000 --domain your-stable-domain.ngrok-free.app
 
 #### 2. Override `APP_BASE_URL` in `.env.local`
 
-`.env.local` wins over `.env` (both are loaded, later file first), and `.env*` is
-gitignored apart from `.env.example`. Keeping the override in a second file means
+`.env.local` takes precedence over `.env`, and `.env*` is gitignored apart from
+`.env.example`. Both loaders agree on that: Next.js applies its own
+`.env.local`-wins rule for `npm run dev`, and the `worker` / `db:migrate` /
+`db:seed` scripts pass `--env-file-if-exists=.env` before
+`--env-file-if-exists=.env.local`, where the later flag overrides the earlier. Keeping the override in a second file means
 your working `.env` stays untouched and switching back is deleting one file.
 
 ```bash

scripts/seed-dev.ts

@@ -17,10 +17,13 @@
  * duplicated lines and would put the e2e suite at risk to make dev nicer.
  * The session hashing is NOT duplicated: createSession below is the real one.
  */
+import { resolve } from "node:path";
+import { fileURLToPath } from "node:url";
 import { eq, sql } from "drizzle-orm";
 import { loadConfig, type Config } from "@/config";
 import { createDb, type Db } from "@/db";
 import { account, character } from "@/db/schema";
+import { TRUNCATE_ALL_SQL } from "@/db/tables";
 import { encryptToken } from "@/lib/crypto";
 import { createSession, revokeAccountSessions } from "@/services/session";
 
@@ -80,13 +83,10 @@ const SEED: SeedSpec[] = [
   },
 ];
 
-/** Same 11 tables as tests/helpers/db.ts. Never touches the pgboss schema. */
+/** Table list is shared with tests/helpers/db.ts so the two cannot drift.
+ *  Never touches the pgboss schema. */
 async function truncateAll(db: Db): Promise<void> {
-  await db.execute(sql`
-    TRUNCATE account, "character", discord_link, session, bootstrap_admin_grant,
-      outbox, oauth_transaction, contact_sync_state, sync_run,
-      wanderer_acl_observation, audit_log RESTART IDENTITY CASCADE
-  `);
+  await db.execute(sql.raw(TRUNCATE_ALL_SQL));
 }
 
 /**
@@ -110,6 +110,15 @@ export function isLocalDatabase(url: string): boolean {
   }
 }
 
+/** The refusal path is reached BY unparseable URLs, so it must not itself throw. */
+function describeHost(url: string): string {
+  try {
+    return new URL(url).host;
+  } catch {
+    return "unparseable DATABASE_URL";
+  }
+}
+
 async function upsertAccount(db: Db, cfg: Config, spec: SeedSpec): Promise<string> {
   // account.main_character_id's composite FK is DEFERRED (checked at COMMIT),
   // so the account and its main character must land in ONE transaction.
@@ -182,7 +191,7 @@ async function main(): Promise<void> {
 
   if (!isLocalDatabase(cfg.databaseUrl) && process.env.ALLOW_REMOTE_SEED !== "1") {
     console.error(
-      `refusing to seed a non-local database (${new URL(cfg.databaseUrl).host}).\n` +
+      `refusing to seed a non-local database (${describeHost(cfg.databaseUrl)}).\n` +
         `This script writes fixture accounts and, with --reset, TRUNCATEs every table.\n` +
         `Set ALLOW_REMOTE_SEED=1 if you genuinely mean it.`,
     );
@@ -224,7 +233,13 @@ async function main(): Promise<void> {
 }
 
 // Only run when invoked as a script, so tests can import seedDev directly.
-if (process.argv[1]?.endsWith("seed-dev.ts")) {
+// Compared as RESOLVED paths rather than by filename: an endsWith("seed-dev.ts")
+// check silently stops running main() if the file is ever renamed or emitted as
+// .js, and the command would exit 0 having seeded nothing.
+const invokedDirectly =
+  process.argv[1] !== undefined &&
+  resolve(process.argv[1]) === resolve(fileURLToPath(import.meta.url));
+if (invokedDirectly) {
   main().catch((err: unknown) => {
     console.error("seed failed:", err instanceof Error ? err.message : err);
     process.exit(1);

src/db/tables.ts

@@ -0,0 +1,36 @@
+/**
+ * Every table this app owns, in one place.
+ *
+ * Two callers TRUNCATE the whole set — the test helper between tests
+ * (tests/helpers/db.ts) and the dev seed's --reset (scripts/seed-dev.ts).
+ * They used to keep separate copies of this list, which drifts silently: a new
+ * table missing from one copy leaves stale rows behind and fails no test.
+ * tests/seed-dev.ts asserts this list matches the database, so adding a table
+ * without adding it here breaks a test instead of leaking rows.
+ *
+ * `character` is quoted because it is a reserved word in SQL.
+ */
+export const MANAGED_TABLES = [
+  "account",
+  '"character"',
+  "discord_link",
+  "session",
+  "bootstrap_admin_grant",
+  "outbox",
+  "oauth_transaction",
+  "contact_sync_state",
+  "sync_run",
+  "wanderer_acl_observation",
+  "audit_log",
+] as const;
+
+/** Bare table names, unquoted — for comparing against information_schema. */
+export const MANAGED_TABLE_NAMES: string[] = MANAGED_TABLES.map((t) =>
+  t.replace(/"/g, ""),
+);
+
+/**
+ * Static SQL built from a hardcoded constant — no user input reaches this, so
+ * it is safe to pass through sql.raw().
+ */
+export const TRUNCATE_ALL_SQL = `TRUNCATE ${MANAGED_TABLES.join(", ")} RESTART IDENTITY CASCADE`;

tests/helpers/db.ts

@@ -1,17 +1,14 @@
 import { sql } from "drizzle-orm";
 import { migrate } from "drizzle-orm/node-postgres/migrator";
 import { createDb, type Db } from "@/db";
+import { TRUNCATE_ALL_SQL } from "@/db/tables";
 
 export const TEST_URL =
   process.env.TEST_DATABASE_URL ?? "postgres://authgd:authgd@localhost:5433/authgd_test";
 
-/** Shared 11-table TRUNCATE used between tests to reset carry-over state. */
+/** Shared TRUNCATE used between tests. Table list lives in src/db/tables.ts. */
 export async function truncateAll(db: Db): Promise<void> {
-  await db.execute(sql`
-    TRUNCATE account, "character", discord_link, session, bootstrap_admin_grant,
-      outbox, oauth_transaction, contact_sync_state, sync_run,
-      wanderer_acl_observation, audit_log RESTART IDENTITY CASCADE
-  `);
+  await db.execute(sql.raw(TRUNCATE_ALL_SQL));
 }
 
 export async function setupTestDb() {

tests/seed-dev.test.ts

@@ -1,6 +1,7 @@
-import { eq } from "drizzle-orm";
+import { eq, sql } from "drizzle-orm";
 import { afterAll, beforeAll, beforeEach, describe, expect, it } from "vitest";
 import { account, character, session } from "@/db/schema";
+import { MANAGED_TABLE_NAMES } from "@/db/tables";
 import { isLocalDatabase, seedDev } from "../scripts/seed-dev";
 import { testConfig } from "./helpers/config";
 import { setupTestDb, truncateAll } from "./helpers/db";
@@ -87,9 +88,10 @@ describe("seedDev", () => {
     const second = await seedDev(ctx.db, cfg);
     // Not accumulating: still one per account, and the ids are new.
     expect(await ctx.db.select().from(session)).toHaveLength(second.length);
-    expect(new Set(second.map((s) => s.sessionId))).not.toEqual(
-      new Set(first.map((s) => s.sessionId)),
-    );
+    // Fully disjoint, not merely "not identical": a set inequality would pass
+    // even if only one of the six cookies had been reissued.
+    const firstIds = new Set(first.map((s) => s.sessionId));
+    expect(second.every((s) => !firstIds.has(s.sessionId))).toBe(true);
   });
 });
 
@@ -129,3 +131,20 @@ describe("isLocalDatabase", () => {
     }
   });
 });
+
+describe("MANAGED_TABLES", () => {
+  // --reset and the test-suite truncation both drive off this list. Before it
+  // was shared, a new table could be missing from a copy and leave stale rows
+  // with no test failing. This makes that drift break a test instead.
+  it("matches every table actually in the database", async () => {
+    const rows = await ctx.db.execute<{ table_name: string }>(sql`
+      select table_name from information_schema.tables
+      where table_schema = 'public' and table_type = 'BASE TABLE'
+    `);
+    const actual = rows.rows
+      .map((r) => r.table_name)
+      .filter((t) => !t.startsWith("__drizzle"))
+      .sort();
+    expect([...MANAGED_TABLE_NAMES].sort()).toEqual(actual);
+  });
+});