Repository navigation
fix(m17): close INC-2026-10 — claim the test database before anything truncates it - #86
Merged
Merged
Conversation
… truncates it `npm run setup` copies `.env.example`'s literal defaults, so every generated `.env` aims all four handles at localhost:5433. On a machine already running 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 one `npx vitest run` from a second checkout silently destroys the first one's data. Takes option (b) from the incident's disposition: a guard at the BOOTSTRAP, not the config step. `vitest.global-setup.ts` runs it before `runMigrations` and therefore before any `beforeEach`, so a refusal aborts with zero TRUNCATEs executed. It is the only choke point that sees every vitest invocation and evaluates the handles as they actually resolve — which is what makes it un-bypassable by hand-editing `.env`, the property a `setup-env.mjs` check would have lacked. Two checks. `assertTestDbIsNotArchive` is pure and needs no connection: it refuses when the test handle names the same host:port:database as `DATABASE_URL`, the case where the first `beforeEach` truncates production. Then an ownership claim stamped on the database itself as a `COMMENT ON DATABASE`, read back via `shobj_description`. First checkout to run stamps its path; every other is refused BY NAME, with `TEST_DB_CLAIM_TAKEOVER=1` as the escape hatch. A comment and not a marker table because the claim must survive the exact operation it defends against, and a row does not — TRUNCATE is the first thing every int test runs. It also needs no schema change, so it perturbs neither the 33-table count this incident's own evidence used nor any TRUNCATE list. Pinned directly by an int test. `COMMENT ON DATABASE` is a utility statement and takes no bind parameters — the same SQLSTATE 42601 trap `provision-app-role.ts` documents for `ALTER ROLE … PASSWORD $1`. Postgres does the quoting via `format(%I, %L)`, which matters because the value is an operator-supplied filesystem path that can contain a quote (an int test uses one). Guarantee, stated exactly: at most one checkout can ever TRUNCATE a given test database. Corollary recorded rather than hidden — if the second checkout runs first it takes the claim and the first is refused later; the collision is still reported. Verification. 29 new tests: 21 pure (the decision oracle) + 8 against real Postgres (claim round-trip, TRUNCATE-survival, quoted path). Negative-controlled twice. Neutering `decideClaim` to always proceed reddens 9 of the 29. And behaviourally, with a canary row in `organizations` and a foreign claim stamped to simulate a second checkout: guard ON, the run aborted in global setup and the canary survived; guard calls stripped, the identical command reported `Test Files 1 passed`, `Tests 6 passed`, exit 0 — and the canary was gone. Green, silent, destructive: the incident reproduced. Tier: hardware-verified on windows-x86_64 against the real compose Postgres 17; 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). Deliberately does NOT change `.env.example` or `setup-env.mjs`: localhost:5433 is correct for the common single-checkout case, and a config-step guess cannot know whether the cluster already there is yours. The claim answers that with evidence. repo-health --require-db: PASS — 1790 passed / 175 files, 688 integration, 0 skipped. lint + format:check exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRL4rKQNVVQPSS6aNy1NrX
There was a problem hiding this comment.
Pull request overview
Adds a safety guard to the integration-test bootstrap so concurrent/local vitest runs cannot silently TRUNCATE a shared or production-adjacent Postgres database. This is implemented as a pure “same DB as archive” check plus a durable “ownership claim” stored in COMMENT ON DATABASE, executed in vitest.global-setup.ts before migrations and before any test file beforeEach.
Changes:
- Run two pre-flight guards in
vitest.global-setup.tsbeforerunMigrations: refuse “test DB == archive DB”, then claim/refuse the test database via a database comment. - Introduce
packages/db/src/test-db-guard.tswith claim logic + explicit error types, and export it from the@420ai/dbbarrel for repo-root consumption. - Add unit + integration tests plus operator-facing troubleshooting/docs updates for the new failure modes and takeover escape hatch.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vitest.global-setup.ts | Adds pre-migration guards to refuse/archive-collision and claim the test DB before any TRUNCATE can run. |
| packages/db/src/test-db-guard.ts | New guard module implementing “same target” detection and the DB ownership claim via COMMENT ON DATABASE. |
| packages/db/src/index.ts | Exposes the new guard APIs/constants through @420ai/db for use by the repo-root vitest setup. |
| packages/db/src/test-db-guard.test.ts | Unit tests for normalization, claim decision logic, and URL/target rendering behavior. |
| packages/db/src/test-db-guard.int.test.ts | Integration tests proving COMMENT ON DATABASE round-trips and survives TRUNCATE; validates refusal/takeover behavior. |
| docs/guide/troubleshooting.md | Documents the new refusal messages and operator remediation/takeover steps. |
| SUMMARY.md | Updates incident write-up to reflect closure via the new bootstrap ownership claim. |
| .agents/research/incidents.md | Marks INC-2026-10 resolved and documents the implemented approach + verification. |
| .agents/plans/m17-cross-platform-collectors.md | Updates plan text to note INC-2026-10 closure via the bootstrap claim. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+156
to
+170
| 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); | ||
| } |
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
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.
npm run setupcopies.env.example's literal defaults, so every generated.envaims all four handles at localhost:5433. On a machine already running 420AI that is the real archive cluster, and it owns a real420ai_test. 61*.int.test.tsfiles gate only on!DATABASE_URL_TESTand open withTRUNCATE … RESTART IDENTITY CASCADE, so onenpx vitest runfrom a second checkout silently destroys the first one's data.Takes option (b) from the incident's disposition: a guard at the BOOTSTRAP, not the config step.
vitest.global-setup.tsruns it beforerunMigrationsand therefore before anybeforeEach, so a refusal aborts with zero TRUNCATEs executed. It is the only choke point that sees every vitest invocation and evaluates the handles as they actually resolve — which is what makes it un-bypassable by hand-editing.env, the property asetup-env.mjscheck would have lacked.Two checks.
assertTestDbIsNotArchiveis pure and needs no connection: it refuses when the test handle names the same host:port:database asDATABASE_URL, the case where the firstbeforeEachtruncates production. Then an ownership claim stamped on the database itself as aCOMMENT ON DATABASE, read back viashobj_description. First checkout to run stamps its path; every other is refused BY NAME, withTEST_DB_CLAIM_TAKEOVER=1as the escape hatch.A comment and not a marker table because the claim must survive the exact operation it defends against, and a row does not — TRUNCATE is the first thing every int test runs. It also needs no schema change, so it perturbs neither the 33-table count this incident's own evidence used nor any TRUNCATE list. Pinned directly by an int test.
COMMENT ON DATABASEis a utility statement and takes no bind parameters — the same SQLSTATE 42601 trapprovision-app-role.tsdocuments forALTER ROLE … PASSWORD $1. Postgres does the quoting viaformat(%I, %L), which matters because the value is an operator-supplied filesystem path that can contain a quote (an int test uses one).Guarantee, stated exactly: at most one checkout can ever TRUNCATE a given test database. Corollary recorded rather than hidden — if the second checkout runs first it takes the claim and the first is refused later; the collision is still reported.
Verification. 29 new tests: 21 pure (the decision oracle) + 8 against real Postgres (claim round-trip, TRUNCATE-survival, quoted path). Negative-controlled twice. Neutering
decideClaimto always proceed reddens 9 of the 29. And behaviourally, with a canary row inorganizationsand a foreign claim stamped to simulate a second checkout: guard ON, the run aborted in global setup and the canary survived; guard calls stripped, the identical command reportedTest Files 1 passed,Tests 6 passed, exit 0 — and the canary was gone. Green, silent, destructive: the incident reproduced.Tier: hardware-verified on windows-x86_64 against the real compose Postgres 17; the ubuntu-latest half is CI-verified (repo-health.yml creates a fresh
420ai_testper run, so CI exercises the unclaimed → stamp path every time).Deliberately does NOT change
.env.exampleorsetup-env.mjs: localhost:5433 is correct for the common single-checkout case, and a config-step guess cannot know whether the cluster already there is yours. The claim answers that with evidence.repo-health --require-db: PASS — 1790 passed / 175 files, 688 integration, 0 skipped. lint + format:check exit 0.
Claude-Session: https://claude.ai/code/session_01PRL4rKQNVVQPSS6aNy1NrX