fix(planner): resolve six planner/verifier bugs blocking reconciliation

[saevarb]

closes TML-2086
closes TML-2076
closes TML-2077
closes TML-2087
closes TML-2088
closes TML-2089
closes TML-2091

Intent

Six bugs in the migration planner and schema verifier made reconciliation operations (ALTER TYPE, SET DEFAULT, ALTER DEFAULT, DROP INDEX/UNIQUE/FK) silently fail or become unreachable. This branch fixes all six, adds operation builders for the three default-related issue kinds, strengthens postchecks across the board, and delivers integration tests that exercise the full plan→run→verify pipeline against a live Postgres instance.

Change map

• Implementation:
  • planner-reconciliation.ts — three new operation builders, conflict conversion for extra_default
  • planner.ts — columnTypeCheck, buildExpectedFormatType, columnDefaultExistsCheck, columnDefaultValueCheck, renderExpectedPgDefault
  • verify-sql-schema.ts — extra_default detection, FK strict-mode fix
  • verify-helpers.ts — indexOrConstraint populated on extra-object issues
  • control-plane/types.ts, config/types.ts — extra_default added to SchemaIssue kind union
• Tests (evidence):
  • planner.reconciliation.integration.test.ts — 11 single-operation + 8 compound integration tests against live PG
  • planner.reconciliation-unit.test.ts — unit tests for new operation builders and invariant checks
  • schema-verify.strict.test.ts — extra FK detection in strict mode

The story

1. Unblock default operations (TML-2076): The planner had no builders for default_missing or default_mismatch — these fell through to conflicts. Add buildSetDefaultOperation (additive) and buildAlterDefaultOperation (widening) that emit ALTER COLUMN SET DEFAULT.

2. Fix ALTER TYPE idempotency probe (TML-2077): The postcheck for ALTER COLUMN TYPE only verified the column existed, not that the type actually changed. The idempotency probe saw "column exists" and skipped the ALTER. Replace with columnTypeCheck that compares format_type(atttypid, atttypmod) against the expected type, correctly handling parameterized types (e.g., varchar(255)) and user-defined types (enums).

3. Make drop operations reachable (TML-2087): The verifier emitted extra_index, extra_unique_constraint, and extra_foreign_key issues but never populated indexOrConstraint. The planner guards on !issue.indexOrConstraint and returned null — drop operations were dead code. Populate the field from schema IR.

4. Fix FK detection in strict mode (TML-2088): verifyForeignKeys was only called when the contract had FKs for a table. Extra FKs in the DB were invisible when the contract had none. Fix: call verification when constraintFks.length > 0 || strict.

5. Strengthen default value postchecks (TML-2089): buildAlterDefaultOperation's postcheck used LIKE for default comparison. Replace with exact = match via columnDefaultValueCheck + renderExpectedPgDefault. Also fix buildSetDefaultOperation which only checked existence (IS NOT NULL) instead of the actual value.

6. Detect stale defaults (TML-2091): No extra_default issue kind existed. When a contract removed a default, the DB kept it silently. Add detection in the verifier, buildDropDefaultOperation in the planner, and extra_default to the SchemaIssue union.

Behavior changes & evidence

• Default operations now plan and execute: Before → planner rejected default_missing/default_mismatch as conflicts. After → emits SET DEFAULT / ALTER DEFAULT / DROP DEFAULT operations.

  • Implementation: planner-reconciliation.ts (L198–L260)
  • Tests: integration tests — "adds default", "changes default value", "drops stale default"

• ALTER COLUMN TYPE actually executes: Before → idempotency probe saw column existed and skipped. After → probe checks the full column type (including typmods) via format_type(atttypid, atttypmod), with buildExpectedFormatType mapping contract types to PG display form.

  • Implementation: planner-reconciliation.ts (L545–L555), planner.ts — columnTypeCheck
  • Tests: integration test — "changes column type"

• Drop index/unique/FK operations reachable: Before → indexOrConstraint was always undefined, planner returned null. After → populated from schema IR, operations emit correctly.

  • Implementation: verify-helpers.ts (L238)
  • Tests: integration tests — "drops extra index", "drops extra unique", "drops extra FK"

• Extra FKs visible in strict mode: Before → verifyForeignKeys skipped when contract had no FKs. After → called when strict, reports extra_foreign_key.

  • Implementation: verify-sql-schema.ts
  • Tests: schema-verify.strict.test.ts — "detects extra foreign keys when contract has no FKs"

• Default postchecks verify values, not just existence: Before → IS NOT NULL / LIKE. After → exact = comparison via renderExpectedPgDefault.

  • Implementation: planner.ts — columnDefaultValueCheck, renderExpectedPgDefault, planner-reconciliation.ts — buildSetDefaultOperation
  • Tests: unit test — "default_missing postcheck verifies actual default value"

• No behavior change (refactors):

  • columnDefaultCheck renamed to columnDefaultExistsCheck for clarity
  • JSDoc added to all postcheck helpers
  • buildSetDefaultOperation / buildAlterDefaultOperation take columnDefault as separate param with Omit<StorageColumn, 'default'> to prevent using wrong default

Compatibility / migration / risk

None noted. All changes are in the migration planner/verifier — no runtime query path affected. New operations only fire when reconciliation mode is enabled with appropriate policy flags.

Follow-ups / open questions

• extra_default false positives on serial/identity columns: The verifier has no way to distinguish implicit nextval() defaults from user-defined ones. Needs SqlColumnIR enrichment with pg_attribute.attidentity. Known-failure test on fix/planner-known-failures branch.
• Duplicate SchemaIssue type: Identical interface in @prisma-next/config and @prisma-next/core-control-plane. Canonical home should be config (shared plane), control-plane should re-export.
• default_mismatch operation class: Currently classified as widening — may warrant its own class since changing a default doesn't widen or narrow.
• Operation ordering in compound scenarios: sortSchemaIssues sorts alphabetically by kind, which may produce wrong ordering for type-sensitive default changes. Pre-existing.

Non-goals / intentionally out of scope

• Primary key replacement operations (no buildReplacePrimaryKeyOperation)
• Unique/FK dependency ordering (planner can't detect when a drop depends on another object)
• Serial/identity column handling in extra_default detection

Summary by CodeRabbit

• New Features

  • Strict mode now reports unexpected database column defaults as a distinct issue.
  • Verification output includes index/constraint identifiers for foreign keys, uniques, and indexes.
  • Migration planner can add, alter, or remove column defaults and performs stronger pre/post-check validations (including expected default and type checks).

• Tests

  • Added unit and extensive integration tests covering strict verification, default handling, reconciliation, and end-to-end Postgres migrations.

[coderabbitai]

Warning

Rate limit exceeded

@saevarb has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 17 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: c5517477-d9b6-4414-b3b1-0b93b8478265

📥 Commits

Reviewing files that changed from the base of the PR and between 7a497f9 and 898c0c9.

📒 Files selected for processing (1)

• packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.integration.test.ts

📝 Walkthrough

Walkthrough

Adds strict-mode detection for columns that have database defaults but none in the contract (new extra_default issue), includes index/constraint names in extra-constraint issues, and implements planner reconciliation operations (SET/ALTER/DROP DEFAULT) with pre/postchecks and accompanying unit and integration tests.

Changes

Cohort / File(s)	Summary
Type Definitions packages/1-framework/1-core/migration/control-plane/src/types.ts, packages/1-framework/1-core/shared/config/src/types.ts	Extended SchemaIssue.kind union to include the new literal 'extra_default'.
Schema verification helpers packages/2-sql/3-tooling/family/src/core/schema-verify/verify-helpers.ts	Extra FK/unique/index issues now include an indexOrConstraint field; populated from existing names or synthesized as fk(...), unique(...), or idx(...).
Schema verification flow packages/2-sql/3-tooling/family/src/core/schema-verify/verify-sql-schema.ts	Propagated strict into column/table helpers; under strict detect and emit extra_default for DB columns with defaults when the contract omits them; run FK verification also when strict is enabled.
Postgres planner helpers packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts	Made buildColumnDefaultSql exported; added exported builders: buildExpectedFormatType, columnTypeCheck, columnDefaultExistsCheck, columnDefaultValueCheck, and renderExpectedPgDefault for constructing pre/postcheck SQL and predicting normalized column_default.
Planner reconciliation packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts	Added reconciliation operation builders: default_missing → SET DEFAULT (additive), default_mismatch → SET/ALTER DEFAULT (widening, gated by allowWidening), and extra_default → DROP DEFAULT (destructive, gated by allowDestructive); each operation has precheck/postcheck SQL; updated conflict mapping and enhanced type-change postcheck to verify formatted type.
Unit & integration tests packages/3-targets/3-targets/postgres/test/migrations/..., packages/2-sql/3-tooling/family/test/schema-verify.strict.test.ts, test/integration/test/cli.db-verify.e2e.test.ts	Added and updated unit and integration tests covering strict-mode extra-default detection, index/constraint naming in issues, planner default operations and policies, postcheck validations, and numerous reconciliation scenarios; minor SQL change in an e2e test table definition.

Sequence Diagram(s)

sequenceDiagram participant Contract participant Verifier as Schema Verifier participant Planner as Reconciliation Planner participant Runner as Migration Executor participant DB as Database Contract->>Verifier: Provide contract (column has no default) with strict=true Verifier->>DB: Introspect table/column metadata DB-->>Verifier: Report column with a default Verifier->>Verifier: Emit issue "extra_default" (include indexOrConstraint when applicable) Verifier->>Planner: Send reported issues Planner->>Planner: Build migration ops (SET/ALTER/DROP DEFAULT) with precheck/postcheck SQL Planner->>Runner: Deliver migration plan Runner->>DB: Execute ALTER TABLE ... (SET/ALTER/DROP DEFAULT) DB-->>Runner: Execution result Runner->>DB: Run postcheck SQL (verify column_default or format_type) DB-->>Runner: Postcheck result 

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 I found a default, tucked out of sight,

a tiny extra not in the light.
With strict little hops I point and propose,
set it, change it, or drop what it chose,
now contracts and columns sleep right.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main change: fixing six planner/verifier bugs blocking reconciliation operations. It directly corresponds to the primary objectives and changes across the codebase.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/planner-issues

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[pkg-pr-new]

Open in StackBlitz

@prisma-next/runtime-executor

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/runtime-executor@248 

@prisma-next/sql-runtime

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-runtime@248 

@prisma-next/extension-paradedb

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-paradedb@248 

@prisma-next/extension-pgvector

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-pgvector@248 

@prisma-next/postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/postgres@248 

@prisma-next/sql-orm-client

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-orm-client@248 

@prisma-next/contract-authoring

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/contract-authoring@248 

@prisma-next/contract-ts

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/contract-ts@248 

@prisma-next/ids

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/ids@248 

@prisma-next/psl-parser

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/psl-parser@248 

@prisma-next/cli

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/cli@248 

@prisma-next/emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/emitter@248 

@prisma-next/eslint-plugin

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/eslint-plugin@248 

@prisma-next/migration-tools

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/migration-tools@248 

@prisma-next/vite-plugin-contract-emit

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/vite-plugin-contract-emit@248 

@prisma-next/sql-contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract@248 

@prisma-next/sql-errors

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-errors@248 

@prisma-next/sql-operations

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-operations@248 

@prisma-next/sql-schema-ir

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-schema-ir@248 

@prisma-next/sql-contract-psl

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-psl@248 

@prisma-next/sql-contract-ts

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-ts@248 

@prisma-next/sql-contract-emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-emitter@248 

@prisma-next/family-sql

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/family-sql@248 

@prisma-next/sql-kysely-lane

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-kysely-lane@248 

@prisma-next/sql-lane-query-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-lane-query-builder@248 

@prisma-next/sql-relational-core

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-relational-core@248 

@prisma-next/sql-lane

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-lane@248 

@prisma-next/target-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-postgres@248 

@prisma-next/adapter-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-postgres@248 

@prisma-next/driver-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-postgres@248 

@prisma-next/core-control-plane

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/core-control-plane@248 

@prisma-next/core-execution-plane

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/core-execution-plane@248 

@prisma-next/config

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/config@248 

@prisma-next/contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/contract@248 

@prisma-next/operations

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/operations@248 

@prisma-next/plan

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/plan@248 

@prisma-next/utils

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/utils@248 

commit: 898c0c9

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (3)

packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.integration.test.ts (1)

1-995: Split this integration suite by feature area.

This new file is ~1,000 lines and mixes helpers, single-operation cases, and compound scenarios. The repo guideline caps test files at 500 lines and asks to split distinct concerns, so this should be broken into smaller suites before it grows further (for example defaults, drops, and compound cases).

As per coding guidelines, "Keep test files under 500 lines to maintain readability and navigability. If a test file exceeds this limit, it should be split into multiple files."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.integration.test.ts` around lines 1 - 995, The file exceeds the 500-line test guideline and mixes helpers and many feature areas; split the "PostgresMigrationPlanner - reconciliation integration" describe suite into multiple smaller test files (e.g., defaults, drops, single-operation reconciliation, compound scenarios) by extracting and reusing the shared helpers makeContract, makeTable, applyBaseline, planAndExecute, introspectSchema and RECONCILIATION_POLICY into a common test-utils module, then move tests that exercise defaults, drops (tables/columns/indexes/constraints), type/nullability changes, and compound scenarios into their own describe suites/files while keeping frameworkComponents/fixtures usage consistent so each resulting file stays under 500 lines. 

packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts (2)

144-826: Split this test file; it exceeds the 500-line limit.

This file now mixes several concerns (drop/nullability/type/default/conflict/order). Please split by functional area (for example: planner.reconciliation.defaults.test.ts, planner.reconciliation.conflicts.test.ts) to keep each file independently readable and under limit.

As per coding guidelines: **/*.test.ts files should stay under 500 lines and be split by logical boundaries.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts` around lines 144 - 826, This test file exceeds the 500-line limit; split the single describe('buildReconciliationPlan', ...) file into multiple smaller test files grouped by logical concern (e.g. move the "destructive drop operations" describe into planner.reconciliation.destructive.test.ts, "nullability operations" into planner.reconciliation.nullability.test.ts, "type mismatch operations" into planner.reconciliation.type.test.ts, "column default operations" into planner.reconciliation.defaults.test.ts, and "conflict conversion" + "deterministic output ordering" into planner.reconciliation.conflicts.test.ts), preserving each inner it(...) tests and top-level helper usage (plan, issue, contractWithColumn*, ADDITIVE_MODE/POLICY) and updating any shared imports/fixtures so each new file imports them; ensure each new test file remains under 500 lines and that test suite names (e.g. describe('destructive drop operations')) are unchanged to keep test semantics. 

---

503-733: Add a unit case for extra_default (DROP DEFAULT).

This suite covers default_missing/default_mismatch, but not the destructive extra_default path. A focused case for operation shape/policy conversion would close the gap for the new reconciliation surface.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts` around lines 503 - 733, Add a unit test for the destructive "extra_default" reconciliation case: create a contract with contractWithColumnDefault('user','col','type', ...) then call plan([{ kind: 'extra_default', table: 'user', column: 'col', actual: '<whatever>', message: 'extra default' }], { contract }) and assert that result.operations has one operation whose id follows the DROP-default pattern (e.g. 'dropDefault.user.col') and whose operationClass is 'destructive' and whose execute SQL contains 'DROP DEFAULT'; also add a case that supplies a policy forbidding destructive operations (use MigrationOperationPolicy or ADDITIVE_ONLY_POLICY) and assert the issue is converted to a conflict (operations length 0 and conflicts length 1). Use the existing helpers plan(), issue(), contractWithColumnDefault(), ADDITIVE_ONLY_POLICY and reference the operation id pattern in assertions to keep the test consistent with other cases. 

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed. Inline comments: In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts`: - Around line 566-654: The SET DEFAULT builders (buildSetDefaultOperation and buildAlterDefaultOperation) currently splice an empty string from buildColumnDefaultSql (for autoincrement()) into "ALTER COLUMN ... SET ${defaultClause}", producing invalid SQL; update both builders to detect autoincrement/defaultClause === '' (or columnDefault.type === 'autoincrement') and handle it specially: either construct a proper sequence/identity statement (e.g., route through the sequence/identity path by emitting an ALTER TABLE ... ALTER COLUMN ... SET DEFAULT nextval(...) or ALTER COLUMN ... ADD GENERATED AS IDENTITY as appropriate) or refuse to generate a SET DEFAULT and surface a clear error/precheck that prevents emitting invalid SQL. Ensure you reference buildColumnDefaultSql, buildSetDefaultOperation and buildAlterDefaultOperation when making the change. - Around line 198-252: The default-change cases ('default_missing', 'default_mismatch', 'extra_default') are being emitted before type changes, causing SET/ALTER DEFAULT to run against the old type; fix by ensuring type changes run first: either update sortSchemaIssues() to rank 'type_mismatch' higher than any 'default_*' kinds so type changes are ordered before defaults, or in planner-reconciliation.ts's default cases check the incoming issues list for a matching 'type_mismatch' on the same table+column and skip/ defer returning buildSetDefaultOperation / buildAlterDefaultOperation / buildDropDefaultOperation if a type change exists; reference the 'type_mismatch' case and the default cases and the builder helpers (buildSetDefaultOperation, buildAlterDefaultOperation, buildDropDefaultOperation) when making the change. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`: - Around line 979-1002: The columnTypeCheck function currently compares only atttypid which loses typmods; update columnTypeCheck to compare the full type string using format_type(a.atttypid, a.atttypmod) instead of comparing a.atttypid to '${escapeLiteral(expectedType)}'::regtype — build the SELECT to compute format_type(a.atttypid, a.atttypmod) and compare that result (properly escaped) to expectedType so parameterized types like varchar(255) or numeric(10,2) are matched exactly. In `@packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts`: - Around line 1-4: The test imports ColumnDefault from the wrong package; change the import so ColumnDefault comes from `@prisma-next/sql-contract/types` instead of `@prisma-next/contract/types`: remove ColumnDefault from the import that currently includes coreHash and profileHash and add a separate import of ColumnDefault from `@prisma-next/sql-contract/types` (leaving coreHash/profileHash as-is) so SQL-storage specific types are sourced from SqlContract types. --- Nitpick comments: In `@packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts`: - Around line 144-826: This test file exceeds the 500-line limit; split the single describe('buildReconciliationPlan', ...) file into multiple smaller test files grouped by logical concern (e.g. move the "destructive drop operations" describe into planner.reconciliation.destructive.test.ts, "nullability operations" into planner.reconciliation.nullability.test.ts, "type mismatch operations" into planner.reconciliation.type.test.ts, "column default operations" into planner.reconciliation.defaults.test.ts, and "conflict conversion" + "deterministic output ordering" into planner.reconciliation.conflicts.test.ts), preserving each inner it(...) tests and top-level helper usage (plan, issue, contractWithColumn*, ADDITIVE_MODE/POLICY) and updating any shared imports/fixtures so each new file imports them; ensure each new test file remains under 500 lines and that test suite names (e.g. describe('destructive drop operations')) are unchanged to keep test semantics. - Around line 503-733: Add a unit test for the destructive "extra_default" reconciliation case: create a contract with contractWithColumnDefault('user','col','type', ...) then call plan([{ kind: 'extra_default', table: 'user', column: 'col', actual: '<whatever>', message: 'extra default' }], { contract }) and assert that result.operations has one operation whose id follows the DROP-default pattern (e.g. 'dropDefault.user.col') and whose operationClass is 'destructive' and whose execute SQL contains 'DROP DEFAULT'; also add a case that supplies a policy forbidding destructive operations (use MigrationOperationPolicy or ADDITIVE_ONLY_POLICY) and assert the issue is converted to a conflict (operations length 0 and conflicts length 1). Use the existing helpers plan(), issue(), contractWithColumnDefault(), ADDITIVE_ONLY_POLICY and reference the operation id pattern in assertions to keep the test consistent with other cases. In `@packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.integration.test.ts`: - Around line 1-995: The file exceeds the 500-line test guideline and mixes helpers and many feature areas; split the "PostgresMigrationPlanner - reconciliation integration" describe suite into multiple smaller test files (e.g., defaults, drops, single-operation reconciliation, compound scenarios) by extracting and reusing the shared helpers makeContract, makeTable, applyBaseline, planAndExecute, introspectSchema and RECONCILIATION_POLICY into a common test-utils module, then move tests that exercise defaults, drops (tables/columns/indexes/constraints), type/nullability changes, and compound scenarios into their own describe suites/files while keeping frameworkComponents/fixtures usage consistent so each resulting file stays under 500 lines. 

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b4e65bdd-2f8f-4865-9966-09848b9776ef

📥 Commits

Reviewing files that changed from the base of the PR and between 9577a0d and 5c97be3.

📒 Files selected for processing (9)

• packages/1-framework/1-core/migration/control-plane/src/types.ts
• packages/1-framework/1-core/shared/config/src/types.ts
• packages/2-sql/3-tooling/family/src/core/schema-verify/verify-helpers.ts
• packages/2-sql/3-tooling/family/src/core/schema-verify/verify-sql-schema.ts
• packages/2-sql/3-tooling/family/test/schema-verify.strict.test.ts
• packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts
• packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
• packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts
• packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.integration.test.ts

[saevarb]

Since we are now stricter in how we detect extra defaults, this test was failing. Making this change should really not alter what is being tested, but it lets the test pass with the stricter checks.

[coderabbitai]

♻️ Duplicate comments (2)

packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts (2)

755-770: ⚠️ Potential issue | 🟠 Major

Type/default dependency ordering is still vulnerable on the same column.

sortSchemaIssues() still sorts by kind lexicographically (Line 757), so default_* can run before type_mismatch. That can emit SET DEFAULT against the old type and fail before ALTER TYPE.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts` around lines 755 - 770, sortSchemaIssues currently orders by SchemaIssue.kind lexicographically which allows default_* to precede type_mismatch; change sortSchemaIssues to use an explicit precedence for kinds (e.g., ensure "type_mismatch" has higher priority than any "default_*") instead of localeCompare on kind — implement a small kindRank mapping inside sortSchemaIssues and compare by kindRank[a.kind] - kindRank[b.kind] (falling back to compareStrings for unknown kinds), keep subsequent comparisons using compareStrings on table, column, and indexOrConstraint to preserve tie-breakers; reference function sortSchemaIssues, fields kind/table/column/indexOrConstraint, and compareStrings. 

---

575-596: ⚠️ Potential issue | 🟠 Major

Autoincrement defaults can still render invalid ALTER COLUMN ... SET SQL.

These builders still splice defaultClause into SET ${defaultClause}. If buildColumnDefaultSql() returns an empty string for autoincrement/identity-style defaults, this produces invalid SQL (... SET ).

Also applies to: 620-640

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts` around lines 575 - 596, The generated ALTER COLUMN SQL incorrectly splices an empty defaultClause into `SET ${defaultClause}`, producing invalid SQL when buildColumnDefaultSql() returns an empty string (e.g., for identity/autoincrement defaults); update the logic in the setDefault.* planner block to guard against empty defaultClause: if defaultClause is non-empty use it, otherwise fall back to expectedDefault (renderExpectedPgDefault) and only emit the execute step `ALTER COLUMN ... SET ...` when that resolved clause is non-empty; if both are empty, omit the execute action (or mark it as no-op). Reference symbols: buildColumnDefaultSql, renderExpectedPgDefault, defaultClause, expectedDefault, the execute SQL that builds `ALTER TABLE ${qualified}\nALTER COLUMN ${quoteIdentifier(columnName)} SET ${defaultClause}`, and the setDefault.<tableName> id. 

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed. Duplicate comments: In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts`: - Around line 755-770: sortSchemaIssues currently orders by SchemaIssue.kind lexicographically which allows default_* to precede type_mismatch; change sortSchemaIssues to use an explicit precedence for kinds (e.g., ensure "type_mismatch" has higher priority than any "default_*") instead of localeCompare on kind — implement a small kindRank mapping inside sortSchemaIssues and compare by kindRank[a.kind] - kindRank[b.kind] (falling back to compareStrings for unknown kinds), keep subsequent comparisons using compareStrings on table, column, and indexOrConstraint to preserve tie-breakers; reference function sortSchemaIssues, fields kind/table/column/indexOrConstraint, and compareStrings. - Around line 575-596: The generated ALTER COLUMN SQL incorrectly splices an empty defaultClause into `SET ${defaultClause}`, producing invalid SQL when buildColumnDefaultSql() returns an empty string (e.g., for identity/autoincrement defaults); update the logic in the setDefault.* planner block to guard against empty defaultClause: if defaultClause is non-empty use it, otherwise fall back to expectedDefault (renderExpectedPgDefault) and only emit the execute step `ALTER COLUMN ... SET ...` when that resolved clause is non-empty; if both are empty, omit the execute action (or mark it as no-op). Reference symbols: buildColumnDefaultSql, renderExpectedPgDefault, defaultClause, expectedDefault, the execute SQL that builds `ALTER TABLE ${qualified}\nALTER COLUMN ${quoteIdentifier(columnName)} SET ${defaultClause}`, and the setDefault.<tableName> id. 

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d97682e9-0c00-446f-9dc6-2ded049a6b3d

📥 Commits

Reviewing files that changed from the base of the PR and between a90a221 and 95146fc.

📒 Files selected for processing (3)

• packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts
• packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
• packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts
• packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts

[jkomyno]

TL;DR

4 blocking issues must be fixed before merge. The core bug fixes are correct and well-tested, but several new postcheck/builder paths will fail at runtime for temporal types, autoincrement defaults, and mixed-case enums. The rest are non-blocking nits or follow-up candidates.

Priority	Count	Verdict
P1 — Blocking	4	Must fix before merge
P2 — Important	3	Should fix (this PR or fast-follow)
P3 — Nits	4	Non-blocking, nice-to-have

---

Comprehensive Review: PR #248

Review method: Multi-agent analysis (architecture, data integrity, security, patterns, performance, simplicity, git history) plus manual deep-dive of implementation against PG catalog behavior and ADR compliance.

---

P1 — BLOCKING (Must Fix Before Merge)

P1-1: Autoincrement defaults produce invalid SQL in SET DEFAULT path

planner-reconciliation.ts:595

buildColumnDefaultSql() returns '' for autoincrement() (planner.ts:848). When buildSetDefaultOperation calls it, the execute SQL becomes:

ALTER TABLE "public"."item" ALTER COLUMN "id" SET 

This is syntactically invalid SQL. The runner will emit an adapter error and halt.

Reachability: The verifier's default_missing path fires when the contract declares a default but the DB column has none. For an autoincrement() default on a non-SERIAL column (e.g., a plain int4 column reconciled to expect autoincrement), this path IS reachable. Additionally, renderExpectedPgDefault returns 'autoincrement()' as the expected PG default string for the postcheck, which PG would never store.

Fix: Guard against autoincrement() in buildSetDefaultOperation / buildAlterDefaultOperation. Either return null (convert to conflict, since SERIAL defaults can only be established during CREATE TABLE) or emit DEFAULT nextval(...) with a generated sequence name.

---

P1-2: buildExpectedFormatType missing temporal type mappings

planner.ts:984-1022

The FORMAT_TYPE_DISPLAY map has 6 entries but no temporal types. PostgreSQL's format_type() returns expanded names:

Contract nativeType	format_type() returns	buildExpectedFormatType returns	Match?
timestamp	timestamp without time zone	timestamp	MISMATCH
timestamptz	timestamp with time zone	timestamptz	MISMATCH
time	time without time zone	time	MISMATCH
timetz	time with time zone	timetz	MISMATCH

Per ADR 038, "Pre OK → Execute OK → Post FAIL → conflict" — so a perfectly valid ALTER COLUMN TYPE will be reported as failed.

Test gap: No integration tests cover temporal type changes.

Fix: Add temporal mappings to FORMAT_TYPE_DISPLAY:

['timestamp', 'timestamp without time zone'], ['timestamptz', 'timestamp with time zone'], ['time', 'time without time zone'], ['timetz', 'time with time zone'],

The inverse mapping already exists in normalizeSchemaNativeType (control-adapter.ts:489-501). Consider extracting a shared bidirectional map to prevent future drift.

---

P1-3: renderExpectedPgDefault type-cast uses contract nativeType, not PG's normalized form

planner.ts:1115-1117

For string/Date literals, the function appends ::${column.nativeType}:

return `${rendered}::${column.nativeType}`;

But PG normalizes the type cast in information_schema.columns.column_default. For a timestamptz column with default '2023-01-01':

• Predicted: '2023-01-01'::timestamptz
• PG stores: '2023-01-01 00:00:00+00'::timestamp with time zone

The columnDefaultValueCheck exact comparison fails. Same issue affects enums — PG may store 'active'::public."StatusType" while the function produces 'active'::StatusType.

Fix: Map column.nativeType through the FORMAT_TYPE_DISPLAY mapping (extended with temporal types per P1-2) before appending the cast.

---

P1-4: buildExpectedFormatType returns unquoted names for mixed-case user-defined types

planner.ts:1017-1018

if (column.typeRef) { return column.nativeType; }

For an enum created as CREATE TYPE "BillingState" AS ENUM (...), format_type() returns "BillingState" (with double quotes, since the unquoted form would be folded to lowercase). But column.nativeType is BillingState (no quotes). The postcheck comparison fails.

Mitigating factor: If the enum was created through the same planner (which uses quoteIdentifier), and the contract stores the exact case, format_type() returns "BillingState". The integration test only covers lowercase status_type, which doesn't need quoting — so this bug is untested.

Fix: For typeRef types, either quote names that contain uppercase characters, or use a case-insensitive comparison in the postcheck SQL.

---

P2 — IMPORTANT (Should Fix, This PR or Fast-Follow)

P2-1: constraintExistsCheck not scoped to table — violates ADR 044

planner.ts:918-934

The check filters only by conname + nspname:

WHERE c.conname = '...' AND n.nspname = '...'

ADR 044 specifies constraintExists(table, name?, kind?) — the table parameter is part of the canonical vocabulary. PostgreSQL allows duplicate constraint names across different tables in the same schema.

Impact: If child1.fk_shared is dropped but child2.fk_shared exists, the drop postcheck (NOT EXISTS) returns false because child2.fk_shared still matches. The runner reports the operation as failed.

This pre-dates the PR for additive FK/unique operations, but this PR introduces its first use in the drop path (planner-reconciliation.ts:405-419), where false positives are more dangerous.

Fix: Add c.conrelid = '${schema}.${table}'::regclass to the WHERE clause.

---

P2-2: extra_default false positives on serial/identity columns

verify-sql-schema.ts:739-758

The verifier emits extra_default when strict && schemaColumn.default && !contractColumn.default. Serial columns have an implicit nextval() default. If the contract omits the default (relying on SERIAL type semantics), the verifier flags this as extra_default, and the planner emits DROP DEFAULT — which breaks autoincrement silently.

The PR description acknowledges this: "Needs SqlColumnIR enrichment with pg_attribute.attidentity. Known-failure test on fix/planner-known-failures branch."

Non-blocking only because this requires strict mode AND a contract that omits the autoincrement default. But the risk is high if triggered — new inserts silently fail.

---

P2-3: Security — assertSafeDefaultExpression denylist is bypassable

planner.ts:761-768

The function blocks ;, --, /*, $$, and SELECT, but passes dangerous PG functions like pg_sleep(999), lo_export(1234, '/tmp/dump'), and pg_read_file('/etc/passwd'). The threat model is a malicious contract.json.

Non-blocking for this PR since the contract is developer-authored and this is pre-existing behavior, but worth hardening with an allowlist approach in a follow-up.

---

P3 — NITS (Non-Blocking)

P3-1: buildSetDefaultOperation / buildAlterDefaultOperation near-duplicate

planner-reconciliation.ts:567-655

These differ in exactly 4 string/enum values (operationClass, label, summary, description). They even share the same id format. Extract a shared buildDefaultOperation(operationClass, verb) to eliminate ~40 lines.

P3-2: FORMAT_TYPE_DISPLAY and normalizeFormattedType encode inverse mappings with no shared source of truth

planner.ts:984-991 vs control-adapter.ts:506-552

If a new type mapping is added to one but not the other, postchecks and verifier will silently disagree. Consider a shared bidirectional map.

P3-3: Commit history has a regression-then-fix pattern

Commit 773a7e36 introduces columnTypeCheck using ::regtype (which ignores typmods). Commit 95146fcf (12 commits later) fixes this by switching to format_type(). Consider squashing before merge.

P3-4: information_schema vs pg_catalog inconsistency

columnTypeCheck queries pg_attribute directly while columnDefaultValueCheck/columnExistsCheck query information_schema.columns. The information_schema path is ~2-5x slower due to view expansion overhead. Functionally fine for one-shot postchecks, but inconsistent.

---

Test Coverage Gaps

Scenario	Covered?
ALTER TYPE (text→int4, text→enum lowercase, varchar typmod change)	Yes
SET/ALTER/DROP DEFAULT (literals, functions)	Yes
DROP table/column/index/unique/FK	Yes
Compound scenarios (8 tests)	Yes
ALTER TYPE with temporal types	Missing
ALTER TYPE with mixed-case enum	Missing
SET DEFAULT on timestamptz column	Missing
SET DEFAULT on autoincrement column	Missing
DROP FK with duplicate constraint names	Missing

---

ADR Compliance

ADR	Status	Notes
ADR 044 (Pre/post check vocabulary)	Violation	constraintExistsCheck missing table parameter
ADR 038 (Operation idempotency)	Compliant	Pre/post/execute pattern correctly implemented
ADR 005 (Thin core, fat targets)	Compliant	Target-specific logic stays in postgres planner
ADR 028 (Migration structure)	Compliant	Operation schema follows canonical form

---

What's Good

• The core fixes are sound — six previously-unreachable paths are now active and well-tested
• The file separation between planner.ts (helpers) and planner-reconciliation.ts (operation builders) is clean
• Integration tests are thorough — 11 single-operation + 8 compound scenarios against live PG
• The Omit<StorageColumn, 'default'> signature pattern prevents misuse of the wrong default
• The seenOperationIds dedup set correctly handles overlapping issues (e.g., extra_unique_constraint + extra_index on the same object)
• Dependency direction is correct: target layer imports from family layer, never the reverse

---

Recommended Path Forward

1. Fix P1-1 through P1-4 — these will cause valid operations to fail at runtime
2. Add integration tests for temporal types, mixed-case enums, and autoincrement defaults
3. Address P2-1 (constraint table scoping) in this PR or an immediate follow-up
4. P2-2, P2-3, and all P3 items can be tracked as follow-ups