Repair the drizzle snapshot so generate emits no spurious migration (#12333)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Paperclip keeps its state in PostgreSQL, and `packages/db` owns that schema through Drizzle > - `drizzle-kit generate` writes a new migration by diffing `packages/db/src/schema/` against the newest snapshot in `packages/db/src/migrations/meta/`, so the snapshot must describe the schema that the migrations produce > - Snapshot `0228` recorded the new `error_count` column on the wrong table, and snapshot `0229` inherited the error, so the newest snapshot no longer matched the schema > - Because of that, `generate` on `master` folded the drift into any new migration: it emitted an `ADD COLUMN` for a column that migration `0228` already creates, which fails on a fresh database, plus an out-of-scope `DROP COLUMN` > - This pull request moves the column entry to the correct table in both snapshots and adds a test that repeats the diff `generate` performs > - The benefit is that the next person who generates a migration gets only their own change, and CI fails if the snapshot drifts again ## Linked Issues or Issue Description No existing issue. The description below follows `.github/ISSUE_TEMPLATE/bug_report.yml`. **What happened?** `drizzle-kit generate` on `master` emits a wrong migration. The newest snapshot, `packages/db/src/migrations/meta/0229_snapshot.json`, disagrees with the schema in two places. It omits `issue_question_response_deliveries.error_count`, which `0228_nasty_grim_reaper.sql` creates. It also carries `decision_archive_notification_outbox.error_count`, which no migration ever creates and the Drizzle schema never declared. Snapshot `0228` introduced both halves of the error: it added the new `error_count` column to `decision_archive_notification_outbox` instead of the table that the same migration creates. Snapshot `0229` copied it forward. Any new migration therefore starts with two statements that do not belong to it: ```sql ALTER TABLE "issue_question_response_deliveries" ADD COLUMN "error_count" integer DEFAULT 0 NOT NULL; ALTER TABLE "decision_archive_notification_outbox" DROP COLUMN "error_count"; ``` The `ADD COLUMN` fails on a fresh database, because migration `0228` already creates that column. The `DROP COLUMN` targets a column that does not exist on any deployment. **Expected behavior** `drizzle-kit generate` reports "No schema changes, nothing to migrate" on a clean checkout of `master`, and a new migration contains only the author's own schema change. **Steps to reproduce** 1. Check out `master` at commit `bc1a21564`. 2. Run `pnpm install`. 3. Run `pnpm --filter @paperclipai/db generate`. 4. Read the emitted `packages/db/src/migrations/0230_*.sql`. It contains the two statements above, and no schema file was changed. **Paperclip version or commit** `master` at `bc1a21564`. The drift entered in #12307 (snapshot `0228`) and was carried forward by #12291 (snapshot `0229`), which worked around it by building its snapshot by hand. **Deployment mode** Not deployment specific. It affects anyone who generates a migration, and it affects any fresh database that would later run the bad migration. **Database mode** All PostgreSQL modes: embedded, local Docker, and hosted. ## What Changed - Moved the `error_count` column entry from `decision_archive_notification_outbox` to `issue_question_response_deliveries` in `packages/db/src/migrations/meta/0228_snapshot.json` and `packages/db/src/migrations/meta/0229_snapshot.json`. Both files keep their `id` and `prevId`, so the snapshot chain is unchanged. - Added `packages/db/src/migration-snapshot-drift.test.ts`. It reads the newest snapshot named by `_journal.json`, serializes the schema modules with `generateDrizzleJson`, and asserts that `generateMigration` returns no statements. This is the same diff that `generate` performs. - Documented the snapshot rule in `doc/DATABASE.md` under a new "Migration snapshots" section. No migration SQL was added, renumbered, or edited. No schema file changed. The database is correct as it is; only the snapshot was wrong. Why both snapshots and not only the newest one: `0229` is the file that `generate` reads, so repairing it is what fixes the bug. `0228` holds the same error, and `drizzle-kit drop` removes the last migration and its snapshot, which would promote `0228` back to newest and bring the drift back. Repairing both removes that trap. Snapshots are never applied to a database, so neither edit changes any deployment. ## Verification Commands run from the repository root. - `pnpm --filter @paperclipai/db generate` — "No schema changes, nothing to migrate 😴". It writes no SQL file, no snapshot, and no journal entry. `git status` stays clean. Before the fix, the same command wrote `0230_fast_caretaker.sql` with the two spurious statements. - The repaired `0229_snapshot.json` is byte-identical to the snapshot that a real `generate` run produced, except for the `id` and `prevId` that keep the chain intact. - Chain check: the repaired `0228` and `0229` snapshots now differ by exactly the two columns that `0229_drop_company_brand_color_and_attachment_max_bytes.sql` drops, `companies.brand_color` and `companies.attachment_max_bytes`, and by nothing else. - Database check: applied all 229 migrations in order to an embedded PostgreSQL, then compared the live schema with the repaired snapshot. 179 tables and 2687 columns match, with no missing column, no extra column, and no nullability difference. The same comparison against the pre-fix snapshot reports exactly two problems: `column only in snapshot: decision_archive_notification_outbox.error_count` and `column only in database: issue_question_response_deliveries.error_count`. This harness was a scratch script and is not part of the pull request. - `pnpm --filter @paperclipai/db typecheck` — pass. It runs `check:migrations`, which is `check-migration-numbering` and `check-migration-safety`. - `npx vitest run --root packages/db` — 28 files, 102 tests, all pass. This includes the new test. - New test, negative case: with the pre-fix `0229_snapshot.json` restored, `migration-snapshot-drift.test.ts` fails and prints exactly the two spurious statements, plus the instruction to run `generate`. It passes on the repaired snapshot. It takes about 1.2 seconds and needs no database. - `node scripts/check-forbidden-tokens.mjs` and `node scripts/check-no-git-push.mjs` — pass. ## Risks Low risk. A Drizzle snapshot is a build-time record for `drizzle-kit generate`. It is never applied to a database, so this change cannot alter any deployment, and no operator action is needed. Databases that already ran migrations `0228` and `0229` are correct today and stay correct. The proof is a clean `generate`: the command that produced the wrong migration now reports "No schema changes, nothing to migrate" and writes nothing. Two smaller notes: - The new test depends on `drizzle-kit/api`, which is already a dev dependency of `packages/db`. If a future `drizzle-kit` upgrade changes that surface, the test fails loudly at import rather than passing silently. - The test imports every module in `packages/db/src/schema/`, which is the same set that `drizzle.config.ts` points the CLI at. It deduplicates by object identity, because the barrel re-exports the same table objects and `drizzle-kit` rejects a table it sees twice. ## Model Used Claude (Anthropic), Claude Opus, agentic tool use via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
Devin Foley
GitHub
parent
bc1a21564f
commit
bdd8f1bedb
|
|
@ -167,6 +167,14 @@ When authoring migrations or one-time backfills:
|
|||
- Split schema changes, index creation, and data backfill into separate phases so each step has clear locking and rollback behavior.
|
||||
- Treat the `check:migrations` CI gate as the enforcement backstop for these rules. If it flags a migration, rewrite the migration or add a suppression comment with the indexed predicate, batch bound, and reason the remaining scan is safe.
|
||||
|
||||
## Migration snapshots
|
||||
|
||||
`drizzle-kit generate` diffs `packages/db/src/schema/` against the newest snapshot in `packages/db/src/migrations/meta/`. That snapshot must describe the schema that every migration produces when they run in order. A snapshot that drifts from the schema makes the *next* migration wrong, because `generate` folds the drift into it. The drift can add a column that an earlier migration already created, which makes that migration fail on a fresh database. It can also drop a column that the schema still uses.
|
||||
|
||||
- Create every migration with `pnpm --filter @paperclipai/db generate`. Do not hand-write a snapshot.
|
||||
- Do not hand-edit a snapshot to resolve a merge conflict. Renumber your migration and run `generate` again, as `packages/db/.gitattributes` describes.
|
||||
- `packages/db/src/migration-snapshot-drift.test.ts` is the enforcement backstop. It repeats the diff that `generate` performs and fails when the newest snapshot no longer matches `packages/db/src/schema/`.
|
||||
|
||||
## Resource membership tables
|
||||
|
||||
Paperclip stores current-user sidebar membership state in:
|
||||
|
|
|
|||
|
|
@ -0,0 +1,74 @@
|
|||
import { readdir, readFile } from "node:fs/promises";
|
||||
import { fileURLToPath, pathToFileURL } from "node:url";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { generateDrizzleJson, generateMigration } from "drizzle-kit/api";
|
||||
|
||||
// The newest snapshot in `src/migrations/meta` is the state `drizzle-kit
|
||||
// generate` diffs the schema against. When it drifts from the schema, the next
|
||||
// generated migration silently carries the drift: it re-adds a column an
|
||||
// earlier migration already created (which fails on a fresh database) and drops
|
||||
// a column the schema never had. This test reproduces the diff `generate`
|
||||
// performs — schema modules versus newest snapshot — and fails when it is not
|
||||
// empty, so drift is caught in CI instead of inside someone else's migration.
|
||||
|
||||
const migrationsDir = fileURLToPath(new URL("./migrations", import.meta.url));
|
||||
const schemaDir = fileURLToPath(new URL("./schema", import.meta.url));
|
||||
|
||||
type JournalEntry = { idx: number; tag: string };
|
||||
|
||||
async function readNewestSnapshot(): Promise<{ file: string; snapshot: Record<string, unknown> }> {
|
||||
const journal = JSON.parse(
|
||||
await readFile(path.join(migrationsDir, "meta", "_journal.json"), "utf8"),
|
||||
) as { entries: JournalEntry[] };
|
||||
const newest = journal.entries.at(-1);
|
||||
if (!newest) throw new Error("migration journal has no entries");
|
||||
const file = `${String(newest.idx).padStart(4, "0")}_snapshot.json`;
|
||||
const snapshot = JSON.parse(await readFile(path.join(migrationsDir, "meta", file), "utf8")) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
return { file, snapshot };
|
||||
}
|
||||
|
||||
// drizzle.config.ts points drizzle-kit at every module in the schema directory,
|
||||
// so the test imports the same set rather than the hand-maintained barrel — a
|
||||
// table missing from the barrel must not hide from this check.
|
||||
async function importSchemaModules(): Promise<Record<string, unknown>> {
|
||||
const files = (await readdir(schemaDir)).filter((file) => file.endsWith(".ts")).sort();
|
||||
const exports: Record<string, unknown> = {};
|
||||
// The barrel re-exports the same table objects the per-table modules export,
|
||||
// so dedupe by identity: serializing one table twice trips drizzle-kit's
|
||||
// duplicate-index guard.
|
||||
const seen = new Set<unknown>();
|
||||
for (const file of files) {
|
||||
const module = (await import(pathToFileURL(path.join(schemaDir, file)).href)) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
for (const [name, value] of Object.entries(module)) {
|
||||
if (typeof value === "object" && value !== null) {
|
||||
if (seen.has(value)) continue;
|
||||
seen.add(value);
|
||||
}
|
||||
exports[`${file}#${name}`] = value;
|
||||
}
|
||||
}
|
||||
return exports;
|
||||
}
|
||||
|
||||
describe("migration snapshot drift", () => {
|
||||
it("keeps the newest snapshot in sync with the drizzle schema", async () => {
|
||||
const { file, snapshot } = await readNewestSnapshot();
|
||||
const current = generateDrizzleJson(await importSchemaModules(), snapshot.id as string);
|
||||
const statements = await generateMigration(
|
||||
snapshot as Parameters<typeof generateMigration>[0],
|
||||
current as Parameters<typeof generateMigration>[1],
|
||||
);
|
||||
|
||||
expect(
|
||||
statements,
|
||||
`${file} no longer matches src/schema. Run \`pnpm --filter @paperclipai/db generate\` and commit the migration it emits; do not hand-edit the snapshot.`,
|
||||
).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -9471,13 +9471,6 @@
|
|||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"error_count": {
|
||||
"name": "error_count",
|
||||
"type": "integer",
|
||||
"primaryKey": false,
|
||||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"last_attempt_at": {
|
||||
"name": "last_attempt_at",
|
||||
"type": "timestamp with time zone",
|
||||
|
|
@ -21193,6 +21186,13 @@
|
|||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"error_count": {
|
||||
"name": "error_count",
|
||||
"type": "integer",
|
||||
"primaryKey": false,
|
||||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"last_attempt_at": {
|
||||
"name": "last_attempt_at",
|
||||
"type": "timestamp with time zone",
|
||||
|
|
|
|||
|
|
@ -9458,13 +9458,6 @@
|
|||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"error_count": {
|
||||
"name": "error_count",
|
||||
"type": "integer",
|
||||
"primaryKey": false,
|
||||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"last_attempt_at": {
|
||||
"name": "last_attempt_at",
|
||||
"type": "timestamp with time zone",
|
||||
|
|
@ -21180,6 +21173,13 @@
|
|||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"error_count": {
|
||||
"name": "error_count",
|
||||
"type": "integer",
|
||||
"primaryKey": false,
|
||||
"notNull": true,
|
||||
"default": 0
|
||||
},
|
||||
"last_attempt_at": {
|
||||
"name": "last_attempt_at",
|
||||
"type": "timestamp with time zone",
|
||||
|
|
|
|||
Loading…
Reference in New Issue