diff --git a/.agents/review-rules/README.md b/.agents/review-rules/README.md index ec65c84ad37..28140c51a3d 100644 --- a/.agents/review-rules/README.md +++ b/.agents/review-rules/README.md @@ -15,6 +15,7 @@ clear, when to stay silent, what never to say — lives once in |-------------|----------|-----------------------------------------------------| | `security/` | Security | backend packages + nodes | | `backend/` | Backend | `cli`, `@n8n/db`, `core`, `workflow`, node packages | +| `db-migrations/` | DB migrations | `@n8n/db` migrations + their tests in `cli` | | `frontend/` | Frontend | `packages/frontend` | | `qa-dx/` | QA & DX | `.github`, `docker`, `scripts`, `patches`, `packages/testing`, the lint/test/TS config packages, baselines | | `testing/` | Backend + Frontend | any package with a test suite | @@ -25,10 +26,12 @@ never a copy per directory. Security and QA & DX deliberately don't link `testing/`: coverage nagging on a credential fix or a Dockerfile is noise those agents shouldn't be able to produce. -One slot of five is left. QA & DX covers the build, test, and CI surface — the -same paths `.github/OWNERS` assigns to `@n8n-io/qa-dx`. Code-quality rules that -happen to apply broadly (error classes, `any`, lazy imports) are backend rules, -not QA & DX ones. +All five slots are used, so a new domain now merges into an existing agent. QA & +DX covers the build, test, and CI surface — the same paths `.github/OWNERS` +assigns to `@n8n-io/qa-dx`. Code-quality rules that happen to apply broadly +(error classes, `any`, lazy imports) are backend rules, not QA & DX ones. DB +migrations is split out of Backend because a migration is permanent and runs +unattended on every instance, so it is judged against a different bar. ## Limits that bite @@ -36,7 +39,7 @@ cubic fails silently on all three of these, which is why `pnpm check:cubic-confi enforces them in CI: - **5 enabled agents per repository.** Rules past the fifth never run and cubic - says nothing. One slot is deliberately left free. + says nothing. All five are in use. - **10,000 characters per agent**, counting the `description`, every linked file, and `custom_instructions` — concatenated in the listed order. Everything past the limit is dropped from the review prompt. The shared block diff --git a/.agents/review-rules/db-migrations/conventions-and-tests.md b/.agents/review-rules/db-migrations/conventions-and-tests.md new file mode 100644 index 00000000000..0385665040e --- /dev/null +++ b/.agents/review-rules/db-migrations/conventions-and-tests.md @@ -0,0 +1,32 @@ +# Migration conventions and tests + +Applies to: `packages/@n8n/db/src/migrations/**`, `packages/cli/test/migration/**`. + +`.agents/skills/db-migrations/SKILL.md` is the standard; prefer it over your +priors. Read a recent migration for the local idiom. + +Flag: + +- a hand-written table prefix or quoted identifier. Use `escape.tableName()` and + `escape.columnName()`; +- `queryRunner.query()` where `runQuery()` belongs, or `console.log` where the + context `logger` belongs; +- a value import of an entity, or a cross-package import; +- a timestamp that is not above every existing migration; +- a hand-edited generated index file; +- an 80-line `up()` that needs named private methods. + +Precedent beats generic style opinion, but it is not a defence. An older migration +doing it wrong makes the finding informational. + +## Tests + +A data migration needs an integration test. Reading cannot verify its assumptions +about row shape, JSON and NULLs. Say so when there is none. A good test sets up +with `initDbUpToMigration`, runs with `runSingleMigration`, and covers both +engines, `down()`, and dirty rows. A happy-path test, on a migration that claims +to handle bad data, is worth a comment. For a schema-only migration the DSL calls +are enough, so a missing test is informational. + +Regenerate the migration index files and the schema docs in `docs/generated/` when +the schema changes. Check the changed file list before you call one stale. diff --git a/.agents/review-rules/db-migrations/cross-db-compatibility.md b/.agents/review-rules/db-migrations/cross-db-compatibility.md new file mode 100644 index 00000000000..18459f412e7 --- /dev/null +++ b/.agents/review-rules/db-migrations/cross-db-compatibility.md @@ -0,0 +1,31 @@ +# Postgres and SQLite compatibility + +Applies to: `packages/@n8n/db/src/migrations/**`. + +A migration in `common/` runs on both engines. Judge them separately. A change +that is safe on Postgres can lose data on SQLite. + +## The CASCADE trap on SQLite + +Six DSL helpers recreate the table on SQLite: `addColumns`, `dropColumns`, +`addNotNull`, `dropNotNull`, `addEnumCheck`, `dropEnumCheck`. TypeORM copies the +rows to a temp table, drops the original and renames. If another table has an +incoming `ON DELETE CASCADE` foreign key, that `DROP` **deletes its rows**. + +This is the most valuable check on any SQLite migration. The +`{ recreatesOnSqlite: true }` argument is an acknowledgement, not a fix. Check the +target table for incoming CASCADE foreign keys. If it has them, the migration +needs a `sqlite/` subclass with `withFKsDisabled = true as const`, or raw +`ALTER TABLE ADD COLUMN` when every new column is nullable or has a default. Say +so if you cannot check. + +## Everything else + +In a `common/` migration, flag Postgres-only SQL — `ALTER COLUMN … TYPE`, +expression indexes, `gen_random_uuid()` — raw SQL that assumes one engine's +boolean literal or quoting, and `INSERT OR REPLACE` where `ON CONFLICT DO NOTHING` +was meant. `varchar(N)` enforcement, transactional DDL, JSON and timestamp +handling, and NULL in unique constraints all differ. + +A dialect migration belongs in `postgresdb/` or `sqlite/`. Read the DSL source +before you claim how a helper behaves. diff --git a/.agents/review-rules/db-migrations/data-safety.md b/.agents/review-rules/db-migrations/data-safety.md new file mode 100644 index 00000000000..e96de8c037c --- /dev/null +++ b/.agents/review-rules/db-migrations/data-safety.md @@ -0,0 +1,27 @@ +# Data safety + +Applies to: `packages/@n8n/db/src/migrations/**`. + +Think of a database last upgraded two years ago. It holds dirty rows, unexpected +NULLs, retired enum values and malformed JSON. + +Flag: + +- a drop in the same release the code stopped writing the column. Expand-contract + must finish first; +- rows deleted with no fallback copy; +- a table copy with no row-count check; +- a backfill that overwrites the old value with no way back, in a + `ReversibleMigration`; +- `JSON.parse` with no `try`/`catch`; +- iteration with no `Array.isArray`, or a property read that assumes the property + exists; +- a backfill that cannot be retried after a partial failure; +- ordering that is assumed, not enforced; +- a `down()` that is empty, or does not restore the previous state. + +`IrreversibleMigration` is correct only when `up()` destroys data that `down()` +would need, and the reason is stated. It is not a way to skip a tedious `down()`. + +Do not flag a hazard the code handles. A guarded parse, a batched update or a +`withFKsDisabled` subclass is the fix. diff --git a/.agents/review-rules/db-migrations/necessity-and-schema-design.md b/.agents/review-rules/db-migrations/necessity-and-schema-design.md new file mode 100644 index 00000000000..2c4c0aa8b48 --- /dev/null +++ b/.agents/review-rules/db-migrations/necessity-and-schema-design.md @@ -0,0 +1,41 @@ +# Necessity and schema design + +Applies to: `packages/@n8n/db/src/migrations/**`. + +## Should it exist? + +Flag: + +- a column or table that duplicates one that exists; +- state that application code or a read-time value could hold instead; +- a backfill that lazy computation would avoid, or a denormalised copy with no + read pattern that needs it; +- a drop before expand-contract is complete; +- two unrelated changes in one file; +- an edit to an already-merged migration. Add a new one instead. + +## Is the shape right? + +Use the narrowest type that fits the value: + +- a numeric type for numbers and byte counts, never `varchar`; +- native `uuid`, not `varchar(36)`; +- `timestampTimezone()` or `timestampNoTimezone()`. Plain `.timestamp()` is + deprecated; +- `json` for structured data, `text` for unbounded user strings; +- never `double` for a version field. + +Also flag: + +- a reference column with no foreign key, a foreign key with no `onDelete`, or + `ON DELETE SET NULL` on a `NOT NULL` column; +- a missing primary key, or one unlike the adjacent tables; +- a default that is wrong for existing rows; +- an enum-like string with no CHECK constraint; +- a standalone index on a boolean, or one that repeats a unique constraint. A + partial index with a `WHERE` clause is correct. Do not flag it; +- an ID type that does not match the column it joins to. + +Read the entity in `packages/@n8n/db/src/entities/`. Its type, nullability, +default, `@Index` and FK must agree with the migration. If you cannot read it, say +parity is unverified rather than guess. diff --git a/.agents/review-rules/db-migrations/performance-and-scale.md b/.agents/review-rules/db-migrations/performance-and-scale.md new file mode 100644 index 00000000000..f57fee1366b --- /dev/null +++ b/.agents/review-rules/db-migrations/performance-and-scale.md @@ -0,0 +1,25 @@ +# Performance and scale + +Applies to: `packages/@n8n/db/src/migrations/**`. + +`execution_entity` and `workflow_entity` hold millions of rows. Never assume a +table is small. + +Flag: + +- an unbounded `SELECT` into Node. Use `runInBatches`; +- row-by-row updates where one `UPDATE … FROM` does the work; +- a dataset in memory that grows with the table; +- a table rewrite on a hot table: `ALTER … TYPE`, `SET NOT NULL`, SQLite + recreation; +- an index built on a huge table; +- a `LIKE '%…%'` scan over a JSON column; +- a large blob column added to a hot row. + +Every index slows every write, so an index with no query to serve is a finding. +But grep for the table and column first. Without that grep, say the query pattern +is unverified. + +A nullable column with no backfill is cheap on Postgres. Do not comment on it. +On SQLite it is cheap only when added with raw `ALTER TABLE ADD COLUMN`. The DSL +`addColumns` recreates the whole table. On a hot table, flag that. diff --git a/cubic.yaml b/cubic.yaml index fb02974860c..6548178eef6 100644 --- a/cubic.yaml +++ b/cubic.yaml @@ -156,6 +156,23 @@ reviews: ESLint already fails the build for `@n8n/typeorm` imports in `packages/cli` business logic, uncaught `JSON.parse`, and `JSON.parse(JSON.stringify())` — never spend a comment on those. + - name: DB migrations + file_paths: + - .agents/review-rules/db-migrations/necessity-and-schema-design.md + - .agents/review-rules/db-migrations/data-safety.md + - .agents/review-rules/db-migrations/performance-and-scale.md + - .agents/review-rules/db-migrations/cross-db-compatibility.md + - .agents/review-rules/db-migrations/conventions-and-tests.md + include: + - packages/@n8n/db/src/migrations/** + - packages/cli/test/migration/** + description: |- + Take the linked rules in order: if the migration should not exist, its + shape matters less. + + A migration is permanent. It runs unattended at startup on every + instance, and blocks that instance until it finishes. Read it in full, + plus the entity it touches and the queries that will use any new column. - name: Frontend file_paths: - .agents/skills/design-system/SKILL.md @@ -204,8 +221,8 @@ reviews: What this surface's scanners cannot see: a documented invariant being undone, a guard being widened, a gate that stops being able to fail. - # cubic silently drops any rule past the fifth, so the last slot is a decision, - # not somewhere to append. Merge into an existing agent instead. + # All five slots are now used. cubic silently drops any rule past the fifth, so + # a new domain has to merge into an existing agent, not append to this list. pr_descriptions: generate: false issues: