mirror of
https://github.com/n8n-io/n8n.git
synced 2026-09-24 23:22:38 +08:00
chore: Add cubic review agent for DB migrations (#37418)
This commit is contained in:
Tomi Turtiainen
@@ -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
|
||||
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
+19
-2
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user