diff --git a/.agents/plans/m17-cross-platform-collectors.md b/.agents/plans/m17-cross-platform-collectors.md index 879b3ff..0c90676 100644 --- a/.agents/plans/m17-cross-platform-collectors.md +++ b/.agents/plans/m17-cross-platform-collectors.md @@ -366,8 +366,9 @@ standard too, so D-M17-4's "free ARM Mac runners" premise holds for the Intel ha - [ ] Every 17.2a **and** 17.2b finding is either fixed in a later slice or carried as an open `.agents/research/incidents.md` entry with a stated reason. (17.2a raised **INC-2026-08…13**; **three** routed to a slice — 08, 09 → **17.3**; 12 → **17.4** — one to a re-derivation of - INC-2026-05 (11), one to a tooling backlog (10), and one (13) a procedure fix already applied - in-slice and inherited by 17.2b.) + INC-2026-05 (11), one to a tooling backlog (10, **since CLOSED `2026-08-09`** out-of-slice: + the bootstrap ownership claim in `packages/db/src/test-db-guard.ts`), and one (13) a + procedure fix already applied in-slice and inherited by 17.2b.) - [ ] **No claim in the release notes, docs or UAT carries a verification tier it did not earn.** Every `linux-x86_64` claim traceable only to 17.2a is `VM-verified`, not `hardware-verified` (`emulated ≠ hardware-verified`) — the same discipline D-M17-4 already applies to diff --git a/.agents/research/incidents.md b/.agents/research/incidents.md index 97bd0eb..25a4cb5 100644 --- a/.agents/research/incidents.md +++ b/.agents/research/incidents.md @@ -250,11 +250,13 @@ _(This sentence undercounted the six as five, and called all of them unfixed, fr the one `DATABASE_URL` points at on a foreign cluster** before it truncates — a guard at the destructive step rather than at the config step, which is the only place that cannot be bypassed by hand-editing `.env`. -- **Regression test?** Yes, and it must sit at the **destructive step**, not the config step: a test - on the integration bootstrap asserting it **refuses to `TRUNCATE`** when `DATABASE_URL_TEST` - shares host:port with a cluster this checkout did not create — negative-controlled by pointing - both handles at one cluster and asserting the guard throws. That fails on today's code (no such - guard exists) and passes only after the fix. +- **Regression test?** Yes — **29 tests, both layers**, and they sit at the **bootstrap**, not the + config step. `packages/db/src/test-db-guard.test.ts` (21) is the pure value oracle for the + decision; `packages/db/src/test-db-guard.int.test.ts` (8) proves against real Postgres that the + claim round-trips and — the property the whole design rests on — **survives the + `TRUNCATE … CASCADE` it defends against**. Negative-controlled twice: neutering `decideClaim` to + always proceed reddens 9 of the 29, and the behavioural control below reproduces the incident + outright. **An earlier draft proposed "a unit test on `setup-env.mjs` asserting the test handles are derivable from the root `.env`", which cannot discriminate**, for two independent reasons. @@ -269,6 +271,53 @@ _(This sentence undercounted the six as five, and called all of them unfixed, fr for `SESSION_SECRET`), and borrowing it here is what made a broken line read as sound. Note the Disposition above already named the right answer as option (b). +- **Resolved:** `2026-08-09`, tooling fix on `inc-2026-10-test-db-claim` — **option (b), the guard + at the bootstrap**. `packages/db/src/test-db-guard.ts` is called from `vitest.global-setup.ts` + BEFORE `runMigrations` and therefore before any test file's `beforeEach`, so a refusal aborts the + run with **zero `TRUNCATE`s executed**. It is the only choke point that sees every vitest + invocation (`npm test`, `repo-health`, a bare `npx vitest run `) and evaluates the handles + as they actually resolve — which is what makes it un-bypassable by hand-editing `.env`, the + property the config-step candidate (a) lacked. + + Two checks. The cheap catastrophic one is pure and needs no connection: + `assertTestDbIsNotArchive` refuses when `DATABASE_URL_TEST` names the same host:port:database as + `DATABASE_URL` — the case where the first `beforeEach` truncates the live archive rather than + test data. The real one is an **ownership claim stamped on the test database itself**, as a + `COMMENT ON DATABASE` read back through `shobj_description(oid, 'pg_database')`. The first + checkout to run stamps its absolute path; every other checkout is refused by name, with + `TEST_DB_CLAIM_TAKEOVER=1` as the documented escape hatch. + + **Why a comment and not a marker table:** the claim has to survive the exact operation it defends + against, and a row does not — `TRUNCATE … CASCADE` is the first thing every int test runs. A + database comment also needs no schema change, so it does not perturb the 33-table count this + incident's own evidence used, nor any TRUNCATE list. `test-db-guard.int.test.ts` pins the + survival property directly. + + **The guarantee, stated exactly:** _at most one checkout can ever `TRUNCATE` a given test + database._ Note the corollary rather than hiding it — if the SECOND checkout runs first it takes + the claim and the FIRST is the one refused later. That is still the invariant, because the hazard + is two checkouts sharing one test database, not which arrived first; either way the collision is + reported instead of silently destroying data. + + **Behavioural negative control (the incident fired, then prevented).** A canary row was inserted + into `organizations` on the real `420ai_test`, and the database was stamped with a foreign claim + (`420ai-test-claim:/home/seanr/cleanroom/420AI`) to simulate the second checkout. With the guard + **on**, `npx vitest run packages/db/src/repositories/organizations.int.test.ts` aborted in global + setup and the canary survived (`count = 1`). With the two guard calls stripped from + `vitest.global-setup.ts`, the identical command reported **`Test Files 1 passed`, `Tests 6 + passed`, exit 0** — and the canary was **gone** (`count = 0`). Green, silent, destructive: the + incident as described, reproduced on demand. + + **Verification tier:** `hardware-verified` on `windows-x86_64` against the real compose Postgres + 17 (the claim round-trip, the TRUNCATE-survival property and the behavioural control were all + executed, not reasoned about). The `ubuntu-latest` half is `CI-verified` — `repo-health.yml` + creates a fresh `420ai_test` per run, so CI exercises the unclaimed → stamp path every time. + + One thing the fix deliberately does **not** do: change `.env.example` or `setup-env.mjs`. The + generated handles still point at `localhost:5433`, because that is correct for the overwhelmingly + common single-checkout case, and a config-step guess cannot know whether the cluster already + there is yours. The claim answers that question with evidence instead. + ## INC-2026-09 — `build-sea.mjs` reports **PASS** on a non-Windows triple while producing an unbundlable, mislabelled artifact - **Date / observed by:** 2026-08-09 / clean-room deploy `linux-x86_64`, slice 17.2a diff --git a/SUMMARY.md b/SUMMARY.md index 859c909..2e723e0 100644 --- a/SUMMARY.md +++ b/SUMMARY.md @@ -1303,7 +1303,9 @@ records` and `0 ÷ 4000` are opposite facts. An HTTP test asserts the literal st (09) and boots `serve` into the operator's **real** collector home (08 — a double-writer hazard, and the one isolation constraint this run breached, repaired in place under D-16.0-2); `npm run setup` aims the test handles at whatever cluster is already on the host, so a second - checkout's `npx vitest run` would TRUNCATE the real test DB (10 — demonstrated, not fired); + checkout's `npx vitest run` would TRUNCATE the real test DB (10 — demonstrated, not fired; + **CLOSED `2026-08-09`** by an ownership claim stamped on the test database itself and checked + in `vitest.global-setup.ts` before any `TRUNCATE` — see the entry's **Resolved** block); **INC-2026-05's recorded root cause contradicts `file-watcher.ts:136`** and 16.3 was designed against it (11); a nonexistent connector source leaves **no trace anywhere** (12); and WSL interop fabricated a plausible wrong value in the transcript (13). **17.3 re-scored 65% → 80% diff --git a/docs/guide/troubleshooting.md b/docs/guide/troubleshooting.md index e9819d7..1b812ef 100644 --- a/docs/guide/troubleshooting.md +++ b/docs/guide/troubleshooting.md @@ -78,11 +78,13 @@ Common failure modes, what they mean, and how to fix them. Grouped by where they ## Tests & gates -| Symptom | Cause | Fix | -| ----------------------------------------------------- | -------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------ | -| Integration tests "skipped" but suite green | `DATABASE_URL_TEST` unset ⇒ `*.int.test.ts` self-skip | For real sign-off: `npm run db:up && npm run db:migrate` then `npm run repo-health -- --require-db` (fails if any int test skipped). | -| `ExperimentalWarning` from `node:sqlite` | Node 24's `node:sqlite` is experimental | **Expected by design** — does not affect correctness; don't suppress it in a way that breaks tests. | -| `repo-health` fails on NUL-byte / stray-artifact scan | A source file has embedded NULs, or emitted `*.js`/`dist/`/`*.sqlite` got staged | Rewrite the offending file as clean UTF-8; unstage build artifacts (`*.sqlite` and `src/`-emitted JS are gitignored). | +| Symptom | Cause | Fix | +| -------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Integration tests "skipped" but suite green | `DATABASE_URL_TEST` unset ⇒ `*.int.test.ts` self-skip | For real sign-off: `npm run db:up && npm run db:migrate` then `npm run repo-health -- --require-db` (fails if any int test skipped). | +| `ExperimentalWarning` from `node:sqlite` | Node 24's `node:sqlite` is experimental | **Expected by design** — does not affect correctness; don't suppress it in a way that breaks tests. | +| `repo-health` fails on NUL-byte / stray-artifact scan | A source file has embedded NULs, or emitted `*.js`/`dist/`/`*.sqlite` got staged | Rewrite the offending file as clean UTF-8; unstage build artifacts (`*.sqlite` and `src/`-emitted JS are gitignored). | +| "Refusing to run integration tests: … is claimed by a DIFFERENT checkout" | INC-2026-10 — this checkout's `DATABASE_URL_TEST` points at a test database another checkout already owns. Every `*.int.test.ts` opens with `TRUNCATE … CASCADE`, so the run was stopped **before** it destroyed that checkout's data. | Give this checkout its own cluster: change the published port in `docker-compose.yml` and the four handles in `.env`. If the database really is yours (you moved or renamed the checkout), re-claim it once with `TEST_DB_CLAIM_TAKEOVER=1 npx vitest run`. | +| "Refusing to run integration tests: `DATABASE_URL_TEST` names the SAME database as `DATABASE_URL`" | The test handle points at the live archive, so the first `beforeEach` would truncate production. | Point `DATABASE_URL_TEST` at the separate `420ai_test` database the compose stack provisions beside `420ai` (`docker/init-test-db.sql`). | --- diff --git a/packages/db/src/index.ts b/packages/db/src/index.ts index 92590ae..e792e20 100644 --- a/packages/db/src/index.ts +++ b/packages/db/src/index.ts @@ -56,6 +56,21 @@ export { encryptField, decryptField, activeKeyId } from "./crypto.js"; export type { EncryptedField } from "./crypto.js"; export { API_KEY_PREFIX, generateToken, hashToken } from "./tokens.js"; export { runMigrations } from "./migrate.js"; +// INC-2026-10 — the integration-suite ownership guards. Called from `vitest.global-setup.ts` +// BEFORE `runMigrations`, so a refusal aborts the run before any `TRUNCATE`. Exported from the +// barrel because the global setup is the consumer and it lives at the repo root. +export { + assertTestDbIsNotArchive, + claimTestDatabase, + decideClaim, + describeTarget, + normalizeCheckout, + sameDatabaseTarget, + ForeignTestDatabaseError, + CLAIM_PREFIX, + TAKEOVER_ENV, +} from "./test-db-guard.js"; +export type { ClaimDecision } from "./test-db-guard.js"; export { createPairingCode, redeemPairingCode, PairingError } from "./repositories/pairing.js"; export { createMachine, diff --git a/packages/db/src/test-db-guard.int.test.ts b/packages/db/src/test-db-guard.int.test.ts new file mode 100644 index 0000000..29da3bc --- /dev/null +++ b/packages/db/src/test-db-guard.int.test.ts @@ -0,0 +1,154 @@ +import { fileURLToPath } from "node:url"; +import { Pool } from "pg"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { + CLAIM_PREFIX, + ForeignTestDatabaseError, + claimTestDatabase, + normalizeCheckout, +} from "./test-db-guard.js"; + +const TEST_URL = process.env.DATABASE_URL_TEST; + +/** + * INC-2026-10 — the guard against a run in the WRONG place, exercised against real Postgres. + * + * The unit suite next door is the value oracle for the decision. This file exists for the two + * things it cannot prove and which the design rests on: that `COMMENT ON DATABASE` / + * `shobj_description` actually round-trip a claim, and that the claim SURVIVES the + * `TRUNCATE … CASCADE` it is defending against (the reason it is a comment and not a row). + * + * Every test restores the original comment in `afterEach`, not merely at the end of a happy + * path: leaving a foreign claim behind would lock this very checkout out of its own test + * database on the next run — the CLAUDE.md 15.5 lesson about releasing in a `finally`, one + * resource over. + */ +describe.skipIf(!TEST_URL)("test-db-guard (INC-2026-10) against real Postgres", () => { + const REPO_ROOT = normalizeCheckout(fileURLToPath(new URL("../../../", import.meta.url))); + const OTHER_CHECKOUT = normalizeCheckout("/tmp/420ai-second-checkout"); + + let pool: Pool; + let original: string | null = null; + + const readComment = async (): Promise => { + const { rows } = await pool.query<{ comment: string | null }>( + `select shobj_description(oid, 'pg_database') as comment + from pg_database where datname = current_database()`, + ); + return rows[0]?.comment ?? null; + }; + + /** Set the database comment directly — `COMMENT ON DATABASE` takes no bind parameters. */ + const writeComment = async (value: string | null): Promise => { + const [built] = ( + await pool.query<{ stmt: string }>( + value === null + ? `select format('COMMENT ON DATABASE %I IS NULL', current_database()) as stmt` + : `select format('COMMENT ON DATABASE %I IS %L', current_database(), $1::text) as stmt`, + value === null ? [] : [value], + ) + ).rows; + await pool.query(built!.stmt); + }; + + beforeAll(async () => { + pool = new Pool({ connectionString: TEST_URL }); + original = await readComment(); + }); + + afterEach(async () => { + await writeComment(original); + }); + + afterAll(async () => { + await pool.end(); + }); + + // End-to-end proof that the wiring in vitest.global-setup.ts ran: by the time any test + // executes, this database is already claimed BY THIS CHECKOUT. + it("the global setup has already stamped this checkout's claim", () => { + expect(original).toBe(`${CLAIM_PREFIX}${REPO_ROOT}`); + }); + + // THE REGRESSION TEST FOR THE INCIDENT: a second checkout aimed at the first one's test + // database is refused before it can truncate anything. + it("refuses a database claimed by another checkout", async () => { + await writeComment(`${CLAIM_PREFIX}${OTHER_CHECKOUT}`); + + await expect( + claimTestDatabase({ connectionString: TEST_URL!, checkout: REPO_ROOT }), + ).rejects.toBeInstanceOf(ForeignTestDatabaseError); + + // The message has to name BOTH sides — diagnosing this from "permission denied" would cost + // an afternoon, and the operator's next question is always "then whose is it?". + await expect( + claimTestDatabase({ connectionString: TEST_URL!, checkout: REPO_ROOT }), + ).rejects.toThrow(new RegExp(OTHER_CHECKOUT.replace(/[\\^$.*+?()[\]{}|]/g, "\\$&"))); + + // And it must NOT have overwritten the other checkout's claim on the way out. + expect(await readComment()).toBe(`${CLAIM_PREFIX}${OTHER_CHECKOUT}`); + }); + + it("claims an unclaimed database and round-trips the comment", async () => { + await writeComment(null); + expect(await readComment()).toBeNull(); + + const decision = await claimTestDatabase({ + connectionString: TEST_URL!, + checkout: REPO_ROOT, + }); + + expect(decision).toEqual({ action: "stamp" }); + expect(await readComment()).toBe(`${CLAIM_PREFIX}${REPO_ROOT}`); + }); + + it("proceeds on a second run without rewriting the claim", async () => { + const decision = await claimTestDatabase({ + connectionString: TEST_URL!, + checkout: REPO_ROOT, + }); + expect(decision).toEqual({ action: "proceed" }); + expect(await readComment()).toBe(`${CLAIM_PREFIX}${REPO_ROOT}`); + }); + + it("refuses a database carrying an unrecognized comment", async () => { + await writeComment("scratch db for the analytics spike"); + await expect( + claimTestDatabase({ connectionString: TEST_URL!, checkout: REPO_ROOT }), + ).rejects.toThrow(/unrecognized comment/); + }); + + it("re-claims under takeover", async () => { + await writeComment(`${CLAIM_PREFIX}${OTHER_CHECKOUT}`); + + const decision = await claimTestDatabase({ + connectionString: TEST_URL!, + checkout: REPO_ROOT, + takeover: true, + }); + + expect(decision).toEqual({ action: "stamp" }); + expect(await readComment()).toBe(`${CLAIM_PREFIX}${REPO_ROOT}`); + }); + + // The design decision, pinned. A claim stored in a ROW would be erased by the very statement + // every *.int.test.ts opens with, leaving the guard silently disabled after the first file. + it("survives the TRUNCATE it defends against", async () => { + await pool.query("TRUNCATE users RESTART IDENTITY CASCADE"); + expect(await readComment()).toBe(`${CLAIM_PREFIX}${REPO_ROOT}`); + }); + + // A quoted path is the realistic hostile input here: `COMMENT ON DATABASE` takes no bind + // parameters, so the statement is built by string-building somewhere. Postgres's own + // `format(%L)` is what makes that safe — a hand-rolled escape gets this wrong. + it("stamps a checkout path containing a single quote", async () => { + const awkward = normalizeCheckout("/tmp/sean's checkout"); + await writeComment(null); + + await claimTestDatabase({ connectionString: TEST_URL!, checkout: awkward }); + + expect(await readComment()).toBe(`${CLAIM_PREFIX}${awkward}`); + // And the database still answers — proof the quote did not terminate the statement early. + expect((await pool.query("select 1 as ok")).rows[0]).toEqual({ ok: 1 }); + }); +}); diff --git a/packages/db/src/test-db-guard.test.ts b/packages/db/src/test-db-guard.test.ts new file mode 100644 index 0000000..51dd934 --- /dev/null +++ b/packages/db/src/test-db-guard.test.ts @@ -0,0 +1,152 @@ +import { sep } from "node:path"; +import { describe, expect, it } from "vitest"; +import { + CLAIM_PREFIX, + ForeignTestDatabaseError, + assertTestDbIsNotArchive, + decideClaim, + describeTarget, + normalizeCheckout, + sameDatabaseTarget, +} from "./test-db-guard.js"; + +/** A claim comment exactly as `stampClaim` writes it: the prefix + the NORMALIZED checkout. */ +const claimFor = (checkout: string) => `${CLAIM_PREFIX}${normalizeCheckout(checkout)}`; + +const HERE = normalizeCheckout("/repo/420AI"); +const ELSEWHERE = normalizeCheckout("/repo/420AI-review"); + +describe("normalizeCheckout", () => { + // Property, not a recomputed literal — a `resolve`-vs-`resolve` assertion would pass for any + // implementation, including the identity function. + it("is idempotent", () => { + expect(normalizeCheckout(HERE)).toBe(HERE); + }); + + it("strips a trailing separator so the two spellings claim the same database", () => { + expect(normalizeCheckout(HERE + sep)).toBe(HERE); + }); + + it("keeps distinct paths distinct", () => { + expect(normalizeCheckout(HERE)).not.toBe(normalizeCheckout(ELSEWHERE)); + }); +}); + +describe("decideClaim", () => { + it("stamps an unclaimed database", () => { + expect(decideClaim(null, HERE, false)).toEqual({ action: "stamp" }); + expect(decideClaim(undefined, HERE, false)).toEqual({ action: "stamp" }); + expect(decideClaim("", HERE, false)).toEqual({ action: "stamp" }); + expect(decideClaim(" \n ", HERE, false)).toEqual({ action: "stamp" }); + }); + + it("proceeds when the claim is this checkout's", () => { + expect(decideClaim(claimFor(HERE), HERE, false)).toEqual({ action: "proceed" }); + }); + + // The discriminating case for the normalization: the STORED claim is normalized, the caller + // hands in a denormalized spelling of the same directory. A raw string comparison refuses here + // and locks the operator out of their own database. + it("proceeds when the caller's spelling is denormalized", () => { + expect(decideClaim(claimFor(HERE), HERE + sep, false)).toEqual({ action: "proceed" }); + expect(decideClaim(claimFor(HERE), `${HERE}${sep}.`, false)).toEqual({ action: "proceed" }); + }); + + // THE INCIDENT ITSELF: a second checkout pointed at the first checkout's test database. + it("refuses a database claimed by another checkout", () => { + expect(decideClaim(claimFor(ELSEWHERE), HERE, false)).toEqual({ + action: "refuse", + heldBy: ELSEWHERE, + reason: "other-checkout", + }); + }); + + it("refuses a database carrying an unrecognized comment", () => { + expect(decideClaim("someone else's scratch db", HERE, false)).toEqual({ + action: "refuse", + heldBy: "someone else's scratch db", + reason: "unrecognized-comment", + }); + }); + + it("re-stamps rather than refusing under takeover, whatever the existing claim", () => { + expect(decideClaim(claimFor(ELSEWHERE), HERE, true)).toEqual({ action: "stamp" }); + expect(decideClaim("someone else's scratch db", HERE, true)).toEqual({ action: "stamp" }); + expect(decideClaim(null, HERE, true)).toEqual({ action: "stamp" }); + }); + + it("tolerates whitespace around a stored claim", () => { + expect(decideClaim(` ${claimFor(HERE)} `, HERE, false)).toEqual({ action: "proceed" }); + }); +}); + +describe("describeTarget", () => { + it("renders host:port/database", () => { + expect(describeTarget("postgres://420ai:420ai@localhost:5433/420ai_test")).toBe( + "localhost:5433/420ai_test", + ); + }); + + // The provision-app-role.ts rule: a library never puts a credential in a message. This string + // is pasted straight into a fatal error the operator will copy into a bug report. + it("never leaks the password", () => { + const rendered = describeTarget("postgres://420ai:hunter2@localhost:5433/420ai_test"); + expect(rendered).not.toContain("hunter2"); + expect(rendered).not.toContain("420ai:"); + }); + + it("falls back to a neutral phrase for a keyword/value DSN", () => { + expect(describeTarget("host=localhost port=5433 dbname=420ai_test")).toBe( + "the configured test database", + ); + }); +}); + +describe("sameDatabaseTarget", () => { + const archive = "postgres://420ai:420ai@localhost:5433/420ai"; + + it("is true for the same host:port/database under different credentials", () => { + expect(sameDatabaseTarget(archive, "postgres://420ai_app:pw@localhost:5433/420ai")).toBe(true); + }); + + // The NORMAL, correct single-checkout setup: same cluster, different database. Treating a + // shared host:port as the hazard would refuse every healthy install. + it("is false for a different database on the same cluster", () => { + expect(sameDatabaseTarget(archive, "postgres://420ai:420ai@localhost:5433/420ai_test")).toBe( + false, + ); + }); + + it("is false for the same database name on a different cluster", () => { + expect(sameDatabaseTarget(archive, "postgres://420ai:420ai@localhost:5432/420ai")).toBe(false); + }); + + it("is false rather than throwing for an unparseable string", () => { + expect(sameDatabaseTarget(archive, "host=localhost dbname=420ai")).toBe(false); + }); +}); + +describe("assertTestDbIsNotArchive", () => { + const archive = "postgres://420ai:420ai@localhost:5433/420ai"; + const test = "postgres://420ai:420ai@localhost:5433/420ai_test"; + + it("passes for the documented layout", () => { + expect(() => assertTestDbIsNotArchive(archive, test)).not.toThrow(); + }); + + it("passes when DATABASE_URL is unset", () => { + expect(() => assertTestDbIsNotArchive(undefined, test)).not.toThrow(); + }); + + it("refuses when the test handle names the live archive", () => { + expect(() => assertTestDbIsNotArchive(archive, archive)).toThrow(ForeignTestDatabaseError); + expect(() => assertTestDbIsNotArchive(archive, archive)).toThrow(/SAME database/); + }); + + it("keeps the password out of the refusal", () => { + const withSecret = "postgres://420ai:hunter2@localhost:5433/420ai"; + expect(() => assertTestDbIsNotArchive(withSecret, withSecret)).toThrow( + expect.objectContaining({ message: expect.not.stringContaining("hunter2") }), + ); + }); +}); diff --git a/packages/db/src/test-db-guard.ts b/packages/db/src/test-db-guard.ts new file mode 100644 index 0000000..63ab479 --- /dev/null +++ b/packages/db/src/test-db-guard.ts @@ -0,0 +1,229 @@ +import { resolve } from "node:path"; +import { Pool } from "pg"; + +/** + * INC-2026-10 — refuse to run the integration suite against a test database this checkout + * does not own. + * + * THE HAZARD. `npm run setup` copies `.env.example`'s literal defaults, so every generated + * `.env` aims all four database handles at `localhost:5433`. On a machine that already runs + * 420AI that is the REAL archive cluster, and it owns a real `420ai_test`. 61 `*.int.test.ts` + * files gate only on `!DATABASE_URL_TEST` and open with + * `TRUNCATE … RESTART IDENTITY CASCADE`, so a single `npx vitest run` from a second checkout — + * a clean room, a staging tree, a worktree opened to review a PR — silently destroys the first + * checkout's test data. Nothing warns, because from the tooling's point of view the config is + * correct: 5433 *is* the documented port. + * + * WHY THE GUARD IS HERE AND NOT IN `scripts/setup-env.mjs`. A config-step check is bypassed by + * hand-editing `.env`, which is the normal way people repoint these handles. This module is + * called from `vitest.global-setup.ts`, which runs once before ANY test file and before + * `runMigrations` — so a refusal aborts the run with zero `TRUNCATE`s executed, and it evaluates + * whatever the handles actually resolve to however they got there. `setup-env.mjs` also cannot + * help the scenario the incident describes: it refuses to run when a `.env` exists, so on the + * fresh second checkout there is no prior file to derive a safer default from. + * + * WHY A DATABASE COMMENT AND NOT A MARKER TABLE. The claim has to survive exactly the operation + * it defends against. A row in a table does not — `TRUNCATE … CASCADE` is the first thing every + * int test does. A `COMMENT ON DATABASE` is database-level metadata: it survives TRUNCATE, DROP + * TABLE and the whole migration set, needs no schema change (so it does not perturb the 33-table + * count or any TRUNCATE list), and costs nothing beyond the connection the bootstrap already + * opens. + * + * WHAT IT GUARANTEES, EXACTLY: **at most one checkout can ever TRUNCATE a given test database.** + * The first checkout to run stamps its claim; every other checkout is refused, loudly, before + * any destructive statement. Note the corollary — if the SECOND checkout runs first, it takes + * the claim and the first is the one refused later. That is still the invariant, because the + * hazard is two checkouts sharing one test database, not which of them arrived first; either + * way the collision is reported instead of silently destroying data. + */ + +/** Prefix that marks a database comment as ours, so an unrelated comment is never mistaken for one. */ +export const CLAIM_PREFIX = "420ai-test-claim:"; + +/** Environment variable that re-claims a database held by another checkout. Deliberately verbose. */ +export const TAKEOVER_ENV = "TEST_DB_CLAIM_TAKEOVER"; + +/** Thrown when the configured test database belongs to someone else. Fatal by design. */ +export class ForeignTestDatabaseError extends Error { + override readonly name = "ForeignTestDatabaseError"; +} + +export type ClaimDecision = + | { action: "proceed" } + | { action: "stamp" } + | { action: "refuse"; heldBy: string; reason: "other-checkout" | "unrecognized-comment" }; + +/** + * Normalize a checkout path into the identity stamped in the claim. `resolve` only — no + * lower-casing, because case matters on POSIX. Two spellings of one directory (a Windows path + * and its `/mnt/c` WSL twin, or two casings on Windows) therefore read as different checkouts, + * which errs toward REFUSING: the safe direction, and `TEST_DB_CLAIM_TAKEOVER` resolves it. + */ +export function normalizeCheckout(checkoutPath: string): string { + return resolve(checkoutPath); +} + +/** + * Pure decision: given the database's existing comment, who we are, and whether takeover was + * requested, decide whether to proceed, stamp a fresh claim, or refuse. Separated from the I/O + * below so the value oracle is testable with no database. + */ +export function decideClaim( + existingComment: string | null | undefined, + checkout: string, + takeover: boolean, +): ClaimDecision { + if (takeover) return { action: "stamp" }; + + const comment = existingComment?.trim() ?? ""; + if (comment === "") return { action: "stamp" }; + + if (!comment.startsWith(CLAIM_PREFIX)) { + // A database somebody bothered to describe is more likely theirs than free. Refusing an + // unrecognized comment is the safe reading, and the message quotes it so the operator can + // judge in one glance. + return { action: "refuse", heldBy: comment, reason: "unrecognized-comment" }; + } + + const heldBy = comment.slice(CLAIM_PREFIX.length).trim(); + if (heldBy === normalizeCheckout(checkout)) return { action: "proceed" }; + return { action: "refuse", heldBy, reason: "other-checkout" }; +} + +/** + * Render `host:port/database` from a connection string for an error message, NEVER the + * credentials (the `provision-app-role.ts` rule: a library must not put a password in a + * message). Falls back to a neutral phrase for a keyword/value DSN, which `new URL` cannot parse. + */ +export function describeTarget(connectionString: string): string { + try { + const url = new URL(connectionString); + const database = decodeURIComponent(url.pathname.replace(/^\//, "")); + return `${url.host}/${database || "(default database)"}`; + } catch { + return "the configured test database"; + } +} + +/** + * Pure: do two connection strings name the SAME host:port:database? Used for the catastrophic + * case — `DATABASE_URL_TEST` accidentally pointing at the live archive, where the first + * `beforeEach` truncates production rather than test data. Unparseable strings compare `false` + * rather than throwing: this check is a bonus safety net, and the claim guard below is the one + * that must run. + */ +export function sameDatabaseTarget(a: string, b: string): boolean { + try { + const [ua, ub] = [new URL(a), new URL(b)]; + return ua.host === ub.host && ua.pathname === ub.pathname; + } catch { + return false; + } +} + +/** + * Refuse when the test handle and the archive handle name the same database. Pure + synchronous; + * no connection needed. A no-op when either is unset. + */ +export function assertTestDbIsNotArchive(archiveUrl: string | undefined, testUrl: string): void { + if (!archiveUrl) return; + if (!sameDatabaseTarget(archiveUrl, testUrl)) return; + throw new ForeignTestDatabaseError( + `Refusing to run integration tests: DATABASE_URL_TEST names the SAME database as ` + + `DATABASE_URL (${describeTarget(testUrl)}).\n\n` + + `Every *.int.test.ts opens with TRUNCATE … RESTART IDENTITY CASCADE, so this run would ` + + `destroy the live archive rather than test data.\n\n` + + `Point DATABASE_URL_TEST at a separate database (the compose stack provisions "420ai_test" ` + + `beside "420ai" — see docker/init-test-db.sql).`, + ); +} + +/** + * Read the database's claim, then proceed / stamp / refuse. Throws `ForeignTestDatabaseError` + * when the database belongs to another checkout — the caller (the vitest global setup) lets that + * abort the run. + * + * Silent library per the CLAUDE.md process boundary: it throws and returns, and never logs. + */ +export async function claimTestDatabase(options: { + connectionString: string; + checkout: string; + takeover?: boolean; +}): Promise { + const { connectionString, checkout, takeover = false } = options; + const self = normalizeCheckout(checkout); + + const pool = new Pool({ connectionString }); + try { + const { rows } = await pool.query<{ comment: string | null }>( + `select shobj_description(oid, 'pg_database') as comment + from pg_database + where datname = current_database()`, + ); + const decision = decideClaim(rows[0]?.comment, self, takeover); + + if (decision.action === "refuse") { + throw new ForeignTestDatabaseError(refusalMessage(decision, connectionString, self)); + } + if (decision.action === "stamp") { + await stampClaim(pool, self); + } + return decision; + } finally { + await pool.end(); + } +} + +/** + * Write the claim. `COMMENT ON DATABASE` is a UTILITY statement and takes no bind parameters — + * the identical trap `provision-app-role.ts` documents for `ALTER ROLE … PASSWORD $1`, which + * fails with SQLSTATE 42601. So Postgres does the quoting: `format(… %I …, %L)` is + * `quote_ident`/`quote_literal`, correct for every edge case a hand-rolled escape gets wrong — + * and the value here is an operator-supplied filesystem path, which really can contain a quote. + */ +async function stampClaim(pool: Pool, self: string): Promise { + const [built] = ( + await pool.query<{ stmt: string }>( + `select format('COMMENT ON DATABASE %I IS %L', current_database(), $1::text) as stmt`, + [`${CLAIM_PREFIX}${self}`], + ) + ).rows; + try { + await pool.query(built!.stmt); + } catch (e) { + // A guard that silently disables itself is the failure mode CLAUDE.md warns about + // ("a backstop that cannot be LOUD is not a substitute for a complete gate"), so a + // write failure is fatal rather than a warning — but it names the remedy. + throw new ForeignTestDatabaseError( + `Could not stamp the INC-2026-10 ownership claim on the test database: ` + + `${e instanceof Error ? e.message : String(e)}\n\n` + + `COMMENT ON DATABASE requires the connecting role to OWN the database. Both documented ` + + `setups satisfy this (docker/init-test-db.sql and the CI "Create test database" step ` + + `both create it as "420ai"). Recreate the test database as the owner role, or set ` + + `${TAKEOVER_ENV}=1 only if you accept that this run may truncate another checkout's data.`, + ); + } +} + +function refusalMessage( + decision: Extract, + connectionString: string, + self: string, +): string { + const held = + decision.reason === "other-checkout" + ? ` claimed by: ${decision.heldBy}\n this run: ${self}` + : ` existing comment: ${decision.heldBy}\n this run: ${self}`; + + return ( + `Refusing to run integration tests: ${describeTarget(connectionString)} is ` + + `${decision.reason === "other-checkout" ? "claimed by a DIFFERENT checkout" : "carrying an unrecognized comment"}.\n\n` + + `${held}\n\n` + + `Every *.int.test.ts opens with TRUNCATE … RESTART IDENTITY CASCADE, so continuing would ` + + `destroy data this checkout does not own (INC-2026-10).\n\n` + + `Fix by giving this checkout its own cluster — change the published port in ` + + `docker-compose.yml and the four handles in .env — or, if this database really is yours, ` + + `re-claim it:\n` + + ` ${TAKEOVER_ENV}=1 npx vitest run` + ); +} diff --git a/vitest.global-setup.ts b/vitest.global-setup.ts index 9ff878a..b96f1ca 100644 --- a/vitest.global-setup.ts +++ b/vitest.global-setup.ts @@ -1,14 +1,36 @@ -import { runMigrations } from "@420ai/db"; +import { fileURLToPath } from "node:url"; +import { + runMigrations, + assertTestDbIsNotArchive, + claimTestDatabase, + TAKEOVER_ENV, +} from "@420ai/db"; /** * Vitest global setup: migrate the test database once before the suite so * *.int.test.ts hit a ready schema. If DATABASE_URL_TEST is unset (no Docker), * this is a no-op and the integration suites self-skip. Migrations are * idempotent, so re-runs across sessions are safe. + * + * INC-2026-10: two ownership guards run FIRST, before `runMigrations` and therefore before any + * test file's `TRUNCATE … RESTART IDENTITY CASCADE`. This is the only choke point that sees every + * vitest invocation (`npm test`, `repo-health`, a single `npx vitest run `) and evaluates + * the handles as they actually resolve — a check in `scripts/setup-env.mjs` is bypassed by + * hand-editing `.env`, which is the normal way people repoint them. Refusing here aborts the run + * with zero destructive statements executed. See `packages/db/src/test-db-guard.ts`. */ export default async function setup(): Promise<() => void> { const url = process.env.DATABASE_URL_TEST; if (url) { + // Catastrophic case first, and it needs no connection: the test handle naming the live + // archive would truncate production in the first `beforeEach`. + assertTestDbIsNotArchive(process.env.DATABASE_URL, url); + // Then the cross-checkout claim — at most one checkout may ever truncate a given test DB. + await claimTestDatabase({ + connectionString: url, + checkout: fileURLToPath(new URL(".", import.meta.url)), + takeover: process.env[TAKEOVER_ENV] === "1", + }); await runMigrations(url); } return () => {};