mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 07:12:00 +01:00
feat(db): classify migrations for application under the running release (#20913)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - `packages/db` owns the schema and its migrations. There are 467
migrations, and several ship each day
> - A zero-downtime deploy starts the new release while the previous
release still serves the same database
> - Core does not say whether a migration is safe to apply under the
previous release. A deployer must treat each pending migration as a
reason to pause, or a person must review it
> - On stacks that may not pause, releases wait for hours
> - Core owns the schema, so Core must own this knowledge and publish it
as data
> - This pull request adds a conservative classifier, an in-file
override, the published result, and a guard for new migrations
> - The benefit is that a deployer can apply routine compatible
migrations automatically, and incompatible changes become visible at
review time
## Linked Issues or Issue Description
**Subsystem affected**
Database migrations (`packages/db`), the cloud migrator artifact
(`scripts/cloud-migrator-artifacts.mjs`), and the pull-request policy
job.
**Problem or motivation**
Core has no machine-readable statement that a migration can be applied
while the previous release still reads and writes the database. A
deployer cannot tell an added nullable column from a dropped column. It
must pause for both, or refuse the release.
**Proposed solution**
Classify each migration as `online_compatible`, `requires_staging`, or
`unknown`. Publish the result in the migrator manifest. Fail a pull
request when a migration it adds is `unknown` and has no reviewed
override in the file.
**Alternatives considered**
- A hand-kept list in the deployer. It goes stale and it is not reviewed
with the SQL.
- A sibling JSON file in the artifact. It needs a workflow change, and
the attestation of `manifest.json` would not cover it.
**Roadmap alignment**
This is release infrastructure. It adds no product feature.
## What Changed
- `packages/db/src/migration-compatibility.ts`:
`classifyMigrationSql(sql)` returns `{ compatibility, classified,
reasons, override, overrideError }`. It parses statement by statement
(breakpoints, `;`, comments, string and dollar quoting, guarded `DO`
blocks). A statement that it does not recognize is `unknown`. A
migration takes the worst class of its statements.
- Lock and duration hazards have their own reason codes
(`lock_hazard_*`) and classify as `unknown`, not as compatible with a
warning.
- Override: `-- paperclip:migration-compat online_compatible:
<justification>` (or `requires_staging`) in the leading comment block of
the migration file. The justification is mandatory (10 characters or
more). A malformed, repeated, or misplaced directive is an error and
never improves the verdict.
- A table counts as new after a plain `CREATE TABLE`, or after `CREATE
TABLE IF NOT EXISTS` when no earlier migration created that table.
`classifyMigrationSequence` classifies migrations in apply order with
that knowledge.
- `packages/db/scripts/migration-compatibility.ts` (run with `pnpm exec
tsx` in `packages/db`): classify files, print the distribution, print
JSON, or run the guard (`--check`). No new package script.
- Published data: `manifest.json` of the cloud migrator artifact has a
new additive field `migrationCompatibility`. `version` stays `1`. No
existing field changes. The build reads the SQL from the packed
`db.tgz`, so the data describes what the migrator applies. `validate`
(publisher) and `verify` reject data that is malformed or does not match
the packed migrations. The workflow file does not change.
- Guard for new migrations:
`.github/scripts/check-pr-migration-order.mjs` (the existing "Validate
migration ordering against target branch" policy step) now also
classifies the migrations that the pull request adds. `check:migrations`
runs the same guard against the merge base with `origin/master`, so the
`@paperclipai/db` typecheck and build report it locally. `unknown`
without an override fails. `requires_staging` prints a notice and
passes. A new migration that uses `CONCURRENTLY` also fails when it is
not in `MIGRATIONS_WITH_TOP_LEVEL_STATEMENTS`
(`packages/db/src/client.ts`), because the migrator would run it inside
a transaction.
- `doc/DATABASE.md`: new section "Migration compatibility with the
running release" under "Schema change workflow".
Published shape:
```json
"migrationCompatibility": {
"version": 1,
"migrations": [
{
"idx": 468,
"tag": "0468_busy_wendell_rand",
"sqlSha256": "56a6e272…",
"compatibility": "online_compatible",
"classified": "online_compatible",
"override": false,
"reasons": [
{ "code": "add_column", "compatibility": "online_compatible", "message": "Adds a column that is nullable or has a constant default.", "statements": [1] }
]
}
]
}
```
`sqlSha256` is the hash that the migrator records in the migration
history. `overrideJustification` is present only when `override` is
`true`. Entries are in apply order (journal `idx`, then file name; the
shipped journal repeats `idx` 178).
### Distribution over the existing migrations
No existing migration is rewritten or annotated.
| Class | All 467 | Newest 100 |
| --- | --- | --- |
| `online_compatible` | 228 (48.8%) | 67 |
| `requires_staging` | 114 (24.4%) | 12 |
| `unknown` | 125 (26.8%) | 21 |
Migrations per blocking reason (one migration can have several):
| Migrations | Class | Reason |
| --- | --- | --- |
| 74 | `requires_staging` | `create_unique_index_existing_table` |
| 70 | `unknown` | `lock_hazard_create_index` |
| 66 | `unknown` | `lock_hazard_validated_foreign_key` |
| 56 | `unknown` | `lock_hazard_data_backfill` |
| 54 | `unknown` | `drop_constraint` |
| 41 | `requires_staging` | `add_validated_check` |
| 33 | `unknown` | `drop_index` |
| 16 | `unknown` | `do_block_procedural` |
| 15 | `unknown` | `replace_function` |
| 13 | `unknown` | `create_trigger_existing_table` |
| 12 | `unknown` | `data_change_existing_table` |
| 10 | `unknown` | `drop_object`, `lock_hazard_set_not_null`,
`set_default` (each) |
| 8 | `requires_staging` | `drop_column` |
## Verification
- `pnpm --filter @paperclipai/db exec vitest run
src/migration-compatibility.test.ts`: 113 tests pass. They cover a
positive and a negative case for each rule, each reason code, the
override (missing, short, misplaced, and repeated justification are
rejected), unknown by default, the guard, the shipped migrations, the
`CONCURRENTLY` list, and the command line.
- `node --test scripts/cloud-migrator-artifacts.test.mjs
.github/scripts/tests/check-pr-migration-order.test.mjs`: 21 tests pass.
- Real bundle at the first commit of this branch: `preview-artifacts.mjs
pack`, then `cloud-migrator-artifacts.mjs build`, `validate`, and
`verify-install` pass. The manifest lists 467 migrations and is 312 KB.
This run found the repeated journal `idx`, which the second commit
handles.
- `pnpm --filter @paperclipai/db run check:migrations` passes. With
`--check --base HEAD~150` the guard fails on three `unknown` migrations
and prints notices for two `requires_staging` migrations.
- `pnpm verify` exits 2 because `.github/scripts/**` is a global CI
input. Required CI supplies the broad coverage.
- Not run: the publish job (it needs the master-only AWS role), and no
deployer reads the new field yet.
## Risks
- The manifest grows from about 1 KB to about 312 KB. A consumer that
rejects unknown manifest fields would fail. The verifier in this
repository ignores unknown fields; consumers outside this repository
were not inspected.
- From this merge, an open pull request that adds an `unknown` migration
fails the policy step and the `@paperclipai/db` typecheck until the
author rewrites the migration or adds the override. `drizzle-kit` emits
validated foreign keys and plain indexes for existing tables, so this is
common. The error text and `doc/DATABASE.md` give the fix.
- `ADD CONSTRAINT ... NOT VALID` classifies as `online_compatible`.
PostgreSQL still enforces it on new writes, so the author must make sure
that the previous release does not write rows that violate it.
- The classifier reads SQL only. It cannot see that new code writes
values that the previous release cannot read (for example a new enum
value). The `requires_staging` override exists for that case.
- `master` cannot fail the guard: the guard only judges migrations that
are not on the merge base.
## Model Used
- Claude (Anthropic), model ID `claude-fable-5-2`, in Claude Code with
tool use and code execution.
## 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
This commit is contained in:
1 parent
fc14f1ade2
commit
e8e548e856
9 files changed
+1621
-28
No files matched your search
@@ -1,5 +1,6 @@
|
||||
#!/usr/bin/env node
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { existsSync, readFileSync } from 'node:fs';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
||||
const MIGRATIONS_DIRECTORY = 'packages/db/src/migrations/';
|
||||
@@ -65,6 +66,35 @@ export function checkMigrationOrder(baseMigrationFiles, prMigrationFiles) {
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Judges whether the migrations a PR adds can be applied while the previous
|
||||
* release is still running. `unknown` without an in-file override fails;
|
||||
* `requires_staging` only produces a notice.
|
||||
*/
|
||||
export async function checkMigrationCompatibility(migrations, { priorMigrationSql, executorSource } = {}) {
|
||||
// Type-stripped by Node: the policy job runs before dependencies are installed.
|
||||
const { checkNewMigrationCompatibility, parseNonTransactionalMigrations } = await import(
|
||||
new URL('../../packages/db/src/migration-compatibility.ts', import.meta.url).href
|
||||
);
|
||||
// `executorSource` is the text of packages/db/src/client.ts. It names the
|
||||
// migrations that run outside a transaction, which CONCURRENTLY needs.
|
||||
const nonTransactionalMigrations = executorSource === undefined
|
||||
? undefined
|
||||
: parseNonTransactionalMigrations(executorSource);
|
||||
if (nonTransactionalMigrations === null) {
|
||||
return {
|
||||
passed: false,
|
||||
errors: ['MIGRATIONS_WITH_TOP_LEVEL_STATEMENTS was not found in packages/db/src/client.ts, so CONCURRENTLY migrations cannot be checked.'],
|
||||
notices: [],
|
||||
};
|
||||
}
|
||||
const { passed, errors, notices } = checkNewMigrationCompatibility(migrations, {
|
||||
priorMigrationSql,
|
||||
nonTransactionalMigrations,
|
||||
});
|
||||
return { passed, errors, notices };
|
||||
}
|
||||
|
||||
function gitPaths(args) {
|
||||
return execFileSync('git', args, { encoding: 'utf8' })
|
||||
.split('\0')
|
||||
@@ -78,7 +108,7 @@ function escapeWorkflowCommand(message) {
|
||||
.replaceAll('\n', '%0A');
|
||||
}
|
||||
|
||||
function main() {
|
||||
async function main() {
|
||||
const [baseSha, headSha] = process.argv.slice(2);
|
||||
const shaPattern = /^[0-9a-f]{40}$/i;
|
||||
if (!shaPattern.test(baseSha ?? '') || !shaPattern.test(headSha ?? '')) {
|
||||
@@ -95,18 +125,45 @@ function main() {
|
||||
]).filter((file) => file.endsWith('.sql'));
|
||||
const result = checkMigrationOrder(baseMigrationFiles, prMigrationFiles);
|
||||
|
||||
if (result.passed) {
|
||||
console.log(result.message);
|
||||
return;
|
||||
if (!result.passed) {
|
||||
console.error(
|
||||
`::error title=Migration numbers must follow the target branch::${escapeWorkflowCommand(result.message)}`,
|
||||
);
|
||||
console.error(result.message);
|
||||
process.exit(1);
|
||||
}
|
||||
console.log(result.message);
|
||||
|
||||
console.error(
|
||||
`::error title=Migration numbers must follow the target branch::${escapeWorkflowCommand(result.message)}`,
|
||||
if (prMigrationFiles.length === 0) return;
|
||||
// The checkout is the merge of the PR into the target branch, so it holds
|
||||
// the target branch's migrations and the migrator source.
|
||||
const root = fileURLToPath(new URL('../../', import.meta.url));
|
||||
const compatibility = await checkMigrationCompatibility(
|
||||
[...prMigrationFiles].sort().map((file) => ({
|
||||
fileName: file,
|
||||
sql: execFileSync('git', ['show', `${headSha}:${file}`], { encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 }),
|
||||
})),
|
||||
{
|
||||
priorMigrationSql: [...baseMigrationFiles].sort()
|
||||
.filter((file) => existsSync(`${root}${file}`))
|
||||
.map((file) => readFileSync(`${root}${file}`, 'utf8')),
|
||||
executorSource: readFileSync(`${root}packages/db/src/client.ts`, 'utf8'),
|
||||
},
|
||||
);
|
||||
console.error(result.message);
|
||||
process.exit(1);
|
||||
for (const notice of compatibility.notices) {
|
||||
console.log(`::notice title=Migration compatibility::${escapeWorkflowCommand(notice)}`);
|
||||
console.log(notice);
|
||||
}
|
||||
for (const error of compatibility.errors) {
|
||||
console.error(
|
||||
`::error title=Migration is not classified as safe under the running release::${escapeWorkflowCommand(error)}`,
|
||||
);
|
||||
console.error(error);
|
||||
}
|
||||
if (!compatibility.passed) process.exit(1);
|
||||
console.log('Every new migration is classified for application under the running release.');
|
||||
}
|
||||
|
||||
if (process.argv[1] === fileURLToPath(import.meta.url)) {
|
||||
main();
|
||||
await main();
|
||||
}
|
||||
@@ -1,6 +1,6 @@
|
||||
import { test } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import { checkMigrationOrder } from '../check-pr-migration-order.mjs';
|
||||
import { checkMigrationCompatibility, checkMigrationOrder } from '../check-pr-migration-order.mjs';
|
||||
|
||||
const migration = (name) => `packages/db/src/migrations/${name}.sql`;
|
||||
|
||||
@@ -41,3 +41,84 @@ test('fails when a PR inserts a migration before the target branch tip', () => {
|
||||
assert.match(result.message, /0229_from_stale_branch\.sql/);
|
||||
assert.doesNotMatch(result.message, /- packages\/db\/src\/migrations\/0231_valid_but_after_stale\.sql/);
|
||||
});
|
||||
|
||||
test('fails a new migration the classifier cannot prove safe under the running release', async () => {
|
||||
const result = await checkMigrationCompatibility([
|
||||
{ fileName: migration('0231_additive'), sql: 'ALTER TABLE "issues" ADD COLUMN "note" text;' },
|
||||
{ fileName: migration('0232_plain_index'), sql: 'CREATE INDEX "issues_note_idx" ON "issues" ("note");' },
|
||||
]);
|
||||
|
||||
assert.equal(result.passed, false);
|
||||
assert.equal(result.errors.length, 1);
|
||||
assert.match(result.errors[0], /0232_plain_index\.sql/);
|
||||
assert.match(result.errors[0], /lock_hazard_create_index/);
|
||||
assert.match(result.errors[0], /-- paperclip:migration-compat online_compatible: </);
|
||||
assert.match(result.errors[0], /expand migration now and a contract migration in a later release/);
|
||||
});
|
||||
|
||||
test('passes an unknown migration with a justified in-file override', async () => {
|
||||
const result = await checkMigrationCompatibility([{
|
||||
fileName: migration('0232_plain_index'),
|
||||
sql: '-- paperclip:migration-compat online_compatible: issues stays under 10k rows on every stack\nCREATE INDEX "issues_note_idx" ON "issues" ("note");',
|
||||
}]);
|
||||
|
||||
assert.equal(result.passed, true);
|
||||
assert.match(result.notices[0], /online_compatible by override/);
|
||||
});
|
||||
|
||||
test('fails an override that has no justification', async () => {
|
||||
const result = await checkMigrationCompatibility([{
|
||||
fileName: migration('0232_plain_index'),
|
||||
sql: '-- paperclip:migration-compat online_compatible:\nCREATE INDEX "issues_note_idx" ON "issues" ("note");',
|
||||
}]);
|
||||
|
||||
assert.equal(result.passed, false);
|
||||
assert.match(result.errors[0], /needs a justification/);
|
||||
});
|
||||
|
||||
test('reports requires_staging as a notice without failing', async () => {
|
||||
const result = await checkMigrationCompatibility([
|
||||
{ fileName: migration('0233_drop'), sql: 'ALTER TABLE "issues" DROP COLUMN "note";' },
|
||||
]);
|
||||
|
||||
assert.equal(result.passed, true);
|
||||
assert.deepEqual(result.errors, []);
|
||||
assert.match(result.notices[0], /0233_drop\.sql: requires_staging/);
|
||||
});
|
||||
|
||||
test('passes when a PR adds no migrations', async () => {
|
||||
assert.deepEqual(await checkMigrationCompatibility([]), { passed: true, errors: [], notices: [] });
|
||||
});
|
||||
|
||||
const executorSource = 'const MIGRATIONS_WITH_TOP_LEVEL_STATEMENTS = new Set([\n "0232_listed.sql",\n]);';
|
||||
const concurrentIndex = 'CREATE INDEX CONCURRENTLY IF NOT EXISTS "issues_note_idx" ON "issues" ("note");';
|
||||
|
||||
test('fails a CONCURRENTLY migration that the migrator would run in a transaction', async () => {
|
||||
const result = await checkMigrationCompatibility(
|
||||
[{ fileName: migration('0232_unlisted'), sql: concurrentIndex }, { fileName: migration('0232_listed'), sql: concurrentIndex }],
|
||||
{ executorSource },
|
||||
);
|
||||
|
||||
assert.equal(result.passed, false);
|
||||
assert.equal(result.errors.length, 1);
|
||||
assert.match(result.errors[0], /0232_unlisted\.sql: uses CONCURRENTLY/);
|
||||
assert.match(result.errors[0], /Add "0232_unlisted\.sql" to MIGRATIONS_WITH_TOP_LEVEL_STATEMENTS in packages\/db\/src\/client\.ts/);
|
||||
});
|
||||
|
||||
test('fails closed when the migrator source no longer names its non-transactional migrations', async () => {
|
||||
const result = await checkMigrationCompatibility([], { executorSource: 'export const nothing = 1;' });
|
||||
|
||||
assert.equal(result.passed, false);
|
||||
assert.match(result.errors[0], /MIGRATIONS_WITH_TOP_LEVEL_STATEMENTS was not found/);
|
||||
});
|
||||
|
||||
test('treats CREATE TABLE IF NOT EXISTS as new only when the target branch never created the table', async () => {
|
||||
const sql = 'CREATE TABLE IF NOT EXISTS "widgets" ("id" uuid);\nALTER TABLE "widgets" DROP COLUMN "title";';
|
||||
const fresh = await checkMigrationCompatibility([{ fileName: migration('0231_widgets'), sql }], { priorMigrationSql: [] });
|
||||
const existing = await checkMigrationCompatibility([{ fileName: migration('0231_widgets'), sql }], {
|
||||
priorMigrationSql: ['CREATE TABLE "widgets" ("id" uuid, "title" text);'],
|
||||
});
|
||||
|
||||
assert.deepEqual(fresh.notices, []);
|
||||
assert.match(existing.notices[0], /0231_widgets\.sql: requires_staging/);
|
||||
});
|
||||
Reference in new issue
Block a user