From 7b73b0825030d06823f05089cbaec49fe87fb714 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 28 Aug 2026 08:47:22 -0500 Subject: [PATCH] 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 --- .github/scripts/check-pr-migration-order.mjs | 112 ++++++++++++++++++ .../tests/check-pr-migration-order.test.mjs | 43 +++++++ .github/workflows/pr.yml | 6 + 3 files changed, 161 insertions(+) create mode 100644 .github/scripts/check-pr-migration-order.mjs create mode 100644 .github/scripts/tests/check-pr-migration-order.test.mjs diff --git a/.github/scripts/check-pr-migration-order.mjs b/.github/scripts/check-pr-migration-order.mjs new file mode 100644 index 0000000000..b67dfb3fab --- /dev/null +++ b/.github/scripts/check-pr-migration-order.mjs @@ -0,0 +1,112 @@ +#!/usr/bin/env node +import { execFileSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; + +const MIGRATIONS_DIRECTORY = 'packages/db/src/migrations/'; +const MIGRATION_FILE_PATTERN = /^packages\/db\/src\/migrations\/(\d{4})_[^/]+\.sql$/; + +function parseMigration(file) { + const match = file.match(MIGRATION_FILE_PATTERN); + return match ? { file, number: Number.parseInt(match[1], 10) } : null; +} + +function formatMigrationNumber(number) { + return String(number).padStart(4, '0'); +} + +export function checkMigrationOrder(baseMigrationFiles, prMigrationFiles) { + const invalidFiles = [...baseMigrationFiles, ...prMigrationFiles] + .filter((file) => !parseMigration(file)); + + if (invalidFiles.length > 0) { + return { + passed: false, + message: [ + 'Migration SQL files must start with a 4-digit number:', + ...invalidFiles.map((file) => `- ${file}`), + ].join('\n'), + }; + } + + if (prMigrationFiles.length === 0) { + return { passed: true, message: 'No new migrations in this PR.' }; + } + + const baseMigrations = baseMigrationFiles.map(parseMigration); + const prMigrations = prMigrationFiles.map(parseMigration); + const latestBaseMigration = baseMigrations.reduce( + (latest, migration) => migration.number > latest.number ? migration : latest, + { file: '(none)', number: -1 }, + ); + const outOfOrder = prMigrations.filter( + (migration) => migration.number <= latestBaseMigration.number, + ); + + if (outOfOrder.length === 0) { + return { + passed: true, + message: `All new migrations follow ${latestBaseMigration.file}.`, + }; + } + + const nextNumber = formatMigrationNumber(latestBaseMigration.number + 1); + return { + passed: false, + message: [ + `The target branch already contains migrations through ${latestBaseMigration.file}.`, + 'This PR adds migration numbers that would be inserted into or collide with that history:', + ...outOfOrder.map((migration) => `- ${migration.file}`), + '', + `Update from the target branch, then renumber this PR's migrations starting at ${nextNumber}`, + 'in their intended order. Keep each SQL filename, matching meta snapshot, and', + 'packages/db/src/migrations/meta/_journal.json entry aligned, then push again.', + 'Migration numbers are append-only and cannot reuse a number already present on the target branch.', + ].join('\n'), + }; +} + +function gitPaths(args) { + return execFileSync('git', args, { encoding: 'utf8' }) + .split('\0') + .filter(Boolean); +} + +function escapeWorkflowCommand(message) { + return message + .replaceAll('%', '%25') + .replaceAll('\r', '%0D') + .replaceAll('\n', '%0A'); +} + +function main() { + const [baseSha, headSha] = process.argv.slice(2); + const shaPattern = /^[0-9a-f]{40}$/i; + if (!shaPattern.test(baseSha ?? '') || !shaPattern.test(headSha ?? '')) { + console.error('Usage: check-pr-migration-order.mjs <40-character base SHA> <40-character head SHA>'); + process.exit(2); + } + + const baseMigrationFiles = gitPaths([ + 'ls-tree', '-r', '--name-only', '-z', baseSha, '--', MIGRATIONS_DIRECTORY, + ]).filter((file) => file.endsWith('.sql')); + const prMigrationFiles = gitPaths([ + 'diff', '--name-only', '--diff-filter=A', '-z', `${baseSha}...${headSha}`, '--', + MIGRATIONS_DIRECTORY, + ]).filter((file) => file.endsWith('.sql')); + const result = checkMigrationOrder(baseMigrationFiles, prMigrationFiles); + + if (result.passed) { + console.log(result.message); + return; + } + + console.error( + `::error title=Migration numbers must follow the target branch::${escapeWorkflowCommand(result.message)}`, + ); + console.error(result.message); + process.exit(1); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main(); +} diff --git a/.github/scripts/tests/check-pr-migration-order.test.mjs b/.github/scripts/tests/check-pr-migration-order.test.mjs new file mode 100644 index 0000000000..48c1b2db8f --- /dev/null +++ b/.github/scripts/tests/check-pr-migration-order.test.mjs @@ -0,0 +1,43 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { checkMigrationOrder } from '../check-pr-migration-order.mjs'; + +const migration = (name) => `packages/db/src/migrations/${name}.sql`; + +test('passes when a PR has no new migrations', () => { + const result = checkMigrationOrder([migration('0230_on_master')], []); + + assert.equal(result.passed, true); +}); + +test('passes when every PR migration follows the target branch', () => { + const result = checkMigrationOrder( + [migration('0230_on_master')], + [migration('0231_first_in_pr'), migration('0232_second_in_pr')], + ); + + assert.equal(result.passed, true); +}); + +test('fails with renumbering guidance when a PR reuses the target branch number', () => { + const result = checkMigrationOrder( + [migration('0230_on_master')], + [migration('0230_from_stale_branch')], + ); + + assert.equal(result.passed, false); + assert.match(result.message, /already contains migrations through .*0230_on_master\.sql/); + assert.match(result.message, /renumber this PR's migrations starting at 0231/); + assert.match(result.message, /meta\/_journal\.json/); +}); + +test('fails when a PR inserts a migration before the target branch tip', () => { + const result = checkMigrationOrder( + [migration('0230_on_master')], + [migration('0229_from_stale_branch'), migration('0231_valid_but_after_stale')], + ); + + assert.equal(result.passed, false); + assert.match(result.message, /0229_from_stale_branch\.sql/); + assert.doesNotMatch(result.message, /- packages\/db\/src\/migrations\/0231_valid_but_after_stale\.sql/); +}); diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index e356bb8a09..f80958c7f1 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -46,6 +46,12 @@ jobs: with: node-version: 24 + - name: Validate migration ordering against target branch + run: >- + node .github/scripts/check-pr-migration-order.mjs + "${{ github.event.pull_request.base.sha }}" + "${{ github.event.pull_request.head.sha }}" + - name: Validate Dockerfile deps stage run: node ./scripts/check-docker-deps-stage.mjs