db:apply: fix the two preflight checks that made every dry run fail, and stage the held September migrations - #1903
Open
njrini99-code wants to merge 1 commit into
Open
njrini99-code wants to merge 1 commit into
njrini99-code wants to merge 1 commit into
Conversation
Every dry run against the September migrations failed at preflight, and
neither failure was about the migrations.
(a) The origin/main reachability check shelled out to
`git log origin/main --name-only --pretty=format:`. That emits every
path of every commit — 2.28 MB in this repo — against execFileSync's
1 MB default maxBuffer, so it threw ENOBUFS on every invocation and
the catch turned that into a permanent FAIL. Fail-closed, but a check
that cannot pass for any file verifies nothing. Replaced with a
scoped `git rev-list -1 origin/main -- <path>`, bounded by one commit
id no matter how large the repo gets. Raising maxBuffer would only
move the cliff.
(b) The HELD.md check matched a literal `**HOLD**` anchored to a `|`
immediately before the basename. Real register rows carry qualified
statuses (`**HOLD — R3, not yet reviewed**`) and group several files
into one row (`A.sql + B.sql + C.sql`), so merged-but-held migrations
passed the hold gate. That is the dangerous direction — it lets an
unreviewed privileged migration through. `isHeldInRegister` now parses
table rows cell by cell, splits the name cell on `+`/`,` and matches
whole filename tokens, anchoring the status to the START of the status
cell so `hold discharged` is not a false positive.
Fixing (a) made a third defect reachable for the first time:
`extractVerifyQueries` treated each `-- VERIFY:` line as its own query, so
a multi-line assertion degraded into a bare unfiltered SELECT that returns
rows against any database. Continuation lines are now joined until `;`.
APPLY_PATH.md and check-migration-headers.mjs updated to match.
Also stages `-- VERIFY:` (and `-- ROLLBACK:` where missing) headers on the
nine held helm_debug/helm_jobs/pgaudit migrations, so `--apply` can verify
itself. Every assertion was executed read-only against production first:
all are valid SQL and all discriminators return 0 today, which is the
correct pre-apply answer. Signature checks use `::regprocedure` rather
than `pg_get_function_identity_arguments`, which returns parameter names
alongside types. Header ratchet re-baselined downward.
These files stay HELD. This change makes them applyable and
self-verifying; it does not clear them to apply. Eight of the nine are R3
and have not been through db-migration-reviewer, `--apply` is owner-only
per docs/operations/APPLY_PATH.md, and `supabase link` has never been run
in this checkout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Every
npm run db:applydry run against the September migrations failed at preflight. Neither failure was about the migrations.(a) The
origin/mainreachability check could never pass — for any file.It shelled out to
git log origin/main --name-only --pretty=format:, which emits every path of every commit: 2.28 MB in this repo, againstexecFileSync's 1 MB defaultmaxBuffer. It threwENOBUFSon every invocation and thecatchturned that intoFAIL. Fail-closed, but a check that cannot pass verifies nothing.Replaced with a scoped
git rev-list -1 origin/main -- <path>— bounded by one commit id regardless of repo size. RaisingmaxBufferwould only move the cliff.(b) The HELD.md gate let held migrations through.
It matched a literal
**HOLD**anchored to a|immediately before the basename. Real register rows carry qualified statuses (**HOLD — R3, not yet reviewed**) and group files into one row (A.sql + B.sql + C.sql), so merged-but-held migrations passed the gate. That is the dangerous direction — it admits an unreviewed privileged migration.isHeldInRegisternow parses rows cell by cell, splits the name cell on+/,, matches whole filename tokens, and anchors the status to the start of the status cell so**APPLIED … — hold discharged**is not a false positive.(c) A third defect that (a) had been masking.
extractVerifyQueriestreated each-- VERIFY:line as a separate query, so a multi-line assertion degraded into a bare unfilteredselect 1 from information_schema.columns— which returns rows against any database at all. Continuation lines are now joined until;.APPLY_PATH.mdandcheck-migration-headers.mjsupdated to match.Also staged
-- VERIFY:(and-- ROLLBACK:where missing) headers on the heldhelm_debug/helm_jobs/pgauditmigrations, so--applycan verify itself.Every assertion was extracted with the shipped
extractVerifyQueriesand executed read-only against production as its literal string:::regprocedure, notpg_get_function_identity_arguments, which returns parameter names alongside types and would have failed post-apply.has_function_privilege(role, 'schema.fn(sig)', 'execute')form was shape-checked against functions that exist today (single- and multi-argument), confirming the role names and the= falsecomparison.Two assertions were rewritten after that pass caught them asserting nothing:
20260906115900— the signature already exists in production, so both existence checks pass before and after. Added aprosrc like '%min_exec_ms%'body discriminator.20260906142000—pg_db_role_setting.setconfigquotes values Postgres considers to need it (search_path="$user", publicsits in this very table), so anilikeon the raw spelling would misspgaudit.log="ddl, role". Now a whitespace- and quote-tolerant regex.Header ratchet re-baselined downward.
What this does NOT do
These migrations stay HELD. This makes them applyable and self-verifying; it does not clear them to apply. Three independent owner-only prerequisites remain:
db-migration-reviewer, whichAPPLY_PATH.mdstep 4 makes mandatory. A grep scan is not that review.--applyis blocked for agents bypermissions.deny;APPLY_PATH.mdstep 8: "Only the owner runs--apply."supabase linkhas never been run in this checkout, so the ledger check cannot run.Test plan
scripts/__tests__/db-apply-preflight.test.mjs— 33 tests, registered explicitly invitest.config.ts(that directory has no glob, so an unlisted file is decorative). Includes the pre-fix regex kept as a regression witness, ahold dischargedfalse-positive test, whole-token matching, and a source-level assertion that the unboundedgit logis gone.npm run typecheck,npm run lint,npm run check:migration-headers— pass.Unrelated, pre-existing:
scripts/__tests__/create-workspace.test.tsfails 13/16 onmainas well — the machine is at 11 GiB free againstcreateWorkspace's 12 GiB reserve, so it refuses. Not touched here.🤖 Generated with Claude Code