2 Commits

Include renames

Author	SHA1	Message	Date
DottaandClaude Fable 5.2	e8e548e856	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>	2026-11-11 14:14:40 -06:00
DottaandPaperclip	7b73b08250	ci: enforce migration order against PR target (#12433) ... ## Thinking Path > - Paperclip is the open source app that people use to manage AI agents for work. > - Paperclip applies database migrations in numeric order. > - Two branches can create the same migration number before either branch merges. > - The existing repository check can find duplicates only after both histories are present in one checkout. > - A pull request must compare its new migrations with the target branch before merge. > - This pull request adds that comparison to the existing PR policy job. > - The benefit is an early failure with exact renumbering instructions. ## Linked Issues or Issue Description **What existing behavior does this improve?** The PR policy check for files in `packages/db/src/migrations`. **Current behavior** A stale branch can add the same migration number as the target branch. The existing check does not compare PR additions with the target branch migration tip. **Proposed behavior** The policy job fails when a new PR migration number is not greater than every migration on the target branch. The error names the conflict, the next safe number, and the related files to update. **Reason and benefit** This prevents duplicate or out-of-order migration numbers from reaching `master`. It also gives contributors and agents a direct repair procedure. **Breaking changes** None. The change rejects migration numbering that is already unsafe. ## What Changed - Added a dependency-free check that compares new PR migration files with the target branch tip. - Added the check to the existing PR policy job. - Added tests for no-op, valid, duplicate, and lower-number cases. ## Verification - `node --test '.github/scripts/tests/*.test.mjs'` passed 133 tests. - `pnpm -r typecheck` passed. - `pnpm build` passed. - `pnpm test:run` passed 4,889 tests and failed 24 unrelated macOS path and wildcard-listener tests that also affect the current `master` checkout. ## Risks - Low risk. The check reads Git history and does not modify migrations. - The check permits gaps. It only requires each new migration number to follow the target branch tip. - The existing migration check continues to validate duplicate numbers, snapshots, and journal entries inside the PR. ## Model Used OpenAI Codex, GPT-5. The exact serving model ID and context-window size are not exposed in this session. Reasoning, tool use, web access, and code execution were enabled. ## 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 - [ ] 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 Co-authored-by: Paperclip <noreply@paperclip.ing>	2026-08-28 08:47:22 -05:00