diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index fe36e6cfa3..7ef2c2df32 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -139,9 +139,30 @@ jobs: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: persist-credentials: false + # Full history so the migration check can diff against the merge + # base with master; blobless to keep the large repo checkout fast + # (ls-tree and merge-base only need commits and trees) + fetch-depth: 0 + filter: blob:none - name: Set up environment uses: ./.github/actions/setup with: download-translations: 'false' - name: Check migrations run: yarn workspace @actual-app/ci-actions tsx bin/check-migrations.ts + - name: Save PR number for the warnings comment workflow + if: always() && hashFiles('packages/ci-actions/migration-warnings.json') != '' + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + run: echo "$PR_NUMBER" > packages/ci-actions/migration-warnings-pr.txt + # Consumed by migration-warnings-comment.yml, which posts the advisory + # warnings as a PR comment (this job's token is read-only on fork PRs, + # so it can't comment itself). + - name: Upload warnings for the comment workflow + if: always() && hashFiles('packages/ci-actions/migration-warnings.json') != '' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: migration-warnings + path: | + packages/ci-actions/migration-warnings.json + packages/ci-actions/migration-warnings-pr.txt diff --git a/.github/workflows/migration-warnings-comment.yml b/.github/workflows/migration-warnings-comment.yml new file mode 100644 index 0000000000..7b138b949d --- /dev/null +++ b/.github/workflows/migration-warnings-comment.yml @@ -0,0 +1,99 @@ +name: Migration warnings comment + +# Posts the advisory migration warnings from the `migrations` job in +# check.yml as a PR comment, so they're visible without opening the job log. +# +# SECURITY: This runs in the trusted base-repo context (workflow_run) so its +# token can comment on fork PRs, whose `pull_request` runs only get a +# read-only token. The artifact produced by the PR run is UNTRUSTED data: +# the PR number is cross-checked against the triggering run's head SHA, and +# the findings are validated against a fixed allowlist before being rendered +# into the comment. Nothing from the artifact is ever executed. +on: + # See SECURITY note above: artifact content is validated and only ever + # treated as data. + workflow_run: # zizmor: ignore[dangerous-triggers] + workflows: ['Test'] + types: + - completed + +permissions: {} + +jobs: + comment: + if: github.event.workflow_run.event == 'pull_request' + runs-on: depot-ubuntu-latest + permissions: + actions: read + pull-requests: write + steps: + - name: Download warnings artifact + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + run-id: ${{ github.event.workflow_run.id }} + # `pattern` instead of `name` so a run without the artifact (PR + # doesn't touch migrations) skips gracefully instead of failing. + pattern: migration-warnings + merge-multiple: true + path: warnings + + - name: Post or update PR comment + if: hashFiles('warnings/migration-warnings.json') != '' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + run: | + set -euo pipefail + JSON=warnings/migration-warnings.json + MARKER='' + + PR=$(cat warnings/migration-warnings-pr.txt) + if ! [[ "$PR" =~ ^[0-9]+$ ]]; then + echo "Invalid PR number in artifact" + exit 1 + fi + + # The artifact is untrusted: make sure the PR it names is really + # the one that produced the triggering run. + if [ "$(gh api "repos/$REPO/pulls/$PR" --jq .head.sha)" != "$HEAD_SHA" ]; then + echo "Artifact PR head does not match triggering run head; skipping" + exit 0 + fi + + existing=$(gh api "repos/$REPO/issues/$PR/comments" --paginate \ + --jq '.[] | select(.user.login == "github-actions[bot]") | select(.body | contains("")) | .id') + existing=${existing%%$'\n'*} + + # Warnings resolved (or none) — remove a stale comment if present. + if jq -e 'length == 0' "$JSON" > /dev/null; then + if [ -n "$existing" ]; then + gh api -X DELETE "repos/$REPO/issues/comments/$existing" + fi + exit 0 + fi + + # Only render findings matching the exact shape and allowlisted + # risk strings produced by check-migrations.ts. + jq -e ' + type == "array" and all(.[]; + (.name | type == "string" and test("^[A-Za-z0-9._-]{1,200}$")) and + (.risks | type == "array" and length > 0 and all(.[]; + IN("drops a table", "drops a column", "renames a table or column")))) + ' "$JSON" > /dev/null + + body="$MARKER + ### ⚠️ This PR contains possibly breaking database migrations + + $(jq -r '.[] | . as $m | .risks[] | "- `\($m.name)` \(.)."' "$JSON") + + Migrations must be additive-only: removing or renaming tables or columns breaks older clients that sync the same budget file. See the [migration guidelines](https://actualbudget.org/docs/contributing/project-details/migrations). + + This is an automated advisory check — it may be a false positive and does not block the PR." + + if [ -n "$existing" ]; then + gh api -X PATCH "repos/$REPO/issues/comments/$existing" -f body="$body" + else + gh api "repos/$REPO/issues/$PR/comments" -f body="$body" + fi diff --git a/packages/ci-actions/.gitignore b/packages/ci-actions/.gitignore index a261f29175..3cdd9a343e 100644 --- a/packages/ci-actions/.gitignore +++ b/packages/ci-actions/.gitignore @@ -1 +1,3 @@ dist/* +migration-warnings.json +migration-warnings-pr.txt diff --git a/packages/ci-actions/bin/check-migrations.ts b/packages/ci-actions/bin/check-migrations.ts index d8d05d98ed..0d7ba069e3 100644 --- a/packages/ci-actions/bin/check-migrations.ts +++ b/packages/ci-actions/bin/check-migrations.ts @@ -1,11 +1,35 @@ -// overview: -// 1. Identify the migrations in packages/loot-core/migrations/* on `master` and HEAD -// 2. Make sure that any new migrations on HEAD are dated after the latest migration on `master`. +// overview: identify the migrations in packages/loot-core/migrations/* on +// `master`, on the merge base, and on HEAD, then: +// 1. Make sure that any new migrations on HEAD are dated after the latest +// migration on `master` (older dates trigger `out-of-sync-migrations` for +// users who already applied the newer one). +// 2. Make sure no migration that exists on the merge base was edited or +// deleted (shipped migrations are append-only: existing installs never +// re-run them, so edits fork the schema across the user base). +// 3. Emit advisory warnings when a new migration contains statements that +// look like they remove or rename schema (removing or renaming breaks +// older clients syncing the same budget file). The warnings are also +// written to migration-warnings.json so the migration-warnings-comment +// workflow can surface them as a PR comment. +// +// See https://actualbudget.org/docs/contributing/project-details/migrations import { spawnSync } from 'child_process'; +import { readFileSync, writeFileSync } from 'fs'; import path from 'path'; import { fileURLToPath } from 'url'; +import { + findAddedMigrations, + findMisdatedMigrations, + findMutatedMigrations, + findRiskyStatements, + parseMigrationTree, +} from '../src/migrations/check'; + +const POLICY_URL = + 'https://actualbudget.org/docs/contributing/project-details/migrations'; + const migrationsDir = path.join( path.dirname(fileURLToPath(import.meta.url)), '..', @@ -16,45 +40,118 @@ const migrationsDir = path.join( 'migrations', ); -function readMigrations(ref: string) { - const { stdout } = spawnSync('git', [ - 'ls-tree', - '--name-only', - ref, - migrationsDir + '/', - ]); - const files = stdout.toString().split('\n').filter(Boolean); - console.log(`Found ${files.length} migrations on ${ref}.`); - return files - .map(file => path.basename(file)) - .filter(file => !file.startsWith('.')) - .map(name => ({ - date: parseInt(name.split('_')[0]), - name: name.match(/^\d+_(.+?)(\.sql)?$/)?.[1] ?? '***' + name, - })); +function git(args: string[]): string { + const { status, stdout, stderr, error } = spawnSync('git', args); + if (error) { + throw error; + } + if (status !== 0) { + throw new Error( + `git ${args.join(' ')} failed (exit ${status}): ${stderr.toString()}`, + ); + } + return stdout.toString(); } -spawnSync('git', ['fetch', 'origin', 'master']); +function readMigrations(ref: string) { + const migrations = parseMigrationTree( + git(['ls-tree', ref, migrationsDir + '/']), + ); + console.log(`Found ${migrations.length} migrations on ${ref}.`); + return migrations; +} + +git(['fetch', 'origin', 'master']); + +const mergeBase = git(['merge-base', 'origin/master', 'HEAD']).trim(); + const masterMigrations = readMigrations('origin/master'); +const mergeBaseMigrations = readMigrations(mergeBase); const headMigrations = readMigrations('HEAD'); -const latestMasterMigration = - masterMigrations[masterMigrations.length - 1].date; -const newMigrations = headMigrations.filter( - migration => !masterMigrations.find(m => m.name === migration.name), +const problems: string[] = []; + +// 1. New migrations must be dated after the latest migration on master. +const latestMasterMigration = Math.max( + 0, + ...masterMigrations.map(migration => migration.id), ); -const badMigrations = newMigrations.filter( - migration => migration.date <= latestMasterMigration, +const newMigrations = findAddedMigrations(masterMigrations, headMigrations); +const misdated = findMisdatedMigrations(newMigrations, latestMasterMigration); + +for (const migration of misdated) { + problems.push( + `Migration ${migration.name} is dated before the latest migration on ` + + `master. Rename it with a newer timestamp, otherwise users who ` + + `already applied the newer migration will fail to load their budget.`, + ); +} + +// 2. Migrations that exist on the merge base must not be edited or deleted. +const { modified, deleted } = findMutatedMigrations( + mergeBaseMigrations, + headMigrations, ); -if (badMigrations.length) { - console.error( - `The following migrations are dated before the latest migration on master:`, +for (const name of modified) { + problems.push( + `Migration ${name} was modified. Shipped migrations are append-only: ` + + `existing installs will never re-run it, so editing it forks the ` + + `database schema across the user base. Add a new migration instead.`, ); - badMigrations.forEach(migration => { - console.error(` ${migration.name}`); +} +for (const name of deleted) { + problems.push( + `Migration ${name} was deleted. Shipped migrations are append-only: ` + + `existing installs already ran it, so deleting it breaks the ` + + `migration check for every existing budget file.`, + ); +} + +// 3. Advisory: warn on new migrations that look like they remove or rename +// schema. Not fatal — regexes over SQL can't be authoritative, so this only +// surfaces an early hint on the PR. +const riskyMigrations: { name: string; risks: string[] }[] = []; +for (const migration of newMigrations) { + let source = ''; + try { + source = readFileSync(path.join(migrationsDir, migration.name), 'utf8'); + } catch { + continue; + } + const risks = findRiskyStatements(source); + for (const risk of risks) { + console.log( + `::warning file=packages/loot-core/migrations/${migration.name},` + + `title=Possibly breaking migration::This migration ${risk}. ` + + `Migrations must be additive-only; removing or renaming schema ` + + `breaks older clients syncing the same budget file. See ${POLICY_URL}`, + ); + } + if (risks.length) { + riskyMigrations.push({ name: migration.name, risks }); + } +} + +// The warnings above don't fail the job, so they're easy to miss. Hand them +// to the migration-warnings-comment workflow (whose token can comment on +// fork PRs) via an artifact. Always written — an empty list tells the +// workflow to remove a stale comment once the warnings are resolved. +writeFileSync( + path.join( + path.dirname(fileURLToPath(import.meta.url)), + '..', + 'migration-warnings.json', + ), + JSON.stringify(riskyMigrations), +); + +if (problems.length) { + console.error(`Migration policy violations found (see ${POLICY_URL}):`); + problems.forEach(problem => { + console.error(` - ${problem}`); }); process.exit(1); } else { - console.log(`All migrations are dated after the latest migration on master.`); + console.log('All migration checks passed.'); } diff --git a/packages/ci-actions/src/migrations/check.test.ts b/packages/ci-actions/src/migrations/check.test.ts new file mode 100644 index 0000000000..971dd4ef59 --- /dev/null +++ b/packages/ci-actions/src/migrations/check.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from 'vitest'; + +import { + findAddedMigrations, + findMisdatedMigrations, + findMutatedMigrations, + findRiskyStatements, + parseMigrationTree, +} from './check'; + +const treeOutput = [ + '100644 blob aaa111\tpackages/loot-core/migrations/1000_first.sql', + '100644 blob bbb222\tpackages/loot-core/migrations/2000_second.js', + '100644 blob ccc333\tpackages/loot-core/migrations/.hidden', +].join('\n'); + +describe('parseMigrationTree', () => { + it('parses names, ids, and hashes, skipping hidden files', () => { + expect(parseMigrationTree(treeOutput)).toEqual([ + { name: '1000_first.sql', id: 1000, hash: 'aaa111' }, + { name: '2000_second.js', id: 2000, hash: 'bbb222' }, + ]); + }); + + it('returns an empty list for empty output', () => { + expect(parseMigrationTree('')).toEqual([]); + }); +}); + +describe('findAddedMigrations', () => { + it('returns migrations present on head but not on base', () => { + const base = parseMigrationTree(treeOutput); + const head = [ + ...base, + { name: '3000_third.sql', id: 3000, hash: 'ddd444' }, + ]; + expect(findAddedMigrations(base, head)).toEqual([ + { name: '3000_third.sql', id: 3000, hash: 'ddd444' }, + ]); + }); +}); + +describe('findMutatedMigrations', () => { + it('detects modified and deleted migrations', () => { + const base = parseMigrationTree(treeOutput); + const head = [{ name: '1000_first.sql', id: 1000, hash: 'changed' }]; + expect(findMutatedMigrations(base, head)).toEqual({ + modified: ['1000_first.sql'], + deleted: ['2000_second.js'], + }); + }); + + it('reports nothing when head matches base', () => { + const base = parseMigrationTree(treeOutput); + expect(findMutatedMigrations(base, base)).toEqual({ + modified: [], + deleted: [], + }); + }); +}); + +describe('findMisdatedMigrations', () => { + it('flags migrations dated at or before the latest existing one', () => { + const added = [ + { name: '1500_late.sql', id: 1500, hash: 'a' }, + { name: '2000_same.sql', id: 2000, hash: 'b' }, + { name: '2500_ok.sql', id: 2500, hash: 'c' }, + ]; + expect(findMisdatedMigrations(added, 2000).map(m => m.name)).toEqual([ + '1500_late.sql', + '2000_same.sql', + ]); + }); +}); + +describe('findRiskyStatements', () => { + it('flags DROP TABLE, DROP COLUMN, and RENAME statements', () => { + expect(findRiskyStatements('ALTER TABLE x DROP COLUMN y;')).toEqual([ + 'drops a column', + ]); + expect(findRiskyStatements('DROP TABLE x;')).toEqual(['drops a table']); + expect(findRiskyStatements('ALTER TABLE x RENAME TO y;')).toEqual([ + 'renames a table or column', + ]); + expect(findRiskyStatements('ALTER TABLE x RENAME COLUMN a TO b;')).toEqual([ + 'renames a table or column', + ]); + }); + + it('ignores additive statements and comments', () => { + expect(findRiskyStatements('ALTER TABLE x ADD COLUMN y TEXT;')).toEqual([]); + expect( + findRiskyStatements('-- do not DROP TABLE here\nCREATE TABLE y (id);'), + ).toEqual([]); + expect( + findRiskyStatements('/* DROP COLUMN in a block comment */ SELECT 1;'), + ).toEqual([]); + }); +}); diff --git a/packages/ci-actions/src/migrations/check.ts b/packages/ci-actions/src/migrations/check.ts new file mode 100644 index 0000000000..fba9eabaa9 --- /dev/null +++ b/packages/ci-actions/src/migrations/check.ts @@ -0,0 +1,87 @@ +// Pure logic for the migration checks that run in CI. Database migrations +// are append-only: once a migration has shipped, editing or deleting it +// forks the schema across the user base (clients that already ran the old +// version will never re-run it), and a migration dated before an already +// shipped one triggers the `out-of-sync-migrations` error for users who +// applied the newer one first. This module also flags statements that look +// like they remove or rename schema, so contributors get a hint in the PR +// checks. +// See https://actualbudget.org/docs/contributing/project-details/migrations + +export type MigrationEntry = { + name: string; + id: number; + hash: string; +}; + +// Parses `git ls-tree -- /` output, where each line looks like +// `100644 blob \t`. +export function parseMigrationTree(lsTreeOutput: string): MigrationEntry[] { + return lsTreeOutput + .split('\n') + .filter(Boolean) + .map(line => { + const [meta, filePath] = line.split('\t'); + const hash = meta?.split(/\s+/)[2] ?? ''; + const name = filePath?.split('/').pop() ?? ''; + return { name, id: Number.parseInt(name.split('_')[0], 10), hash }; + }) + .filter(entry => entry.name !== '' && !entry.name.startsWith('.')) + .filter(entry => Number.isFinite(entry.id)); +} + +export function findAddedMigrations( + base: MigrationEntry[], + head: MigrationEntry[], +): MigrationEntry[] { + const baseNames = new Set(base.map(migration => migration.name)); + return head.filter(migration => !baseNames.has(migration.name)); +} + +export function findMutatedMigrations( + base: MigrationEntry[], + head: MigrationEntry[], +): { modified: string[]; deleted: string[] } { + const headByName = new Map( + head.map(migration => [migration.name, migration]), + ); + + const modified: string[] = []; + const deleted: string[] = []; + for (const migration of base) { + const headMigration = headByName.get(migration.name); + if (!headMigration) { + deleted.push(migration.name); + } else if (headMigration.hash !== migration.hash) { + modified.push(migration.name); + } + } + return { modified, deleted }; +} + +export function findMisdatedMigrations( + added: MigrationEntry[], + latestExistingId: number, +): MigrationEntry[] { + return added.filter(migration => migration.id <= latestExistingId); +} + +const RISKY_PATTERNS = [ + { pattern: /\bDROP\s+TABLE\b/i, description: 'drops a table' }, + { pattern: /\bDROP\s+COLUMN\b/i, description: 'drops a column' }, + { + pattern: /\bRENAME\s+(COLUMN|TO)\b/i, + description: 'renames a table or column', + }, +]; + +// Advisory only: regexes over SQL text can't see every destructive change, +// so this just surfaces an early hint on obviously suspicious SQL. +export function findRiskyStatements(source: string): string[] { + const withoutComments = source + .replace(/--[^\n]*/g, '') + .replace(/\/\*[\s\S]*?\*\//g, ''); + return RISKY_PATTERNS.filter(({ pattern }) => + pattern.test(withoutComments), + ).map(({ description }) => description); +} diff --git a/upcoming-release-notes/enforce-append-only-migrations-ci.md b/upcoming-release-notes/enforce-append-only-migrations-ci.md new file mode 100644 index 0000000000..6a44beccb5 --- /dev/null +++ b/upcoming-release-notes/enforce-append-only-migrations-ci.md @@ -0,0 +1,6 @@ +--- +category: Maintenance +authors: [MatissJanis] +--- + +Enforce in CI that shipped database migration files are never edited or deleted, and warn on migrations that look destructive