From ae55fa7079195b6b6ca149d6bb6c0ec80a495f3c Mon Sep 17 00:00:00 2001 From: Ziad Antar <230683452+ziadantar90@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:34:26 +0000 Subject: [PATCH] A new migration that only adds runs in small mode, with the database tests and one code review --- changelog.d/additive-migrations.md | 1 + scripts/kit-selftest.sh | 95 +++++- template/harness/MODES.md | 18 +- template/harness/templates/small-plan.md | 2 +- template/project/CONTROLS.md | 2 +- template/scripts/risk-check.sh | 371 +++++++++++++++++++++- updates/2026-11-03-additive-migrations.md | 58 ++++ 7 files changed, 535 insertions(+), 12 deletions(-) create mode 100644 changelog.d/additive-migrations.md create mode 100644 updates/2026-11-03-additive-migrations.md diff --git a/changelog.d/additive-migrations.md b/changelog.d/additive-migrations.md new file mode 100644 index 0000000..f13bd7b --- /dev/null +++ b/changelog.d/additive-migrations.md @@ -0,0 +1 @@ +- **A new migration that only adds runs in small mode.** `scripts/risk-check.sh` reads a new `.sql` migration on a database risk path: when every statement only adds (new tables with row-level security, columns, indexes, policies and grants on the new tables), it prints `Mode: plain` with a `Needs:` line. The change then runs in small mode with the database tests and one code review (`harness/MODES.md` § Additive migration). Sign-in, secret, money and permission tables, edits to merged migrations and any other risk path stay full. The kit's self-test covers both sides. diff --git a/scripts/kit-selftest.sh b/scripts/kit-selftest.sh index f7eaf9f..b0729e1 100755 --- a/scripts/kit-selftest.sh +++ b/scripts/kit-selftest.sh @@ -2015,9 +2015,102 @@ def check(name, fx, t): if code or not out.startswith("Mode: full\n") or "scripts/risk-check.sh: the risk check itself" not in out: bad(f"risk-check.sh after an edit to itself printed {out!r} (exit {code})") write(script, saved) + # I2: a new SQL migration that only adds runs plain, with the database tests and one review; anything more is full + git_id = ["-c", "user.name=selftest", "-c", "user.email=selftest@example.com"] + mig = os.path.join(rk, "supabase/migrations") + os.makedirs(mig, exist_ok=True) + os.makedirs(os.path.join(rk, "supabase/tests"), exist_ok=True) + write(os.path.join(mig, "0000_base.sql"), "create table notes (id int);\ncreate function title_ok(t bigint) returns boolean language sql as $$ select true $$;\ncreate policy v on notes for select to authenticated using (\"can_view\"(id));\n") + for cmd in (["add", "supabase"], [*git_id, "commit", "-q", "-m", "base"], ["branch", "-f", integ]): + run(["git", *cmd], rk) + labels = ("-- labels a user puts on notes; only the owner sees them\n" + "create table public.labels (id uuid primary key, user_id uuid not null references auth.users, name text);\n" + "alter table public.labels enable row level security;\n" + "create policy \"own labels\" on public.labels for all to authenticated using (user_id = auth.uid()) with check (user_id = auth.uid());\n" + "grant select, insert on public.labels to authenticated;\n" + "create index labels_user_idx on public.labels (user_id);\n" + "create function public.label_ok(t text) returns boolean language sql immutable as $$ select length(t) < 31; $$;\n") + column = "alter table public.notes add column color text, add constraint notes_color check (color <> '');\ncomment on column public.notes.color is 'a;b';\n" + for mname, body in (("0003_labels.sql", labels), ("0003_color.sql", column)): + write(os.path.join(mig, mname), body) + code, out = risk() + if (code or not out.startswith("Mode: plain (1 file, database changes that only add)\n") + or f"+ supabase/migrations/{mname}: additive migration" not in out or "\nNeeds: the database tests and one code review" not in out): + bad(f"risk-check.sh on a new migration that only adds ({mname}) printed {out!r} (exit {code})") + os.remove(os.path.join(mig, mname)) + write(os.path.join(rk, "supabase/tests/labels.test.sql"), "select 1;\n") + code, out = risk() + if code or not out.startswith("Mode: plain") or "+ supabase/tests/labels.test.sql: database test" not in out: + bad(f"risk-check.sh on a new database test printed {out!r} (exit {code})") + os.remove(os.path.join(rk, "supabase/tests/labels.test.sql")) + for body, why in (("create table labels (id int);", "no row-level security"), + ("create policy p on public.notes for select using (user_id = auth.uid());", "a policy on an existing table"), + ("create table t (id int); alter table t enable row level security; create policy p on t for select using (true);", "an open policy"), + ("create table t (id int); alter table t enable row level security; grant select on t to anon;", "a grant to anon"), + ("alter table notes drop column body;", "a dropped column"), + ("alter table notes rename column body to text;", "a rename"), + ("alter table notes disable row level security;", "row-level security off"), + ("alter table public.users add column bio text;", "a sign-in table"), + ("alter table notes add column api_key text;", "a secret column"), + ("create table public.invoices (id int); alter table public.invoices enable row level security;", "a money table"), + ("create function f() returns int language sql stable security definer as $$ select 1 $$;", "a security definer function"), + ("create or replace function f() returns int language sql stable as $$ select 1 $$;", "a replaced function"), + ("create function g() returns void language plpgsql as $$ begin update notes set id = 1; end $$;", "a function that can write"), + ("update notes set id = 1;", "a data change"), + ("insert into notes values (1);", "new rows"), + # shapes that look additive but aren't (I2 review) + ("create table if not exists notes (id int); alter table notes enable row level security; create policy p on notes for select to authenticated using (1=1);", "if not exists on an existing table"), + ("create table extensions.notes (id int); alter table extensions.notes enable row level security; grant select, update on public.notes to authenticated;", "another schema"), + ('create table "Notes" (id int); alter table "Notes" enable row level security; grant update on notes to authenticated;', "a quoted name"), + ('create table t (id int); alter table t enable row level security; create policy "x on t" on notes for select to authenticated using (1=1);', "a policy name with on"), + ("comment on table notes is E'\\''; update notes set id = 1; comment on table notes is 'z';", "an E string"), + ("create index i$a$ on notes (id); update notes set id = 1; create index j$a$ on notes (id);", "a $ in a name"), + ("create table t (a) as (select id from notes); alter table t enable row level security;", "a copy of a table"), + ("create table t (id int) inherits (notes); alter table t enable row level security;", "inherits"), + ("create function f(stable int) returns int language plpgsql as $$ begin update notes set id = 1; return 1; end $$;", "a plpgsql function"), + ("create function f() returns int language c immutable as 'x', 'y';", "a C function"), + ("alter table notes add column c int default f();", "a default that calls a function"), + ("alter table notes add column c text generated always as (g(id)) stored;", "a generated column"), + ("alter table notes add constraint fk foreign key (id) references other(id) on delete cascade;", "a cascade on an existing table"), + ("create table t (id int); alter table t enable row level security; create policy p on t for select to authenticated using ((true));", "a policy that is always true"), + ("create table t (id int); alter table t enable row level security; create policy p on t for select using (id > 0);", "a policy with no role"), + ("create table t (id int); alter table t enable row level security; grant all on t to authenticated with grant option;", "a grant option"), + ("create trigger x after insert on notes for each row execute function f();", "a trigger"), + ("do $$ begin null; end $$;", "a do block"), + ('create table "no""tes" (id int); alter table notes enable row level security; grant select, update on notes to authenticated;', "a doubled quote in a name"), + ("create function public.g(x int) returns int language sql immutable as $$ select x $$; alter table notes add constraint c check (other.g(id) > 0);", "another schema's function"), + ("create function public.title_ok(x int) returns boolean language sql stable as $$ select true $$;", "an overload of a function the project has"), + ("create function public.can_view(x int) returns boolean language sql stable as $$ select true $$;", "an overload of a function a policy calls quoted"), + ("alter table notes enable row level security;", "row-level security on an existing table"), + ("alter table notes add constraint c check (title ~ 'a\\b');", "a backslash in a string")): + write(os.path.join(mig, "0004_more.sql"), body + "\n") + code, out = risk() + if code or not out.startswith("Mode: full\n") or "- supabase/migrations/0004_more.sql:" not in out or "does more than add" not in out: + bad(f"risk-check.sh on a migration with {why} printed {out!r} (exit {code}): it must stay full") + os.remove(os.path.join(mig, "0004_more.sql")) + write(os.path.join(mig, "0000_base.sql"), "create table notes (id int);\nalter table notes add column x int;\n") + code, out = risk() + if code or not out.startswith("Mode: full\n") or "changes a migration the integration branch already has" not in out: + bad(f"risk-check.sh on an edited migration that is already merged printed {out!r} (exit {code})") + run(["git", "checkout", "-q", "--", "supabase"], rk) + code, out = risk("supabase/migrations/0005_planned.sql") + if code or not out.startswith("Mode: full\n") or "\nNote: supabase/migrations/0005_planned.sql isn't written yet." not in out: + bad(f"risk-check.sh on a planned migration not written yet printed {out!r} (exit {code})") + write(os.path.join(mig, "0003_labels.sql"), labels) + code, out = risk("src/auth/session.ts") + if code or not out.startswith("Mode: full\n") or "- supabase/migrations/0003_labels.sql:" not in out or "\n+ " in out or "Needs:" in out: + bad(f"risk-check.sh on an additive migration with a sign-in file printed {out!r} (exit {code}): the migration is a risk path again") + code, out = risk("--force", "full") + if code or not out.startswith("Mode: full (the owner chose full") or "- supabase/migrations/0003_labels.sql:" not in out or "Needs:" in out: + bad(f"risk-check.sh --force full on an additive migration printed {out!r} (exit {code}): the review must still see it as a risk path") + os.remove(os.path.join(mig, "0003_labels.sql")) + write(os.path.join(mig, "0006_x.test.sql"), "update notes set id = 1;\n") + code, out = risk() + if code or not out.startswith("Mode: full\n") or "+ supabase/migrations/0006_x.test.sql" in out: + bad(f"risk-check.sh on a test-named file among the migrations printed {out!r} (exit {code}): it is applied as a migration") + os.remove(os.path.join(mig, "0006_x.test.sql")) os.makedirs(os.path.join(rk, "supabase/migrations"), exist_ok=True) write(os.path.join(rk, "supabase/migrations/0002_tags.sql"), "create table tags (id int);\n") - git_id = ["-c", "user.name=selftest", "-c", "user.email=selftest@example.com"] for cmd in (["checkout", "-q", "-b", "selftest-risk"], ["add", "supabase"], [*git_id, "commit", "-q", "-m", "tags"]): run(["git", *cmd], rk) code, out = risk() diff --git a/template/harness/MODES.md b/template/harness/MODES.md index ba4ca8d..9a6b794 100644 --- a/template/harness/MODES.md +++ b/template/harness/MODES.md @@ -37,7 +37,7 @@ Working alone, the main chat follows the builder's rules as well as its own: ## Small mode -**Use it when** the risk check prints `Mode: plain` for the files the change will touch and the change alters behaviour (a flag, a field, a format, a message, a sort order, a limit, pagination). The script, not a reading of the request, decides: a change to a risk path is full mode however small, and a change off them is small mode however it is worded. A bug fix off the risk paths is small mode with the steps in § Fixing a bug, and no plan file; on a risk path it is bug mode. +**Use it when** the risk check prints `Mode: plain` for the files the change will touch and the change alters behaviour (a flag, a field, a format, a message, a sort order, a limit, pagination). The script, not a reading of the request, decides: a change to a risk path is full mode however small, and a change off them is small mode however it is worded. A bug fix off the risk paths is small mode with the steps in § Fixing a bug, and no plan file; on a risk path it is bug mode. A new migration that only adds is small mode with the steps in § Additive migration. **Process** @@ -48,6 +48,18 @@ The main chat plans, builds and checks the change itself (§ Working alone); the 3. **Check.** On the final commit, run the `before_signoff` checks in full and fill the plan's § Check: each check with its exit code (`end-of-task hook` for the `checks.on_stop` checks, which run when the turn ends: [CONTROLS.md](./CONTROLS.md) § End-of-task checks), and each criterion with the test or output that shows it holds. A criterion nothing shows is not met: fix it or stop and tell the human. No `progress.md` or `qa-result.md`. 4. **Close.** § After QA pass as in full mode, with § Check in place of the QA verdict. No `retro.md`: the learnings file has the verdict and cost lines and **Retro:** `none (small mode)`, with an action item only when something went wrong. It counts toward the prune counter, so the prune sees whether small mode lets problems through. +### Additive migration + +When the risk check prints `Mode: plain` with a `+ : additive migration` or `+ : database test` line, the change runs in small mode with two more steps (learned: a migration that only added a column sent a whole feature to full mode, the agents' runs cost more than the work, and the risk the script can't rule out is in the code that reads the new column, which a review covers): + +- **Database tests.** The plan's § Acceptance criteria include the database tests: the new tables' row-level security lets the owner in and keeps everyone else out, and each new column or constraint holds. § Check records the project's database check (`project/CONTROLS.md`) with its exit code. With the `data-access` module, stage A runs too ([modules/data-access/](./modules/data-access/README.md)), its rows in § Check. +- **One code review.** Before the hand-off, run `/code-review medium` on the branch and fix or answer each finding; § Check lists them with what was done. When a finding is about who can read or change data, stop: it is full mode, with this plan linked. +- **The owner applies it.** No agent applies a migration to a shared environment; the steps file's **Apply to the integration environment** block names the migration files in order for the owner (`harness/SETUP.md` § After QA pass, step 4). + +A `+` line always means small mode with a plan from [templates/small-plan.md](./templates/small-plan.md), even for a bug fix (not § Fixing a bug's commit record) or a change that alters no behaviour (not light mode). + +The script reads the file against a short allow-list and calls it additive only when it is a new `.sql` file in a risk area named for the database, and every statement has one of these shapes, in the `public` schema: a new table (no `if not exists`, no copy of another table) with row-level security turned on; columns with plain-value defaults, checks, unique keys and foreign keys added to a table; indexes; policies for named roles (never `anon` or `public`, never a condition written as always true such as `true` or `1=1`; the code review reads the rest) and grants (never `with grant option`) on the new tables, and row-level security turned on for them; `stable` or `immutable` SQL functions whose body only selects and whose name no `.sql` file of the integration branch uses yet (a new overload can take over an existing call; the script can't see functions that live only in the database, so the code review checks the names against it); and comments. Expressions may call only plain value functions and the functions the file creates. Tables and columns named for sign-in, secrets, money or permissions stay full, and so does anything it can't read safely (a prefixed string, a backslash in a string, a `$` outside a `$$` body, a doubled quote in a name, another schema). A `.sql` file in a test folder, outside the migrations, counts as a database test. Anything else the change touches on a risk path, or the owner choosing full, makes the database files risk paths again. A migration it can't read yet (named but not written) is full with a note: write it first, then run the check again. When it still prints `Mode: full`, move the draft into the full plan as the proposed migration and delete the file, so Full-stack writes it. + ### Fixing a bug A bug fix whose files the risk check prints `Mode: plain` for runs as small mode, alone, with bug mode's order kept and no plan file. These steps replace § Small mode steps 1 to 4 (learned: a small fix off every risk path cost several times a plain session once Full-stack and QA took it, and still cost far more than one once the main chat wrote a plan, a check record and learnings for a fix it made alone). @@ -106,6 +118,6 @@ No application code changes, no QA, no retro, and research files don't count tow The first match wins. The human's wording ("quick fix", "ship it fast", "no time for QA") never changes the mode; only the owner choosing one does ("use full mode", "run it plain"), passed to the script as `--force full` or `--force plain`, or set for every change in `harness.config.yml` → `mode.force`. 1. **A question, no change asked** ("how does", "why does", "which should we use", "compare") → research mode. A question one or two files the Technical Lead reads anyway can answer gets a direct answer, with no research file. -2. **Something that worked is broken and a fix is asked** ("bug", "broken", "error in production", "this used to work"): read the code path, name the files the fix will touch and run `bash scripts/risk-check.sh `. `Mode: full` → bug mode, and its Full-stack never runs at `medium`. `Mode: plain` → small mode with the bug steps (§ Fixing a bug). A fix that can't wait for the next release is bug mode on a hotfix branch either way. -3. **Any other change:** name the files it will touch and run `bash scripts/risk-check.sh `. `Mode: full` → full mode. `Mode: plain` → light mode when the change alters no behaviour (§ Light mode), small mode otherwise. When the owner forced plain and the script still names a risk path, the change runs alone too, with no QA agent (§ Working alone): forcing plain is the owner's choice to go without the agents, so say so in the plan or the PR record. The Technical Lead may raise plain to full when the request leaves a real product choice open, naming the choice; it never lowers full to plain. The script's output goes in the small plan's § Why small, a bug fix's failing-test commit, or a light change's PR or commit record. +2. **Something that worked is broken and a fix is asked** ("bug", "broken", "error in production", "this used to work"): read the code path, name the files the fix will touch and run `bash scripts/risk-check.sh `. `Mode: full` → bug mode, and its Full-stack never runs at `medium`. `Mode: plain` → small mode with the bug steps (§ Fixing a bug); with a `+ ` line, small mode with a plan (§ Additive migration). A fix that can't wait for the next release is bug mode on a hotfix branch either way. +3. **Any other change:** name the files it will touch and run `bash scripts/risk-check.sh `. `Mode: full` → full mode. `Mode: plain` → light mode when the change alters no behaviour (§ Light mode) and the script printed no `+ ` line, small mode otherwise (§ Additive migration). When the owner forced plain and the script still names a risk path, the change runs alone too, with no QA agent (§ Working alone): forcing plain is the owner's choice to go without the agents, so say so in the plan or the PR record. The Technical Lead may raise plain to full when the request leaves a real product choice open, naming the choice; it never lowers full to plain. The script's output goes in the small plan's § Why small, a bug fix's failing-test commit, or a light change's PR or commit record. 4. **Before the hand-off** of a small or light change, run `bash scripts/risk-check.sh` again with no files: it reads what the branch really changed. When it now prints `Mode: full`, stop and switch to full mode (a plan, with the first one linked, and QA) before anything is handed off; for a bug fix, bug mode, reusing the failing-test commit (§ Working alone). diff --git a/template/harness/templates/small-plan.md b/template/harness/templates/small-plan.md index 80fd317..72be9a4 100644 --- a/template/harness/templates/small-plan.md +++ b/template/harness/templates/small-plan.md @@ -27,7 +27,7 @@ The paths to change and the function in each, read before planning (`file:line`) ## Limit -Stop before the change, and write a full plan with this one linked, when it needs: a file not listed, a risk path (`project/CONTROLS.md` § Risk paths), a change to who can read or change data, to what a customer is charged or where money moves, to how secrets are handled, to deleting or sharing people's data, a schema change, a new dependency, a new page or flow, or behaviour § Acceptance criteria doesn't name. +Stop before the change, and write a full plan with this one linked, when it needs: a file not listed, a risk path (`project/CONTROLS.md` § Risk paths), a change to who can read or change data, to what a customer is charged or where money moves, to how secrets are handled, to deleting or sharing people's data, a schema change beyond the additive migration the risk check named (`harness/MODES.md` § Additive migration), a new dependency, a new page or flow, or behaviour § Acceptance criteria doesn't name. ## QA scope diff --git a/template/project/CONTROLS.md b/template/project/CONTROLS.md index f8208b8..0390cff 100644 --- a/template/project/CONTROLS.md +++ b/template/project/CONTROLS.md @@ -55,7 +55,7 @@ A change to any of these paths can break pages or flows it doesn't name, so it g ## Risk paths -A change to any of these paths always gets full mode (plan, Full-stack and QA), however small: `bash scripts/risk-check.sh` reads this list ([harness/MODES.md](../harness/MODES.md) § Choosing mode). One bullet per risk area, its paths in backticks, `.gitignore` style: a pattern with a slash is from the repo root, one without matches in any folder, `**` crosses folders. Keep each to the code that holds the risky behaviour, not a whole layer: every path listed here sends every change to it to full mode. +A change to any of these paths always gets full mode (plan, Full-stack and QA), however small: `bash scripts/risk-check.sh` reads this list ([harness/MODES.md](../harness/MODES.md) § Choosing mode). One bullet per risk area, its paths in backticks, `.gitignore` style: a pattern with a slash is from the repo root, one without matches in any folder, `**` crosses folders. Keep each to the code that holds the risky behaviour, not a whole layer: every path listed here sends every change to it to full mode. One exception: a new `.sql` migration that only adds, or a database test, in an area named for the database runs in small mode with the database tests and one review ([harness/MODES.md](../harness/MODES.md) § Additive migration). - **Database schema and migrations:** `**/migrations/`, `*.sql`, `schema.prisma` - **Sign-in, sessions and access rules:** `**/auth/`, `auth.*`, `**/session/`, `**/sessions/`, `session.*`, `middleware.*`, `**/policies/` diff --git a/template/scripts/risk-check.sh b/template/scripts/risk-check.sh index 0011311..4b1353a 100755 --- a/template/scripts/risk-check.sh +++ b/template/scripts/risk-check.sh @@ -8,7 +8,12 @@ # and doesn't see an ORM's delete calls); or the branch drops a risk path from that list or # changes this script; or more files change than mode.plain_max_files (documentation and # _harness/ records not counted) -# plain none of these +# plain none of these. A file on a risk path in a database area (its name says database, schema, migration +# or sql, and nothing a risk path guards) counts as plain when it is a new .sql migration whose every +# statement fits the strict allow-list in add_statement() below, or a .sql database test in a test +# folder outside the migrations: printed as `+ : …` lines and a `Needs:` line +# (harness/MODES.md § Additive migration). When anything else makes the change full, or the owner +# forces full, they are risk paths again. # The files: the ones named on the command line (what a plan will change), plus what the branch has # changed so far (its commits since it left the integration branch, uncommitted changes, new files). # The owner's word wins over the paths: --force full|plain, or mode.force in harness.config.yml. @@ -189,12 +194,353 @@ def to_regex(g): return re.compile(r"^" + out + r"(?:/.*)?$", re.I) compiled = [(area, g, to_regex(g)) for area, g in rules] -reasons = [] + +# A database file can count as plain when it only adds (harness/MODES.md § Small mode → Additive migration): +# a new .sql migration whose every statement is on the list below, or a database test in a test folder. Only when +# every risk area the file matches is about the database; anything the script can't read stays full. +# A strict allow-list: each statement must match one of the shapes below in full, or the file is not additive. +# Anything the reader is unsure of (prefixed strings, $ outside a $$ body, schemas other than public, a function +# call it doesn't know) makes the migration full. False "full" answers are fine; false "plain" ones are not. + +class NotAdditive(Exception): + pass + + +def sql_skeleton(text): + """The statements of an SQL file, each as (skeleton, bodies): string literals become 'S', quoted names become + plain names ("notes" is notes, "Notes" becomes q_), $$ bodies become $$Bn$$ with the body kept, and + comments go. Raises NotAdditive on anything it can't read safely.""" + out, cur, bodies, i, n = [], [], [], 0, len(text) + def ident_char(c): + return c.isalnum() or c in "_$" + while i < n: + c = text[i] + if text.startswith("--", i): + j = text.find("\n", i) + i = n if j < 0 else j + continue + if text.startswith("/*", i): + j = text.find("*/", i + 2) + if j < 0 or "/*" in text[i + 2:j]: + raise NotAdditive("a block comment it can't read") + cur.append(" ") + i = j + 2 + continue + if c == "'": + if i and (ident_char(text[i - 1]) or text[i - 1] == "&"): + raise NotAdditive("a prefixed string (E'', U&'' or similar)") + j = i + 1 + while True: + if j >= n: + raise NotAdditive("a string that doesn't end") + if text[j] == "\\": + raise NotAdditive("a backslash in a string") # read as an escape when standard_conforming_strings is off + if text[j] == "'": + if j + 1 < n and text[j + 1] == "'": + j += 2 + continue + break + j += 1 + cur.append("'S'") + i = j + 1 + continue + if c == '"': + j = text.find('"', i + 1) + if j < 0 or text.startswith('""', j): + raise NotAdditive("a quoted name it can't read") + name = text[i + 1:j] + cur.append(name if re.fullmatch(r"[a-z_][a-z0-9_]*", name) else "q_" + name.encode().hex()) + i = j + 1 + continue + if c == "$": + if text.startswith("$$", i) and not (i and ident_char(text[i - 1])): + j = text.find("$$", i + 2) + if j < 0: + raise NotAdditive("a $$ body that doesn't end") + cur.append(f"$$B{len(bodies)}$$") + bodies.append(text[i + 2:j]) + i = j + 2 + continue + raise NotAdditive("a $ outside a $$ body") + if c == ";": + s = " ".join("".join(cur).lower().split()) + if s: + out.append((s, list(bodies))) + cur, bodies = [], [] + i += 1 + continue + cur.append(c) + i += 1 + s = " ".join("".join(cur).lower().split()) + if s: + out.append((s, list(bodies))) + return out + + +IDENT = r"[a-z_][a-z0-9_]*" +QNAME = rf"(?:{IDENT}\.)?{IDENT}" +# Functions an expression may call: plain value functions that read nothing but their arguments (and the +# session's user id), so a default, a check or a policy built from them can't change or copy other data. +SAFE_CALLS = {"length", "char_length", "character_length", "octet_length", "cardinality", "array_length", "lower", + "upper", "btrim", "trim", "ltrim", "rtrim", "coalesce", "nullif", "greatest", "least", "now", + "gen_random_uuid", "uuid_generate_v4", "auth.uid", "auth.jwt", "auth.role", "unnest", "abs", + "round", "floor", "ceil", "date_trunc", "position", "substring", "starts_with", "jsonb_typeof", + "array_position", "to_tsvector", "regexp_like", "string_to_array", "array_to_string", + "regexp_replace", "split_part", "concat"} +KEYWORDS = {"in", "any", "all", "some", "exists", "array", "and", "or", "not", "check", "values", "row", "using", + "with", "key", "unique", "references", "primary", "on", "where", "select", "from", "as", "is", "then", + "else", "when", "case", "filter", "over", "distinct", "between", "like", "ilike", "similar", "default", + "null", "foreign", "include", "constraint"} +TYPE_CALLS = {"varchar", "character varying", "char", "character", "numeric", "decimal", "time", "timestamp", + "timestamptz", "interval", "bit", "varbit", "float"} +SENSITIVE = re.compile(r"(user|account|auth|session|token|password|secret|credential|billing|payment|invoice|" + r"subscription|charge|price|plan|role|permission|member|admin|key|grant|policy|owner)") +LITERAL = r"(?:'s'|-?\d+(?:\.\d+)?|true|false|null|current_timestamp|current_date)(?:\s*::\s*[a-z_ ]+(?:\[\])?)?" + + +def full_name(name): + """public.notes and notes are the same table; another schema is never additive.""" + schema, _, table = name.rpartition(".") + if schema and schema != "public": + raise NotAdditive(f"a table outside the public schema ({name})") + return table + + +def balanced(s): + depth = 0 + for c in s: + depth += (c == "(") - (c == ")") + if depth < 0: + return False + return depth == 0 + + +def calls_ok(expr, funcs, extra=()): + for m in re.finditer(r"([a-z_][a-z0-9_.]*)\s*\(", expr): + f = m.group(1) + if f in KEYWORDS or f in SAFE_CALLS or f in funcs or f in extra: + continue + raise NotAdditive(f"a call to {f}()") + + +def split_top(s): + out, depth, cur = [], 0, [] + for c in s: + depth += (c == "(") - (c == ")") + if c == "," and depth == 0: + out.append("".join(cur).strip()) + cur = [] + continue + cur.append(c) + out.append("".join(cur).strip()) + return [a for a in out if a] + + +def additive(text, others=""): + """None when the migration only adds; otherwise why not, in a few words. others: the project's other SQL, + where a function the migration creates must not be named yet (a new overload can take over an existing call).""" + try: + stmts = sql_skeleton(text) + if not stmts: + return "no SQL statement" + new, rls, funcs = set(), set(), set() + for s, bodies in stmts: + add_statement(s, bodies, new, rls, funcs) + for f in sorted(funcs): + if "." not in f and others is None: + return "can't read the project's other SQL to check the function names" + if "." not in f and re.search(rf"(? 70 else "") + + +def add_statement(s, bodies, new, rls, funcs): + if s in ("begin", "commit", "begin transaction", "start transaction", "commit transaction"): + return + m = re.fullmatch(rf"create table ({QNAME}) \((.*)\)", s) + if m: + table, body = full_name(m.group(1)), m.group(2) + if not balanced(body) or re.search(r"\b(select|like|inherits|partition|including|generated)\b", body): + raise NotAdditive(short(s)) + if SENSITIVE.search(table): + raise NotAdditive(f"a table named for sign-in, secrets, money or permissions ({table})") + calls_ok(body, funcs, TYPE_CALLS) + new.add(table) + return + m = re.fullmatch(rf"alter table (?:only )?({QNAME}) (?:enable|force) row level security", s) + if m: + table = full_name(m.group(1)) + if table not in new: + raise NotAdditive(f"row-level security turned on for a table it doesn't create ({table})") + rls.add(table) + return + m = re.fullmatch(rf"alter table (?:only )?({QNAME}) (add .*)", s) + if m: + table = full_name(m.group(1)) + if SENSITIVE.search(table): + raise NotAdditive(f"a table named for sign-in, secrets, money or permissions ({table})") + for action in split_top(m.group(2)): + add_action(action, table in new, funcs) + return + m = re.fullmatch(rf"create (?:unique )?index (?:concurrently )?(?:if not exists )?{IDENT} on (?:only )?({QNAME})" + rf"(?: using {IDENT})? \((.*)\)(?: where (.*))?", s) + if m: + full_name(m.group(1)) + expr = m.group(2) + " " + (m.group(3) or "") + if not balanced(m.group(2)) or re.search(r"\bselect\b", expr): + raise NotAdditive(short(s)) + calls_ok(expr, funcs) + return + m = re.fullmatch(rf"create policy {IDENT} on ({QNAME})(?: as (?:permissive|restrictive))?" + rf"(?: for (?:all|select|insert|update|delete))? to ({IDENT}(?:, ?{IDENT})*)(.*)", s) + if m: + table, roles, rest = full_name(m.group(1)), {r.strip() for r in m.group(2).split(",")}, m.group(3) + if table not in new: + raise NotAdditive(f"a policy on a table it doesn't create ({table})") + if roles & {"public", "anon"}: + raise NotAdditive("a policy for public or anon") + parts = re.fullmatch(r"(?: using \((.*?)\))?(?: with check \((.*)\))?", rest) + if not parts or not (parts.group(1) or parts.group(2)): + raise NotAdditive(short(s)) + for expr in (parts.group(1), parts.group(2)): + if expr is None: + continue + if not balanced(expr) or re.sub(r"[()\s]", "", expr) in ("true", "1=1", "'s'='s'", "notfalse"): + raise NotAdditive("a policy that lets every row through") + calls_ok(expr, funcs) + return + m = re.fullmatch(rf"grant ((?:select|insert|update|delete|all(?: privileges)?)(?:, ?(?:select|insert|update|delete))*)" + rf" on (?:table )?({QNAME}) to ({IDENT}(?:, ?{IDENT})*)", s) + if m: + if full_name(m.group(2)) not in new: + raise NotAdditive(f"a grant on a table it doesn't create ({m.group(2)})") + if {r.strip() for r in m.group(3).split(",")} & {"public", "anon"}: + raise NotAdditive("a grant to public or anon") + return + m = re.fullmatch(rf"create function ({QNAME}) ?\(([^()]*)\) returns ([a-z_ \[\]]+?) (.*?)as \$\$b(\d+)\$\$(.*)", s) + if m: + name = m.group(1) + if SENSITIVE.search(full_name(name)): + raise NotAdditive(f"a function named for sign-in, secrets, money or permissions ({name})") + header = (m.group(4) + " " + m.group(6)).split() + allowed = {"language", "sql", "immutable", "stable", "strict", "parallel", "safe", "set", "search_path", + "=", "to", "'s'", "public", "returns", "null", "on", "input", "called"} + words = set(header) + if not words <= allowed or "sql" not in words or not words & {"immutable", "stable"}: + raise NotAdditive("a function that isn't a stable or immutable SQL function") + body = bodies[int(m.group(5))] + clean = " ".join(re.sub(r"'(?:[^']|'')*'", "'s'", body.lower()).split()) + if not clean.startswith("select ") or ";" in clean.rstrip("; ") or re.search( + r"\b(insert|update|delete|merge|copy|call|truncate|drop|alter|grant|revoke|create|set|perform|execute|do)\b", clean): + raise NotAdditive("a function body that does more than select") + calls_ok(clean, funcs) + funcs.add(full_name(name)) # called as f() or public.f(), never another schema's f() + funcs.add("public." + full_name(name)) + return + if re.fullmatch(rf"comment on (?:table|column) {QNAME}(?:\.{IDENT})? is 's'", s): + return + raise NotAdditive(short(s)) + + +def add_action(a, on_new, funcs): + m = re.fullmatch(rf"add (?:column )?(?:if not exists )?({IDENT}) (.*)", a) + if m and m.group(1) not in ("constraint", "primary", "unique", "foreign", "check"): + col, rest = m.group(1), m.group(2) + if SENSITIVE.search(col): + raise NotAdditive(f"a column named for sign-in, secrets, money or permissions ({col})") + if re.search(r"\b(generated|select|collate)\b", rest) or re.search(r"\bon (delete|update)\b", rest): + raise NotAdditive(short(a)) + for d in re.finditer(r"\bdefault\s+", rest): + if not re.match(LITERAL, rest[d.end():]): + raise NotAdditive("a default that isn't a plain value") + calls_ok(re.sub(r"\bdefault\s+" + LITERAL, "", rest), funcs, TYPE_CALLS) + return + m = re.fullmatch(rf"add constraint {IDENT} check \((.*)\)", a) + if m: + if not balanced(m.group(1)) or re.search(r"\bselect\b", m.group(1)): + raise NotAdditive(short(a)) + calls_ok(m.group(1), funcs) + return + if re.fullmatch(rf"add constraint {IDENT} unique \({IDENT}(?:, ?{IDENT})*\)", a): + return + m = re.fullmatch(rf"add constraint {IDENT} foreign key \({IDENT}(?:, ?{IDENT})*\) references ({QNAME})" + rf"(?: ?\({IDENT}(?:, ?{IDENT})*\))?(.*)", a) + if m: + if m.group(2).strip() and (not on_new or not re.fullmatch(r"( on delete (cascade|restrict|no action|set null))?", m.group(2))): + raise NotAdditive(short(a)) + return + raise NotAdditive(short(a)) + +# An area counts as the database's when its name says so and names nothing else a risk path guards. +DB_AREA = re.compile(r"database|schema|migration|\bsql\b", re.I) +NOT_DB_AREA = re.compile(r"auth|sign|session|access|billing|money|payment|secret|permission|policy|polic|tenan", re.I) +# A database test lives in a test folder, never next to the migrations (a .sql file there is applied). +SQL_TEST = re.compile(r"(^|/)(tests?|spec)/[^ ]*\.sql$", re.I) + +def other_sql(p): + """The project's SQL as the integration branch has it, as one text: what existed before this change.""" + if base is None or base == EMPTY_TREE: + return "" + listing = git("ls-tree", "-r", "-z", "--name-only", base) + if listing is None: + return None + out = [] + for q in listing.split("\0"): + if q.lower().endswith(".sql") and q != p: + text = git("show", f"{base}:{q}") + if text is None: + return None + out.append(text) + return "\n".join(out) + +def is_new(p): + """Not in the integration branch yet: the branch adds it, or it is still untracked.""" + if base == EMPTY_TREE: + return True + if base is None: + return False # no git history to compare with: never counted as new + return git("cat-file", "-e", f"{base}:{p}") is None + +reasons, additions, unwritten = [], [], [] for p in files: - for area, g, rx in compiled: - if rx.match(p): - reasons.append(f"- {p}: {area} (`{g}`)") - break + hits = [(area, g) for area, g, rx in compiled if rx.match(p)] + if not hits: + continue + reason = f"- {p}: {hits[0][0]} (`{hits[0][1]}`)" + if not p.lower().endswith(".sql") or not all(DB_AREA.search(area) and not NOT_DB_AREA.search(area) for area, _ in hits): + reasons.append(reason) + continue + if SQL_TEST.search(p) and "migration" not in p.lower(): + additions.append((reason, f"+ {p}: database test")) + continue + if not os.path.isfile(p): + if p in changed: + reasons.append(reason + ": the branch deletes it") + else: + unwritten.append(p) + reasons.append(reason) + continue + if not is_new(p): + reasons.append(reason + ": changes a migration the integration branch already has") + continue + try: + with open(p, encoding="utf-8-sig", errors="replace") as fh: + more = additive(fh.read(), other_sql(p)) + except OSError: + more = "can't read it" + if more: + reasons.append(reason + f": does more than add ({more})") + else: + additions.append((reason, f"+ {p}: additive migration (new tables with row-level security, new columns)")) DELETES = re.compile(r"\b(drop\s+(table|column|schema|database)\b|truncate\s+(table\s+)?[\w\"`.]+|delete\s+from\b" r"|alter\s+table\b.*\bdrop\b)", re.I) @@ -221,16 +567,29 @@ if len(counted) > max_files: reasons.append(f"- {len(counted)} files change, more than mode.plain_max_files ({max_files}), documentation not counted") auto = "full" if reasons else "plain" +if auto == "full" or force == "full": + # anything else already needs full mode, or the owner chose it: the database files are risk paths again + reasons = [r for r, _ in additions] + reasons + additions = [] if force and force != auto: print(f"Mode: {force} (the owner chose {force}; without that: {auto})") elif force: print(f"Mode: {force} (the owner chose {force})") elif auto == "plain" and not counted: print("Mode: plain (documentation only)") +elif auto == "plain" and additions: + print(f"Mode: plain ({len(counted)} file{'s' if len(counted) != 1 else ''}, database changes that only add)") elif auto == "plain": print(f"Mode: plain ({len(counted)} file{'s' if len(counted) != 1 else ''}, none on a risk path)") else: print("Mode: full") for r in reasons: print(r) +for _, line in additions: + print(line) +if additions: + print("Needs: the database tests and one code review before the hand-off (harness/MODES.md § Small mode)") +if unwritten and auto == "full": + print("Note: " + ", ".join(unwritten) + " isn't written yet. A new migration that only adds tables or columns with " + "row-level security can run in small mode: write it first, then run this again (harness/MODES.md § Choosing mode)") PY diff --git a/updates/2026-11-03-additive-migrations.md b/updates/2026-11-03-additive-migrations.md new file mode 100644 index 0000000..3437e68 --- /dev/null +++ b/updates/2026-11-03-additive-migrations.md @@ -0,0 +1,58 @@ +--- +id: 2026-11-03-additive-migrations +title: A new migration that only adds runs in small mode, with the database tests and one code review +kind: template +release: unreleased +applies_to: { modules: [] } +files: + - harness/MODES.md + - harness/templates/small-plan.md + - project/CONTROLS.md + - scripts/risk-check.sh +removed: [] +reviewed: "It changes scripts/risk-check.sh, which picks the mode. A new .sql migration in a risk area named for the database now prints Mode: plain, with + lines and a Needs: line, when every statement fits a strict allow-list in the public schema: a new table with row-level security on, added columns with plain-value defaults, checks, unique and foreign keys, indexes, policies for named roles and grants on the new tables, stable or immutable SQL functions that only select and whose names no .sql file of the integration branch uses yet, and comments. A .sql database test in a test folder counts too. Tables and columns named for sign-in, secrets, money or permissions, anything it cannot read safely (a doubled quote in a name, a backslash in a string, another schema), row-level security changes on existing tables, a failed read of the integration branch, edits to merged migrations, unwritten migrations, the owner forcing full and any other risk path stay full. No guard rule, permission or config value changes." +--- + +## What changed + +- **`scripts/risk-check.sh`** reads a database file on a risk path before it calls the change full. + - It counts as plain when the file matches only risk areas named for the database, and it is either: + - a new `.sql` migration whose every statement only adds, or + - a `.sql` database test in a test folder, outside the migrations. + - "Only adds" is a strict allow-list, in the `public` schema: + - a new table, with no `if not exists` and no copy of another table, with row-level security turned on (turning it on or forcing it counts only for the new tables); + - columns with plain-value defaults, checks, unique keys and foreign keys added to a table; + - indexes; + - policies for named roles, and grants, on the new tables (never to `anon` or `public`, never a condition that lets every row through, never `with grant option`); + - `stable` or `immutable` SQL functions whose body only selects, and whose name no `.sql` file of the integration branch uses yet (a new overload can take over an existing call; functions that live only in the database are left to the code review); + - comments. + + Expressions may call only plain value functions and the functions the file creates. Anything it can't read safely is full: a prefixed string, a backslash in a string, a doubled quote in a name, a `$` outside a `$$` body, another schema, or an integration branch it can't read. + - The output keeps `Mode: plain` on the first line, adds one `+ : …` line per such file, and ends with a `Needs:` line asking for the database tests and one code review. + - These stay full: + - tables or columns named for sign-in, secrets, money or permissions; + - an edit to a migration the integration branch already has; + - a migration that is named but not written yet (with a `Note:` line); + - any change that is full for another reason, or that the owner forces full. Then the database files are risk paths again. +- **`harness/MODES.md` § Additive migration** (new, under § Small mode): the change runs in small mode, with three additions: + - the database tests in the acceptance criteria and in § Check; + - `/code-review medium` before the hand-off; + - the owner applying the migration, through the steps file. + + It also says how to handle a migration not written yet. A `+` line always means small mode with a plan, even for a bug fix or a change that alters no behaviour, and § Choosing mode says so. +- **`harness/templates/small-plan.md` § Limit** allows the additive migration the risk check named. +- **`project/CONTROLS.md` § Risk paths** names the exception. + +## Why + +Every migration was a risk path, so a feature that only added a column ran the full process with its agents, and that cost more than the work. The risk the script can't rule out is in the code that uses the new column, which the review reads. + +## How to apply + +1. Replace `scripts/risk-check.sh` with the kit's version. It has no project-specific lines. +2. Add § Additive migration to `harness/MODES.md`, before § Fixing a bug, and merge the § Small mode sentence that names it. +3. Merge the § Limit sentence in `harness/templates/small-plan.md` and the § Risk paths sentence in `project/CONTROLS.md`. Keep this project's paths and area names. The exception only applies to areas whose name says database, schema, migration or sql. + +## Skip if + +The project has no SQL migrations: the change does nothing there, but apply it anyway so the risk check stays the kit's.