Skip to content

fix(migrate-ts): let Postgres canonicalise expected CHECK and index expressions - #506

Merged
dmealing merged 3 commits into
mainfrom
fm/mo-check-pg-canon
Oct 29, 2026
Merged

dmealing merged 3 commits into
mainfrom
fm/mo-check-pg-canon

Conversation

@dmealing

@dmealing dmealing commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

What Changed

  • On Postgres, verify --db, migrate --from-db and migrate --apply now ask the server how it stores each expected CHECK and index @expr/@where. The probe runs in a rolled-back transaction (SAVEPOINT when already in one) against a TEMP table, then compares the stored strings. Fallback to the string normaliser on any probe failure, on SQLite/D1, and offline. An expression containing a ; anywhere, or an index @using that is not a plain identifier, is never sent to the server; a 42501 (privilege) answer disables the probe for that table only, and only a read-only transaction (25006) disables it for the run.
  • migrate baseline --from-db writes the generated expression text into the snapshot where the probe proves it equal to the live one, so the offline plan compares like with like. Unchanged when the metadata does not load.
  • check-expr-compare.ts header and the docs (CHANGELOG.md, docs/features/migrations-and-drift.md) updated to describe the probe and its limits.
Validation

Risk Assessment

✅ Low: Probe is Postgres-only, falls back to the old normaliser on every failure, runs in a rolled-back transaction or savepoint, and the round-1 fixes (any ; skips the probe, @using allowlist, 25006-only run-wide disable) hold under tracing; tests are behavioural against a live engine.

Testing

The Test agent exceeded its invocation budget before its live-evidence turn finished. Test exception approved by the operator: gate baseline green; live-PG suite run by hand at 4f0d040: 961 passed (0 failed, 0 skipped) against a disposable Postgres 16, including the expression-canon probe, false-negative, read-only fallback and round-trip tests. The final commit on top of 4f0d040 changes docs and CHANGELOG wording only.

  • Outcome: ⚠️ 1 warning across 1 run (1h43m18s)

Pipeline

Updates from git push no-mistakes

✅ **Intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 2 issues found → auto-fixed ✅
  • ⚠️ server/typescript/packages/migrate-ts/src/expr-canon-probe.ts:32 - The statement-separator guard models only '...' and "..." quoting, while Postgres's lexer also recognises E'' escape strings, dollar quotes and comments. So the invariant "an expression containing a statement separator outside quotes is never sent to the server" does not hold. Example: the CHECK a = E'\''; COMMIT; DROP TABLE t; --'. The scanner pairs \'' as a doubled quote and sees every ; as inside a literal. Postgres closes the literal after E'\'' and runs COMMIT, which ends the rolled-back transaction, and then runs DROP TABLE in autocommit. Sending raw SQL with no parameters uses the simple query protocol, which runs several statements. Other ways past the guard: $a$'$a$; ...; ' and -- '\n; .... Also, ix.using is interpolated raw into CREATE INDEX at line 105 and is never scanned. @using is a free-form, trimmed metadata string (expected-schema.ts:674/715), and committed-snapshot JSON reaches the probe through verify's checkCommittedSnapshot. Both inputs come from repo-controlled metadata or snapshots, which verify --db reads, often in CI against a live database. Simple fix: do not probe any expression that contains ; anywhere; it falls back to the string normaliser. Apply the same rule to ix.using, or accept only an identifier matching ^[A-Za-z_][A-Za-z0-9_]*$.
  • ℹ️ server/typescript/packages/migrate-ts/src/expr-canon-probe.ts:88 - SQLSTATE 42501 sets the run-wide unavailable flag. That code means not only "no TEMP privilege": CREATE TEMPORARY TABLE ... (LIKE <live>) also raises 42501 when the role lacks SELECT on that one live table or USAGE on its schema. verify --db shares one probe across the whole run (verify.ts creates it once). So one unreadable table turns the probe off for every later table, and those tables silently go back to the normaliser, including its precedence/NOT false negatives. The fallback is allowed, but the outcome is wider than the cause. Possible fix: set unavailable only on 25006, or only when a privilege-free probe such as has_database_privilege(current_database(), 'TEMP') says false. Otherwise treat 42501 per table.

🔧 Fix applied.
✅ Re-checked - no issues remain.

⚠️ **Test** - 1 warning
  • ⚠️ The Test agent did not finish within its invocation budget. Reported: agent run tests timed out at its 1h30m0s still-working cap (silent budget 1h15m0s; ran 1h30m0s): agent last produced output 23m19s ago (3 observed); agent reported: claude exited: signal: terminated: glm-claude: step=test route=light glm-claude: trying haiku55 (effort low, thinking off)... glm-claude: haiku55 failed (timed out after 2000s, partial stream discarded) glm-claude: haiku55 failed — failing over glm-claude: trying sonnet55 (effort high)... glm-claude: sonnet55 failed (timed out after 2000s, partial stream discarded) glm-claude: sonnet55 failed — fa.... This is a budget or provider-slowness cut, not a code failure. Re-running the same request costs another full budget, so no further attempt is made automatically. If this repository's targeted tests or evidence gathering routinely stay quiet longer than the default 30m0s, raise test_agent_timeout in global config. If the turn was still producing output or running a child process, set or raise test_agent_working_timeout there instead: a turn runs past test_agent_timeout only up to that still-working cap. Respond with fix to spend another budget: a repair turn runs only for selected findings other than this budget cut, then validation re-runs. Or abort and retry after raising the budget.
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

…K and index-expression comparisons

On Postgres, `meta verify --db` and `meta migrate` now ask the database whether an expected
CHECK or index @expr/@where is the one stored: the expression is added to a rolled-back
scratch table (a savepoint when the caller is already in a transaction), Postgres's own
pg_get_constraintdef / pg_get_indexdef / pg_get_expr is read back, and the result is compared
with the live definition as a plain string. Postgres-shaped rewrites stop accumulating in the
string normaliser, and its verified false negatives (precedence, NOT grouping) report as drift.

The normaliser stays for SQLite/D1, the offline plan, read-only connections, roles without
TEMP, and expressions that do not compile against the live columns. A probe failure never
reports drift or throws. `baseline --from-db` records the generated text for an expression the
probe proves equal, so the offline plan compares like with like.
…e. CHANGELOG and migrations-and-drift.md now say "anywhere" for `;`, document `@using` refusal, and per-table 42501 fallback. Overlong lines reflowed. No unresolved gaps.
@dmealing
dmealing merged commit 2433746 into main Oct 29, 2026
1 check passed
@dmealing
dmealing deleted the fm/mo-check-pg-canon branch October 29, 2026 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant