fix(migrate-ts): refuse dropping a live serial default when @generation is undeclared - #282
Merged
Merged
Conversation
… is undeclared PR #279 stopped `meta migrate` from emitting a destructive `ALTER COLUMN ... DROP DEFAULT` against a live Postgres `serial` PK, but the guard only fires when the metadata explicitly declares `identity.primary @generation: increment`. An adopter who omits `@generation` still hit the same destructive drop, since `ec.identity` is undefined and the #279 guard never engages. We do not widen that guard to key off the live column instead ("if it's serial, never touch the default") — an undeclared `@generation` is genuinely ambiguous between "never declared it" and "deliberately removing auto-increment" (e.g. moving to app-assigned ULIDs). The diff cannot tell those apart, so `blockedReasonFor` now refuses this one narrow shape instead: `change-column-default` is blocked only when the change drops the default (`to === undefined`) and the dropped value is a live Postgres auto-sequence default (reusing `isPgAutoSequenceDefault`, not a second regex). The blocked reason names the table, the column, and both remedies. A new `--allow drop-identity-default` (`AllowOptions.dropIdentityDefault`) lets the deliberate-removal path through. Same shape as #258 (refuse a PK move rather than emit an un-appliable migration). Ordinary default changes (literal-to-literal, dropping a plain literal default, dropping a non-sequence expr default) are unaffected, as is the existing #279 explicit-@generation no-op path and the uuid-identity path. Gated by a diff-level unit suite plus a real-Postgres integration test (create a live SERIAL PRIMARY KEY table, diff against metadata declaring identity.primary with no @generation, assert blocked, then assert the allow-flag escape both diffs allowed and applies).
…the lists sdk's AllowTokenEnum (validates the static migrate.allow array in .metaobjects/config.json) had drifted from the CLI's authoritative ALLOW_TOKENS (validates --allow <csv>) — it carried only 6 of 11 tokens, missing drop-check, drop-view, drop-view-cascade, adopt-view, and the drop-identity-default just added. A user who set any of those five in migrate.allow got a schema rejection for a flag that worked fine on the CLI; adopt-view had been affected since it shipped. - sdk/src/config.ts: AllowTokenEnum now carries the full 11 tokens and is exported (re-exported from sdk's index.ts) so the drift guard below can read it. - cli/src/lib/args.ts: ALLOW_TOKENS exported for the same reason. - cli/README.md: --allow CSV in the migrate docs synced to the same 11. - cli/test/unit/allow-tokens-pinned.test.ts: new drift guard — imports both lists (no third hardcoded copy) and asserts they're the same set. sdk cannot import cli (dependency runs cli -> sdk), so the test lives in cli, the importable direction; sdk's AllowTokenEnum carries a doc comment pointing back at it. Verified this test actually fails (not just green by construction) by temporarily dropping a token from one side.
allow-tokens-pinned.test.ts covered ALLOW_TOKENS (the --allow validator) and sdk's AllowTokenEnum, but not ALLOW_TOKEN_MAP (lib/allow.ts) — the structure that actually grants a validated token's permission. A token present in ALLOW_TOKENS but missing from the map would pass validation cleanly and tokensToAllowOptions would silently grant nothing for it: --allow <token> looks accepted but authorizes no destructive drop, and the diff blocks it anyway with no indication the flag was ever a no-op. Same drift class that left adopt-view broken in sdk's config schema since 0.20.4, but worse here because there's no error to notice. - cli/src/lib/allow.ts: export ALLOW_TOKEN_MAP (was module-local) so the test can import the real structure instead of a hardcoded copy. - allow-tokens-pinned.test.ts: two new assertions — ALLOW_TOKEN_MAP's key set matches ALLOW_TOKENS in both directions, and its AllowOptions values are unique (no two tokens silently grant the same permission). Verified non-vacuous: removing an entry from ALLOW_TOKEN_MAP makes the new assertion fail (confirmed with adopt-view specifically); restoring it goes green again.
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.
Intent
Close the remaining destructive path in the legacy-Postgres-serial-PK adoption fix, by REFUSING rather than guessing — plus fix a pre-existing cross-package drift the work exposed.
THE GAP. PR #279 stopped
meta migrateemittingALTER COLUMN "id" DROP DEFAULTagainst a live legacy Postgresserialprimary key during adoption (destructive: every insert that omits the id starts failing). But that guard fires only when the metadata explicitly declares@generation: increment, because expected-schema.ts setsidentityonly for an explicit declaration — there is no default. So an adopter who writesidentity.primarywith@fieldsbut no@generationstill got the destructive drop. Same bug, different door.WHY REFUSE INSTEAD OF WIDENING THE GUARD — this is the design decision, made by the maintainer after I presented three options. Keying the guard on the ACTUAL side ("if the live column is serial, never touch its default") would silently refuse a DELIBERATE migration off auto-increment: someone moving to app-assigned ULIDs drops
@generationprecisely because they want the default gone. The two readings of an undeclared@generation— "I never declared it" vs "I want increment removed" — are genuinely ambiguous, and no amount of cleverness lets the code distinguish them. Option A (leave it) keeps a destructive path reachable; option B (key on the actual side, gate dropping behind a flag) guesses in the adopter's favor and silently blocks deliberate removal until they find the flag. Option C — detect and refuse, naming both remedies — is the only one that does not guess. It has direct precedent: #258 mademeta migraterefuse a primary-key move with a clear error rather than emit an un-appliable migration.IMPLEMENTATION. Uses the existing blocking mechanism rather than inventing one:
blockedReasonForin diff/status.ts already gates destructive changes and setsstate: "blocked", andDiffResult.blockedis already surfaced for CLI error messaging.change-column-defaultpreviously sat in the always-allowed group; it now has its own case that blocks EXACTLY ONE shape —to === undefined(the default is being dropped outright) ANDfrom.kind === "expr"AND the value satisfiesisPgAutoSequenceDefault, the predicate already shared between the introspector and diff/index.ts (deliberately reused; a second nextval regex would defeat the point of extracting it). Gated by a new--allow drop-identity-default.THE NARROWNESS IS PROVABLE, not merely tested: an expected column carrying
identity: "increment"can NEVER reach this branch, because diff/index.ts's skipIdentityDefaultDiff already suppressed the change for that exact live shape. So an undeclared@generationis the only way it fires. Ordinary default changes — literal→literal, dropping a plain literal default, uuid identities — fall through to the unconditional allow, pinned by no-churn tests.THE ERROR MESSAGE IS THE FEATURE. It names the table, the column, the actual live default expression, states the ambiguity explicitly, states the consequence (every insert that omits the column starts failing), and gives BOTH remedies: declare
@generation: incrementto keep the sequence, or pass--allow drop-identity-defaultif removal is intentional. Written for a stranger mid-adoption.PRE-EXISTING BUG FOUND AND FIXED. The implementer reported, rather than silently expanding past, that the new flag was only half-wired. Verifying that claim showed something bigger: there are THREE
--allowtoken lists and they had drifted badly. The authoritative CLI validator carries 11 tokens; sdk'sAllowTokenEnum(which validates the STATICmigrate.allowarray in.metaobjects/config.json) had only 6, and cli/README.md had 8. Consequence, independent of this change:adopt-viewhas SHIPPED SINCE 0.20.4 and was rejected by the config schema the entire time. All three lists are now synced, and — the durable part — a new test pins the CLI and sdk lists to the same set by IMPORTING BOTH rather than hardcoding a third expected copy (which would just become a fourth thing to drift), checking both directions. Verified non-vacuous: removing a token from one side makes it genuinely fail. It lives inclibecauseclidepends onsdkone-way and the reverse would cycle; sdk's enum carries a doc comment pointing at it.VERIFICATION. Real Postgres 16 in a throwaway container (torn down; pre-existing containers untouched): the new integration test creates a real
id SERIAL PRIMARY KEY, runs the diff with metadata declaring identity.primary WITHOUT@generation, and asserts the result is BLOCKED rather than emitting a DROP DEFAULT — then asserts the allow flag DOES let it through, so the deliberate-removal path still works. That escape half is what makes the design defensible rather than just a wall. Unit suites: migrate-ts 695 pass / 21 skip, cli 439 pass / 2 skip, sdk 150 pass, whole-workspace build + typecheck clean.Scope: migrate-ts (status.ts, types.ts) + cli wiring/help/README + sdk config enum + tests. No metamodel vocabulary, no other port — schema is TypeScript-owned per ADR-0015. The #279 explicit-@generation path is untouched and byte-identical.
What Changed
meta migratenow refuses (rather than guessing) when an undeclared@generationwould drop a live legacy Postgresserialprimary-key default — an ambiguous case (never-declared vs. deliberate removal) that previously emitted a destructiveALTER COLUMN ... DROP DEFAULT. A new--allow drop-identity-defaultflag is the escape hatch for intentional removal.--allowtoken lists — the CLI validator, sdkAllowTokenEnum(which validates the static.metaobjects/config.json), and the CLI README — to the full 11 tokens;adopt-view(shipped since 0.20.4) had been rejected by the config schema. A new test imports both code lists to pin them against future drift.Risk Assessment
✅ Low: Well-bounded, thoroughly tested change: the destructive-gap block is provably narrow (skipIdentityDefaultDiff guarantees an increment-identity expected column can never reach it), is enforced end-to-end (emit() throws on blocked changes; integration test confirms against real Postgres SERIAL including the allow-flag escape hatch), and the sdk AllowTokenEnum sync fixes a real pre-existing bug with a non-vacuous bidirectional pinning test.
Testing
Against a real Postgres 16 throwaway container I confirmed the refusal works end-to-end (real SERIAL PK → blocked with a reason naming both remedies → emit throws; --allow drop-identity-default → DROP DEFAULT emitted and applied, and an id-less insert then fails as the refusal exists to prevent), reproduced the same behavior through the actual
meta migrateCLI (exit 1 / no files on refusal, exit 0 + reversible DROP DEFAULT on allow), and verified the drift fix (CLI and sdk now both carry 11 identical tokens, pinned by a non-vacuous cross-import test) plus no-churn on the #279 explicit-@generation path via its real-PG integration test and the focused unit suite. One transient #279-test failure was traced to atasktable my own evidence runs had left in the shared DB (whole-DB introspection); dropping it made the test pass — an environmental contaminant I introduced and removed, not a code regression. Transient artifacts (container, temp CLI project, throwaway repro test) were cleaned up; the worktree is clean.Evidence: Real meta migrate CLI — refusal (identity.primary, no @generation, live SERIAL)
$ meta migrate --from-db --db postgresql://... --dialect postgres --slug test (identity.primary, NO @generation) (live DB: CREATE TABLE task (id SERIAL PRIMARY KEY, title text NOT NULL)) changes[1]{kind,count}: change-column-default,1 written: [] summary: 1 change-column-default; not applied help[1]: "re-run with --allow drop-identity-default to apply: task.id" EXIT CODE: 1Evidence: Real meta migrate CLI — escape hatch (--allow drop-identity-default --dry-run)
$ meta migrate --from-db --db postgresql://... --dialect postgres --slug test --allow drop-identity-default --dry-run (deliberate removal path — same metadata + live SERIAL) -- UP -- ALTER TABLE "task" ALTER COLUMN "id" DROP DEFAULT; -- DOWN -- ALTER TABLE "task" ALTER COLUMN "id" SET DEFAULT nextval('task_id_seq'::regclass); ... EXIT CODE: 0Evidence: The blocked-reason message (the feature) captured against real PG
=== PREMISE === expected id.identity (no @generation declared): undefined live id.identity (real SERIAL): increment live id.default.value: nextval('task_id_seq'::regclass) === WITHOUT --allow (must refuse) === change-column-default status: blocked result.blocked includes it? : true === THE BLOCKED REASON (the feature) === column "task"."id" has a live Postgres auto-increment default (nextval('task_id_seq'::regclass)) but its metadata declares no @generation — this is ambiguous: it could mean @generation was never declared, or that auto-increment is being removed on purpose. Dropping the default is destructive (every insert that omits the column starts failing), so this refuses rather than guessing. Declare @generation: increment on the identity to keep the sequence, or pass --allow drop-identity-default if removing it is intentional === WITH --allow drop-identity-default (deliberate removal still works) === change-column-default status: allowed | result.blocked length: 0Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
server/typescript/packages/cli/test/unit/allow-tokens-pinned.test.ts:32- The drift-guard test pins ALLOW_TOKENS (cli/src/lib/args.ts) against AllowTokenEnum.options (sdk/src/config.ts) — the loud-failure path, where an out-of-sync token is rejected by the config.json validator with an explicit error. It does not cover ALLOW_TOKEN_MAP (cli/src/lib/allow.ts:8), the token→AllowOptions-field map that actually grants permissions. That map is the silent-failure path: a token present in ALLOW_TOKENS but absent from ALLOW_TOKEN_MAP is accepted by the args.ts:277/426 validator, then silently ignored by tokensToAllowOptions (allow.ts:37-38if (field !== undefined)), granting no permission with no error — the user passes--allow X, sees the blocked-changes message again, passes it again, and it still blocks. This change itself is correct (drop-identity-default was added to all four lists), so this is a coverage boundary of the new 'durable' guard rather than a current bug. Importing ALLOW_TOKEN_MAP alongside the other two and asserting its keys equal ALLOW_TOKENS would close the window for future additions.✅ **Test** - passed
✅ No issues found.
bun test server/typescript/packages/migrate-ts/test/unit/diff-status-identity-default.test.ts (7 pass) — refuse logic, escape hatch, and 4 no-churn guardsbun test server/typescript/packages/cli/test/unit/allow-tokens-pinned.test.ts (1 pass) — pins CLI ALLOW_TOKENS to sdk AllowTokenEnum; verified non-vacuous (dropping a token from either side fails it)MIGRATE_TS_PG_URL=... bun test server/typescript/packages/migrate-ts/test/integration/pg-identity-no-generation-refuse.test.ts (2 pass) — real SERIAL PK → introspect → BLOCKED → emit throws; allow flag → applies + id-less insert failsMIGRATE_TS_PG_URL=... bun test server/typescript/packages/migrate-ts/test/integration/pg-serial-identity-adoption.test.ts (1 pass) — #279 explicit @generation path unchanged; re-diff empty, id-less insert still succeedsbun test server/typescript/packages/cli/test/cli.test.ts (18 pass) — migrate --help snapshot matches the synced 11-token --allow list incl. drop-identity-defaultRealmeta migrate --from-db --dialect postgresagainst live SERIAL PK: refused without --allow (exit 1, no files written, guidancere-run with --allow drop-identity-default to apply: task.id); allowed (exit 0, emits DROP DEFAULT + reversible SET DEFAULT nextval(...))Code reading: isPgAutoSequenceDefault is shared across diff/status.ts, diff/index.ts, introspect/postgres.ts (no second regex); skipIdentityDefaultDiff (diff/index.ts:411) proves an explicit identity:'increment' expected column can never reach the new change-column-default block✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.